Hi Anton,
On 2026-05-27T11:23:38, Anton Ivanov <[email protected]> wrote:
> image-fit-sig: Validate hashed-strings region size
>
> fit_config_check_sig() reads the hashed-strings property and uses
> its size value without validation when building the region list for
> signature verification. A crafted FIT image can specify an arbitrary
> size, causing the hash calculation to read beyond the end of the FIT
> image.
>
> Validate that the declared strings region fits within the FIT
> before adding it to the region list.
>
> Signed-off-by: Anton Ivanov <[email protected]>
>
> boot/image-fit-sig.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
> diff --git a/boot/image-fit-sig.c b/boot/image-fit-sig.c
> @@ -512,8 +512,18 @@ static int fit_config_check_sig(const void *fit, int
> noffset, int conf_noffset,
> * The strings region offset must be a static 0x0.
> * This is set in tool/image-host.c
> */
> - fdt_regions[count].offset = fdt_off_dt_strings(fit);
> - fdt_regions[count].size = fdt32_to_cpu(strings[1]);
> + int offset = fdt_off_dt_strings(fit);
> + int size = fdt32_to_cpu(strings[1]);
> + /*
> + * The offset should be already validated by fdt_check_header();
> + * validate the size here.
> + */
> + if (size < 0 || size > fdt_totalsize(fit) - offset) {
> + *err_msgp = "Strings region is out of bounds";
> + return -1;
> + }
Thanks for fixing this.
tools/image-host.c writes string_size = fdt_size_dt_strings(fit), so
the tighter bound here is fdt_size_dt_strings(fit) rather than
fdt_totalsize(fit) - offset. The latter still permits a crafted size
that overlaps the structs block while staying within the FIT.
Also, fdt_getprop() is called with a NULL lenp on line 509, so
strings[1] is read without first checking that the property is at
least 2 * sizeof(fdt32_t) bytes long. A crafted FIT could shorten
hashed-strings to make that read go past the property - please can you
validate the property length too while you are here.
fdt_off_dt_strings() and fdt32_to_cpu() both return uint32_t, so the
locals would be better as uint32_t (and the size < 0 check then goes
away). Please move them up with the other locals rather than declaring
them mid-block.
Since this is a security fix, please mention in the commit message
that the OOB read is reachable during signature verification of an
untrusted FIT, and add a test in test/boot/ (or extend an existing
one) that feeds a FIT with an oversized hashed-strings size and checks
that verification fails cleanly.
Regards,
Simon
