Wireshark · Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 33464: /trunk/epan/dissectors/ /trunk/epan/dissectors/:

[Gerald Combs <gerald@xxxxxxxxxxxxx>]

morriss@xxxxxxxxxxxxx wrote:
> http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=rev&revision=33464
> 
> User: morriss
> Date: 2010/07/07 08:52 AM
> 
> Log:
>  Fix infinite recursion reported in https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=4984 : In try_get_ber_length() make sure we move forward before recursing.
> 
> Directory: /trunk/epan/dissectors/
>   Changes    Path            Action
>   +57 -56    packet-ber.c    Modified

If I apply the attached debugging code to packet-ber.c I see some high
stack counts in the fuzz capture from bug 4984. It looks like we're
still vulnerable to a stack overflow.

Shouldn't we remove recursion from try_get_ber_length or at least throw
an exception when we run into, say, 5 levels of nesting?

It looks like plugins/asn1/packet-asn1.c has a similar problem.

Index: epan/dissectors/packet-ber.c
===================================================================
--- epan/dissectors/packet-ber.c	(revision 33465)
+++ epan/dissectors/packet-ber.c	(working copy)
@@ -948,6 +948,7 @@
  * an indefinite length and haven't reached EOC.
  */
 /* 8.1.3 Length octets */
+static int tgbl_stack_height;
 static gboolean
 try_get_ber_length(tvbuff_t *tvb, int *bl_offset, gboolean pc, guint32 *length, gboolean *ind) {
 	int offset = *bl_offset;
@@ -965,6 +966,7 @@
 	oct = tvb_get_guint8(tvb, offset);
 	offset += 1;
 
+tgbl_stack_height++;
 	if(!(oct&0x80)) {
 		/* 8.1.3.4 */
 		tmp_length = oct;
@@ -1029,10 +1031,12 @@
 	save_pc = last_pc;
 	save_tag = last_tag;
 
+tgbl_stack_height = 0;
 	if(!try_get_ber_length(tvb, &bl_offset, last_pc, &bl_length, ind)) {
 	  /* we couldn't get a length */
 	  bl_offset = offset;
 	}
+if (tgbl_stack_height > 10) fprintf(stderr, "stack height: %d\n", tgbl_stack_height);
 	if (length)
 	  *length = bl_length;