I tried to reproduce this:


martin@reykholt:~/src/wireshark.git$ ./tools/valgrind-wireshark.sh -T ~/Downloads/asan_heap-uaf_b2c1db_1289_1d9906979621bf069e580bed996d9ff5.pcap 

the tvb contains 324 captured bytes

** (process:3237): WARNING **: seq 0, frag num 3584, offs beg 256, len 129, more 0                                                     
                        
fd_head is created, data for this fragment is unavailable, no more fragments                                                           
-> ignore this fragment and remove all other fragments of this frame from the fragment table (there are none)                          
                        
reassembly returns NULL                                                                                                                
        
* (process:3237): WARNING **: seq 0, frag num 0, offs beg 256, len 131, more 220                                                       
        
fd_head is created, data for this fragment is unavailable, fragment number 0, there are more fragments                                 
-> treat as unfragmented, don't add the fragment to the fragment table

reassembly returns NULL                                                                                                                

* (process:3237): WARNING **: seq 0, frag num 0, offs beg 256, len 65, more 0                                                          

fd_head already exists, we have the fragment's data in the tvb, no more fragments                                                      
-> do reassembly with the previous fragment, create a new data source of 65 bytes                                                      

* (process:3237): WARNING **: seq 0, frag num 0, offs beg 256, len 256, more 158                                                       

fd_head already exists, data for this fragment is unavailable, fragment number 0, there are more fragments                             
-> treat as unfragmented, don't add the fragment to the fragment table                                                                 
                
reassembly returns NULL 
                
* (process:3237): WARNING **: seq 0, frag num 256, offs beg 256, len 129, more 0                                                       
                
fd_head already exists, data for this fragment is unavailable, fragment number != 0, no more fragments                                 
-> ignore this fragment and remove all other fragments of this frame from the fragment table                                           
                
this removes the tvb for the data source of 65 bytes that we just created                                                              
                        
                
we have two data sources        
   entire tvb == 324 bytes
   reassembled frame == 65 bytes
      -> its tvb was destroyed, the out-of-bounds access happens when its hex dump is displayed                                        

                                
I'm not sure how to fix this properly. The questionable code is in fragment_add_seq_common() where it says                             
                        
XXX I've copied this over from the old separate                                                                                        
fragment_add_seq_check_work, but I'm not convinced it's doing the                                                                      
right thing -- rav                                                                                                                     
                                                                                                                                       
For frag_number 0, no data present, we create an fd_head and don't add it to                                                           
the fragment table. That's why we do reassembly with a single fragment of 65                                                           
bytes.          

For frag_number != 0, !more_frags, we free all fragments of that frame even if                                                         
we've already done reassembly and created a new data source for them...

An overview of the fragments from that capture (X denotes the same fragment start):

#  number  len  more   fd_head(returned)
1  3584    129  false  NULL
2     0    131  true   X
3     0     65  false  X
4     0    256  true   X
5   256    129  false  NULL

Comments for the fragments (based on the fragment_add_seq documentation in epan/reassemble.h):

Fragment #1
- "first fragment seen for this datagram" -> create new fd_head
- not "the only fragment in the datagram" -> skip special case
- not "packet completes assembly" -> return NULL
- NOTE: Martin suggests that this is removes from the fragment table. The doc comment does not suggest that. It would make sense to do this though as no reassembly is ever possible and the fragments are presumably not needed?

Fragment #2
- "first fragment seen for this datagram" -> create new fd_head
- "packet completes assembly" -> return fd_head
- NOTE: should it really return fd_head since more_frags is set?

Fragment #3:
- This overwrites a previous fragment. Shouldn't this be disallowed?

Fragment #4:
- Like #3, this overwrites a previous fragment. Shouldn't this be disallowed?

Fragment #5:
 - Since fragment #4 is (should be) dropped, shouldn't this fail as well (incomplete reassembly, like fragment #1)?

FYI: Since a fix for the vulnerability hasn't been submitted yet, the Google Project Zero 90-day disclosure deadline expires in three days (on 2016-02-25). 

The corresponding bug at https://code.google.com/p/google-security-research/issues/detail?id=651 in our tracker will be opened then, unless a patch is scheduled for release on a specific day within 14 days following the deadline (see http://googleprojectzero.blogspot.com/2015/02/feedback-and-data-driven-updates-to.html for more details).

In my previous analysis I missed that the tvb is not large enough. There is only 324 bytes in the tvb and with an offset of 256, there is only 68 bytes that can be added to the fragment. Anything larger than that triggers the special behavior (all but fragment 3).

Fragments 1 and 5 have a non-zero fragment block index and are the last fragments in their list (more_frags is FALSE) so the special behavior kicks in:
 - The fragments list identified by (pinfo, id, data) is removed from the fragments table.
 - All remaining fragments are removed (but as the fragment_table hashtable has a value_destroy function that also calls free_all_fragments, I guess that this is a no-op?)

Fragments 2, 3 and 4 are added to the same fragments list as they have the same fragment id (0). When fragment 5 clears the fragments list (as described above), it also frees all tvbs that were returned before for fragments 2, 3 and 4. This matches Martin's explanation.

(In reply to Martin Kaiser from comment #1)
[..]
> I'm not sure how to fix this properly. The questionable code is in
> fragment_add_seq_common() where it says                             
>
>   XXX I've copied this over from the old separate
>   fragment_add_seq_check_work, but I'm not convinced it's doing the
>   right thing -- rav

I guess that rejecting fragments with insufficient data in the tvb is the best way to fix this. If the dissector tries to add a fragment where the tvb does not contain frag_data_len data, the reassembly would normally fail.

This special-coded case can be avoided if dissectors just behave and pass no more than tvb_captured_length(tvb). Returning the full fragment if the first fragment number is zero is quite an arbitrary check...

Possible patch at https://code.wireshark.org/review/14082. I also considered rejecting all fragments with short tvbs, but that can be too restrictive when a protocol exists which can send overlapping fragments (as a form of redundancy). Does such protocols actually exists...? If not, then an alternative (more risky) patch is at https://code.wireshark.org/review/14083

Friendly ping.

Fixed on master since c5b2c1e8f40cee913bd70fcc00284483b3c92fcd. master-2.0 is pending. I could not trigger a crash on 1.12 so I'll leave that branch alone.

This is the message I get for 1.12:

tshark: The file "asan_heap-uaf_b2c1db_2079_deefe798881104a81328b47698ab1d94.pcap" appears to be damaged or corrupt.
(pcap: File claims packet was 4203872580 bytes on the wire)

Change 14923 had a related patch set uploaded by Peter Wu:
reassemble: remove special treatment for truncated data

https://code.wireshark.org/review/14923

Change 14923 merged by Peter Wu:
reassemble: remove special treatment for truncated data

https://code.wireshark.org/review/14923

Thanks for the report with reproducers!

(pong)