Ethereal-dev: Re: Patch in reassemble.c (was Re: [Ethereal-dev] Crash in ethereal 0.10.8, some

Note: This archive is from the project's previous web site, ethereal.com. This list is no longer active.

From: Guy Harris <gharris@xxxxxxxxx>
Date: Wed, 13 Jul 2005 00:14:05 -0700
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.