Wireshark · Wireshark-dev: Re: [Wireshark-dev] TCP reassembly and Return value of a new-style dissector

[Peter Wu <peter@xxxxxxxxxxxxx>]

On Tuesday 09 December 2014 21:01:37 Anders Broman wrote:
> Hi,
> I have recently come across some problems with reassembly of SIP 
> messages over TCP. One problem seems to be related to when a segment 
> contains one full PDU and a segment of the next (following) PDU (in
> this case the first SIP line of the following PDU) is not complete.

(added punctuation for easier reading)

HTTP does not seem to have problems with it, probably because it tries
to consume as much of a PDU as possible, saving incomplete data for
later desegmentation. See the attached crafted packet for example which
contains three HTTP responses:

 1. PDU 1 contains one full HTTP response and the begin of the next.
 2. PDU 2 contains all the remainder of the second HTTP response, minus
    one last character.
 3. PDU 3 has the last character of the second request and the begin of
    the third request .
 4. PDU 4 contains the remainder of the third HTTP request.

'HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 1\r\n\r\n1'
'HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\n', 'Content-Length: 2\r\n\r\n1', '2'
'HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nC', 'ontent-Length: 3\r\n\r\n123'

> I think the ultimate solution would be for the TCP dissector to call the 
> SIP dissector again with the next incomplete PDU
> after receiving the number of bytes "accepted" by the SIP dissector e.g 
> using the "new-style dissector interface.
> also see http://seclists.org/wireshark/2014/Jun/289

It would indeed be nice if the dissector core could be improved to
handle this situation. As I mentioned before, the HTTP dissector has no
problems with this because it tries to dissect all data until more data
is needed.

This is from dissect_sip:

    remaining_length = tvb_reported_length(tvb);
    len = dissect_sip_common(tvb, 0, remaining_length, pinfo, tree, FALSE, FALSE);
    if (len < 0)
        return 0;   /* not SIP */
    else
        return len;

dissect_http instead always returns tvb_captured_length ("all data in
the PDU is part of the HTTP protocol") while returning 0 means "I know
for sure that this is not SIP".

dissect_sip_common returns -1 meaning "need more data", but then
dissect_sip return 0 meaning "no, not SIP, try a different dissector!".

> As I read the code the first step would be to have
> call_dissector()                                                [OK]
> try_conversation_dissector()
> dissector_try_heuristic()
> dissector_try_uint_new                                 [OK]
> 
> Return the number of bytes consumed, 0 or -1(need more data) not sure 
> about DESEGMENT_UNTIL_FIN (-2?).
> 
> If people agree the biggest change is to change 
> dissector_try_heuristic() to return an int.
> What do you think?

I would not particularly mind changing the return value, but what should
be the new semantics of the return value? I found that documentation is
quite lacking here. When I tried to make the core to actual handle
return values[1], many dissectors broke because they were not written
with the documented behavior (see also the mail you linked before).

Here is a document I wrote back then to clarify it for myself
https://git.lekensteyn.nl/peter/wireshark-notes/tree/doc/dissector.txt
-- 
Kind regards,
Peter
https://lekensteyn.nl

 [1]: https://git.lekensteyn.nl/peter/wireshark/commit/?h=reassembly-fixes&id=aef08cc2434b5ba5aee4422fbcf481004c62583a

Attachment: http-fragmented-nonempty.pcapng.gz
Description: application/gzip