Guy Harris wrote:
>
> > Please, please in new dissector routines, check for
> > truncated packets, especially when string operations
> > or loop on bytes are used (to avoid display of erroneous
> > data and infinite loop or segmentation violation) !
>
> Perhaps we should make it easier to do that check, e.g. by centralizing
> it?
>
> For example, if we had "proto_tree_add_...()" routines that took, as
> arguments, a starting offset and a length, but *didn't* take the actual
> value as an argument, but also took, say, a pointer to the packet data,
> and extracted the value *themselves* (and, in the case of
> variable-length data types, e.g. '\0'-terminated strings, perhaps didn't
> take the length as an argument, or took a maximum length as an argument,
> and somehow returned the new offset), we could have those routines to
> the checking.
>
> Either they could return a value saying "oops, that one ran past the end
> of the packet", or could do a "longjmp()" that jumped back to the
> top-level dissector.
>
> There are places in dissectors where the dissector itself needs to know
> the value, e.g. packet type fields, and the dissector itself would have
> to do the check itself (although we might be able to provide macros or
> routines to extract a value and do said check), but, in a lot of cases,
> the dissector doesn't need to know the value to decide what to do next,
> so the value could be extracted by the "proto_tree_add...()" routine.
Yes, this should be a good idea.
But in many situations, the check is trivial. E.g. :
if (!BYTES_ARE_IN_FRAME(offset, sizeof(header_to_decode))) {
dissect_data(pd, offset, fd, tree);
return;
}
There is no need to add such a test for each field since testing
the presence of the whole header seems to be sufficient (except if we
want to decode the fields at the beginning of an incomplete header) ?
But there are some cases where the check is not so trivial (decoding of
non '\0' terminated strings or of fields which size depends on a previous
decoded field or worst on the data itself for instance), and for such cases,
having the lower level decoding routines checking for the correctness of the
data is not sufficient and maybe not convenient enough). Moreover, if such
routines extract the value themselves, we will have to pass the endianness
and maybe some other parameters, this may complicate a bit some decoding of
non trivial protocol headers.
A little related point: if each packet was ended by a '\0' (non counted in
the captured or packet length), this could made work some broken decoding
routines that manipulate strings ;-)
Laurent.
--
Laurent DENIEL | E-mail: deniel@xxxxxxxxxxx
Paris, FRANCE | laurent.deniel@xxxxxxxxxxxxxxxxxxxxxxxxxxxx
| WWW : http://www.worldnet.fr/~deniel
All above opinions are personal, unless stated otherwise.
