Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 49644: /trunk/epan/ /trunk/epan/: pr
From: Jeff Morriss <[email protected]>
Date: Mon, 03 Jun 2013 20:06:34 -0400
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:

On Thu, May 30, 2013 at 10:23:05PM -0400, Evan Huus wrote:
On Thu, May 30, 2013 at 10:15 PM, Evan Huus <[email protected]> wrote:
On Thu, May 30, 2013 at 9:46 PM,  <[email protected]> wrote:

User: morriss
Date: 2013/05/30 06:46 PM

   (Finally!) check in part of Didier's patch to fix
   (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...
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):

real    5m41.759s
user    5m21.718s
sys     0m13.510s

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:

real    1m19.675s
user    1m6.997s
sys    0m10.238s

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: