[Libguestfs] [libhivex] Undefined behavior when accessing invalid (too small) registry hives

[Richard W.M. Jones]

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 at 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
-------------- next part --------------
>From 357f26fa64fd1d9ccac2331fe174a8ee9c607adb Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones at 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

-------------- next part --------------
>From 4bbdf555f88baeae0fa804a369a81a83908bd705 Mon Sep 17 00:00:00 2001
From: "Richard W.M. Jones" <rjones at 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