Wireshark · Ethereal-dev: [Ethereal-dev] Re: Buildbot crash output

[ronnie sahlberg <ronniesahlberg@xxxxxxxxx>]

To illustrate the various places that might benefit from such a macro
just in packet-ndps   i have attached a diff showing where we would
need this fix.

the diff doies not compile but is unly useful to illustrate how
frequent it is to invent these "check for sanity and abort if it does
not look right).
this dissector actually tries to check the length for sanity every now
and then but still does not get it right.

the faulkt imho lies with us not documenting how to deal with these
situations and providing a nice and easy macro.


comments?





On 10/24/05, ronnie sahlberg <ronniesahlberg@xxxxxxxxx> wrote:
> This one is that the malformed packet causes
> packet-ndps.c/attribute_value()/
> case 14: /
> reading the length (which is corrupted)
> causes foffset to go beyonds the end of the packet.
>
>
> While one could do a
> DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
> this macro is really for reporting dissector bugs and not for
> indicating a known malformed packet.
>
> Alternatively one can add a lot code such as :
> if(length>tvb_reported_length_remaining(tvp, foffset)){
>      proto_tree_add_text(...   some nice string...);
>      tvb_get_guint8(tvb, 999999);   or something similar to trigger a
> malformed packet and abort dissection.
>
>
>
> We dont really have very good documentation on what to do in this
> situation for developers and most of us use different styles.
> ( i like a tvb_get_guint8(tvb, 9999999) when i really think the packet
> is malformed and if there is no point in even attempting to contunue
> dissection)
>
>
> Should we/someone add a new macro
> ASSERT_MALFORMED_PACKET( <expression>, format-string, ... );
>
> that can be used when we want to trigger what is definitely not a
> dissector bug but just plainly a malformed packet?
>
>
> There are several other situations in the same function in the ndps
> file which needs the same fix.
>
>
> comments?
>
>
> On 10/24/05, Buildbot <buildbot-do-not-reply@xxxxxxxxxxxx> wrote:
> > Problems have been found with the following capture file(s):
> >
> >
> http://www.ethereal.com/distribution/buildbot-builds/randpkt/editcap.435cafa5.pcap
> >
> >
> > Error information:
> > (no core file found)
> >
> >
> > stderr follows:
> >
> > ** (process:78082): WARNING **: Dissector bug, protocol NDPS, in packet
> 1:
> > proto.c:2614: failed assertion "end >= fi->start"
> >
> >
> > Bug 549 posted.
> >
> > _______________________________________________
> > Ethereal-dev mailing list
> > Ethereal-dev@xxxxxxxxxxxx
> > http://www.ethereal.com/mailman/listinfo/ethereal-dev
> >
>

Index: epan/dissectors/packet-ndps.c
===================================================================
--- epan/dissectors/packet-ndps.c	(revision 16292)
+++ epan/dissectors/packet-ndps.c	(working copy)
@@ -1812,6 +1812,7 @@
         foffset += 1;
         length = tvb_get_guint8(tvb, foffset);
         foffset += 1;
+        DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
         tvb_ensure_bytes_exist(tvb, foffset, length);
         proto_tree_add_item(atree, hf_ndps_oid, tvb, foffset, length, FALSE);
         foffset += length;
@@ -1820,6 +1821,7 @@
     {
         if (!found) 
         {
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             tvb_ensure_bytes_exist(tvb, foffset, length);
             foffset += length;
         }
@@ -1829,6 +1831,7 @@
             length = tvb_get_guint8(tvb, foffset);
             foffset += 1;
             tvb_ensure_bytes_exist(tvb, foffset, length);
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             foffset += length;
         }
     }
@@ -1955,6 +1958,7 @@
     address_len = tvb_get_ntohl(tvb, foffset);
     proto_tree_add_item(ndps_tree, hf_address_len, tvb, foffset, 4, FALSE);
     foffset += 4;
+    DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
     switch(address_type)
     {
     case 0x00000000:
@@ -1973,6 +1977,7 @@
             break;
     default:
         tvb_ensure_bytes_exist(tvb, foffset, tvb_get_ntohl(tvb, foffset -4));
+needs a fix
         foffset += tvb_get_ntohl(tvb, foffset -4);
         break;
     }
@@ -2056,6 +2061,7 @@
             atree = proto_item_add_subtree(aitem, ett_ndps);
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length!=0)
             {
                 tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -2068,6 +2074,7 @@
     case 1:
         length = tvb_get_ntohl(tvb, foffset);
         foffset += 4;
+        DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
         if (length!=0)
         {
             tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -2212,6 +2219,7 @@
         atree = proto_item_add_subtree(aitem, ett_ndps);
         length = tvb_get_ntohl(tvb, foffset);
         foffset += 4;
+        DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
         if (length==4)
         {
             tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -2423,6 +2431,7 @@
         case 14:         /* Cardinal Seq */
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length==4)
             {
                 tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -2685,6 +2694,7 @@
             foffset = qualifiedname(tvb, ndps_tree, foffset);
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length==4)
             {
                 proto_tree_add_item(ndps_tree, hf_ndps_attribute_value, tvb, foffset, length, FALSE);
@@ -2840,6 +2850,7 @@
                 atree = proto_item_add_subtree(aitem, ett_ndps);
                 length = tvb_get_ntohl(tvb, foffset);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length==4)
                 {
                     proto_tree_add_item(atree, hf_ndps_attribute_value, tvb, foffset, length, FALSE);
@@ -3030,6 +3041,7 @@
             {
                 length = tvb_get_ntohl(tvb, foffset);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length!=0)
                 {
                     proto_tree_add_item(ndps_tree, hf_ndps_octet_string, tvb, foffset, length, FALSE);
@@ -3176,6 +3188,7 @@
                 atree = proto_item_add_subtree(aitem, ett_ndps);
                 length = tvb_get_ntohl(tvb, foffset);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length==4)
                 {
                     proto_tree_add_item(atree, hf_ndps_attribute_value, tvb, foffset, length, FALSE);
@@ -3198,6 +3211,7 @@
                 atree = proto_item_add_subtree(aitem, ett_ndps);
                 length = tvb_get_ntohl(tvb, foffset);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length==4)
                 {
                     proto_tree_add_item(atree, hf_ndps_attribute_value, tvb, foffset, length, FALSE);
@@ -3254,6 +3268,7 @@
             foffset += 4;
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length!=0)
             {
                 proto_tree_add_item(ndps_tree, hf_ndps_add_bytes, tvb, foffset, 4, FALSE);
@@ -3426,6 +3441,7 @@
         case 106:         /* Octet String Pair */
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length!=0)
             {
                 tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -3435,6 +3451,7 @@
             foffset += (length%2);
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length!=0)
             {
                 tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -3448,6 +3465,7 @@
         case 107:         /* Octet String Integer Pair */
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length!=0)
             {
                 tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -3468,6 +3486,7 @@
             foffset = qualifiedname(tvb, ndps_tree, foffset);
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length!=0)
             {
                 tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -3491,6 +3510,7 @@
                 case 3:     /*OCTET_STRING*/
                     length = tvb_get_ntohl(tvb, foffset);
                     foffset += 4;
+                    DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                     if (length!=0)
                     {
                         tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -4304,6 +4324,8 @@
                         proto_tree_add_uint(btree, hf_ndps_included_doc_len, tvb, foffset, 4, length);
                         foffset += 4;
                         length_remaining = tvb_length_remaining(tvb, foffset);
+/*should we have the macro here instead of the explicit code?*/
+                        DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                         if (length_remaining == -1 || length > (guint32) length_remaining) /* Segmented Data */
                         {
                             proto_tree_add_item(btree, hf_ndps_data, tvb, foffset, -1, FALSE);
@@ -4530,6 +4552,7 @@
                 length = tvb_get_ntohl(tvb, foffset);
                 proto_tree_add_uint(ndps_tree, hf_ndps_context_len, tvb, foffset, 4, length);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length!=0)
                 {
                     tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -5082,11 +5105,12 @@
                 atree = proto_item_add_subtree(aitem, ett_ndps);
                 length = tvb_get_ntohl(tvb, foffset);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length==4)
                 {
                     proto_tree_add_item(atree, hf_ndps_attribute_value, tvb, foffset, length, FALSE);
                 }
-                tvb_ensure_bytes_exist(tvb, foffset, length);
+/*redundant*/   tvb_ensure_bytes_exist(tvb, foffset, length);
                 foffset += length;
                 proto_item_set_end(aitem, tvb, foffset);
             }
@@ -5171,6 +5195,7 @@
                 length = tvb_get_ntohl(tvb, foffset);
                 proto_tree_add_uint(ndps_tree, hf_ndps_context_len, tvb, foffset, 4, length);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length!=0)
                 {
                     tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -5218,6 +5243,7 @@
         case 0x00000022:    /* Map GUID to NDS Name */
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length!=0)
             {
                 tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -5349,6 +5375,7 @@
                 length = tvb_get_ntohl(tvb, foffset);
                 proto_tree_add_uint(ndps_tree, hf_ndps_context_len, tvb, foffset, 4, length);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length!=0)
                 {
                     tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -5413,6 +5440,7 @@
 	        }
                 length = tvb_get_ntohl(tvb, foffset);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length!=0)
                 {
                     tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -5535,6 +5563,7 @@
                 length = tvb_get_ntohl(tvb, foffset);
                 proto_tree_add_uint(ndps_tree, hf_ndps_context_len, tvb, foffset, 4, length);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length!=0)
                 {
                     tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -5623,6 +5652,7 @@
             proto_item_set_end(aitem, tvb, foffset);
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length==4)
             {
                 proto_tree_add_item(ndps_tree, hf_ndps_attribute_value, tvb, foffset, length, FALSE);
@@ -5676,11 +5706,12 @@
                 atree = proto_item_add_subtree(aitem, ett_ndps);
                 length = tvb_get_ntohl(tvb, foffset);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length==4)
                 {
                     proto_tree_add_item(atree, hf_ndps_attribute_value, tvb, foffset, length, FALSE);
                 }
-                tvb_ensure_bytes_exist(tvb, foffset, length);
+/*redundant*/   tvb_ensure_bytes_exist(tvb, foffset, length);
                 foffset += length;
                 proto_item_set_end(aitem, tvb, foffset);
             }
@@ -5765,6 +5796,7 @@
                 length = tvb_get_ntohl(tvb, foffset);
                 proto_tree_add_uint(ndps_tree, hf_ndps_context_len, tvb, foffset, 4, length);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length!=0)
                 {
                     tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -5965,6 +5997,7 @@
                 length = tvb_get_ntohl(tvb, foffset);
                 proto_tree_add_uint(ndps_tree, hf_ndps_context_len, tvb, foffset, 4, length);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length!=0)
                 {
                     tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -6046,6 +6079,7 @@
                 atree = proto_item_add_subtree(aitem, ett_ndps);
                 length=tvb_get_ntohl(tvb, foffset);
                 length_remaining = tvb_length_remaining(tvb, foffset);
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if(length_remaining == -1 || (guint32) length_remaining < length)
                 {
                     return;
@@ -6209,6 +6243,7 @@
                 btree = proto_item_add_subtree(bitem, ett_ndps);
                 length = tvb_get_ntohl(tvb, foffset);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length==4)
                 {
                     proto_tree_add_item(btree, hf_ndps_attribute_value, tvb, foffset, length, FALSE);
@@ -6270,6 +6305,7 @@
                 btree = proto_item_add_subtree(bitem, ett_ndps);
                 length = tvb_get_ntohl(tvb, foffset);
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length==4)
                 {
                     proto_tree_add_item(btree, hf_ndps_attribute_value, tvb, foffset, length, FALSE);
@@ -6753,6 +6789,7 @@
                 btree = proto_item_add_subtree(bitem, ett_ndps);
                 length=tvb_get_ntohl(tvb, foffset);
                 length_remaining = tvb_length_remaining(tvb, foffset);
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if(length_remaining == -1 || (guint32) length_remaining < length)
                 {
                     return;
@@ -7001,6 +7038,7 @@
         case 0x0000001d:    /* List Event Profiles */
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length==4)
             {
                 proto_tree_add_item(ndps_tree, hf_ndps_attribute_value, tvb, foffset, length, FALSE);
@@ -7051,6 +7089,7 @@
             /* End of Eventhandling */
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length!=0)
             {
                 tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -7165,6 +7204,7 @@
                 /* End of Eventhandling2 */
                 length = tvb_get_ntohl(tvb, foffset); /* Added on 10-17-03 */
                 foffset += 4;
+                DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
                 if (length!=0)
                 {
                     tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -7277,6 +7317,7 @@
             }
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length!=0)
             {
                 tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -7303,6 +7344,7 @@
             }
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length!=0)
             {
                 tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -7395,6 +7437,7 @@
             proto_item_set_end(aitem, tvb, foffset);
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length==4)
             {
                 proto_tree_add_item(ndps_tree, hf_ndps_attribute_value, tvb, foffset, length, FALSE);
@@ -7430,6 +7473,7 @@
             /* End of ProfileResultSet */
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length!=0)
             {
                 tvb_ensure_bytes_exist(tvb, foffset, length);
@@ -7442,6 +7486,7 @@
             /* Start of IntegerSeq */
             length = tvb_get_ntohl(tvb, foffset);
             foffset += 4;
+            DISSECTOR_ASSERT(length<=tvb_reported_length_remaining(tvb, foffset));
             if (length==4)
             {
                 proto_tree_add_item(ndps_tree, hf_ndps_language_id, tvb, foffset, length, FALSE);