Wireshark · Ethereal-dev: Re: [ethereal-dev] Error in certificate request parsing in packet-isakmp.c

[Guy Harris <gharris@xxxxxxxxxxxx>]

On Sat, Jul 01, 2000 at 07:13:06PM -0700, Guy Harris wrote:
> I'll see whether making it
> 
> 	struct certreq_hdr {
> 	  guint8        next_payload;
> 	  guint8        reserved;
> 	  guint8        length[2];
> 	  guint8        cert_type;
> 	};
> 
> fixes the problem.

It looks as if it did (although I'm not an ISAKMP expert - all I know
about ISAKMP I learned in kindergarten^H^H^H^H^H^H^H^H^H^H^H^Ha quick
look at some bits of RFC 2408).

Here's a patch which changes all the structures.


Making that change turned up another problem - changing "identifier" in
"struct cfg_hdr" from "guint16 identifier" to "guint8 identifier[2]"
caused a warning in the statement

	proto_tree_add_text(ntree, NullTVB, offset, sizeof(hdr->identifier),
	                    "Identifier: %u",hdr->identifier);

I suspect that if "identifier" is truly an integral data type, it needs
to be converted from network byte order to host byte order, so the right
fix is to change that code to

	proto_tree_add_text(ntree, NullTVB, offset, sizeof(hdr->identifier),
	                    "Identifier: %u", pntohs(&hdr->identifier));

In addition, the structure was

	struct cfg_hdr {
	  guint8	next_payload;
	  guint8	reserved;
	  guint8	length;
	  guint8	type;
	  guint8	reserved2;
	  guint16	identifier;
	};

which consists, on most if not all platforms, of:

	one byte of "next_payload";

	one reserved byte;

	one byte of length;

	one byte of type;

	another reserved byte;

	a padding byte to put "identifier" on a 16-bit boundary;

	2 bytes of identifier.

Changing it to

	struct cfg_hdr {
	  guint8	next_payload;
	  guint8	reserved;
	  guint8	length;
	  guint8	type;
	  guint8	reserved2;
	  guint8	identifier[2];
	};

removes the padding byte, thus changing the location of "identifier".

That structure doesn't seem to be described in RFC 2408, so I don't know
whether "reserved2" is really 2 bytes:

	struct cfg_hdr {
	  guint8	next_payload;
	  guint8	reserved;
	  guint8	length;
	  guint8	type;
	  guint8	reserved2[2];
	  guint8	identifier[2];
	};

or if "identifier" truly *isn't* aligned on a natural (16-bit) boundary
(in which case the old "guint16 identifier" version was wrong), or if
"length" is 2 bytes as it is in the other headers:

	struct cfg_hdr {
	  guint8	next_payload;
	  guint8	reserved;
	  guint8	length[2];
	  guint8	type;
	  guint8	reserved2;
	  guint8	identifier[2];
	};

or if something else is the case.

Given that a "struct cfg_hdr" is dissected by 'dissect_config()", which
does

  guint16		length	 = pntohs(&hdr->length);

which is incorrect unless "length" is a 16-bit field, I suspect that the
problem is that "length" should be 2 bytes, so that's what I'll check
in, and what's in the patch attached to this message.

Index: packet-isakmp.c
===================================================================
RCS file: /usr/local/cvsroot/ethereal/packet-isakmp.c,v
retrieving revision 1.22
diff -c -r1.22 packet-isakmp.c
*** packet-isakmp.c	2000/05/31 05:07:11	1.22
--- packet-isakmp.c	2000/07/02 03:19:16
***************
*** 1,5 ****
! /* packet-gre.c
!  * Routines for the Internet Security Association and Key Management Protocol (ISAKMP)
   * Brad Robel-Forrest <brad.robel-forrest@xxxxxxxxxxxxxx>
   *
   * $Id: packet-isakmp.c,v 1.22 2000/05/31 05:07:11 guy Exp $
--- 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.22 2000/05/31 05:07:11 guy Exp $
***************
*** 180,201 ****
  #define E_FLAG		0x01
  #define C_FLAG		0x02
  #define A_FLAG		0x04
!   guint32	message_id;
!   guint32	length;
  };
  
  struct sa_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint16	length;
!   guint32	doi;
!   guint32	situation;
  };
  
  struct proposal_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint16	length;
    guint8	proposal_num;
    guint8	protocol_id;
    guint8	spi_size;
--- 181,202 ----
  #define E_FLAG		0x01
  #define C_FLAG		0x02
  #define A_FLAG		0x04
!   guint8	message_id[4];
!   guint8	length[4];
  };
  
  struct sa_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length[2];
!   guint8	doi[4];
!   guint8	situation[4];
  };
  
  struct proposal_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length[2];
    guint8	proposal_num;
    guint8	protocol_id;
    guint8	spi_size;
***************
*** 205,214 ****
  struct trans_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint16	length;
    guint8	transform_num;
    guint8	transform_id;
!   guint16	reserved2;
  };
  
  #define TRANS_LEN(p)	(pntohs(&((struct trans_hdr *)(p))->length))
--- 206,215 ----
  struct trans_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length[2];
    guint8	transform_num;
    guint8	transform_id;
!   guint8	reserved2[2];
  };
  
  #define TRANS_LEN(p)	(pntohs(&((struct trans_hdr *)(p))->length))
***************
*** 216,298 ****
  struct ke_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint16	length;
  };
  
  struct id_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint16	length;
    guint8	id_type;
    guint8	protocol_id;
!   guint16	port;
  };
  
  struct cert_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint16	length;
    guint8	cert_enc;
  };
  
  struct certreq_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint16	length;
    guint8	cert_type;
  };
  
  struct hash_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint16	length;
  };
  
  struct sig_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint16	length;
  };
  
  struct nonce_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint16	length;
  };
  
  struct notif_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint16	length;
!   guint32	doi;
    guint8	protocol_id;
    guint8	spi_size;
!   guint16	msgtype;
  };
  
  struct delete_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint16	length;
!   guint32	doi;
    guint8	protocol_id;
    guint8	spi_size;
!   guint16	num_spis;
  };
  
  struct vid_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint16	length;
  };
  
  struct cfg_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length;
    guint8	type;
    guint8	reserved2;
!   guint16	identifier;
  };
  
  static void dissect_none(const u_char *, int, frame_data *, proto_tree *);
--- 217,299 ----
  struct ke_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length[2];
  };
  
  struct id_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length[2];
    guint8	id_type;
    guint8	protocol_id;
!   guint8	port[2];
  };
  
  struct cert_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length[2];
    guint8	cert_enc;
  };
  
  struct certreq_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length[2];
    guint8	cert_type;
  };
  
  struct hash_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length[2];
  };
  
  struct sig_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length[2];
  };
  
  struct nonce_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length[2];
  };
  
  struct notif_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length[2];
!   guint8	doi[4];
    guint8	protocol_id;
    guint8	spi_size;
!   guint8	msgtype[2];
  };
  
  struct delete_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length[2];
!   guint8	doi[4];
    guint8	protocol_id;
    guint8	spi_size;
!   guint8	num_spis[2];
  };
  
  struct vid_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length[2];
  };
  
  struct cfg_hdr {
    guint8	next_payload;
    guint8	reserved;
!   guint8	length[2];
    guint8	type;
    guint8	reserved2;
!   guint8	identifier[2];
  };
  
  static void dissect_none(const u_char *, int, frame_data *, proto_tree *);
***************
*** 1059,1065 ****
    offset += (sizeof(hdr->type) + sizeof(hdr->reserved2));
    
    proto_tree_add_text(ntree, NullTVB, offset, sizeof(hdr->identifier),
!                       "Identifier: %u",hdr->identifier);
    offset += sizeof(hdr->identifier);
    length -= sizeof(*hdr);
    
--- 1060,1066 ----
    offset += (sizeof(hdr->type) + sizeof(hdr->reserved2));
    
    proto_tree_add_text(ntree, NullTVB, offset, sizeof(hdr->identifier),
!                       "Identifier: %u", pntohs(&hdr->identifier));
    offset += sizeof(hdr->identifier);
    length -= sizeof(*hdr);