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

The following patches address issues when dealing with hives that have
corrupted data in them but are otherwise readable/writable. Those were
found on some rather rare Windows installations that seem to work fine
but current hivex fails to even open.

Those patches change hivex to simply log and ignore such "corrupted"
regions instead of aborting because the caller might be looking at keys
that are perfectly readable/writable (e.g. to identify Windows version
from HKLM/Software/Microsoft/Windows NT/CurrentVersion) and other
"corrupted" and irrelevant keys might prevent one from doing so.

Changes in v2:
 * fixed code formatting
 * added more debug statements
 * do not depend on bad_root_block variable, instead report error and
   exit if root block is corrupted
 * handle the missed case where check_child_is_nk_block was still
   returning instead of skipping faulty block with debug message

Regards,

Dawid Zamirski (2):
  lib: change how hbin sections are read.
  lib: allow to walk registry with corrupted blocks

 lib/handle.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++---------
 lib/node.c   | 21 ++++++++++++---------
 2 files changed, 58 insertions(+), 18 deletions(-)

-- 
2.9.3

* 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) {
+        off += 0x1000;
+        page = (struct ntreg_hbin_page *) ((char *) h->addr + off);
+        if (page->magic[0] == 'h' &&
+            page->magic[1] == 'b' &&
+            page->magic[2] == 'i' &&
+            page->magic[3] == 'n') {
+          DEBUG (2, "found next page by seeking at 0x%zx", off);
+          found = 1;
+          break;
+        }
+      }
+
+      if (!found) {
+        DEBUG (2, "page not found and end of pages section reached");
+        break;
+      }
     }
 
     size_t page_size = le32toh (page->page_size);
@@ -254,6 +273,16 @@ hivex_open (const char *filename, int flags)
       goto error;
     }
 
+    size_t page_offset = le32toh(page->offset_first) + 0x1000;
+
+    if (page_offset != off) {
+      SET_ERRNO (ENOTSUP,
+                 "%s: declared page offset (0x%zx) does not match computed
"
+                 "offset (0x%zx), bad registry",
+                 filename, page_offset, off);
+      goto error;
+    }
+
     /* Read the blocks in this page. */
     size_t blkoff;
     struct ntreg_hbin_block *block;
-- 
2.9.3

There are some corrupted registry files that have invalid hbin cells
but are still readable. This patch makes the following changes:

* hivex_open - do not abort with complete failure if we run across a
  block with invalid size (unless it's the root block). Instead just
  log the event, and move on. This will allow open hives that have
  apparent invalid blocks but the ones of potential interest might be
  perfectly accessible.
* _hivex_get_children - similiarly, if the's invalid subkey, just skip
  it instead of failing so one can continue to browse other valid
  subkeys.

The above is similar to the behavior to Windows regedit where one can
load such corrupted hives with e.g. "reg load HKU\Corrupted" and
browse/change it despite some keys might be missing.
---
 lib/handle.c | 16 ++++++++++++----
 lib/node.c   | 21 ++++++++++++---------
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/lib/handle.c b/lib/handle.c
index e183ff2..2a0f592 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -300,10 +300,18 @@ hivex_open (const char *filename, int flags)
       int used;
       seg_len = block_len (h, blkoff, &used);
       if (seg_len <= 4 || (seg_len & 3) != 0) {
-        SET_ERRNO (ENOTSUP,
-                   "%s: block size %" PRIi32 " at 0x%zx, bad
registry",
-                   filename, le32toh (block->seg_len), blkoff);
-        goto error;
+        if (is_root) {
+          SET_ERRNO (ENOTSUP,
+                     "%s, the root block at 0x%zx has invalid size %"
PRIi32
+                     ", bad registry",
+                     filename, blkoff, le32toh (block->seg_len));
+          goto error;
+        } else {
+          DEBUG (2,
+                 "%s: block at 0x%zx has invalid size %" PRIi32
", skipping",
+                 filename, blkoff, le32toh (block->seg_len));
+          break;
+        }
       }
 
       if (h->msglvl >= 2) {
diff --git a/lib/node.c b/lib/node.c
index 822c250..a20bbc5 100644
--- a/lib/node.c
+++ b/lib/node.c
@@ -343,11 +343,10 @@ _hivex_get_children (hive_h *h, hive_node_h node,
    */
   size_t nr_children = _hivex_get_offset_list_length (&children);
   if (nr_subkeys_in_nk != nr_children) {
-    SET_ERRNO (ENOTSUP,
-               "nr_subkeys_in_nk = %zu "
-               "is not equal to number of children read %zu",
-               nr_subkeys_in_nk, nr_children);
-    goto error;
+    DEBUG (2,
+           "nr_subkeys_in_nk = %zu "
+           "is not equal to number of children read %zu",
+           nr_subkeys_in_nk, nr_children);
   }
 
  out:
@@ -407,8 +406,10 @@ _get_children (hive_h *h, hive_node_h blkoff,
     for (i = 0; i < nr_subkeys_in_lf; ++i) {
       hive_node_h subkey = le32toh (lf->keys[i].offset);
       subkey += 0x1000;
-      if (check_child_is_nk_block (h, subkey, flags) == -1)
-        return -1;
+      if (check_child_is_nk_block (h, subkey, flags) == -1) {
+        DEBUG (2, "subkey at 0x%zx is not an NK block, skipping",
subkey);
+        continue;
+      }
       if (_hivex_add_to_offset_list (children, subkey) == -1)
         return -1;
     }
@@ -435,8 +436,10 @@ _get_children (hive_h *h, hive_node_h blkoff,
     for (i = 0; i < nr_offsets; ++i) {
       hive_node_h subkey = le32toh (ri->offset[i]);
       subkey += 0x1000;
-      if (check_child_is_nk_block (h, subkey, flags) == -1)
-        return -1;
+      if (check_child_is_nk_block (h, subkey, flags) == -1) {
+        DEBUG (2, "subkey at 0x%zx is not an NK block, skipping",
subkey);
+        continue;
+      }
       if (_hivex_add_to_offset_list (children, subkey) == -1)
         return -1;
     }
-- 
2.9.3

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.
> +        off += 0x1000;
> +        page = (struct ntreg_hbin_page *) ((char *) h->addr + off);
> +        if (page->magic[0] == 'h' &&
> +            page->magic[1] == 'b' &&
> +            page->magic[2] == 'i' &&
> +            page->magic[3] == 'n') {
> +          DEBUG (2, "found next page by seeking at 0x%zx", off);
> +          found = 1;
> +          break;
> +        }
> +      }
> +
> +      if (!found) {
> +        DEBUG (2, "page not found and end of pages section
reached");
> +        break;
> +      }
>      }
>  
>      size_t page_size = le32toh (page->page_size);
> @@ -254,6 +273,16 @@ hivex_open (const char *filename, int flags)
>        goto error;
>      }
>  
> +    size_t page_offset = le32toh(page->offset_first) + 0x1000;
> +
> +    if (page_offset != off) {
> +      SET_ERRNO (ENOTSUP,
> +                 "%s: declared page offset (0x%zx) does not match
computed "
> +                 "offset (0x%zx), bad registry",
> +                 filename, page_offset, off);
> +      goto error;
> +    }
> +
>      /* Read the blocks in this page. */
>      size_t blkoff;
>      struct ntreg_hbin_block *block;
> -- 
> 2.9.3
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
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 Tue, Feb 14, 2017 at 12:05:19PM -0500, Dawid Zamirski
wrote:
> The following patches address issues when dealing with hives that have
> corrupted data in them but are otherwise readable/writable. Those were
> found on some rather rare Windows installations that seem to work fine
> but current hivex fails to even open.
> 
> Those patches change hivex to simply log and ignore such
"corrupted"
> regions instead of aborting because the caller might be looking at keys
> that are perfectly readable/writable (e.g. to identify Windows version
> from HKLM/Software/Microsoft/Windows NT/CurrentVersion) and other
> "corrupted" and irrelevant keys might prevent one from doing so.
> 
> Changes in v2:
>  * fixed code formatting
>  * added more debug statements
>  * do not depend on bad_root_block variable, instead report error and
>    exit if root block is corrupted
>  * handle the missed case where check_child_is_nk_block was still
>    returning instead of skipping faulty block with debug message
This is fine now, except for that annoying while loop issue ...
I couldn't see an easy for for that.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW