Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 49644: /trunk/epan/ /trunk/epan/: pr
From: Evan Huus <[email protected]>
Date: Fri, 31 May 2013 09:14:30 -0400
On Thu, May 30, 2013 at 10:40 PM, Jeff Morriss
<[email protected]> wrote:
> On 05/30/2013 10:15 PM, Evan Huus 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!
>
>
> 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. :-)

Wise words. I'm looking forward to meeting everybody at Sharkfest this year :)

>> 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).

And one more that Jakub caught on code review which I fixed in r49652...

 ___________________________________________________________________________
> Sent via:    Wireshark-dev mailing list <[email protected]>
> Archives:    http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>             mailto:[email protected]?subject=unsubscribe