On 05/30/2013 10:15 PM, Evan Huus wrote:
On Thu, May 30, 2013 at 9:46 PM, <morriss@xxxxxxxxxxxxx> wrote:
http://anonsvn.wireshark.org/viewvc/viewvc.cgi?view=rev&revision=49644
User: morriss
Date: 2013/05/30 06:46 PM
Log:
(Finally!) check in part of Didier's patch to fix
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=3290
(TRY_TO_FAKE_THIS_ITEM disables bounds errors):
Before calling TRY_TO_FAKE_THIS_ITEM() check if the length given (or, in
the case of FT_UINT_{STRING,BYTES}, the length we retrieve from the TVB)
exceeds what's left in the TVB.
Do this only for proto_tree_add_item() for now (it's the most commonly used
and thus the biggest trouble maker in this area).
Similar changes for other APIs will come later (if nothing blows up). Despite
the fuzz failures this bug has caused I'm not sure about back-porting it...
Directory: /trunk/epan/
Changes Path Action
+28 -3 proto.c Modified
Thank you for this!
You're welcome. It was really just a question of:
0) Finally getting annoyed enough at the problem.
1) Biting the bullet and sitting down and staring at it for a while.
2) Finally realizing it doesn't have to all be fixed at once (greatly
reducing the time of #1).
3) Remembering (again) something Anders pointed out to me years ago at
Sharkfest: incremental improvements are better than no movement (and if
you start the movement, others may help you with the mess you've created
;-)). And we can always back it out if it blows up horribly. :-)
If we get through a round or two of fuzz-testing without any failures
I would really like to see this backported to every stable branch
(even 1.6). It closes an entire class of security vulnerabilities, and
while it is a fairly non-trivial behavioural change in a hot code
path, it is relatively short and clearly not doing anything too odd.
Fingers crossed for no unexpected side-effects...
Well, found one already (r49648)... Fortunately a minor one (in the
grand scheme).
