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

On Wed, Feb 15, 2017 at 01:48:29PM -0500, Dawid Zamirski
wrote:
> On Wed, 2017-02-15 at 16:54 +0000, Richard W.M. Jones wrote:
> > On Tue, Feb 14, 2017 at 12:05:20PM -0500, Dawid Zamirski wrote:
> > > * hivex_open: when looping over hbin sections (aka pages), handle
a
> > >   case where following hbin section may not begin at exactly at
the
> > > end
> > >   of previous one. If this happens, scan the page section until
> > > next
> > >   one is found and validate it by checking declared offset with
> > > actual
> > >   one - if they match, all is good and we can safely move on.
> > > 
> > > Rationale: there are registry hives there is some garbage data
> > > between
> > > hbin section but the hive is still perfectly usable as long as
the
> > > offsets stated in hbin headers are correct.
> > > ---
> > >  lib/handle.c | 39 ++++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 34 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/lib/handle.c b/lib/handle.c
> > > index 4565d7d..e183ff2 100644
> > > --- a/lib/handle.c
> > > +++ b/lib/handle.c
> > > @@ -226,11 +226,30 @@ hivex_open (const char *filename, int
flags)
> > >          page->magic[1] != 'b' ||
> > >          page->magic[2] != 'i' ||
> > >          page->magic[3] != 'n') {
> > > -      SET_ERRNO (ENOTSUP,
> > > -                 "%s: trailing garbage at end of file
"
> > > -                 "(at 0x%zx, after %zu pages)",
> > > -                 filename, off, pages);
> > > -      goto error;
> > > +
> > > +      DEBUG (2,
> > > +             "page not found at expected offset 0x%zx,
"
> > > +             "seeking until one is found or EOF is
reached",
> > > +             off);
> > > +
> > > +      int found = 0;
> > > +      while (off < h->endpages) {
> > 
> > GCC 7 warns:
> > 
> > handle.c: In function 'hivex_open':
> > handle.c:236:13: error: missed loop optimization, the loop counter
> > may overflow [-Werror=unsafe-loop-optimizations]
> >        while (off < h->endpages) {
> >              ^
> > 
> > I suspect this means that GCC might try to turn this into an infinite
> > loop.
> > 
> > There are actually a few more of these in the existing code - I'm
> > going to push a patch to fix these in a minute.
> > 
> > Rich.
> > 
> 
> I've just sent out v3 that fix this warning - it was a valid complaint
> from GCC7 because off += 0x1000; in the loop body is not a good idea
> anyway and it should have been off++;
But surely headers can only ever be at 4K offsets from the start of
the file?  I wouldn't trust a random "hbin" somewhere in the
middle
since that might easily come from registry data.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Wed, 2017-02-15 at 18:52 +0000, Richard W.M. Jones
wrote:
> On Wed, Feb 15, 2017 at 01:48:29PM -0500, Dawid Zamirski wrote:
> > On Wed, 2017-02-15 at 16:54 +0000, Richard W.M. Jones wrote:
> > > On Tue, Feb 14, 2017 at 12:05:20PM -0500, Dawid Zamirski wrote:
> > > > * hivex_open: when looping over hbin sections (aka pages),
> > > > handle a
> > > >   case where following hbin section may not begin at exactly
at
> > > > the
> > > > end
> > > >   of previous one. If this happens, scan the page section
until
> > > > next
> > > >   one is found and validate it by checking declared offset
with
> > > > actual
> > > >   one - if they match, all is good and we can safely move
on.
> > > > 
> > > > Rationale: there are registry hives there is some garbage
data
> > > > between
> > > > hbin section but the hive is still perfectly usable as long
as
> > > > the
> > > > offsets stated in hbin headers are correct.
> > > > ---
> > > >  lib/handle.c | 39 ++++++++++++++++++++++++++++++++++-----
> > > >  1 file changed, 34 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/lib/handle.c b/lib/handle.c
> > > > index 4565d7d..e183ff2 100644
> > > > --- a/lib/handle.c
> > > > +++ b/lib/handle.c
> > > > @@ -226,11 +226,30 @@ hivex_open (const char *filename, int
> > > > flags)
> > > >          page->magic[1] != 'b' ||
> > > >          page->magic[2] != 'i' ||
> > > >          page->magic[3] != 'n') {
> > > > -      SET_ERRNO (ENOTSUP,
> > > > -                 "%s: trailing garbage at end of file
"
> > > > -                 "(at 0x%zx, after %zu pages)",
> > > > -                 filename, off, pages);
> > > > -      goto error;
> > > > +
> > > > +      DEBUG (2,
> > > > +             "page not found at expected offset 0x%zx,
"
> > > > +             "seeking until one is found or EOF is
reached",
> > > > +             off);
> > > > +
> > > > +      int found = 0;
> > > > +      while (off < h->endpages) {
> > > 
> > > GCC 7 warns:
> > > 
> > > handle.c: In function 'hivex_open':
> > > handle.c:236:13: error: missed loop optimization, the loop
> > > counter
> > > may overflow [-Werror=unsafe-loop-optimizations]
> > >        while (off < h->endpages) {
> > >              ^
> > > 
> > > I suspect this means that GCC might try to turn this into an
> > > infinite
> > > loop.
> > > 
> > > There are actually a few more of these in the existing code -
I'm
> > > going to push a patch to fix these in a minute.
> > > 
> > > Rich.
> > > 
> > 
> > I've just sent out v3 that fix this warning - it was a valid
> > complaint
> > from GCC7 because off += 0x1000; in the loop body is not a good
> > idea
> > anyway and it should have been off++;
> 
> But surely headers can only ever be at 4K offsets from the start of
> the file?  I wouldn't trust a random "hbin" somewhere in the
middle
> since that might easily come from registry data.
> 
> Rich.
> 
Correct, however there's also no guarantee that seeking by 4k in
"garbage" data would not land you in registry data that happens to
evaluate to "hbin" as well. That's why I put "hbin"
offset validation
check couple of lines below to make sure that the "hbin" we found by
searching is a proper one. The offset check I'm referring to is:

/* get "stated" hbin offset from header */
size_t page_offset = le32to(page->offset_first) + 0x1000;

/* if that does not match our current file offset,
   then exit with error */
if (page_offset != off) { 
  SET_ERRNO...
}

On Wed, Feb 15, 2017 at 02:28:41PM -0500, Dawid Zamirski
wrote:
> Correct, however there's also no guarantee that seeking by 4k in
> "garbage" data would not land you in registry data that happens
to
> evaluate to "hbin" as well. That's why I put "hbin"
offset validation
> check couple of lines below to make sure that the "hbin" we found
by
> searching is a proper one. The offset check I'm referring to is:
> 
> /* get "stated" hbin offset from header */
> size_t page_offset = le32to(page->offset_first) + 0x1000;
> 
> /* if that does not match our current file offset,
>    then exit with error */
> if (page_offset != off) { 
>   SET_ERRNO...
> }
Still, what kind of corruption would move a genuine hbin to a
non-page-sized offset in the file?  It seems unlikely to me ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html