Wireshark · Ethereal-dev: Re: [Ethereal-dev] Crash in ethereal 0.10.8, somewhat reproducible

[Peter Johansson <Peter.Johansson@xxxxxxxxxxxx>]

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