Wireshark · Ethereal-dev: Re: [Ethereal-dev] Bug 72 (huge fragmentation offset)

[Gerald Combs <gerald@xxxxxxxxxxxx>]

Checked in.  Thanks.

Dinesh G Dutt wrote:
> I don't think this is a problem with reassemble.c. The frame in question
> was a bogus fragment. No attempt should have been made to reassemble it.
> I've added a check in packet-fc.c to deal with this. An additional
> "match_strval()" that could cause a crash has been replaced by a
> val_to_str() also.
> 
> Dinesh
> On Mon, 2005-04-18 at 17:09 -0500, Gerald Combs wrote:
> 
>>The capture referenced by bug 72
>>(http://bugs.ethereal.com/bugzilla/show_bug.cgi?id=72) triggers a
>>segmentation fault in the reassembly code, apparently due to packet-fc.c
>>passing a too-large offset value to fragment_add().  Should this be
>>fixed in packet-fc.c or reassemble.c?
>>
>>_______________________________________________
>>Ethereal-dev mailing list
>>Ethereal-dev@xxxxxxxxxxxx
>>http://www.ethereal.com/mailman/listinfo/ethereal-dev
>>
>>
>>------------------------------------------------------------------------
>>
>>Index: packet-fc.c
>>===================================================================
>>--- packet-fc.c	(revision 14131)
>>+++ packet-fc.c	(working copy)
>>@@ -361,7 +361,7 @@
>> 
>> static void fc_defragment_init(void)
>> {
>>-  fragment_table_init(&fc_fragment_table);
>>+  fragment_table_init (&fc_fragment_table);
>> }
>> 
>> 
>>@@ -838,7 +838,8 @@
>>     ftype = fc_get_ftype (fchdr.r_ctl, fchdr.type);
>>     
>>     if (check_col (pinfo->cinfo, COL_INFO)) {
>>-        col_add_str (pinfo->cinfo, COL_INFO, match_strval (ftype, fc_ftype_vals));
>>+         col_add_str (pinfo->cinfo, COL_INFO, val_to_str (ftype, fc_ftype_vals,
>>+                                                          "Unknown Type (0x%x)"));
>> 
>>         if (ftype == FC_FTYPE_LINKCTL)
>>             col_append_fstr (pinfo->cinfo, COL_INFO, ", %s",
>>@@ -1211,38 +1212,51 @@
>>         else {
>>             real_seqcnt = fchdr.seqcnt;
>>         }
>>+
>>+        /* Verify that this is a valid fragment */
>>+        if (is_lastframe_inseq && !is_1frame_inseq && !real_seqcnt) {
>>+             /* This is a frame that purports to be the last frame in a
>>+              * sequence, is not the first frame, but has a seqcnt that is
>>+              * 0. This is a bogus frame, don't attempt to reassemble it.
>>+              */
>>+             next_tvb = tvb_new_subset (tvb, next_offset, -1, -1);
>>+             if (check_col (pinfo->cinfo, COL_INFO)) {
>>+                  col_append_str (pinfo->cinfo, COL_INFO, " (Bogus Fragment)");
>>+             }
>>+        } else {
>>         
>>-        frag_id = ((pinfo->oxid << 16) ^ seq_id) | is_exchg_resp ;
>>+             frag_id = ((pinfo->oxid << 16) ^ seq_id) | is_exchg_resp ;
>> 
>>-        /* We assume that all frames are of the same max size */
>>-        fcfrag_head = fragment_add (tvb, FC_HEADER_SIZE, pinfo, frag_id,
>>-                                    fc_fragment_table,
>>-                                    real_seqcnt * fc_max_frame_size,
>>-                                    frag_size,
>>-                                    !is_lastframe_inseq);
>>-        
>>-        if (fcfrag_head) {
>>-            next_tvb = tvb_new_real_data (fcfrag_head->data,
>>-                                          fcfrag_head->datalen,
>>-                                          fcfrag_head->datalen);
>>-            tvb_set_child_real_data_tvbuff(tvb, next_tvb);
>>-            
>>-            /* Add the defragmented data to the data source list. */
>>+             /* We assume that all frames are of the same max size */
>>+             fcfrag_head = fragment_add (tvb, FC_HEADER_SIZE, pinfo, frag_id,
>>+                                         fc_fragment_table,
>>+                                         real_seqcnt * fc_max_frame_size,
>>+                                         frag_size,
>>+                                         !is_lastframe_inseq);
>>+             
>>+             if (fcfrag_head) {
>>+                  next_tvb = tvb_new_real_data (fcfrag_head->data,
>>+                                                fcfrag_head->datalen,
>>+                                                fcfrag_head->datalen);
>>+                  tvb_set_child_real_data_tvbuff(tvb, next_tvb);
>>+                  
>>+                  /* Add the defragmented data to the data source list. */
>>             add_new_data_source(pinfo, next_tvb, "Reassembled FC");
>>             
>>             if (tree) {
>>-                proto_tree_add_boolean_hidden (fc_tree, hf_fc_reassembled,
>>-                                               tvb, offset+9, 1, 1);
>>+                 proto_tree_add_boolean_hidden (fc_tree, hf_fc_reassembled,
>>+                                                tvb, offset+9, 1, 1);
>>             }
>>-        }
>>-        else {
>>-            if (tree) {
>>-                proto_tree_add_boolean_hidden (fc_tree, hf_fc_reassembled,
>>-                                               tvb, offset+9, 1, 0);
>>+             }
>>+             else {
>>+                  if (tree) {
>>+                       proto_tree_add_boolean_hidden (fc_tree, hf_fc_reassembled,
>>+                                                      tvb, offset+9, 1, 0);
>>             }
>>-            next_tvb = tvb_new_subset (tvb, next_offset, -1, -1);
>>-            call_dissector (data_handle, next_tvb, pinfo, tree);
>>-            return;
>>+                  next_tvb = tvb_new_subset (tvb, next_offset, -1, -1);
>>+                  call_dissector (data_handle, next_tvb, pinfo, tree);
>>+                  return;
>>+             }
>>         }
>>     } else {
>>         if (tree) {
>>
>>
>>------------------------------------------------------------------------
>>
>>_______________________________________________
>>Ethereal-dev mailing list
>>Ethereal-dev@xxxxxxxxxxxx
>>http://www.ethereal.com/mailman/listinfo/ethereal-dev