On 06/03/2013 05:15 PM, Gerald Combs wrote:
On 5/31/13 10:33 AM, Jeff Morriss wrote:
On 05/31/13 12:59, Jakub Zawadzki wrote:
Hi,
On Thu, May 30, 2013 at 10:23:05PM -0400, Evan Huus wrote:
On Thu, May 30, 2013 at 10:15 PM, Evan Huus <eapache@xxxxxxxxx> 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!
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...
Evan
P.S. I don't know when we're expecting 1.10 final but I think delaying
it (if necessary) in order to include this fix is fully justified.
Has anyone performed some benchmarks before/after this patch? :)
It appears to have a fairly negligible effect. Here's a comparison
using tools/test-captures.sh (on a pool of files):
before:
real 5m41.759s
user 5m21.718s
sys 0m13.510s
after:
real 5m39.395s
user 5m23.165s
sys 0m13.639s
(That's about a 0.46% increase.)
And here's one focusing just on the "no tree" case:
before:
real 1m19.675s
user 1m6.997s
sys 0m10.238s
after:
real 1m20.773s
user 1m7.913s
sys 0m10.387s
(That's about a 1.36% increase.)
It's been fuzzing all weekend without any failures, so at the risk of
inviting disaster I added r49644 & r49652. I'm hoping to release 1.10.0
on Wednesday (June 5th) and 1.8.8 + 1.6.16 on Friday (June 7). The 1.6
branch reaches EOL on Friday as well.
It would probably make sense to bring 49645 over too then: we have proof
that it is needed (with r49644) to fix some fuzz failures:
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=3290#c16
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=8728
