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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/06/13 19:27, Ian Jackson wrote:
> From: Matthew Daley <mattjd@gmail.com>
>
> This is part of the fix to a security issue, XSA-55.
>
> Signed-off-by: Matthew Daley <mattjd@gmail.com>
>
> v6: This patch is new in v6 of the series.
> ---
>  tools/libxc/xc_dom_core.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/tools/libxc/xc_dom_core.c b/tools/libxc/xc_dom_core.c
> index 2a9c5a2..525b364 100644
> --- a/tools/libxc/xc_dom_core.c
> +++ b/tools/libxc/xc_dom_core.c
> @@ -278,6 +278,10 @@ size_t xc_dom_check_gzip(xc_interface *xch, void *blob, size_t ziplen)
>      unsigned char *gzlen;
>      size_t unziplen;
>  
> +    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@citrix.com>


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.

>     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.

>     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.

>     {
>          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.

>     return 0;
>     }
>
>     return unziplen + 16;

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

> }

~Andrew