Richard W.M. Jones wrote:

Subject: [PATCH 1/5] Really fix the case where a UTF-16 string contains junk after the string.

The previous commit b71b88f588f8660935a7d462e97b84aa2d669249 attempted to fix this, but got the test the wrong way round so the length would never be shorter. --- lib/hivex.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/hivex.c b/lib/hivex.c index 860c85c..3f4c629 100644 --- a/lib/hivex.c +++ b/lib/hivex.c @@ -1335,7 +1335,7 @@ hivex_value_string (hive_h *h, hive_value_h value) * (Found by Hilko Bengen in a fresh Windows XP SOFTWARE hive). */ size_t slen = utf16_string_len_in_bytes_max (data, len); - if (slen > len) + if (slen < len) len = slen;

char *ret = windows_utf16_to_utf8 (data, len);

That looks obviously correct.

Richard W.M. Jones wrote: ...

diff --git a/lib/hivex.c b/lib/hivex.c index 3f4c629..b1f6ea6 100644 --- a/lib/hivex.c +++ b/lib/hivex.c @@ -1245,6 +1245,10 @@ hivex_value_value (hive_h *h, hive_value_h value, fprintf (stderr, "hivex_value_value: warning: declared data length is longer than the block it is in (data 0x%zx, data len %zu, block len %zu)\n", data_offset, len, blen); len = blen - 4; + + /* Return the smaller length to the caller too. */ + if (len_rtn) + *len_rtn = len;

This looks good.

Richard W.M. Jones wrote: ...

diff --git a/lib/hivex.c b/lib/hivex.c index b1f6ea6..71ea5c3 100644 --- a/lib/hivex.c +++ b/lib/hivex.c @@ -1384,7 +1384,7 @@ utf16_string_len_in_bytes_max (const char *str, size_t len) { size_t ret = 0;

- while (len > 0 && (str[0] || str[1])) { + while (len >= 2 && (str[0] || str[1])) { str += 2; ret += 2; len -= 2;

Good one ;-)

Richard W.M. Jones wrote:

Subject: [PATCH 4/5] hivex_value_multiple_strings: Don't read uninitialized data.

If hivex_value_multiple_strings was given a value which had an odd length or if the data in the value was unterminated, hivex_value_multiple_strings could read uninitialized data.

Potentially (although very unlikely) this could cause a non-exploitable segfault in the calling program. --- lib/hivex.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/lib/hivex.c b/lib/hivex.c index 71ea5c3..d2ab23d 100644 --- a/lib/hivex.c +++ b/lib/hivex.c @@ -1421,7 +1421,8 @@ hivex_value_multiple_strings (hive_h *h, hive_value_h value) char *p = data; size_t plen;

- while (p < data + len && (plen = utf16_string_len_in_bytes (p)) > 0) { + while (p < data + len && + (plen = utf16_string_len_in_bytes_max (p, data + len - p)) > 0) { nr_strings++; char **ret2 = realloc (ret, (1 + nr_strings) * sizeof (char *)); if (ret2 == NULL) {

That looks correct.

Richard W.M. Jones wrote:

Subject: [PATCH 5/5] Remove no longer used internal function utf16_string_len_in_bytes.

--- lib/hivex.c | 18 ++---------------- 1 files changed, 2 insertions(+), 16 deletions(-)

diff --git a/lib/hivex.c b/lib/hivex.c index d2ab23d..573c446 100644 --- a/lib/hivex.c +++ b/lib/hivex.c @@ -62,7 +62,6 @@ #define HIVEX_MAX_ALLOCATION 1000000

static char *windows_utf16_to_utf8 (/* const */ char *input, size_t len); -static size_t utf16_string_len_in_bytes (const char *str); static size_t utf16_string_len_in_bytes_max (const char *str, size_t len);

struct hive_h { @@ -1363,23 +1362,10 @@ free_strings (char **argv) }

/* Get the length of a UTF-16 format string. Handle the string as - * pairs of bytes, looking for the first \0\0 pair. + * pairs of bytes, looking for the first \0\0 pair. Only read up to + * 'len' maximum bytes. */ static size_t -utf16_string_len_in_bytes (const char *str) -{ - size_t ret = 0; - - while (str[0] || str[1]) { - str += 2; - ret += 2; - } - - return ret; -}

ACK'd as obviously welcome and safe, assuming compilation succeeded.