From fe1dcb080d61115a144c0f2076a5acd066d2e4b9 Mon Sep 17 00:00:00 2001 From: Daniele Lacamera Date: Thu, 20 Aug 2026 23:59:53 +0200 Subject: [PATCH] F-9757: length-bound the FIT name/compression string properties fit_find_images() took the FIT configuration's image names (kernel/fdt/ramdisk/fpga) and the configuration name (default) straight from fdt_getprop() and passed them on to fdt_find_node_offset(), which strlen()s them; fit_load_image_inner() strcmp()'d the compression property after only checking it was non-empty. A property value not NUL-terminated within its declared length makes those calls scan past the property - and past the end of the blob for a property at the tail. Add fit_getprop_string(), which returns the property value only when it is NUL-terminated within its declared length, and use it for the five name properties (a malformed value is rejected and the type-based search still applies). Compare compression within the declared length: the value must be exactly "gzip" or "none"; any other shape fails closed with the existing unsupported-compression path instead of being strcmp()'d past the property. unit-fdt gains a FIT whose configuration kernel property is unterminated (the valid default is still honored, the image name is rejected); unit-fit-gzip gains a truncated compression="none" value, which used to pass the subimage through as raw and now fails closed. Both build variants (gzip enabled/disabled) run it. --- src/fdt.c | 44 +++++++++++++++------ tools/unit-tests/unit-fdt.c | 67 ++++++++++++++++++++++++++++++++ tools/unit-tests/unit-fit-gzip.c | 39 +++++++++++++++++++ 3 files changed, 139 insertions(+), 11 deletions(-) diff --git a/src/fdt.c b/src/fdt.c index c11ef965..b388d0d1 100644 --- a/src/fdt.c +++ b/src/fdt.c @@ -940,6 +940,22 @@ int fdt_fixup_val64(void* fdt, int off, const char* node, const char* name, /* FIT Specific */ + +/* Returns the property value only when it is a NUL-terminated C string + * within its declared length, else NULL: property values are opaque + * byte arrays and the names taken from them are passed to + * fdt_find_node_offset()/strcmp(), which strlen() them. */ +static const char* fit_getprop_string(const void* fdt, int offset, + const char* name) +{ + int len = 0; + const char* val = (const char*)fdt_getprop(fdt, offset, name, &len); + + if (val == NULL || len <= 0 || memchr(val, '\0', len) == NULL) + return NULL; + return val; +} + const char* fit_find_images(void* fdt, const char** pkernel, const char** pflat_dt, const char** pramdisk, const char** pfpga) { @@ -968,19 +984,16 @@ const char* fit_find_images(void* fdt, const char** pkernel, const char** pflat_ if (conf == NULL) #endif { - val = fdt_getprop(fdt, off, "default", &len); - if (val != NULL && len > 0) { - conf = (const char*)val; - } + conf = fit_getprop_string(fdt, off, "default"); } } if (conf != NULL) { off = fdt_find_node_offset(fdt, -1, conf); if (off > 0) { - kernel = fdt_getprop(fdt, off, "kernel", &len); - flat_dt = fdt_getprop(fdt, off, "fdt", &len); - ramdisk = fdt_getprop(fdt, off, "ramdisk", &len); - fpga = fdt_getprop(fdt, off, "fpga", &len); + kernel = fit_getprop_string(fdt, off, "kernel"); + flat_dt = fit_getprop_string(fdt, off, "fdt"); + ramdisk = fit_getprop_string(fdt, off, "ramdisk"); + fpga = fit_getprop_string(fdt, off, "fpga"); } } if (kernel == NULL) { @@ -1219,11 +1232,20 @@ static void* fit_load_image_inner(void* fdt, const char* image, int* lenp, * raw. */ comp = (const char*)fdt_getprop(fdt, off, "compression", &complen); - if (comp != NULL && complen > 0) { - if (strcmp(comp, "gzip") == 0) { + /* Compare within the declared property length: the value + * must be exactly "gzip" or "none" (NUL-terminated). Any + * other shape - including an unterminated value - fails + * closed instead of being strncmp()'d past the property. */ + if (comp != NULL) { + if (complen == 5 && comp[4] == '\0' && + memcmp(comp, "gzip", 4) == 0) { is_gzip = 1; } - else if (strcmp(comp, "none") != 0) { + else if (complen == 5 && comp[4] == '\0' && + memcmp(comp, "none", 4) == 0) { + /* uncompressed */ + } + else { is_unknown_comp = 1; } } diff --git a/tools/unit-tests/unit-fdt.c b/tools/unit-tests/unit-fdt.c index 516eca3f..5f05b241 100644 --- a/tools/unit-tests/unit-fdt.c +++ b/tools/unit-tests/unit-fdt.c @@ -341,6 +341,72 @@ START_TEST(test_fdt_check_header_rejects_overlapping_areas) } END_TEST +/* FIT whose configuration `kernel` property is not NUL-terminated + * within its declared length: fit_find_images() must still honor the + * valid `default` but reject the malformed image name instead of + * passing it on as a C string. */ +static const uint8_t fit_cfg_unterminated_kernel[] = { + /* header */ + 0xd0, 0x0d, 0xfe, 0xed, /* magic */ + 0x00, 0x00, 0x00, 0xa7, /* totalsize = 167 */ + 0x00, 0x00, 0x00, 0x38, /* off_dt_struct = 56 */ + 0x00, 0x00, 0x00, 0x98, /* off_dt_strings = 152 */ + 0x00, 0x00, 0x00, 0x28, /* off_mem_rsvmap = 40 */ + 0x00, 0x00, 0x00, 0x11, /* version = 17 */ + 0x00, 0x00, 0x00, 0x10, /* last_comp_version = 16 */ + 0x00, 0x00, 0x00, 0x00, /* boot_cpuid_phys */ + 0x00, 0x00, 0x00, 0x0f, /* size_dt_strings = 15 */ + 0x00, 0x00, 0x00, 0x60, /* size_dt_struct = 96 */ + /* mem_rsvmap terminator (offset 40) */ + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + /* struct block (offset 56) */ + 0x00, 0x00, 0x00, 0x01, /* BEGIN_NODE root */ + 0x00, 0x00, 0x00, 0x00, /* "" */ + 0x00, 0x00, 0x00, 0x01, /* BEGIN_NODE */ + 0x63, 0x6f, 0x6e, 0x66, 0x69, 0x67, 0x75, 0x72, + 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x73, 0x00, 0x00, /* "configurations\0" */ + 0x00, 0x00, 0x00, 0x03, /* FDT_PROP */ + 0x00, 0x00, 0x00, 0x07, /* len = 7 */ + 0x00, 0x00, 0x00, 0x00, /* nameoff = 0 ("default") */ + 0x63, 0x6f, 0x6e, 0x66, 0x2d, 0x31, 0x00, 0x00, /* "conf-1\0" */ + 0x00, 0x00, 0x00, 0x01, /* BEGIN_NODE */ + 0x63, 0x6f, 0x6e, 0x66, 0x2d, 0x31, 0x00, 0x00, /* "conf-1\0" */ + 0x00, 0x00, 0x00, 0x03, /* FDT_PROP */ + 0x00, 0x00, 0x00, 0x08, /* len = 8 */ + 0x00, 0x00, 0x00, 0x08, /* nameoff = 8 ("kernel") */ + 0x6b, 0x65, 0x72, 0x6e, 0x65, 0x6c, 0x2d, 0x31, /* "kernel-1" -- no NUL */ + 0x00, 0x00, 0x00, 0x02, /* END_NODE conf-1 */ + 0x00, 0x00, 0x00, 0x02, /* END_NODE configurations */ + 0x00, 0x00, 0x00, 0x02, /* END_NODE root */ + 0x00, 0x00, 0x00, 0x09, /* FDT_END */ + /* strings block (offset 152) */ + 0x64, 0x65, 0x66, 0x61, 0x75, 0x6c, 0x74, 0x00, /* "default\0" */ + 0x6b, 0x65, 0x72, 0x6e, 0x65, 0x6c, 0x00, /* "kernel\0" */ +}; + +START_TEST(test_fit_find_images_rejects_unterminated_image_name) +{ + static uint8_t fit_scratch[sizeof(fit_cfg_unterminated_kernel)]; + const char *conf = NULL, *kern = NULL, *fdt = NULL; + const char *rd = NULL, *fpga = NULL; + + memcpy(fit_scratch, fit_cfg_unterminated_kernel, sizeof(fit_scratch)); + + conf = fit_find_images(fit_scratch, &kern, &fdt, &rd, &fpga); + + /* The valid `default` is still honored... */ + ck_assert_str_eq(conf, "conf-1"); + /* ...but the config's `kernel` property is not NUL-terminated + * within its declared length, so it must be rejected rather than + * passed on to fdt_find_node_offset()/strlen(). */ + ck_assert_ptr_null(kern); + ck_assert_ptr_null(fdt); + ck_assert_ptr_null(rd); + ck_assert_ptr_null(fpga); +} +END_TEST + static Suite *fdt_suite(void) { Suite *s = suite_create("fdt"); @@ -359,6 +425,7 @@ static Suite *fdt_suite(void) tcase_add_test(tc, test_fdt_compatible_prefix_entry_no_match); tcase_add_test(tc, test_fdt_check_header_rejects_unbounded_string_area); tcase_add_test(tc, test_fdt_check_header_rejects_overlapping_areas); + tcase_add_test(tc, test_fit_find_images_rejects_unterminated_image_name); suite_add_tcase(s, tc); tcase_set_timeout(tc_dos, 5); diff --git a/tools/unit-tests/unit-fit-gzip.c b/tools/unit-tests/unit-fit-gzip.c index 373a4be8..7c175fda 100644 --- a/tools/unit-tests/unit-fit-gzip.c +++ b/tools/unit-tests/unit-fit-gzip.c @@ -269,6 +269,44 @@ END_TEST #endif /* WOLFBOOT_GZIP */ +START_TEST(test_fit_to_none_unterminated_fails_closed) +{ + /* The `compression` property is truncated to 4 bytes ("none" with + * no NUL): the declared length no longer covers the terminator. + * Before the fix the value was strcmp()'d and the following byte + * (the dropped NUL) terminated it, so the subimage was passed + * through as raw; a malformed value must fail closed. */ + uint8_t buf[64]; + int len = -1; + void *ret; + static uint8_t fit_scratch[sizeof(fit_with_none_comp)]; + uint8_t *p; + unsigned i; + + memcpy(fit_scratch, fit_with_none_comp, sizeof(fit_scratch)); + + /* the "none" value appears once in the blob (not as a C string - + * scan raw bytes); the property length word sits 8 bytes before + * the data */ + p = NULL; + for (i = 0; i + 4 <= sizeof(fit_scratch); i++) { + if (memcmp(fit_scratch + i, "none", 4) == 0) { + p = fit_scratch + i; + break; + } + } + ck_assert_ptr_nonnull(p); + p[-8] = 0; + p[-7] = 0; + p[-6] = 0; + p[-5] = 4; + + ret = fit_load_image_to(fit_scratch, "kernel-1", + buf, (uint32_t)sizeof(buf), &len); + ck_assert_ptr_null(ret); +} +END_TEST + START_TEST(test_fit_to_lzma_unknown_returns_null) { /* Independent of WOLFBOOT_GZIP - any unknown compression scheme is @@ -320,6 +358,7 @@ static Suite *fit_gzip_suite(void) tcase_add_test(tc, test_fit_to_gzip_disabled_returns_null); #endif tcase_add_test(tc, test_fit_to_lzma_unknown_returns_null); + tcase_add_test(tc, test_fit_to_none_unterminated_fails_closed); tcase_add_test(tc, test_fit_to_none_oversized_rejected); suite_add_tcase(s, tc);