Re: [Wireshark-commits] rev 49644: /trunk/epan/ /trunk/epan/: proto.c

[Evan Huus <eapache () gmail com>]

On Fri, May 31, 2013 at 12:59 PM, Jakub Zawadzki
<darkjames-ws () darkjames pl> 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 () gmail com> wrote:
> > > On Thu, May 30, 2013 at 9:46 PM,  <morriss () wireshark org> 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? :)

I've been too afraid :P

However, security > performance. Protocols that want performance can
still manually put blocks of proto_tree_ calls under if (tree), they
just have to be sure they're not breaking anything when they do so.

Still worth backporting :)
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe