[PATCH] Re: Possible Vulnerability in httpd.c

[Rich Felker]

On Tue, Nov 22, 2016 at 02:30:50AM +0100, Denys Vlasenko wrote:
> On Mon, Nov 21, 2016 at 8:18 PM, Simon Rettberg
> <simon.rettberg at rz.uni-freiburg.de> wrote:
> > On Mon, 21 Nov 2016 20:37:14 +0200
> >  Timo Teras <timo.teras at iki.fi> wrote:
> >>
> >> It is still good practice to fill it with snprintf. If this is done,
> >> proper error checking should be done to check the final 'len' that it
> >> does not exceed IOBUF_SIZE or you have information leak bug (since
> >> snprintf returns the length it would generate if buffer was unbounded;
> >> not the length it actually wrote to the buffer).
> >>
> > Exactly. It typically goes like this: Someone is using functions generally
> > considered "unsafe" because they know for sure it is not exploitable the way
> > it's being used in this specific instance. Then eventually someone else comes
> > along, adds feature X using unsafe functions as well (you try to do it the same
> > way the rest is written, right?), and boom, you suddenly got your exploit
> > because X happens to enable the remote user to inject arbitrarily long data
> > (think some %s the user can control).
> 
> Yes, I am aware this happens.
> 
> > It should be fixed properly, handling the case where the return value is either
> >> BUFSIZ or even < 0.
> 
> If you are afraid that there can be bugs of the "we thought it can't overflow,
> but it in fact can", then why you aren't afraid that length checking code
> wouldn't have bugs?

The latter is more obvious and easier for people not familiar with the
code to check. The former relies on a lot of hidden assumptions
including the value of BUFSIZ and the sizes of types.

> Different projects choose their paranoias differently.
> >From its inception, bbox was paranoid about code size.
> 
> If you see an actual bug where buffer can overflow,
> I'm more than willing to fix it.
> 
> But if there is no actual bug, and it's just a general concern
> that "it looks unsafe", then code size trumps it.

Have you stopped to consider the size from pulling in the deprecated
sprintf function to begin with? If all references to it were removed,
then static-linked busybox would only have snprintf, not sprintf. On
musl/i386 this would only save about 50 bytes but it might save more
on other archs or libcs.

Rich