Libguestfs - Feb 2017 - Re: [PATCH v2 1/2] lib: change how hbin sections are read.

OK, I ended up turning the warning off.  It appears from the
info file that the warning is about GCC not being able to make
an optimization, not a bug in the code.

However I do have a more substantial problem with the patch.
By checking the offset against h->endpages, we're using an untrusted
field supplied to us by the hive, which means that a crafted hive
could cause us to walk through memory past the end of the file --
a security issue.

So I think the test should be using h->size with the additional
check for off >= h->endpages, as in the existing outer loop.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Wed, Feb 15, 2017 at 10:59:33PM +0000, Richard W.M. Jones
wrote:
> 
> OK, I ended up turning the warning off.  It appears from the
> info file that the warning is about GCC not being able to make
> an optimization, not a bug in the code.
> 
> However I do have a more substantial problem with the patch.
> By checking the offset against h->endpages, we're using an untrusted
> field supplied to us by the hive, which means that a crafted hive
> could cause us to walk through memory past the end of the file --
> a security issue.
> 
> So I think the test should be using h->size with the additional
> check for off >= h->endpages, as in the existing outer loop.
Also if we're going to start using heuristics to deal with broken
hives, we should prevent writing when this happens.  So check the
write flag and give an error in that case (or have another flag to
indicate that the caller wants heuristics).

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Thu, 2017-02-16 at 08:43 +0000, Richard W.M. Jones
wrote:
> On Wed, Feb 15, 2017 at 10:59:33PM +0000, Richard W.M. Jones wrote:
> > 
> > OK, I ended up turning the warning off.  It appears from the
> > info file that the warning is about GCC not being able to make
> > an optimization, not a bug in the code.
> > 
> > However I do have a more substantial problem with the patch.
> > By checking the offset against h->endpages, we're using an
> > untrusted
> > field supplied to us by the hive, which means that a crafted hive
> > could cause us to walk through memory past the end of the file --
> > a security issue.
> > 
> > So I think the test should be using h->size with the additional
> > check for off >= h->endpages, as in the existing outer loop.
> 
> Also if we're going to start using heuristics to deal with broken
> hives, we should prevent writing when this happens.  So check the
> write flag and give an error in that case (or have another flag to
> indicate that the caller wants heuristics).
> 
> Rich.
> 
In this case, I'd opt for a new flag because in our use case we still
might want to modify such hives - we do something similar to v2v on
backup images.

Dawid