ISAKMP: add traffic selector subtree

Create a new subtree for each traffic selector.

Use the length field to find the next traffic selector
rather than accumulating the individual TS field sizes.

Split dissect_ts() to introduce separate offset variables.
After adding the final field, do not advance offset again
to avoid unused variable warnings.

Change-Id: I9951662c1bb3958994162b25b70b43ec1da103a7
Reviewed-on: https://code.wireshark.org/review/19440
Petri-Dish: Anders Broman <a.broman58@gmail.com>
Tested-by: Petri Dish Buildbot <buildbot-no-reply@wireshark.org>
Reviewed-by: Anders Broman <a.broman58@gmail.com>

1 files changed, 100 insertions, 105 deletions

diff --git a/epan/dissectors/packet-isakmp.c b/epan/dissectors/packet-isakmp.c
index c9cbc19940..d56e9752c5 100644
--- a/epan/dissectors/packet-isakmp.c
+++ b/epan/dissectors/packet-isakmp.c
@@ -382,6 +382,7 @@ static gint ett_isakmp_sa = -1;
 static gint ett_isakmp_attr = -1;
 static gint ett_isakmp_id = -1;
 static gint ett_isakmp_notify_data = -1;
+static gint ett_isakmp_ts = -1;
 #ifdef HAVE_LIBGCRYPT
 /* For decrypted IKEv2 Encrypted payload*/
 static gint ett_isakmp_decrypted_data = -1;
@@ -454,7 +455,7 @@ static const fragment_items isakmp_frag_items = {
  */
 #define IKEV2_TS_IPV4_ADDR_RANGE        7
 #define IKEV2_TS_IPV6_ADDR_RANGE        8
-#define IKEV2_TS_FC_ADDR_RANGE          9
+#define IKEV2_TS_FC_ADDR_RANGE          9  /* RFC 4595 */
 /*
  * Configuration Payload Attribute Types
  *   draft-ietf-ipsec-isakmp-mode-cfg-05.txt for IKEv1
@@ -2198,7 +2199,7 @@ static int dissect_vid(tvbuff_t *, int, int, proto_tree *);
 static void dissect_config(tvbuff_t *, packet_info *, int, int, proto_tree *, int, gboolean);
 static void dissect_nat_discovery(tvbuff_t *, int, int, proto_tree * );
 static void dissect_nat_original_address(tvbuff_t *, int, int, proto_tree *, int );
-static void dissect_ts(tvbuff_t *, int, int, proto_tree *);
+static void dissect_ts_payload(tvbuff_t *, int, int, proto_tree *);
 static tvbuff_t * dissect_enc(tvbuff_t *, int, int, proto_tree *, packet_info *, guint8, gboolean, void*, gboolean);
 static void dissect_eap(tvbuff_t *, int, int, proto_tree *, packet_info *);
 static void dissect_gspm(tvbuff_t *, int, int, proto_tree *);
@@ -3003,7 +3004,7 @@ dissect_payloads(tvbuff_t *tvb, proto_tree *tree,
             break;
           case PLOAD_IKE2_TSI:
           case PLOAD_IKE2_TSR:
-            dissect_ts(tvb, offset + 4, payload_length - 4, ntree);
+            dissect_ts_payload(tvb, offset + 4, payload_length - 4, ntree);
             break;
           case PLOAD_IKE2_SK:
             if(isakmp_version == 2)
@@ -5106,132 +5107,125 @@ dissect_nat_original_address(tvbuff_t *tvb, int offset, int length _U_, proto_tr
   }
 }
 
-static void
-dissect_ts(tvbuff_t *tvb, int offset, int length, proto_tree *tree)
+static int
+dissect_ts(tvbuff_t *tvb, int offset, proto_tree *payload_tree)
 {
-  guint8        num, tstype, protocol_id;
+  guint8        tstype, protocol_id;
+  guint16       len;
+  proto_item    *ts_item;
+  proto_tree    *tree;
+  const gchar   *ts_typename;
+
+  len = tvb_get_guint16(tvb, offset + 2, ENC_BIG_ENDIAN);
+  if (len < 4)
+    return 4;
 
-  num = tvb_get_guint8(tvb, offset);
-  proto_item_append_text(tree," # %d", num);
-  proto_tree_add_item(tree, hf_isakmp_ts_number_of_ts, tvb, offset, 1, ENC_BIG_ENDIAN);
+  ts_item = proto_tree_add_item(payload_tree, hf_isakmp_ts_data, tvb, offset, len, ENC_NA);
+  tree = proto_item_add_subtree(ts_item, ett_isakmp_ts);
+
+  tstype = tvb_get_guint8(tvb, offset);
+  proto_tree_add_item(tree, hf_isakmp_ts_type, tvb, offset, 1, ENC_BIG_ENDIAN);
+  ts_typename = rval_to_str(tstype, traffic_selector_type, "Unknown Type (%d)");
+  proto_item_append_text(ts_item, ": %s", ts_typename);
 
   offset += 1;
-  length -= 1;
 
-  offset += 3; /* Reserved */
-  length -= 3;
+  switch (tstype) {
+  case IKEV2_TS_IPV4_ADDR_RANGE:
+    protocol_id = tvb_get_guint8(tvb, offset);
+    if (protocol_id == 0)
+        proto_tree_add_uint_format_value(tree, hf_isakmp_ts_protoid, tvb, offset,1,
+                           protocol_id, "Unused");
+    else
+        proto_tree_add_item(tree, hf_isakmp_ts_protoid, tvb, offset, 1, ENC_BIG_ENDIAN);
+    offset += 1;
+
+    proto_tree_add_item(tree, hf_isakmp_ts_selector_length, tvb, offset, 2, ENC_BIG_ENDIAN);
+    offset += 2;
+
+    proto_tree_add_item(tree, hf_isakmp_ts_start_port, tvb, offset, 2, ENC_BIG_ENDIAN);
+    offset += 2;
+
+    proto_tree_add_item(tree, hf_isakmp_ts_end_port, tvb, offset, 2, ENC_BIG_ENDIAN);
+    offset += 2;
 
-  while (length > 0) {
-    tstype = tvb_get_guint8(tvb, offset);
-    proto_tree_add_item(tree, hf_isakmp_ts_type, tvb, offset, 1, ENC_BIG_ENDIAN);
+    proto_tree_add_item(tree, hf_isakmp_ts_start_addr_ipv4, tvb, offset, 4, ENC_BIG_ENDIAN);
+    offset += 4;
 
+    proto_tree_add_item(tree, hf_isakmp_ts_end_addr_ipv4, tvb, offset, 4, ENC_BIG_ENDIAN);
+    break;
+
+  case IKEV2_TS_IPV6_ADDR_RANGE:
+    protocol_id = tvb_get_guint8(tvb, offset);
+    if (protocol_id == 0)
+        proto_tree_add_uint_format_value(tree, hf_isakmp_ts_protoid, tvb, offset,1,
+                           protocol_id, "Unused");
+    else
+        proto_tree_add_item(tree, hf_isakmp_ts_protoid, tvb, offset, 1, ENC_BIG_ENDIAN);
     offset += 1;
-    length -= 1;
-    switch (tstype) {
-    case IKEV2_TS_IPV4_ADDR_RANGE:
-      protocol_id = tvb_get_guint8(tvb, offset);
-      if (protocol_id == 0)
-          proto_tree_add_uint_format_value(tree, hf_isakmp_ts_protoid, tvb, offset,1,
-                             protocol_id, "Unused");
-      else
-          proto_tree_add_item(tree, hf_isakmp_ts_protoid, tvb, offset, 1, ENC_BIG_ENDIAN);
-      offset += 1;
-      length -= 1;
 
-      proto_tree_add_item(tree, hf_isakmp_ts_selector_length, tvb, offset, 2, ENC_BIG_ENDIAN);
-      offset += 2;
-      length -= 2;
+    proto_tree_add_item(tree, hf_isakmp_ts_selector_length, tvb, offset, 2, ENC_BIG_ENDIAN);
+    offset += 2;
 
-      proto_tree_add_item(tree, hf_isakmp_ts_start_port, tvb, offset, 2, ENC_BIG_ENDIAN);
-      offset += 2;
-      length -= 2;
+    proto_tree_add_item(tree, hf_isakmp_ts_start_port, tvb, offset, 2, ENC_BIG_ENDIAN);
+    offset += 2;
 
-      proto_tree_add_item(tree, hf_isakmp_ts_end_port, tvb, offset, 2, ENC_BIG_ENDIAN);
-      offset += 2;
-      length -= 2;
+    proto_tree_add_item(tree, hf_isakmp_ts_end_port, tvb, offset, 2, ENC_BIG_ENDIAN);
+    offset += 2;
 
-      proto_tree_add_item(tree, hf_isakmp_ts_start_addr_ipv4, tvb, offset, 4, ENC_BIG_ENDIAN);
-      offset += 4;
-      length -= 4;
-      proto_tree_add_item(tree, hf_isakmp_ts_end_addr_ipv4, tvb, offset, 4, ENC_BIG_ENDIAN);
-      offset += 4;
-      length -= 4;
-      break;
-    case IKEV2_TS_IPV6_ADDR_RANGE:
-      protocol_id = tvb_get_guint8(tvb, offset);
-      if (protocol_id == 0)
-          proto_tree_add_uint_format_value(tree, hf_isakmp_ts_protoid, tvb, offset,1,
-                             protocol_id, "Unused");
-      else
-          proto_tree_add_item(tree, hf_isakmp_ts_protoid, tvb, offset, 1, ENC_BIG_ENDIAN);
-      offset += 1;
-      length -= 1;
+    proto_tree_add_item(tree, hf_isakmp_ts_start_addr_ipv6, tvb, offset, 16, ENC_NA);
+    offset += 16;
+
+    proto_tree_add_item(tree, hf_isakmp_ts_end_addr_ipv6, tvb, offset, 16, ENC_NA);
+    break;
 
-      proto_tree_add_item(tree, hf_isakmp_ts_selector_length, tvb, offset, 2, ENC_BIG_ENDIAN);
-      offset += 2;
-      length -= 2;
+  case IKEV2_TS_FC_ADDR_RANGE:
+    offset += 1; /* Reserved */
 
-      proto_tree_add_item(tree, hf_isakmp_ts_start_port, tvb, offset, 2, ENC_BIG_ENDIAN);
-      offset += 2;
-      length -= 2;
+    proto_tree_add_item(tree, hf_isakmp_ts_selector_length, tvb, offset, 2, ENC_BIG_ENDIAN);
+    offset += 2;
 
-      proto_tree_add_item(tree, hf_isakmp_ts_end_port, tvb, offset, 2, ENC_BIG_ENDIAN);
-      offset += 2;
-      length -= 2;
+    offset += 1; /* Reserved */
 
-      proto_tree_add_item(tree, hf_isakmp_ts_start_addr_ipv6, tvb, offset, 16, ENC_NA);
-      offset += 16;
-      length -= 16;
+    proto_tree_add_item(tree, hf_isakmp_ts_start_addr_fc, tvb, offset, 3, ENC_BIG_ENDIAN);
+    offset += 3;
 
-      proto_tree_add_item(tree, hf_isakmp_ts_end_addr_ipv6, tvb, offset, 16, ENC_NA);
-      offset += 16;
-      length -= 16;
-      break;
+    offset += 1; /* Reserved */
 
-    case IKEV2_TS_FC_ADDR_RANGE:
-      offset += 1; /* Reserved */
-      length -= 1;
+    proto_tree_add_item(tree, hf_isakmp_ts_end_addr_fc, tvb, offset, 3, ENC_BIG_ENDIAN);
+    offset += 3;
 
-      proto_tree_add_item(tree, hf_isakmp_ts_selector_length, tvb, offset, 2, ENC_BIG_ENDIAN);
-      offset += 2;
-      length -= 2;
+    proto_tree_add_item(tree, hf_isakmp_ts_start_r_ctl, tvb, offset, 1, ENC_BIG_ENDIAN);
+    offset += 1;
 
-      offset += 1; /* Reserved */
-      length -= 1;
+    proto_tree_add_item(tree, hf_isakmp_ts_end_r_ctl, tvb, offset, 1, ENC_BIG_ENDIAN);
+    offset += 1;
 
-      proto_tree_add_item(tree, hf_isakmp_ts_start_addr_fc, tvb, offset, 3, ENC_BIG_ENDIAN);
-      offset += 3;
-      length -= 3;
+    proto_tree_add_item(tree, hf_isakmp_ts_start_type, tvb, offset, 1, ENC_BIG_ENDIAN);
+    offset += 1;
 
-      offset += 1; /* Reserved */
-      length -= 1;
+    proto_tree_add_item(tree, hf_isakmp_ts_end_type, tvb, offset, 1, ENC_BIG_ENDIAN);
+    break;
+  }
 
-      proto_tree_add_item(tree, hf_isakmp_ts_end_addr_fc, tvb, offset, 3, ENC_BIG_ENDIAN);
-      offset += 3;
-      length -= 3;
+  return len;
+}
 
-      proto_tree_add_item(tree, hf_isakmp_ts_start_r_ctl, tvb, offset, 1, ENC_BIG_ENDIAN);
-      offset += 1;
-      length -= 1;
+static void
+dissect_ts_payload(tvbuff_t *tvb, int offset, int length, proto_tree *tree)
+{
+  guint8        num;
+  int           offset_end = offset + length;
 
-      proto_tree_add_item(tree, hf_isakmp_ts_end_r_ctl, tvb, offset, 1, ENC_BIG_ENDIAN);
-      offset += 1;
-      length -= 1;
+  num = tvb_get_guint8(tvb, offset);
+  proto_item_append_text(tree," # %d", num);
+  proto_tree_add_item(tree, hf_isakmp_ts_number_of_ts, tvb, offset, 1, ENC_BIG_ENDIAN);
 
-      proto_tree_add_item(tree, hf_isakmp_ts_start_type, tvb, offset, 1, ENC_BIG_ENDIAN);
-      offset += 1;
-      length -= 1;
+  offset += 1;
+  offset += 3; /* Reserved */
 
-      proto_tree_add_item(tree, hf_isakmp_ts_end_type, tvb, offset, 1, ENC_BIG_ENDIAN);
-      offset += 1;
-      length -= 1;
-      break;
-    default:
-      proto_tree_add_item(tree, hf_isakmp_ts_data, tvb, offset, length, ENC_NA);
-      offset += length;
-      length -= length;
-      break;
-    }
+  while (offset < offset_end) {
+    offset += dissect_ts(tvb, offset, tree);
   }
 }
 
@@ -6389,9 +6383,9 @@ proto_register_isakmp(void)
         FT_UINT8, BASE_DEC, NULL, 0x0,
         NULL, HFILL }},
     { &hf_isakmp_ts_data,
-      { "Traffic Selector Data", "isakmp.ts.data",
-        FT_BYTES, BASE_NONE, NULL, 0x0,
-        NULL, HFILL }},
+      { "Traffic Selector", "isakmp.ts.data",
+        FT_NONE, BASE_NONE, NULL, 0x0,
+        "An individual traffic selector", HFILL }},
 
     { &hf_isakmp_num_spis,
       { "Number of SPIs", "isakmp.spinum",
@@ -7057,6 +7051,7 @@ proto_register_isakmp(void)
     &ett_isakmp_attr,
     &ett_isakmp_id,
     &ett_isakmp_notify_data,
+    &ett_isakmp_ts,
 #ifdef HAVE_LIBGCRYPT
     &ett_isakmp_decrypted_data,
     &ett_isakmp_decrypted_payloads