Libguestfs - Oct 2014 - Re: [libhivex] Undefined behavior when accessing invalid (too small) registry hives

On Oct 29, 2014, at 3:39 PM, Richard W.M. Jones <rjones@redhat.com>
wrote:
> 
>> Or is it expected that certain sanity checks would be performed prior
to
>> passing along any files to libhivex? What would those checks be?
> 
> No, hivex should definitely have those checks.
> 
> I'll have a proper look at this in the morning.
> 
> Thanks,
> 
> Rich.
Thanks, Rich.

As far as I can tell, the only sanity checks in the initial loading of a
registry hive are the magic bits (“regf”), major_ver = 1, and the checksum
match.

When calling hivex_open with a file under 4 bytes, you run into the
out-of-bounds access when comparing against the magic bits; pass in a file 4
bytes long with “regf” correctly set, you’ll get an out-of-bounds access to
major_ver; pass in a file truncated at 0x18 (major_ver, set to 1), and you’ll
get through to the checksum routine, which will read out-of-bounds the first 128
bytes.

If you pass in a file truncated at 0x200, you’ll get past the checksum tests but
accesses (if any) to other registry header members will be out of bounds. (I
don’t think that’s the case, because that’s all unused unknown_guid stuff,
though.)

After that, offsets are checked against hdr->size; from a brief glance I’m
unsure but I think there might be an issue if the file is truncated after a page
offset. "off < h->size” will return true, but accesses to page
contents will be out-of-bounds. So I think that would need to be “off +
sizeof(ntreg_hbin_page) < h->size”?

For example, truncating a registry file at h->rootoffs  and with a
purposely-wrong hdr->offset = 0, I think you’ll get past "if (off >=
h->endpages)” and you’ll be reading the page out-of-bounds while checking
hbin magic.

I have to run, but I think there may be a few more instances of things like
this.. I know these are only reads, but I have a suspicion there’s an
out-of-bounds write somewhere along similar lines because I was getting
segfaults in some untraced code when processing bulk, untrusted registry files,
though I could be wrong.

Thanks for looking into this, I hope I haven’t led you on a wild goose chase.

Mahmoud

On Wed, Oct 29, 2014 at 09:26:30PM -0500, Mahmoud Al-Qudsi
wrote:
> On Oct 29, 2014, at 3:39 PM, Richard W.M. Jones <rjones@redhat.com>
wrote:
> > 
> >> Or is it expected that certain sanity checks would be performed
prior to
> >> passing along any files to libhivex? What would those checks be?
> > 
> > No, hivex should definitely have those checks.
> > 
> > I'll have a proper look at this in the morning.
> > 
> > Thanks,
> > 
> > Rich.
> 
> Thanks, Rich.
> As far as I can tell, the only sanity checks in the initial loading
> of a registry hive are the magic bits (“regf”), major_ver = 1, and
> the checksum match.
>
> When calling hivex_open with a file under 4 bytes, you run into the
> out-of-bounds access when comparing against the magic bits; pass in
> a file 4 bytes long with “regf” correctly set, you’ll get an
> out-of-bounds access to major_ver; pass in a file truncated at 0x18
> (major_ver, set to 1), and you’ll get through to the checksum
> routine, which will read out-of-bounds the first 128 bytes.
>
> If you pass in a file truncated at 0x200, you’ll get past the
> checksum tests but accesses (if any) to other registry header
> members will be out of bounds. (I don’t think that’s the case,
> because that’s all unused unknown_guid stuff, though.)
So I believe it's impossible for a hive to be smaller than 8192 bytes,
since such a hive couldn't contain the header page and the first data
page (containing the root node).  Hence the first attached patch
simply refuses to open such files.
> After that, offsets are checked against hdr->size; from a brief
> glance I’m unsure but I think there might be an issue if the file is
> truncated after a page offset. "off < h->size” will return true,
but
> accesses to page contents will be out-of-bounds. So I think that
> would need to be “off + sizeof(ntreg_hbin_page) < h->size”?
I added a second check that the page we're reading in the loop at line
~ 220 doesn't extend beyond the end of the file, which I think should
be sufficient.  That's the second attached patch.
> For example, truncating a registry file at h->rootoffs and with a
> purposely-wrong hdr->offset = 0, I think you’ll get past "if (off
>> h->endpages)” and you’ll be reading the page out-of-bounds while
> checking hbin magic.
> I have to run, but I think there may be a few more instances of
> things like this.. I know these are only reads, but I have a
> suspicion there’s an out-of-bounds write somewhere along similar
> lines because I was getting segfaults in some untraced code when
> processing bulk, untrusted registry files, though I could be wrong.
If you can get full stack traces from these (or supply the untrusted
registry files) then I'd be very interested.  It's important that
hivex behaves well on untrusted or corrupted data.

Thanks, 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

--UHN/qo2QbUvPLonB
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="0001-handle-Refuse-to-open-files-8192-bytes-in-size.patch"
>From 357f26fa64fd1d9ccac2331fe174a8ee9c607adb Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Thu, 30 Oct 2014 13:50:39 +0000
Subject: [PATCH 1/2] handle: Refuse to open files < 8192 bytes in size.

These cannot be valid hives, since they don't contain a full header
page and at least a single page of data (in other words they couldn't
contain a root node).

Thanks: Mahmoud Al-Qudsi
---
 lib/handle.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/handle.c b/lib/handle.c
index 62a8644..a3cbcf7 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -104,6 +104,13 @@ hivex_open (const char *filename, int flags)
 
   h->size = statbuf.st_size;
 
+  if (h->size < 0x2000) {
+    SET_ERRNO (EINVAL,
+               "%s: file is too small to be a Windows NT Registry hive
file",
+               filename);
+    goto error;
+  }
+
   if (!h->writable) {
     h->addr = mmap (NULL, h->size, PROT_READ, MAP_SHARED, h->fd, 0);
     if (h->addr == MAP_FAILED)
-- 
2.0.4


--UHN/qo2QbUvPLonB
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="0002-handle-Check-that-pages-do-not-extend-beyond-the-end.patch"
>From 4bbdf555f88baeae0fa804a369a81a83908bd705 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones@redhat.com>
Date: Thu, 30 Oct 2014 14:02:25 +0000
Subject: [PATCH 2/2] handle: Check that pages do not extend beyond the end of
 the file.

Thanks: Mahmoud Al-Qudsi
---
 lib/handle.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/handle.c b/lib/handle.c
index a3cbcf7..3a8f09b 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -247,6 +247,13 @@ hivex_open (const char *filename, int flags)
       goto error;
     }
 
+    if (off + page_size > h->size) {
+      SET_ERRNO (ENOTSUP,
+                 "%s: page size %zu at 0x%zx extends beyond end of file,
bad registry",
+                 filename, page_size, off);
+      goto error;
+    }
+
     /* Read the blocks in this page. */
     size_t blkoff;
     struct ntreg_hbin_block *block;
-- 
2.0.4


--UHN/qo2QbUvPLonB--

These two patches are included in hivex 1.3.11.

I tried a bit of fuzz testing, but not very hard.  It didn't hit any
problems, but better fuzz testing is required.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

> On Nov 11, 2014, at 1:57 AM, Richard W.M. Jones <rjones@redhat.com>
wrote:
> 
> Yes I was also meaning to do that after reading lcamtuf's postings.
Yup. That's the one. 
> I just started a run now ..  Will let it run for a few days and report
> any issues on the list.
Thank you. Do you mind running it under valgrind to catch out-of-bound reads?

Mahmoud

On Tue, Nov 11, 2014 at 05:56:54AM -0600, Mahmoud Al-Qudsi
wrote:
> > On Nov 11, 2014, at 1:57 AM, Richard W.M. Jones
<rjones@redhat.com> wrote:
> > 
> > Yes I was also meaning to do that after reading lcamtuf's
postings.
> 
> Yup. That's the one. 
As an update here:

I found some crashes, but they turned out to be user error.  I was
accidentally testing a local build of hivexml linked to
/usr/lib64/libhivex.so, but since they used different versions of
iconv (glibc vs gnulib), the crashes were not real ones.
> > I just started a run now ..  Will let it run for a few days and report
> > any issues on the list.
> 
> Thank you. Do you mind running it under valgrind to catch out-of-bound
reads?
However I've set the test going again, on a more powerful machine than
before.  I will also test under valgrind (later).

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