Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 49644: /trunk/epan/ /trunk/epan/: pr
From: Jeff Morriss <[email protected]>
Date: Fri, 31 May 2013 13:33:46 -0400
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 <[email protected]> wrote:
On Thu, May 30, 2013 at 9:46 PM,  <[email protected]> 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.)