Wireshark-bugs: [Wireshark-bugs] [Bug 3290] TRY_TO_FAKE_THIS_ITEM disables bounds errors
Comment # 22
on bug 3290
from Evan Huus
Just to provide a single, clean explanation of the problem for future
reference:
If we're running a dissection with a NULL tree (as is commonly the case) and a
proto_tree_add_* function is called with an offset+length combination that goes
beyond the end of the TVB, no exception is thrown because TRY_TO_FAKE_THIS_ITEM
returns NULL before a bounds check is ever done.
This causes problems because many dissectors contain code like the following:
guint32 loopCount, i;
loopCount = tvb_get_ntohl(tvb, offset);
for (i=0; i<loopCount; i++) {
proto_tree_add_item(tree, hf, tvb, offset, 1, ENC_NA);
offset++;
}
This code can potentially do 2^32 iterations (which is effectively infinite).
It relies on proto_tree_add_item throwing an exception if the the offset runs
past the end of the packet since that is guaranteed to happen quickly for
reasonable packet sizes. Right now we're not throwing that exception when tree
is NULL, leading to a number of infinite-loop bugs (see the many 'duplicates'
of this one).
The obvious solution is to do a bounds check in the proto_tree_add_* functions
-before- running TRY_TO_FAKE_THIS_ITEM, but this is not necessarily trivial. Of
particular note are FT_UINT_STRING and FT_UINT_BYTES, since their length values
are retrieved from the packet, not specified by the dissector.
Didier provided a few patches, but I suspect that they no longer apply cleanly
(if at all) to trunk.
Jeff, you seem to have been most involved with this one. Do you have a better
idea what will be involved in fixing it? I don't have the time at the moment,
but if there's a clear path layed out hopefully somebody will grab it. It would
be great to have this done for 1.10.
You are receiving this mail because:
- You are the assignee for the bug.
- You are watching all bug changes.
