Ethereal-dev: Re: [Ethereal-dev] Re: [Ethereal-cvs] rev 13924: /trunk/epan/dissectors/: packet
On Mar 27, 2005, at 0:23 Uhr, Guy Harris wrote:
Michael Tuexen wrote:
Have the main loop for dissecting chunks check that the chunk size
is
large enough for the chunk header, and have it pass the chunk size,
minus the size of the chunk header, to dissectors for particular
chunk
types. Make those dissectors check that value to make sure it's
large
enough for any fixed-length portion before subtracting the length of
that portion and using the result as a remaining data length.
Why?
So that the individual chunk dissectors don't each have to check
whether the chunk length is less than the chunk header size.
They weren't doing so - they were just (re-)fetching the chunk size
and subtracting the chunk header length from it, which, for corrupt
packets (such as the ones generated by the randpkt tests done by the
buildbot), means that the length might be *negative*.
The check to make sure the length is correct *HAS* to be done if
you're going to subtract something from the chunk length. There's not
much point in requiring each chunk dissector to make sure the chunk
length is >= CHUNK_HEADER_LENGTH - they'd *ALL* have to make that
check, and, if they have to make that check, we run the risk of
somebody adding a new chunk type dissector, if a new chunk type is
defined, and forgetting to make the check.
Hmm. I have no problems with the checks here, I already played some
time ago with
the idea to insert some macros to check the length in each dissector
but what I want
is to raise an exception, because I think people are used to the
'Malformed frame'
in case they are sending wrong packets. (I also want to check for too
long packets).
Or is this the wrong way? I did not figure out a good way to raise the
exception...
The SCTP dissector has seen a lot of bogus packets but always handled
them using the
exception. It did not crash... or made ethereal exiting...
The principle I used was
- if the chunk is too short I'll try sooner or later to access a
non-existing field
and an exception will be raised.
- if the chunk is too long, I'll ignore it ...
Why did it work? Or was it only luck?
The principle I based the dissector on was that if the tvb is too
short
I will call proto_tree_add_item() with an index out of bound. This
raised
the 'malformed frame' exception and that is fine.
Unfortunately, that wasn't helping here. Instead, negative lengths
were being handed to various proto_tree_ routines, which:
before Ulf's changes to use the DISSECTOR_ASSERT macros, caused
g_assert()s to fail, causing core dumps, which is not fine;
after Ulf's changes to use the DISSECTOR_ASSERT macros, caused
"Dissector bug" messages to be produced, which is not fine, especially
when those messages started to be caught by the randpkt tests and
mailed to ethereal-dev as buildbot crash messages.
I have never seen ethereal core dumping on bogus SCTP packets, it
displayed always
'Malformed frame'.
The old style was used with some thinking to handle the different
cases
of chunk length values and the length of the tvbs (which is not
always equal) correctly.
Unfortunately, it wasn't handling too-short chunk lengths correctly,
as per the above.
The common chunk dissector is *ONLY* checking against
CHUNK_HEADER_LENGTH. The individual chunk dissectors are checking
against the appropriate length for the type of chunk in question.
_______________________________________________
Ethereal-dev mailing list
Ethereal-dev@xxxxxxxxxxxx
http://www.ethereal.com/mailman/listinfo/ethereal-dev
