Enabling the openwire protocol is leaking memory and crashes wireshark in few seconds with out of memory message

Murali said:

Created attachment 11738

Capture file

Build Information:

Tried all the builds from 1.8.7 to latest SVN build

added crash oswindows uigtk version1.10 labels

Evan Huus said:

Looks like the heart of the loop is somewhere in:

dissect_openwire_type (packet-openwire.c:684)
dissect_openwire_type (packet-openwire.c:945)
dissect_openwire_type (packet-openwire.c:887)
dissect_openwire_type (packet-openwire.c:1068)
dissect_openwire_command (packet-openwire.c:1250)
dissect_openwire (packet-openwire.c:1357)
tcp_dissect_pdus (packet-tcp.c:2237)
dissect_openwire_tcp (packet-openwire.c:1378)

which allocates memory in a wmem strbuf.

Out of time to dig any deeper for now.

Evan Huus said:

Memory usage reduced in r52457, exposing the underlying near-infinite for loop at line 935.

Evan Huus said:

Quick hacked fix in r52458. Leaving the ticket open in the hopes of a better solution and for backporting.

Michael Mann said:

Dissector isn't that old, so perhaps the original author can comment. Adding to CC

Michael Mann said:

Created attachment 11741 Bugfix loop

I think the "loop break" should be a little higher than what was done in r52458 because if a "type" is unrecognized the rest of the tvb is "claimed", and tvb_length_remaining should be 0 in the "map loop".

Also found a few places where offset wasn't properly incremented, which should prevent some of the near-infinite loops.

While the capture in this bug wasn't a fuzz produced file, it does seem like there is a "field mismatch" if the "expected" map length is significantly larger than the packet size.

Evan Huus said:

Comment on attachment 11741 Bugfix loop

This patch looks good to me. Agree that there's probably an underlying field mismatch though.

Michael Mann said:

(In reply to comment #6)  
> Comment on attachment 11741 [details]  

Bugfix loop

This patch looks good to
> me. Agree that there's probably an underlying field mismatch though.  


Good enough to backport and mark the issue closed?  I know the unincremented offset stuff needs to be fixed, but the other part was just a "cleaner" version of your hack (in fact, I've cleaned it up some more (locally), but it doesn't change the functionality of "aborting" when there's no data left in the tvb)

Evan Huus said:

(In reply to comment #7)  
> Good enough to backport and mark the issue closed?  I know the unincremented  
> offset stuff needs to be fixed, but the other part was just a "cleaner"  
> version of your hack (in fact, I've cleaned it up some more (locally), but  
> it doesn't change the functionality of "aborting" when there's no data left  
> in the tvb)  

Ideally I think we wouldn't have to check at all, and just trust that we eventually run off the end of the TVB and throw an exception if the loop runs too long. I don't know if your fixes to offset incrementation make that possible or not.

I'm coming to the conclusion that 99% of the length_remaining checks are unnecessary, probably the original author thought it was unsafe to run past the end of the TVB.

Whether this stays open depends on how much time you want to spend on it, and whether the original author can provide any more guidance. Your current patch is good enough for the amount of time I have left this week :)

Michael Mann said:

(In reply to comment #8)  

> Ideally I think we  
> wouldn't have to check at all, and just trust that we eventually run off the  
> end of the TVB and throw an exception if the loop runs too long. I don't  
> know if your fixes to offset incrementation make that possible or not.  

I tried adding just the offset incrementation and removing your length_remaining check and it was still stuck in the loop (per "map loop" problem mentioned in comment #5)

> I'm  
> coming to the conclusion that 99% of the length_remaining checks are  
> unnecessary, probably the original author thought it was unsafe to run past  
> the end of the TVB.  

I believe the length_remaining checks prevent one "bogus" length value from not screwing up the entire packet.  I've been focusing on packet 36 (seems to be first instance of near-infinite loop), and with the length_remaining checks removed, "good" dissection would cease (too early IMO) when the that loop is hit because a bounds check would be thrown.

With the recursiveness of dissect_openwire_type(), the key may be finding the 1% of length_remaining checks that really are necessary.

> Whether this stays open depends on how much time you  
> want to spend on it, and whether the original author can provide any more  
> guidance. Your current patch is good enough for the amount of time I have  
> left this week :)  

Committed a fix to r52463 and scheduled for backporting.  Closing because "cleaning up dissector" shouldn't really be part of this bug.

Evan Huus said:

*** Bug #9259 (closed) has been marked as a duplicate of this bug. ***

Evan Huus said:

Backported my quick-fix to 1.8 and 1.10 both to pacify their respective bots and because your patch doesn't apply cleanly without mine already there.

closed

mentioned in issue #9259 (closed)

Enabling the openwire protocol is leaking memory and crashes wireshark in few seconds with out of memory message

Child items 0

Activity