Wireshark · Ethereal-dev: Re: Patch in reassemble.c (was Re: [Ethereal-dev] Crash in ethereal 0.10.8, somewhat reproducible)

[Guy Harris <gharris@xxxxxxxxx>]

Peter Johansson wrote:
> Pilz Rene wrote:
>
> > Peter Johansson wrote:
> >
> > > ronnie sahlberg wrote:
> > >
> > > > ok   jag avvaktar ett tag.
> > > >
> > > > en idee,   det kan eventuellt vara fel i bittorrent dissectorn
> > > > eftersom det inte verkar vara nagra storre fel pa hur andra
> > > > dissectorer anvander reassemble.
> > > >
> > > > kan du kolla att bittorrent dissectorn ENDAST anropar reassembly OMM
> > > > packetet verkligen ar fragmenterat.  DVS om hela PDUn redan finns i
> > > > paketet och darfor ingen reassembly behovs,  da skall inte reassembly
> > > > rutinerna anropas.
> > > >
> > > >
> > > > reassembly rutinerna ar lite risiga,   de var nar jag skrev
> > > > reassemble.c  mest ett snabbt hack  men jag fick aldrig tid att skriva
> > > > om koden battre :-(
> > > >
> > > >
> > > > On Sat, 22 Jan 2005 15:41:22 +0100, Peter Johansson
> > > > <Peter.Johansson@xxxxxxxxxxxx> wrote:
> > > >  
> > > >
> > > > > ronnie sahlberg wrote:
> > > > >
> > > > >  
> > > > >
> > > > > > Hej,
> > > > > >
> > > > > > Har du en exempel capturefil du kan skicka till mig sa kan jag titta
> > > > > > pa vad som ar fel.
> > > > > >
> > > > > >
> > > > > >     
> > > > >
> > > > >
> > > > >
> > > > > Jag har två capturefiler, vardera om 20MB (jag körde capture till
> > > > > roterande filer). Om först laddar den ena och sedan den andra kan man
> > > > > reproducera krashen.
> > > > > Jag håller på och felsöker och tror jag hittat en workaround för
> > > > > krashen, tyvärr har jag ännu inte hittat den egentliga orsaken till
> > > > > problemet.
> > > > > Den info jag samlat på mig är:
> > > > > 1. Kraschen är ett faktum (förr eller senare) endast om den trafik
> > > > > ethereal avkodar innehåller Bittorrent data, det kan alltså vara ett
> > > > > problem med packet-bittorrent.c (har inte hunnit börja kika på den
> > > > > filen), se också punkt 4.
> > > > > 2. Kraschen i reassemble.c som ju sker i ett anrop till memcpy sker
> > > > > eftersom source (fd_i->data) inte verkar vara allokerat minne,
> > > > > fd_i->flags anger bla att biten FD_NOT_MALLOCED är satt.
> > > > > 3. Kraschen inträffar endast när ethereal avkodar insamlat data, dvs
> > > > > bara om man gör capture med "Update list of packets in realtime" 
> > > > > påslaget.
> > > > > 4. Kraschen inträffar endast om Bittorrent avkodaren är med i listan
> > > > > över "enabled protocols", därför tror jag att orsaken till kraschen
> > > > > egentligen ligger i packet-bittorent.c.
> > > > >
> > > > > Min workaround (som troligtvis ska permanentas även om felet 
> > > > > troligtvis
> > > > > ligger i packet-bittorrent.c) är att skriva om ett fåtal rader i
> > > > > reassemble.c (se den bifogade bilden), det garanterar att man inte kan
> > > > > krascha där för att man försöker hantera data som ej är allokerat. Så
> > > > > vitt jag har förstått bör konsekvensen bli att ethereal kanske inte
> > > > > avkodar just denna PDU på korrekt sätt. Man bör kanske därför lägga in
> > > > > någon kod som kan tala om för användaren att så är fallet (kanske en
> > > > > error popup där framenumret är angivet, etc). Därutöver ska 
> > > > > naturligtvis
> > > > > den egentliga orsaken till kraschen grävas fram.
> > > > >
> > > > > Vidare tror jag att reassemble.c bör skrivas om såsom den bifogade
> > > > > bilden visar. Dvs man ska aldrig anropa memcpy om src->flags eller
> > > > > destination->flags har biten FD_NOT_MALLOCED satt.
> > > > > Tyvärr rättar ju inte det orsaken till problemet, det ser bara till 
> > > > > att
> > > > > kraschen inte sker. Anledningen till att jag tidigare skrev att jag 
> > > > > inte
> > > > > visste vad jag skulle göra med problemet var att jag då inte kunde
> > > > > reproducera det på ett kontrollerat sätt, det kan jag nu.
> > > > > Är du fortfarande intresserad av filerna eller vill du avvakta?
> > > > >
> > > > > Mvh Peter
> > > > >   
> > > I have not forgot about this, I have just been a little bit more busy 
> > > than usual. I finally tracked down the problem though.
> > > My conclusion is this:
> > > Ethereal crashes in reassemble.c because reassemble.c copies data to 
> > > a memory area that is not yet allocated (fd_i->flags has the 
> > > FD_NOT_MALLOCED bit set). I have a solution to this (ensuring that a 
> > > crash does not occur) which I will post once I have done some 
> > > cleaning up.
> > >
> > > The crash in reassemble.c occurs only as the result of a faulty 
> > > protocol dissector. In this case it is packet-bittorrent.c that is 
> > > the reason for the crash.
> > > The Bittorrent dissector registers only a heur-dissector (which 
> > > should be fine). But once the heur test function detects that this 
> > > TCP stream is in fact Bittorrent data, it creates a conversation, 
> > > making sure that all future data in the same TCP stream is decoded by 
> > > the Bittorrent protocol dissector without the use of the heur test 
> > > function (this too should be fine I guess).
> > > The heur test function is capable of telling the calling framework 
> > > whether the PDU was in fact decoded by this dissector or not by 
> > > returning TRUE or FALSE
> > > packet-bittorrent.c. The function that dissects Bittorrent data based 
> > > on the fact that it belongs to a conversation does not have the 
> > > opportunity of telling the calling framework that it in fact cannot 
> > > decode the supplied PDU if necessary. And this is necessary in the 
> > > rare event that packet-tcp has marked the current PDU with "[TCP 
> > > previous segment lost]". In this case some data is missing but the 
> > > Bittorrent dissector still assumes that the first 4 bytes of the PDU 
> > > denotes the length of the PDU to be dissected. The problem now is 
> > > that since data was lost, the length is read using a random offset 
> > > into the original Bittorrent packet (since some data was lost).
> > > My guess is that this could happen for any dissector that is called 
> > > since the data belongs to a conversation created by the specific 
> > > dissector when data has been lost.
> > >
> > > Should packet-tcp perhaps not call higher level dissector when the 
> > > PDU is marked with "[TCP previous segment lost]" or at least not 
> > > perform the try_conversation_dissector(...) call?
> > > What would be the better way of ensuring that this does not happen 
> > > with any of the already existing dissectors?
> > > Should perhaps the API at hand for dissectors be changed so that when 
> > > decoding PDU data, the dissector would be able to return TRUE or 
> > > FALSE in a similar way to the heur functions? This way, any dissector 
> > > would be able to tell the lower layer dissector that although it 
> > > should have handled this PDU, it could not.
> > >
> > > What is your opinion?
> > >
> > > / Peter
> > >
> > > _______________________________________________
> > > Ethereal-dev mailing list
> > > Ethereal-dev@ethereal.
> > > http://www.ethereal.com/mailman/listinfo/ethereal-dev

Unfortunately:

	1) this completely breaks the feature wherein a TCP dissector, handed a 
reassembled chunk of data, can indicate that it needs at least N more 
bytes of data to be added to the reassembled chunk, so that the 
reassembly has to be continued

and

	2) I don't understand Swedish so I can't easily tell what the technical 
discussion above says.

That feature is used by several dissectors, such as the HTTP dissector 
which, when it's reassembling the entity headers of an HTTP request or 
response, keeps requesting more data until it sees the blank line at the 
end of the entity headers, at which point it says the reassembly is 
complete.

That feature is now broken because:

	The "If it was already defragmented and this new fragment goes beyond 
data limits" loop at the top of "fragment_add_work()" "undoes" the 
reassembly by pointing fragments that no longer have data, because it 
was copied to the reassembled chunk and then freed, at the target of the 
copy in the reassembled chunk, and sets the FD_NOT_MALLOCED flag on 
those fragments.

	The "we have received an entire packet, defragment it and free all 
fragments" code in "fragment_add_work()" saves the pointer to the old 
reassembled chunk, allocates a new chunk to hold the reassembled data, 
and then falls into the "add all data fragments" loop.

	The "add all data fragments" loop in "fragment_add_work()" then used to 
copy *all* the fragments, regardless of whether FD_NOT_MALLOCED was set 
on the fragment or not, into the newly-allocated chunk.  It now copies 
only the chunks with FD_NOT_MALLOCED set, and reports the others as 
being "Reassemble error"s.

This means that, in the reassemblies after the first reassembly, some of 
the data in the reassembled chunk is whatever just happened to be there 
at the time of the allocation.

The old code *did* work correctly for some captures I have with HTTP 
traffic in them - FD_NOT_MALLOCED doesn't mean "fd_i->data isn't valid", 
it means "it's not the address of a mallocated chunk, it's an address 
*in* a mallocated chunk".

What are the details of the cases where the old code *didn't* work?

It might be that the correct fix is to, in the "we have received an 
entire packet..." code, set "fd_head->data" to "g_realloc(fd_head->data, 
max)", which means that the data that was already copied there during 
previous reassemblies will still be there.  However, we *still* need to 
get rid of the printout of the "Reassemble error" message, because it's 
bogus to print that message every time we, for example, reassemble HTTP 
entity headers - which means we should really figure out why we're doing 
that in cases where it *is* an error, and figure out where to fix that.

"tcp_dissect_pdus()" uses the "continue reassembly" feature - it first 
tries reassembling the fixed-length portion of the PDU, so that the "get 
the length" routine has enough of that portion to find out how large the 
packet is, and then tries reassembling the entire packet, so if the 
4-byte header of a presumed BitTorrent packet is split across TCP 
segments, that code path would be used.

One place where there's *definitely* a risk of problems is a presumed 
BitTorrent packet where the presumed length field is greater than 
2^32-5, so that when 4 is added to it we overflow and get a value *less* 
than 4.  However, going back to at least 0.10.8, if the get_pdu_len 
routine called by "tcp_dissect_pdus()" returns a value less than the 
length of the fixed-length portion of the PDU, that's assumed to be an 
overflow, so it just shows a "Malformed packet" error and quits.