From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 22/22] libxc: check blob size before proceeding in xc_dom_check_gzip Date: Mon, 10 Jun 2013 21:10:30 +0100 Message-ID: <51B632B6.9040007@citrix.com> References: <1370629642-6990-1-git-send-email-ian.jackson@eu.citrix.com> <1370629642-6990-23-git-send-email-ian.jackson@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1370629642-6990-23-git-send-email-ian.jackson@eu.citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: "xen-devel@lists.xensource.com" , "mattjd@gmail.com" , "security@xen.org" List-Id: xen-devel@lists.xenproject.org On 07/06/13 19:27, Ian Jackson wrote: > From: Matthew Daley > > This is part of the fix to a security issue, XSA-55. > > Signed-off-by: Matthew Daley > > 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 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