CIP: Log more errors when expected data is missing

1. CIP: Instead of exiting early in dissect_cip_generic_service_req/rsp when there is no data, keep processing so that a malformed packet warning will be displayed when there should be data.
2. CIP Safety: Remove copy-paste. Use load_cip_request_data
3. CIP Safety: Use more constants.

Change-Id: Ic364201f1e587b43cf2bda407fb77b50032974ae
Reviewed-on: https://code.wireshark.org/review/20549
Petri-Dish: Michael Mann <mmann78@netscape.net>
Tested-by: Petri Dish Buildbot <buildbot-no-reply@wireshark.org>
Reviewed-by: Michael Mann <mmann78@netscape.net>

3 files changed, 51 insertions, 61 deletions

diff --git a/epan/dissectors/packet-cip.c b/epan/dissectors/packet-cip.c
index 80d7547c0e..6b522ee8ad 100644
--- a/epan/dissectors/packet-cip.c
+++ b/epan/dissectors/packet-cip.c
@@ -5522,8 +5522,6 @@ dissect_cip_generic_service_req(tvbuff_t *tvb, packet_info *pinfo, proto_tree *t
 
    req_path_size = tvb_get_guint8( tvb, offset+1);
    offset += ((req_path_size*2)+2);
-   if (tvb_reported_length_remaining(tvb, offset) <= 0)
-      return tvb_reported_length(tvb);
 
    int parsed_len = 0;
 
@@ -5536,8 +5534,12 @@ dissect_cip_generic_service_req(tvbuff_t *tvb, packet_info *pinfo, proto_tree *t
       parsed_len = dissect_cip_set_attribute_list_req(tvb, pinfo, cmd_data_tree, cmd_data_item, offset, req_data);
       break;
    case SC_RESET:
-      proto_tree_add_item(cmd_data_tree, hf_cip_sc_reset_param, tvb, offset, 1, ENC_LITTLE_ENDIAN);
-      parsed_len = 1;
+      // Parameter to reset is optional.
+      if (tvb_reported_length_remaining(tvb, offset) > 0)
+      {
+         proto_tree_add_item(cmd_data_tree, hf_cip_sc_reset_param, tvb, offset, 1, ENC_LITTLE_ENDIAN);
+         parsed_len = 1;
+      }
       break;
    case SC_MULT_SERV_PACK:
       parsed_len = dissect_cip_multiple_service_packet(tvb, pinfo, cmd_data_tree, cmd_data_item, offset, TRUE);
@@ -5852,7 +5854,7 @@ dissect_cip_find_next_object_rsp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *
    return 1 + num_instances * 2;
 }
 
-static void load_cip_request_data(packet_info *pinfo, cip_simple_request_info_t *req_data)
+void load_cip_request_data(packet_info *pinfo, cip_simple_request_info_t *req_data)
 {
     cip_req_info_t* preq_info;
     preq_info = (cip_req_info_t*)p_get_proto_data(wmem_file_scope(), pinfo, proto_cip, 0);
@@ -5871,6 +5873,22 @@ static void load_cip_request_data(packet_info *pinfo, cip_simple_request_info_t
     }
 }
 
+static gboolean should_dissect_cip_response(tvbuff_t *tvb, int offset, guint8 gen_status)
+{
+    // Only parse the response if there is data left or it has a response status that allows additional data
+    //   to be returned.
+    if ((tvb_reported_length_remaining(tvb, offset) == 0)
+        && gen_status != CI_GRC_SUCCESS
+        && gen_status != CI_GRC_ATTR_LIST_ERROR
+        && gen_status != CI_GRC_SERVICE_ERROR
+        && gen_status != CI_GRC_INVALID_LIST_STATUS)
+    {
+        return FALSE;
+    }
+
+    return TRUE;
+}
+
 static int
 dissect_cip_generic_service_rsp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
 {
@@ -5878,7 +5896,7 @@ dissect_cip_generic_service_rsp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *t
    proto_tree *cmd_data_tree;
    cip_simple_request_info_t req_data;
    int offset = 0;
-   int item_length = tvb_reported_length(tvb);
+   guint8 gen_status = tvb_get_guint8(tvb, offset + 2);
    guint8 service = tvb_get_guint8(tvb, offset) & CIP_SC_MASK;
    guint16 add_stat_size = tvb_get_guint8( tvb, offset+3 ) * 2;
 
@@ -5886,20 +5904,17 @@ dissect_cip_generic_service_rsp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *t
 
    add_cip_service_to_info_column(pinfo, service, cip_sc_vals);
 
-   /* If there is any command specific data create a sub-tree for it */
-   if( (item_length-4-add_stat_size ) != 0 )
-   {
-      cmd_data_tree = proto_tree_add_subtree(tree, tvb, offset, item_length-4-add_stat_size,
-                           ett_cmd_data, &cmd_data_item, val_to_str(service, cip_sc_vals , "Unknown Service (0x%02x)"));
-      proto_item_append_text(cmd_data_item, " (Response)");
-   }
-   else
-   {
-      return tvb_reported_length(tvb);
-   }
+   cmd_data_tree = proto_tree_add_subtree(tree, tvb, offset, -1,
+       ett_cmd_data, &cmd_data_item, val_to_str(service, cip_sc_vals, "Unknown Service (0x%02x)"));
+   proto_item_append_text(cmd_data_item, " (Response)");
 
    load_cip_request_data(pinfo, &req_data);
 
+   if (should_dissect_cip_response(tvb, offset, gen_status) == FALSE)
+   {
+      return 0;
+   }
+
    int parsed_len = 0;
 
    switch(service)
@@ -5931,8 +5946,8 @@ dissect_cip_generic_service_rsp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *t
       parsed_len = 1;
       break;
    default:
-       // No specific handling for other services.
-       break;
+      // No specific handling for other services.
+      break;
    }
 
    // Display any remaining unparsed data.
diff --git a/epan/dissectors/packet-cip.h b/epan/dissectors/packet-cip.h
index eec135abe0..37aa723dff 100644
--- a/epan/dissectors/packet-cip.h
+++ b/epan/dissectors/packet-cip.h
@@ -349,6 +349,7 @@ extern void dissect_cip_date_and_time(proto_tree *tree, tvbuff_t *tvb, int offse
 extern attribute_info_t* cip_get_attribute(guint class_id, guint instance, guint attribute);
 extern int dissect_cip_get_attribute_all_rsp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree,
     int offset, cip_simple_request_info_t* req_data);
+extern void load_cip_request_data(packet_info *pinfo, cip_simple_request_info_t *req_data);
 
 /*
 ** Exported variables
diff --git a/epan/dissectors/packet-cipsafety.c b/epan/dissectors/packet-cipsafety.c
index 93ad6dbc05..2bcd1b8dd7 100644
--- a/epan/dissectors/packet-cipsafety.c
+++ b/epan/dissectors/packet-cipsafety.c
@@ -469,7 +469,6 @@ dissect_cip_s_supervisor_data( proto_tree *item_tree,
    int                        req_path_size;
    int                        temp_data;
    guint8                     service, gen_status, add_stat_size;
-   cip_req_info_t*            preq_info;
    cip_simple_request_info_t  req_data;
 
    col_set_str(pinfo->cinfo, COL_PROTOCOL, "CIPS Supervisor");
@@ -482,27 +481,15 @@ dissect_cip_s_supervisor_data( proto_tree *item_tree,
    proto_tree_add_item( rrsc_tree, hf_cip_reqrsp, tvb, offset, 1, ENC_LITTLE_ENDIAN );
 
    proto_item_append_text( rrsc_item, "%s (%s)",
-               val_to_str( ( service & 0x7F ), cip_sc_vals_ssupervisor , "Unknown Service (0x%02x)"),
-               val_to_str_const( ( service & 0x80 )>>7, cip_sc_rr, "") );
+               val_to_str( ( service & CIP_SC_MASK ), cip_sc_vals_ssupervisor , "Unknown Service (0x%02x)"),
+               val_to_str_const( ( service & CIP_SC_RESPONSE_MASK )>>7, cip_sc_rr, "") );
 
    /* Add Service code */
    proto_tree_add_item(rrsc_tree, hf_cip_ssupervisor_sc, tvb, offset, 1, ENC_LITTLE_ENDIAN );
 
-   preq_info = (cip_req_info_t*)p_get_proto_data(wmem_file_scope(), pinfo, proto_cip, 0);
-   if ((preq_info != NULL) &&
-       (preq_info->ciaData != NULL))
-   {
-      memcpy(&req_data, preq_info->ciaData, sizeof(cip_simple_request_info_t));
-   }
-   else
-   {
-      req_data.iClass     = (guint32)-1;
-      req_data.iInstance  = (guint32)-1;
-      req_data.iAttribute = (guint32)-1;
-      req_data.iMember    = (guint32)-1;
-   }
+   load_cip_request_data(pinfo, &req_data);
 
-   if(service & 0x80 )
+   if (service & CIP_SC_RESPONSE_MASK)
    {
       /* Response message */
 
@@ -518,7 +505,7 @@ dissect_cip_s_supervisor_data( proto_tree *item_tree,
 
          if( gen_status == CI_GRC_SUCCESS || gen_status == CI_GRC_SERVICE_ERROR )
          {
-            switch (service & 0x7F)
+            switch (service & CIP_SC_MASK)
             {
             case SC_SSUPER_VALIDATE_CONFIGURATION:
                proto_tree_add_item(cmd_data_tree, hf_cip_ssupervisor_validate_configuration_sccrc,
@@ -531,7 +518,7 @@ dissect_cip_s_supervisor_data( proto_tree *item_tree,
                break;
             }
          }
-         else if ((gen_status == 0xD0) && ((service & 0x7F) == SC_SSUPER_VALIDATE_CONFIGURATION))
+         else if ((gen_status == 0xD0) && ((service & CIP_SC_MASK) == SC_SSUPER_VALIDATE_CONFIGURATION))
          {
             if (add_stat_size > 0)
             {
@@ -1078,7 +1065,6 @@ dissect_cip_s_validator_data( proto_tree *item_tree,
    proto_tree                *rrsc_tree, *cmd_data_tree;
    int                        req_path_size;
    guint8                     service, gen_status, add_stat_size;
-   cip_req_info_t*            preq_info;
    cip_simple_request_info_t  req_data;
 
    col_set_str(pinfo->cinfo, COL_PROTOCOL, "CIPS Validator");
@@ -1091,29 +1077,17 @@ dissect_cip_s_validator_data( proto_tree *item_tree,
    proto_tree_add_item( rrsc_tree, hf_cip_reqrsp, tvb, offset, 1, ENC_LITTLE_ENDIAN );
 
    proto_item_append_text( rrsc_item, "%s (%s)",
-               val_to_str( ( service & 0x7F ),
+               val_to_str( ( service & CIP_SC_MASK ),
                   cip_sc_vals_svalidator , "Unknown Service (0x%02x)"),
-               val_to_str_const( ( service & 0x80 )>>7,
+               val_to_str_const( ( service & CIP_SC_RESPONSE_MASK )>>7,
                   cip_sc_rr, "") );
 
    /* Add Service code */
    proto_tree_add_item(rrsc_tree, hf_cip_svalidator_sc, tvb, offset, 1, ENC_LITTLE_ENDIAN );
 
-   preq_info = (cip_req_info_t*)p_get_proto_data(wmem_file_scope(), pinfo, proto_cip, 0);
-   if ((preq_info != NULL) &&
-       (preq_info->ciaData != NULL))
-   {
-      memcpy(&req_data, preq_info->ciaData, sizeof(cip_simple_request_info_t));
-   }
-   else
-   {
-      req_data.iClass     = (guint32)-1;
-      req_data.iInstance  = (guint32)-1;
-      req_data.iAttribute = (guint32)-1;
-      req_data.iMember    = (guint32)-1;
-   }
+   load_cip_request_data(pinfo, &req_data);
 
-   if(service & 0x80 )
+   if (service & CIP_SC_RESPONSE_MASK)
    {
       /* Response message */
 
@@ -1130,7 +1104,7 @@ dissect_cip_s_validator_data( proto_tree *item_tree,
          if( gen_status == CI_GRC_SUCCESS || gen_status == CI_GRC_SERVICE_ERROR )
          {
             /* Success responses */
-            if (((service & 0x7F) == SC_GET_ATT_ALL) &&
+            if (((service & CIP_SC_MASK) == SC_GET_ATT_ALL) &&
                 (req_data.iInstance != (guint32)-1) &&
                 (req_data.iInstance != 0))
             {
@@ -1199,12 +1173,12 @@ dissect_class_svalidator_heur(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tre
    int             offset  = 0;
 
    service = tvb_get_guint8( tvb, offset );
-   service_code = service & 0x7F;
+   service_code = service & CIP_SC_MASK;
 
    /* Handle GetAttributeAll and SetAttributeAll in CCO class */
    if (service_code == SC_GET_ATT_ALL)
    {
-      if (service & 0x80)
+      if (service & CIP_SC_RESPONSE_MASK)
       {
          /* Service response */
          preq_info = (cip_req_info_t*)p_get_proto_data(wmem_file_scope(), pinfo, proto_cip, 0);
@@ -1620,7 +1594,7 @@ proto_register_cipsafety(void)
    {
       { &hf_cip_reqrsp,
         { "Request/Response", "cip.rr",
-          FT_UINT8, BASE_HEX, VALS(cip_sc_rr), 0x80, "Request or Response message", HFILL }
+          FT_UINT8, BASE_HEX, VALS(cip_sc_rr), CIP_SC_RESPONSE_MASK, "Request or Response message", HFILL }
       },
       { &hf_cip_data,
         { "Data", "cip.data",
@@ -1793,7 +1767,7 @@ proto_register_cipsafety(void)
    static hf_register_info hf_ssupervisor[] = {
       { &hf_cip_ssupervisor_sc,
         { "Service", "cipsafety.ssupervisor.sc",
-          FT_UINT8, BASE_HEX, VALS(cip_sc_vals_ssupervisor), 0x7F, NULL, HFILL }
+          FT_UINT8, BASE_HEX, VALS(cip_sc_vals_ssupervisor), CIP_SC_MASK, NULL, HFILL }
       },
       { &hf_cip_ssupervisor_recover_data,
         { "Data", "cipsafety.ssupervisor.recover.data",
@@ -2228,7 +2202,7 @@ proto_register_cipsafety(void)
    static hf_register_info hf_svalidator[] = {
       { &hf_cip_svalidator_sc,
         { "Service", "cipsafety.svalidator.sc",
-          FT_UINT8, BASE_HEX, VALS(cip_sc_vals_svalidator), 0x7F, NULL, HFILL }
+          FT_UINT8, BASE_HEX, VALS(cip_sc_vals_svalidator), CIP_SC_MASK, NULL, HFILL }
       },
 
       { &hf_cip_svalidator_sconn_fault_count,