Wireshark · Ethereal-dev: Re: [Ethereal-dev] Parsing error for ISAKMP QM proposals

[Guy Harris <guy@xxxxxxxxxx>]

> A small bug report: Ethereal skips the second proposal payload when
> parsing the attached packet (tcpdump shows it correctly).

The ISAKMP spec does not appear to be written as clearly as one might
like when describing Security Association payloads, which may be why it
wasn't looping over the rest of the payload, repeatedly dissecting
sub-payloads.

Here's a patch that should fix the problem; I'll check it in.

Index: packet-isakmp.c
===================================================================
RCS file: /usr/local/cvsroot/ethereal/packet-isakmp.c,v
retrieving revision 1.44
diff -c -r1.44 packet-isakmp.c
*** packet-isakmp.c	2001/10/22 20:45:58	1.44
--- packet-isakmp.c	2001/10/25 23:33:47
***************
*** 1,6 ****
  /* packet-isakmp.c
   * Routines for the Internet Security Association and Key Management Protocol
!  * (ISAKMP) (RFC 2408)
   * Brad Robel-Forrest <brad.robel-forrest@xxxxxxxxxxxxxx>
   *
   * $Id: packet-isakmp.c,v 1.44 2001/10/22 20:45:58 guy Exp $
--- 1,7 ----
  /* packet-isakmp.c
   * Routines for the Internet Security Association and Key Management Protocol
!  * (ISAKMP) (RFC 2408) and the Internet IP Security Domain of Interpretation
!  * for ISAKMP (RFC 2407)
   * Brad Robel-Forrest <brad.robel-forrest@xxxxxxxxxxxxxx>
   *
   * $Id: packet-isakmp.c,v 1.44 2001/10/22 20:45:58 guy Exp $
***************
*** 224,229 ****
--- 225,231 ----
  
  static gboolean get_num(tvbuff_t *, int, guint16, guint32 *);
  
+ #define LOAD_TYPE_NONE		0	/* payload type for None */
  #define LOAD_TYPE_PROPOSAL	2	/* payload type for Proposal */
  #define	LOAD_TYPE_TRANSFORM	3	/* payload type for Transform */
  #define NUM_LOAD_TYPES		15
***************
*** 255,260 ****
--- 257,309 ----
  static dissector_handle_t ah_handle;
  
  static void
+ dissect_payloads(tvbuff_t *tvb, proto_tree *tree, guint8 initial_payload,
+ 		 int offset, int length)
+ {
+   guint8 payload, next_payload;
+   guint16		payload_length;
+   proto_tree *		ntree;
+ 
+   for (payload = initial_payload; length != 0; payload = next_payload) {
+     if (payload == LOAD_TYPE_NONE) {
+       /*
+        * What?  There's more stuff in this chunk of data, but the
+        * previous payload had a "next payload" type of None?
+        */
+       proto_tree_add_text(tree, tvb, offset, length,
+ 			  "Extra data: %s",
+ 			  tvb_bytes_to_str(tvb, offset, length));
+       break;
+     }
+     ntree = dissect_payload_header(tvb, offset, length, payload,
+       &next_payload, &payload_length, tree);
+     if (ntree == NULL)
+       break;
+     if (payload_length >= 4) {	/* XXX = > 4? */
+       if (payload < NUM_LOAD_TYPES) {
+         if (next_payload == LOAD_TYPE_TRANSFORM)
+           dissect_transform(tvb, offset + 4, payload_length - 4, ntree, 0);
+             /* XXX - protocol ID? */
+         else
+           (*strfuncs[payload].func)(tvb, offset + 4, payload_length - 4, ntree);
+       }
+       else {
+         proto_tree_add_text(ntree, tvb, offset + 4, payload_length - 4,
+             "Payload");
+       }
+     }
+     else {
+         proto_tree_add_text(ntree, tvb, offset + 4, 0,
+             "Payload (bogus, length is %u, must be at least 4)",
+             payload_length);
+         payload_length = 4;
+     }
+     offset += payload_length;
+     length -= payload_length;
+   }
+ }
+ 
+ static void
  dissect_isakmp(tvbuff_t *tvb, packet_info *pinfo, proto_tree *tree)
  {
    int			offset = 0;
***************
*** 263,271 ****
    proto_tree *		isakmp_tree = NULL;
    struct udp_encap_hdr * encap_hdr;
    guint32		len;
-   guint8		payload, next_payload;
-   guint16		payload_length;
-   proto_tree *		ntree;
    static const guint8	non_ike_marker[8] = { 0, 0, 0, 0, 0, 0, 0, 0 };
    tvbuff_t *		next_tvb;
  
--- 312,317 ----
***************
*** 398,432 ****
  			"Encrypted payload (%d byte%s)",
  			len, plurality(len, "", "s"));
        }
!     } else {
!       for (payload = hdr->next_payload; len != 0; payload = next_payload) {
!         ntree = dissect_payload_header(tvb, offset, len, payload,
!             &next_payload, &payload_length, isakmp_tree);
!         if (ntree == NULL)
!           break;
!         if (payload_length >= 4) {	/* XXX = > 4? */
!           if (payload < NUM_LOAD_TYPES) {
!             if (next_payload == LOAD_TYPE_TRANSFORM)
!               dissect_transform(tvb, offset + 4, payload_length - 4, ntree, 0);
!             	/* XXX - protocol ID? */
!             else
!               (*strfuncs[payload].func)(tvb, offset + 4, payload_length - 4, ntree);
!           }
!           else {
!             proto_tree_add_text(ntree, tvb, offset + 4, payload_length - 4,
!                 "Payload");
!           }
!         }
!         else {
!             proto_tree_add_text(ntree, tvb, offset + 4, 0,
!                 "Payload (bogus, length is %u, must be at least 4)",
!                 payload_length);
!             payload_length = 4;
!         }
!         offset += payload_length;
!         len -= payload_length;
!       }
!     }
    }
  }
  
--- 444,451 ----
  			"Encrypted payload (%d byte%s)",
  			len, plurality(len, "", "s"));
        }
!     } else
!       dissect_payloads(tvb, isakmp_tree, hdr->next_payload, offset, len);
    }
  }
  
***************
*** 471,480 ****
  {
    guint32		doi;
    guint32		situation;
-   guint8		next_payload;
-   guint16		payload_length;
-   proto_tree *		ntree;
  
    doi = tvb_get_ntohl(tvb, offset);
    proto_tree_add_text(tree, tvb, offset, 4,
  		      "Domain of interpretation: %s (%u)",
--- 490,502 ----
  {
    guint32		doi;
    guint32		situation;
  
+   if (length < 4) {
+     proto_tree_add_text(tree, tvb, offset, length,
+ 			"DOI %s (length is %u, should be >= 4)",
+ 			tvb_bytes_to_str(tvb, offset, length), length);
+     return;
+   }
    doi = tvb_get_ntohl(tvb, offset);
    proto_tree_add_text(tree, tvb, offset, 4,
  		      "Domain of interpretation: %s (%u)",
***************
*** 482,498 ****
    offset += 4;
    length -= 4;
    
!   situation = tvb_get_ntohl(tvb, offset);
!   proto_tree_add_text(tree, tvb, offset, 4,
! 		      "Situation: %s (%u)",
! 		      situation2str(situation), situation);
!   offset += 4;
!   length -= 4;
    
!   ntree = dissect_payload_header(tvb, offset, length, LOAD_TYPE_PROPOSAL,
!     &next_payload, &payload_length, tree);
!   if (ntree != NULL)
!     dissect_proposal(tvb, offset + 4, payload_length - 4, ntree);
  }
  
  static void
--- 504,531 ----
    offset += 4;
    length -= 4;
    
!   if (doi == 1) {
!     /* IPSEC */
!     if (length < 4) {
!       proto_tree_add_text(tree, tvb, offset, length,
! 			  "Situation: %s (length is %u, should be >= 4)",
! 			  tvb_bytes_to_str(tvb, offset, length), length);
!       return;
!     }
!     situation = tvb_get_ntohl(tvb, offset);
!     proto_tree_add_text(tree, tvb, offset, 4,
! 			"Situation: %s (%u)",
! 			situation2str(situation), situation);
!     offset += 4;
!     length -= 4;
    
!     dissect_payloads(tvb, tree, LOAD_TYPE_PROPOSAL, offset, length);
!   } else {
!     /* Unknown */
!     proto_tree_add_text(tree, tvb, offset, length,
! 			"Situation: %s",
! 			tvb_bytes_to_str(tvb, offset, length));
!   }
  }
  
  static void
***************
*** 530,536 ****
    length -= 1;
  
    if (spi_size) {
!     proto_tree_add_text(tree, tvb, offset, spi_size, "SPI");
      offset += spi_size;
      length -= spi_size;
    }
--- 563,570 ----
    length -= 1;
  
    if (spi_size) {
!     proto_tree_add_text(tree, tvb, offset, spi_size, "SPI: %s",
! 			tvb_bytes_to_str(tvb, offset, spi_size));
      offset += spi_size;
      length -= spi_size;
    }