Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 49644: /trunk/epan/ /trunk/epan/: pr
From: Jeff Morriss <[email protected]>
Date: Thu, 30 May 2013 22:40:35 -0400
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. :-)
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).