Re: [PATCH][BZ #14771] Fortify tweak for snprintf et al.

[Florian Weimer <fweimer at redhat dot com>]

On 10/20/2013 04:47 AM, Rich Felker wrote:
> On Fri, Oct 18, 2013 at 01:30:07PM +0200, Florian Weimer wrote:
> > +#ifndef _FORTIFY_H
> > +#define _FORTIFY_H 1
> > +
> > +#include <limits.h>
> > +
> > +__BEGIN_DECLS
> > +
> > +/* Check that USER length does not exceed the COMPILER length (which
> > +   should have been obtained through __builtin_object_size), and that
> > +   USER does not exceed INT_MAX minus some reserve.
> > +
> > +   Technically, a caller to functions like snprintf could specify
> > +   (size_t)-1 as the buffer length to get the behavior of sprintf
> > +   (without target buffer length checking), but we do not support this
> > +   use in fortify mode.  */
> > +static inline void
> > +__check_buffer_length_int_max (size_t user, size_t compiler)
> > +{
> > +  if (__builtin_expect (compiler < user || user >= INT_MAX - 1024, 0))
> > +    __chk_fail ();
> > +}
>
> POSIX already requires an error from snprintf if the size passed to
> snprintf is greater than INT_MAX, although this possibly conflicts
> with the requirements of ISO C (this needs to be addressed by the ISO
> C committee). If you believe there is any value in rejecting sizes
> above INT_MAX-1024, I think you need a strong argument for doing so.

See the patch from Exim I posted.

Note that this is for fortify mode only.  Both ISO C and POSIX require 
that you end up with sprintf behavior when you specify (size_t)-1 as the 
buffer length.  (Neither standard requires that the size argument is the 
length of the buffer.)  The fortify implementation already aborts in 
that case (even if the result would fit into the supplied buffer), so my 
proposal is less radical than you might think.

I'm open to tweaking the magic number, perhaps to ((size_t)-1)/2 - 1024 
or something like that.  But I do think that check makes sense, as 
exemplified by Exim.

--
Florian Weimer / Red Hat Product Security Team