Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 49644: /trunk/epan/ /trunk/epan/: pr
From: Gerald Combs <[email protected]>
Date: Mon, 03 Jun 2013 14:15:03 -0700
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 <[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.)

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.