Re: [Xen-devel] [PATCH 22/22] libxc: check blob size before proceeding in xc_dom_check_gzip

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Andrew Cooper writes ("Re: [PATCH 22/22] libxc: check blob size before 
proceeding in xc_dom_check_gzip"):
> On 07/06/13 19:27, Ian Jackson wrote:
> > +    if ( ziplen < 6 )
> > +        /* too small */
> > +        return 0;
> > +
> >      if ( strncmp(blob, "\037\213", 2) )
> >          /* not gzipped */
> >          return 0;
> 
> The above change is certainly good and correct.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Thanks.  I'll leave off your Reviewed-by in my copy of the series
until we've resolved these comments.

> I do however have concerns about the rest of the function, immediately
> after the context.  I spot any other specific security related issues,
> but would appreciate comments/thoughts as to whether these should be
> part of this patch or part of a subsequent set of bugfix patches.

I think it would be best to do all needed fixes to xc_dom_check_gzip
(and perhaps related functions) in one patch, if they're not intricate.

> >     gzlen = blob + ziplen - 4;
> 
> Arithmatic on a void pointer (blob), and a possibility to overflow,
> although this seems unlikely given that it would have to pass
> xc_dom_malloc_filemap() in the first place.  There is a GNU extention
> which allows this line to Do The Right Thing, despite violating the C spec.

This code already relies on the GNU extension to do arithmetic on a
void pointer.  This is defined to do the same thing as arithmetic on a
char*, and is safe under the same conditions as the char* arithmetic.

In this case (it's left associative) this expression computes
   (blob + ziplen) - 4
Now, blob + ziplen is safe because that's the input image size.  Ie,
it's a pointer to the end of the image, which one is permitted to
compute.

Since ziplen >= 6 (because of the earlier test) it is then safe to
retreat by 4 bytes.
   
> >     unziplen = gzlen[3] << 24 | gzlen[2] << 16 | gzlen[1] << 8 | gzlen[0];
> >     if ( (unziplen < 0) || (unziplen > XC_DOM_DECOMPRESS_MAX) )
> 
> unziplen is unsigned, so (unziplen < 0) is necessarily false.

Yes, but I don't intend to change this, obviously.

> >     {
> >          xc_dom_printf
> >             (xch,
> >              "%s: size (zip %zd, unzip %zd) looks insane, skip gunzip",
> >              __FUNCTION__, ziplen, unziplen);
> 
> ziplen and unziplen are both unsigned, so the format string should be
> %zu instead.

xc_dom_printf uses stdarg to get the arguments.  Given %zd it will use
va_arg(,int).  The requirement for va_arg is that the actual promoted
argument type and the specified type are "compatible" (C99 7.15.1.1
para 2).  `int' and `unsigned' are indeed not compatible.  So this has
undefined behaviour.

However I do not propose to try to audit our entire codebase for calls
to print a signed integer with %x (which has, technically, undefined
behaviour if the value is negative).  Instead, I propose that we
declare that we require of the C implementation that it tolerate use
of va_arg where the actual and supplied types are not compatible but
do pass the strict aliasing rules.

As a justification for this I observe that gcc's format string warning
system tolerates passing wrong-signedness arguments to printf %d etc.

> While XC_DOM_DECOMPRESS_MAX is set to a default of 1GB, if it is
> changes, unziplen + 16 can overflow.

This should be dealt with by a comment near its definition.  TBH IMO
if anyone defines it to >= nearly 4G  they deserve to lose.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel