11:05 a.m.
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
11:05 a.m.
New subject: [PATCH v2 1/2] lib: change how hbin sections are read.
* 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
10:54 a.m.
New subject: [PATCH v2 1/2] lib: change how hbin sections are read.
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(a)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
12:48 p.m.
New subject: [PATCH v2 1/2] lib: change how hbin sections are read.
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++;
Dawid
12:52 p.m.
New subject: [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
1:28 p.m.
New subject: [PATCH v2 1/2] lib: change how hbin sections are read.
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...
}
3:14 p.m.
New subject: [PATCH v2 1/2] lib: change how hbin sections are read.
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
3:50 p.m.
New subject: [PATCH v2 1/2] lib: change how hbin sections are read.
On Wed, 2017-02-15 at 21:14 +0000, Richard W.M. Jones wrote:
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.
None that I know of :-) However I don't see any other simple way to
silence that GCC7 warning and it seems harmless that way to me (besides
the fact that the loop will have to make more iterations)
4:22 p.m.
New subject: [PATCH v2 1/2] lib: change how hbin sections are read.
On Wed, 2017-02-15 at 16:50 -0500, Dawid Zamirski wrote:
On Wed, 2017-02-15 at 21:14 +0000, Richard W.M. Jones wrote:
> 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.
>
None that I know of :-) However I don't see any other simple way to
silence that GCC7 warning and it seems harmless that way to me
(besides
the fact that the loop will have to make more iterations)
So I've found a way to keep offsetting by 4k and keep GCC7 happy:
while (off < h->endpages) {
if (off + 0x1000 > off)
off += 0x1000;
else
break; // off would overflow
Is this acceptable?
Regards,
Dawid
4:29 p.m.
New subject: [PATCH v2 1/2] lib: change how hbin sections are read.
On Wed, Feb 15, 2017 at 05:22:52PM -0500, Dawid Zamirski wrote:
On Wed, 2017-02-15 at 16:50 -0500, Dawid Zamirski wrote:
> On Wed, 2017-02-15 at 21:14 +0000, Richard W.M. Jones wrote:
> > 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.
> >
>
> None that I know of :-) However I don't see any other simple way to
> silence that GCC7 warning and it seems harmless that way to me
> (besides
> the fact that the loop will have to make more iterations)
So I've found a way to keep offsetting by 4k and keep GCC7 happy:
while (off < h->endpages) {
if (off + 0x1000 > off)
off += 0x1000;
else
break; // off would overflow
Is this acceptable?
Yes, or even how about this (not tried it):
while (off <= h->endpages - 0x1000) {
...
}
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
4:35 p.m.
New subject: [PATCH v2 1/2] lib: change how hbin sections are read.
On Wed, Feb 15, 2017 at 10:29:41PM +0000, Richard W.M. Jones wrote:
Yes, or even how about this (not tried it):
while (off <= h->endpages - 0x1000) {
...
}
In fact this doesn't work either :-(
I'll have another look at this tomorrow morning.
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
4:55 p.m.
New subject: [PATCH v2 1/2] lib: change how hbin sections are read.
On Wed, 2017-02-15 at 22:35 +0000, Richard W.M. Jones wrote:
On Wed, Feb 15, 2017 at 10:29:41PM +0000, Richard W.M. Jones wrote:
> Yes, or even how about this (not tried it):
>
> while (off <= h->endpages - 0x1000) {
> ...
> }
In fact this doesn't work either :-(
I'll have another look at this tomorrow morning.
Rich.
Yep, GCC7 complains about that off could overflow over SIZE_MAX when
incremented with 0x1000 and could cause infinite loop, i.e
size_t off = SIZE_MAX - 50;
size_t endpages = SIZE_MAX;
off += 100; // off is now 50
if (off < endpages)
prinf("off is still smaller!");
To prevent this the while loop could be written as:
while (off + 0x1000 < off && off < h->endpages) {
off += 0x1000;
4:59 p.m.
New subject: [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
2:43 a.m.
New subject: [PATCH v2 1/2] lib: change how hbin sections are read.
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
10:04 a.m.
New subject: [PATCH v2 1/2] lib: change how hbin sections are read.
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
10:37 a.m.
New subject: [PATCH v2 1/2] lib: change how hbin sections are read.
On Thu, Feb 16, 2017 at 11:04:12AM -0500, Dawid Zamirski wrote:
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.
Yup, agreed.
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/
11:05 a.m.
New subject: [PATCH v2 2/2] lib: allow to walk registry with corrupted blocks
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
11:01 a.m.
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
2835
days inactive
2837
days old
17 comments
2 participants
participants (2)
-
Dawid Zamirski -
Richard W.M. Jones