From 98fd51eccfed3b91b49d3e583403d185648a0dc7 Mon Sep 17 00:00:00 2001 From: Mohammed Hassan Date: Thu, 8 Oct 2026 23:58:38 +0530 Subject: [PATCH] Fix heap out-of-bounds reads in the Android binary XML decoder ##crash --- libr/util/axml.c | 54 ++++++---- test/unit/meson.build | 1 + test/unit/test_axml.c | 226 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 261 insertions(+), 20 deletions(-) create mode 100644 test/unit/test_axml.c diff --git a/libr/util/axml.c b/libr/util/axml.c index 9ca041fdf22b7..7a81534921cf6 100644 --- a/libr/util/axml.c +++ b/libr/util/axml.c @@ -102,13 +102,20 @@ R_PACKED( }) namespace_t; -static char *string_lookup(string_pool_t *pool, const ut8 *data, ut64 data_size, ut32 i, size_t *length) { - if (i > r_read_le32 (&pool->string_count)) { +static char *string_lookup(string_pool_t *pool, ut32 pool_size, const ut8 *data, ut64 data_size, ut32 i, size_t *length) { + if (pool_size < sizeof (string_pool_t)) { + return NULL; + } + const ut32 offsets_count = (pool_size - sizeof (string_pool_t)) / sizeof (ut32); + if (i >= R_MIN (r_read_le32 (&pool->string_count), offsets_count)) { return NULL; } - ut32 offset = r_read_le32 (&pool->offsets[i]); - ut8 *start = (ut8 *) ((uintptr_t)data + r_read_le32 (&pool->strings_offset) + 8 + offset); + const ut64 string_offset = (ut64)r_read_le32 (&pool->strings_offset) + 8 + r_read_le32 (&pool->offsets[i]); + if (string_offset >= data_size) { + return NULL; + } + ut8 *start = (ut8 *)(data + string_offset); char *name = NULL; if (pool->flags & FLAG_UTF8) { @@ -199,7 +206,7 @@ static char *string_lookup(string_pool_t *pool, const ut8 *data, ut64 data_size, return name; } -static char *resource_value(string_pool_t *pool, const ut8 *data, ut64 data_size, +static char *resource_value(string_pool_t *pool, ut32 pool_size, const ut8 *data, ut64 data_size, resource_value_t *value) { switch (value->type) { case RESOURCE_NULL: @@ -207,7 +214,7 @@ static char *resource_value(string_pool_t *pool, const ut8 *data, ut64 data_size case RESOURCE_REFERENCE: return r_str_newf ("@0x%x", value->data.d); case RESOURCE_STRING: - return string_lookup (pool, data, data_size, r_read_le32 (&value->data.d), NULL); + return string_lookup (pool, pool_size, data, data_size, r_read_le32 (&value->data.d), NULL); case RESOURCE_FLOAT: return r_str_newf ("%f", value->data.f); case RESOURCE_INT_DEC: @@ -223,13 +230,18 @@ static char *resource_value(string_pool_t *pool, const ut8 *data, ut64 data_size return strdup ("null"); } -static bool dump_element(PJ *pj, RStrBuf *sb, string_pool_t *pool, namespace_t *namespace, +static bool dump_element(PJ *pj, RStrBuf *sb, string_pool_t *pool, ut32 pool_size, namespace_t *namespace, const ut8 *data, ut64 data_size, void *element, size_t element_size, const ut32 *resource_map, ut32 resource_map_length, st32 *depth, bool start) { ut32 i; + if (element_size < (start? sizeof (start_element_t): sizeof (end_element_t))) { + R_LOG_ERROR ("Truncated element"); + return false; + } + end_element_t *common = element; - char *name = string_lookup (pool, data, data_size, r_read_le32 (&common->name), NULL); + char *name = string_lookup (pool, pool_size, data, data_size, r_read_le32 (&common->name), NULL); for (i = 0; i < *depth; i++) { r_strbuf_append (sb, "\t"); } @@ -245,8 +257,8 @@ static bool dump_element(PJ *pj, RStrBuf *sb, string_pool_t *pool, namespace_t * } ut16 count = r_read_le16 (&e->attribute_count); if (*depth == 0 && namespace) { - char *key = string_lookup (pool, data, data_size, namespace->prefix, NULL); - char *value = string_lookup (pool, data, data_size, namespace->uri, NULL); + char *key = string_lookup (pool, pool_size, data, data_size, namespace->prefix, NULL); + char *value = string_lookup (pool, pool_size, data, data_size, namespace->uri, NULL); if (pj) { pj_ko (pj, "xmlns"); pj_ks (pj, key, value); @@ -257,7 +269,7 @@ static bool dump_element(PJ *pj, RStrBuf *sb, string_pool_t *pool, namespace_t * free (value); } - if (count * sizeof (attribute_t) > element_size) { + if ((size_t)count > (element_size - sizeof (start_element_t)) / sizeof (attribute_t)) { r_strbuf_append (sb, " />"); if (pj) { pj_end (pj); @@ -273,7 +285,7 @@ static bool dump_element(PJ *pj, RStrBuf *sb, string_pool_t *pool, namespace_t * for (i = 0; i < count; i++) { attribute_t a = e->attributes[i]; ut32 key_index = r_read_le32 (&a.name); - char *key = string_lookup (pool, data, data_size, key_index, NULL); + char *key = string_lookup (pool, pool_size, data, data_size, key_index, NULL); // If the key is empty, it is a cached resource name if (R_STR_ISEMPTY (key)) { R_FREE (key); @@ -290,11 +302,11 @@ static bool dump_element(PJ *pj, RStrBuf *sb, string_pool_t *pool, namespace_t * key = strdup ("null"); } } - char *value = resource_value (pool, data, data_size, &a.value); + char *value = resource_value (pool, pool_size, data, data_size, &a.value); // If there is a namespace on the value, and there is an active // namespace, assume it is the same if (r_read_le32 (&a.namespace) != -1 && namespace && namespace->prefix != -1) { - char *ns = string_lookup (pool, data, data_size, namespace->prefix, NULL); + char *ns = string_lookup (pool, pool_size, data, data_size, namespace->prefix, NULL); r_strbuf_appendf (sb, "%s:%s=\"%s\"", ns, key, value); if (pj) { char *k = r_str_newf ("%s:%s", ns, key); @@ -328,6 +340,7 @@ static bool dump_element(PJ *pj, RStrBuf *sb, string_pool_t *pool, namespace_t * R_API char *r_axml_decode(const ut8 *data, const ut64 data_size, PJ *pj) { R_RETURN_VAL_IF_FAIL (data, NULL); string_pool_t *pool = NULL; + ut32 pool_size = 0; namespace_t *namespace = NULL; const ut32 *resource_map = NULL; ut32 resource_map_length = 0; @@ -375,8 +388,8 @@ R_API char *r_axml_decode(const ut8 *data, const ut64 data_size, PJ *pj) { switch (type) { case TYPE_STRING_POOL: { - ut16 header_size = header.size; - if (header_size == 0 || header_size > data_size) { + ut32 header_size = header.size; + if (header_size == 0 || header_size > data_size) { goto bad; } pool = malloc (header_size); @@ -387,6 +400,7 @@ R_API char *r_axml_decode(const ut8 *data, const ut64 data_size, PJ *pj) { if (r_buf_read_at (buffer, offset, (void *)pool, header_size) != header_size) { goto bad; } + pool_size = header_size; } break; case TYPE_START_ELEMENT: @@ -395,8 +409,8 @@ R_API char *r_axml_decode(const ut8 *data, const ut64 data_size, PJ *pj) { if (!pool) { goto bad; } - ut16 header_size = header.size; - if (header_size == 0 || header_size > data_size) { + ut32 header_size = header.size; + if (header_size == 0 || header_size > data_size) { goto bad; } start_element_t *element = malloc (header_size); @@ -407,7 +421,7 @@ R_API char *r_axml_decode(const ut8 *data, const ut64 data_size, PJ *pj) { free (element); goto bad; } - if (!dump_element (pj, sb, pool, namespace, data, data_size, element, header_size, + if (!dump_element (pj, sb, pool, pool_size, namespace, data, data_size, element, header_size, resource_map, resource_map_length, &depth, true)) { free (element); goto bad; @@ -429,7 +443,7 @@ R_API char *r_axml_decode(const ut8 *data, const ut64 data_size, PJ *pj) { if (r_buf_read_at (buffer, offset, (void *)&end, sizeof (end)) != sizeof (end)) { goto bad; } - if (!dump_element (pj, sb, pool, namespace, data, data_size, &end, sizeof (end), + if (!dump_element (pj, sb, pool, pool_size, namespace, data, data_size, &end, sizeof (end), resource_map, resource_map_length, &depth, false)) { goto bad; } diff --git a/test/unit/meson.build b/test/unit/meson.build index ec7148a76b324..c98afac59af53 100644 --- a/test/unit/meson.build +++ b/test/unit/meson.build @@ -14,6 +14,7 @@ if get_option('enable_tests') 'anal_xrefs', 'anal_x86', 'arena', + 'axml', 'codemeta', 'base64', 'big', diff --git a/test/unit/test_axml.c b/test/unit/test_axml.c new file mode 100644 index 0000000000000..0f6630d564f71 --- /dev/null +++ b/test/unit/test_axml.c @@ -0,0 +1,226 @@ +#include +#include "minunit.h" + +enum { + CHUNK_XML = 0x0003, + CHUNK_STRING_POOL = 0x0001, + CHUNK_START_NAMESPACE = 0x0100, + CHUNK_END_NAMESPACE = 0x0101, + CHUNK_START_ELEMENT = 0x0102, + CHUNK_END_ELEMENT = 0x0103, +}; + +typedef struct { + ut8 bytes[1024]; + size_t size; +} AxmlBuf; + +static void put16(AxmlBuf *b, ut16 v) { + r_write_le16 (b->bytes + b->size, v); + b->size += sizeof (ut16); +} + +static void put32(AxmlBuf *b, ut32 v) { + r_write_le32 (b->bytes + b->size, v); + b->size += sizeof (ut32); +} + +static void put_chunk(AxmlBuf *b, ut16 type, ut16 header_size, ut32 size) { + put16 (b, type); + put16 (b, header_size); + put32 (b, size); +} + +static void put_utf16(AxmlBuf *b, const char *s) { + const size_t len = strlen (s); + size_t i; + put16 (b, (ut16)len); + for (i = 0; i < len; i++) { + put16 (b, (ut8)s[i]); + } + put16 (b, 0); +} + +static void put_xml_header(AxmlBuf *b, ut32 size) { + put_chunk (b, CHUNK_XML, 8, size); +} + +// A pool chunk of `size` bytes only keeps room for (size - 20) / 4 offsets +static void put_small_string_pool(AxmlBuf *b, ut32 size, ut32 string_count) { + const size_t start = b->size; + put_chunk (b, CHUNK_STRING_POOL, 28, size); + put32 (b, string_count); + put32 (b, 0); // style_count + put32 (b, 0); // flags, UTF-16 + put32 (b, 0); // strings_offset + put32 (b, 0); // styles_offset + while (b->size < start + size) { + put32 (b, 0); // offsets[] + } +} + +static void put_start_element(AxmlBuf *b, ut32 size, ut32 name, ut16 attribute_count) { + put_chunk (b, CHUNK_START_ELEMENT, 16, size); + put32 (b, 0); // line + put32 (b, 0); // comment + put32 (b, 0); // namespace + put32 (b, name); + put32 (b, 0); // flags + put16 (b, attribute_count); + put16 (b, 0); + put16 (b, 0); + put16 (b, 0); +} + +static void pad(AxmlBuf *b, size_t size) { + b->size = size; +} + +static const char *const manifest_strings[] = { + "android", + "http://schemas.android.com/apk/res/android", + "manifest", + "package", + "com.example" +}; + +static void put_manifest_string_pool(AxmlBuf *b) { + AxmlBuf strings = {{ 0 }, 0}; + ut32 offsets[R_ARRAY_SIZE (manifest_strings)]; + size_t i; + for (i = 0; i < R_ARRAY_SIZE (manifest_strings); i++) { + offsets[i] = strings.size; + put_utf16 (&strings, manifest_strings[i]); + } + const ut32 strings_offset = (ut32)(28 + sizeof (offsets)); + put_chunk (b, CHUNK_STRING_POOL, 28, (ut32)(strings_offset + strings.size)); + put32 (b, (ut32)R_ARRAY_SIZE (manifest_strings)); + put32 (b, 0); // style_count + put32 (b, 0); // flags, UTF-16 + put32 (b, strings_offset); + put32 (b, 0); // styles_offset + for (i = 0; i < R_ARRAY_SIZE (offsets); i++) { + put32 (b, offsets[i]); + } + memcpy (b->bytes + b->size, strings.bytes, strings.size); + b->size += strings.size; +} + +static void put_manifest(AxmlBuf *b) { + AxmlBuf body = {{ 0 }, 0}; + put_manifest_string_pool (&body); + + put_chunk (&body, CHUNK_START_NAMESPACE, 16, 24); + put32 (&body, 1); // line + put32 (&body, UT32_MAX); // comment + put32 (&body, 0); // prefix, "android" + put32 (&body, 1); // uri + + put_chunk (&body, CHUNK_START_ELEMENT, 16, 36 + 20); + put32 (&body, 1); // line + put32 (&body, UT32_MAX); // comment + put32 (&body, UT32_MAX); // namespace + put32 (&body, 2); // name, "manifest" + put16 (&body, 20); // attribute start + put16 (&body, 20); // attribute size + put16 (&body, 1); // attribute count + put16 (&body, 0); + put16 (&body, 0); + put16 (&body, 0); + put32 (&body, UT32_MAX); // attribute namespace + put32 (&body, 3); // attribute name, "package" + put32 (&body, 4); // attribute raw value + put16 (&body, 8); // value size + put16 (&body, 0x0300); // unused and RESOURCE_STRING + put32 (&body, 4); // value, "com.example" + + put_chunk (&body, CHUNK_END_ELEMENT, 16, 24); + put32 (&body, 1); + put32 (&body, UT32_MAX); + put32 (&body, UT32_MAX); + put32 (&body, 2); + + put_chunk (&body, CHUNK_END_NAMESPACE, 16, 24); + put32 (&body, 1); + put32 (&body, UT32_MAX); + put32 (&body, 0); + put32 (&body, 1); + + put_xml_header (b, (ut32)(8 + body.size)); + memcpy (b->bytes + b->size, body.bytes, body.size); + b->size += body.size; +} + +static bool test_axml_decode_manifest(void) { + AxmlBuf b = {{ 0 }, 0}; + put_manifest (&b); + char *s = r_axml_decode (b.bytes, b.size, NULL); + mu_assert_notnull (s, "manifest must decode"); + mu_assert_streq_free (s, "\n\n", "decoded manifest"); + mu_end; +} + +static bool test_axml_string_index_past_pool_chunk(void) { + AxmlBuf b = {{ 0 }, 0}; + put_xml_header (&b, 80); + // 32 byte pool chunk, but the file claims a million strings + put_small_string_pool (&b, 32, 0x00100000); + put_start_element (&b, 32, 4, 0); + pad (&b, 80); + char *s = r_axml_decode (b.bytes, b.size, NULL); + mu_assert_notnull (s, "oversized string index must not read past the pool chunk"); + free (s); + mu_end; +} + +static bool test_axml_string_pool_chunk_too_small(void) { + AxmlBuf b = {{ 0 }, 0}; + put_xml_header (&b, 80); + // a 12 byte pool chunk does not even hold the pool header + put_chunk (&b, CHUNK_STRING_POOL, 28, 12); + put32 (&b, 4); // string_count + put_start_element (&b, 32, 0, 0); + pad (&b, 80); + char *s = r_axml_decode (b.bytes, b.size, NULL); + mu_assert_notnull (s, "truncated pool chunk must not be parsed as a pool header"); + free (s); + mu_end; +} + +static bool test_axml_element_chunk_too_small(void) { + AxmlBuf b = {{ 0 }, 0}; + put_xml_header (&b, 80); + put_small_string_pool (&b, 32, 4); + // a 16 byte element chunk stops before attribute_count + put_start_element (&b, 16, 0, 0); + pad (&b, 80); + char *s = r_axml_decode (b.bytes, b.size, NULL); + mu_assert_null (s, "truncated element chunk must be rejected"); + mu_end; +} + +static bool test_axml_attributes_past_element_chunk(void) { + AxmlBuf b = {{ 0 }, 0}; + put_xml_header (&b, 120); + put_small_string_pool (&b, 32, 4); + // 48 bytes only hold the element header, not two 20 byte attributes + put_start_element (&b, 48, 0, 2); + pad (&b, 120); + char *s = r_axml_decode (b.bytes, b.size, NULL); + mu_assert_null (s, "attribute count must be bound by the element chunk"); + mu_end; +} + +int all_tests(void) { + mu_run_test (test_axml_decode_manifest); + mu_run_test (test_axml_string_index_past_pool_chunk); + mu_run_test (test_axml_string_pool_chunk_too_small); + mu_run_test (test_axml_element_chunk_too_small); + mu_run_test (test_axml_attributes_past_element_chunk); + return tests_passed != tests_run; +} + +int main(int argc, char **argv) { + return all_tests (); +}