From 6e117a5b5058f8fe52a2fcd1771c956656c5b3ee Mon Sep 17 00:00:00 2001 From: Marco Oliverio Date: Tue, 23 Jun 2026 11:27:36 +0200 Subject: [PATCH] update_disk.c: enforce bound over memcpy --- config/examples/zynq7000_sdcard.config | 9 + config/examples/zynqmp_sdcard.config | 9 + src/update_disk.c | 21 +- tools/unit-tests/Makefile | 15 +- tools/unit-tests/unit-update-disk-oob.c | 305 ++++++++++++++++++++++++ 5 files changed, 356 insertions(+), 3 deletions(-) create mode 100644 tools/unit-tests/unit-update-disk-oob.c diff --git a/config/examples/zynq7000_sdcard.config b/config/examples/zynq7000_sdcard.config index 4ddd666cc1..50dcba1a6a 100644 --- a/config/examples/zynq7000_sdcard.config +++ b/config/examples/zynq7000_sdcard.config @@ -41,6 +41,15 @@ ELF=1 # Stage payload at low DDR (clear of wolfBoot at 0x04000000-0x040FFFFF). WOLFBOOT_LOAD_ADDRESS=0x10000000 +# Cap on the RAM load region. The disk image payload is copied to +# WOLFBOOT_LOAD_ADDRESS before its header is authenticated, so the on-disk +# fw_size must be bounded first (see src/update_disk.c). This is an example +# config not tied to a real board, so the cap is just a sane sanity bound: +# 700 MB is far larger than any realistic FIT image yet keeps the load well +# clear of a 32-bit wrap back onto wolfBoot at 0x04000000. Size it to your +# board's DDR (top_of_DDR - WOLFBOOT_LOAD_ADDRESS) for a real target. +WOLFBOOT_RAMBOOT_MAX_SIZE=0x2BC00000 + # DTB load address (Linux only, used by update_disk.c when a FIT image # carries a DTB). Ignored for bare-metal and for the appended-DTB Linux # flow. 16 MB clear of WOLFBOOT_LOAD_ADDRESS. diff --git a/config/examples/zynqmp_sdcard.config b/config/examples/zynqmp_sdcard.config index e5b0c666a0..3eb8beb32a 100644 --- a/config/examples/zynqmp_sdcard.config +++ b/config/examples/zynqmp_sdcard.config @@ -129,6 +129,15 @@ WOLFBOOT_ORIGIN=0x8000000 # Load Partition to RAM Address (Linux kernel loads here) WOLFBOOT_LOAD_ADDRESS?=0x10000000 +# Cap on the RAM load region. The disk image payload is copied to +# WOLFBOOT_LOAD_ADDRESS before its header is authenticated, so the on-disk +# fw_size must be bounded first (see src/update_disk.c). This is an example +# config not tied to a real board, so the cap is just a sane sanity bound: +# 700 MB is far larger than any realistic FIT image yet keeps the load well +# clear of a 32-bit wrap back onto wolfBoot. Size it to your board's DDR +# (top_of_DDR - WOLFBOOT_LOAD_ADDRESS) for a real target. +WOLFBOOT_RAMBOOT_MAX_SIZE=0x2BC00000 + # DTS (Device Tree) load address WOLFBOOT_LOAD_DTS_ADDRESS?=0x1000 diff --git a/src/update_disk.c b/src/update_disk.c index dfcd8e456f..0607e7a71c 100644 --- a/src/update_disk.c +++ b/src/update_disk.c @@ -94,6 +94,10 @@ static uint8_t disk_encrypt_nonce[ENCRYPT_NONCE_SIZE]; #define DISK_BLOCK_SIZE 512 #endif +#if !defined(WOLFBOOT_FSP) && !defined(WOLFBOOT_RAMBOOT_MAX_SIZE) +# error "WOLFBOOT_RAMBOOT_MAX_SIZE required to bound the disk image RAM load" +#endif + #ifdef DISK_ENCRYPT /* Module-level storage for encryption key */ @@ -430,11 +434,26 @@ void RAMFUNCTION wolfBoot_start(void) continue; } + /* Bound the UNAUTHENTICATED image length before it drives the disk + * read into the RAM load region. An attacker controlling the boot + * media could otherwise declare an arbitrary fw_size and overrun the + * load region before any integrity or signature check runs. Mirrors + * the cap update_ram.c applies to RAMBOOT loads; on FSP the tolum + * check below applies in addition (whichever is tighter wins). */ +#ifdef WOLFBOOT_RAMBOOT_MAX_SIZE + if (os_image.fw_size > WOLFBOOT_RAMBOOT_MAX_SIZE) { + wolfBoot_printf("Image size %u exceeds max RAM load size\r\n", + os_image.fw_size); + selected ^= 1; + continue; + } +#endif + #ifdef WOLFBOOT_FSP /* Verify image size fits in low memory */ if (os_image.fw_size > ((uint32_t)(stage2_params->tolum) - (uint32_t)(uintptr_t)load_address)) { - wolfBoot_printf("Image size %d doesn't fit in low memory\r\n", + wolfBoot_printf("Image size %u doesn't fit in low memory\r\n", os_image.fw_size); break; } diff --git a/tools/unit-tests/Makefile b/tools/unit-tests/Makefile index 7bf48e6b7d..068f34fe0f 100644 --- a/tools/unit-tests/Makefile +++ b/tools/unit-tests/Makefile @@ -52,7 +52,7 @@ TESTS:=unit-parser unit-fdt unit-extflash unit-string unit-spi-flash unit-aes128 unit-update-flash-hook \ unit-update-flash-self-update \ unit-update-flash-enc unit-update-ram unit-update-ram-enc unit-update-ram-enc-nopart unit-update-ram-nofixed unit-update-ram-noramboot unit-update-flash-hwswap unit-pkcs11_store unit-psa_store unit-disk \ - unit-update-disk unit-multiboot unit-boot-x86-fsp unit-loader-tpm-init unit-qspi-flash unit-fwtpm-stub unit-tpm-rsa-exp \ + unit-update-disk unit-update-disk-oob unit-multiboot unit-boot-x86-fsp unit-loader-tpm-init unit-qspi-flash unit-fwtpm-stub unit-tpm-rsa-exp \ unit-image-nopart unit-image-sha384 unit-image-sha3-384 unit-store-sbrk \ unit-tpm-blob unit-policy-create unit-policy-sign unit-rot-auth unit-sdhci-response-bits \ unit-sdhci-disk-unaligned unit-sign-encrypted-output \ @@ -186,7 +186,15 @@ unit-update-ram-nofixed:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN \ -DWOLFBOOT_RAMBOOT_MAX_SIZE=WOLFBOOT_PARTITION_SIZE \ -DWOLFBOOT_ORIGIN=MOCK_ADDRESS_BOOT \ -DBOOTLOADER_PARTITION_SIZE=WOLFBOOT_PARTITION_SIZE -unit-update-disk:CFLAGS+=-DMOCK_PARTITIONS -DPRINTF_ENABLED \ +# Bound the non-FSP disk load to this test's 64-byte load_buffer (TEST_PAYLOAD_SIZE), +# the cap update_disk.c now requires; all images here are exactly that size. +unit-update-disk:CFLAGS+=-DMOCK_PARTITIONS -DPRINTF_ENABLED -DWOLFBOOT_RAMBOOT_MAX_SIZE=0x40 \ + -DWOLFBOOT_ORIGIN=MOCK_ADDRESS_BOOT -DBOOTLOADER_PARTITION_SIZE=WOLFBOOT_PARTITION_SIZE +# Non-FSP disk-boot OOB regression (CRIT-03). WOLFBOOT_RAMBOOT_MAX_SIZE is the +# cap the loader applies to the unauthenticated header fw_size before loading to +# RAM, the same bound update_disk.c and update_ram.c enforce. +unit-update-disk-oob:CFLAGS+=-DMOCK_PARTITIONS -DPRINTF_ENABLED \ + -DWOLFBOOT_RAMBOOT_MAX_SIZE=0x1000 \ -DWOLFBOOT_ORIGIN=MOCK_ADDRESS_BOOT -DBOOTLOADER_PARTITION_SIZE=WOLFBOOT_PARTITION_SIZE unit-string:CFLAGS+=-fno-builtin @@ -511,6 +519,9 @@ unit-update-flash-hwswap: ../../include/target.h unit-update-flash-hwswap.c unit-update-disk: ../../include/target.h unit-update-disk.c gcc -o $@ unit-update-disk.c $(CFLAGS) $(LDFLAGS) +unit-update-disk-oob: ../../include/target.h unit-update-disk-oob.c + gcc -o $@ unit-update-disk-oob.c $(CFLAGS) $(LDFLAGS) + unit-pkcs11_store: ../../include/target.h unit-pkcs11_store.c gcc -o $@ $(WOLFCRYPT_SRC) unit-pkcs11_store.c $(CFLAGS) $(WOLFCRYPT_CFLAGS) $(LDFLAGS) diff --git a/tools/unit-tests/unit-update-disk-oob.c b/tools/unit-tests/unit-update-disk-oob.c new file mode 100644 index 0000000000..39ef7d57c5 --- /dev/null +++ b/tools/unit-tests/unit-update-disk-oob.c @@ -0,0 +1,305 @@ +/* unit-update-disk-oob.c + * + * Copyright (C) 2026 wolfSSL Inc. + * + * This file is part of wolfBoot. + * + * wolfBoot is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * wolfBoot is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335, USA + */ + +#define WOLFBOOT_UPDATE_DISK +#define WOLFBOOT_SELF_UPDATE_MONOLITHIC +#define RAM_CODE +#define WOLFBOOT_SELF_HEADER +#define IMAGE_HEADER_SIZE 256 +#define BOOT_PART_A 0 +#define BOOT_PART_B 1 +#define MOCK_ADDRESS_BOOT 0xCD000000 + +#include +#include +#include +#include + +#include "hal.h" +#include "target.h" +#include "wolfboot/wolfboot.h" +#include "image.h" +#include "loader.h" + +/* The secure cap on the RAM load region. Kept in sync with the + * -DWOLFBOOT_RAMBOOT_MAX_SIZE passed by the Makefile so the same value both + * sizes the load buffer here and bounds the loader (src/update_disk.c). */ +#ifdef WOLFBOOT_RAMBOOT_MAX_SIZE +#define DISK_LOAD_MAX WOLFBOOT_RAMBOOT_MAX_SIZE +#else +#define DISK_LOAD_MAX 0x1000 +#endif + +/* How far past the load region the malicious header drives the write, and the + * size of the guard band used to detect it. The overshoot is intentionally + * kept smaller than the canary so the OOB write is contained and observable + * (rather than wandering into unrelated memory). */ +#define OOB_OVERSHOOT 1024 +#define CANARY_SIZE 2048 +#define ATTACKER_FW_SIZE (DISK_LOAD_MAX + OOB_OVERSHOOT) +#define CANARY_FILL 0xCC + +/* Load region followed immediately by a guard band. Members of a single + * struct are laid out in declaration order, so any write past load[] lands + * in canary[] where we can detect it without relying on ASan. */ +static struct { + uint8_t load[DISK_LOAD_MAX]; + uint8_t canary[CANARY_SIZE]; +} region; +#ifdef WOLFBOOT_LOAD_ADDRESS +#undef WOLFBOOT_LOAD_ADDRESS +#endif +#define WOLFBOOT_LOAD_ADDRESS ((uintptr_t)region.load) + +/* Backing store for the partitions. Partition A is sized to the attacker's + * declared image so the mock disk serves every requested byte (modelling a + * large attacker-written partition); partition B is left blank. */ +static uint8_t part_a_image[IMAGE_HEADER_SIZE + ATTACKER_FW_SIZE]; +static uint8_t part_b_image[IMAGE_HEADER_SIZE]; + +static int mock_disk_init_ret; +static int mock_disk_close_called; +static int mock_do_boot_called; +static const uint32_t *mock_boot_address; +static int mock_verify_integrity_ret; +static int mock_verify_authenticity_ret; +static int mock_flash_protect_called; +static haladdr_t mock_flash_protect_addr; +static int mock_flash_protect_len; + +static void build_part_a(uint32_t version, uint32_t fw_size, uint8_t fill) +{ + uint32_t magic = WOLFBOOT_MAGIC; + uint16_t hdr_version = HDR_VERSION; + uint16_t ver_len = 4; + + memset(part_a_image, 0, sizeof(part_a_image)); + memcpy(part_a_image, &magic, sizeof(magic)); + memcpy(part_a_image + sizeof(uint32_t), &fw_size, sizeof(fw_size)); + memcpy(part_a_image + IMAGE_HEADER_OFFSET, &hdr_version, + sizeof(hdr_version)); + memcpy(part_a_image + IMAGE_HEADER_OFFSET + sizeof(uint16_t), &ver_len, + sizeof(ver_len)); + memcpy(part_a_image + IMAGE_HEADER_OFFSET + 2 * sizeof(uint16_t), &version, + sizeof(version)); + memset(part_a_image + IMAGE_HEADER_SIZE, fill, + sizeof(part_a_image) - IMAGE_HEADER_SIZE); +} + +static void reset_mocks(void) +{ + memset(region.load, 0, sizeof(region.load)); + memset(region.canary, CANARY_FILL, sizeof(region.canary)); + memset(part_b_image, 0, sizeof(part_b_image)); + mock_disk_init_ret = 0; + mock_disk_close_called = 0; + mock_do_boot_called = 0; + mock_boot_address = NULL; + mock_verify_integrity_ret = 0; + mock_verify_authenticity_ret = 0; + mock_flash_protect_called = 0; + mock_flash_protect_addr = 0; + mock_flash_protect_len = 0; + wolfBoot_panicked = 0; +} + +static int canary_intact(void) +{ + size_t i; + for (i = 0; i < sizeof(region.canary); i++) { + if (region.canary[i] != CANARY_FILL) + return 0; + } + return 1; +} + +int disk_init(int drv) +{ + (void)drv; + return mock_disk_init_ret; +} + +int disk_open(int drv) +{ + (void)drv; + return 0; +} + +void disk_close(int drv) +{ + (void)drv; + mock_disk_close_called++; +} + +/* Serve bytes from the backing partition, clamped only to the partition's own + * size - exactly like a real disk driver, and crucially NOT to the size of the + * RAM destination. */ +int disk_part_read(int drv, int part, uint64_t off, uint64_t sz, uint8_t *buf) +{ + const uint8_t *image; + uint64_t max; + + (void)drv; + if (part == BOOT_PART_A) { + image = part_a_image; + max = sizeof(part_a_image); + } else { + image = part_b_image; + max = sizeof(part_b_image); + } + if (off >= max) + return -1; + if (sz > (max - off)) + sz = max - off; + memcpy(buf, image + off, (size_t)sz); + return (int)sz; +} + +uint32_t wolfBoot_get_blob_version(uint8_t *blob) +{ + uint32_t magic; + uint32_t version; + + memcpy(&magic, blob, sizeof(magic)); + if (magic != WOLFBOOT_MAGIC) + return 0; + memcpy(&version, blob + IMAGE_HEADER_OFFSET + 2 * sizeof(uint16_t), + sizeof(version)); + return version; +} + +int wolfBoot_open_image_address(struct wolfBoot_image* img, uint8_t* image) +{ + uint32_t magic; + uint32_t fw_size; + + memcpy(&magic, image, sizeof(magic)); + if (magic != WOLFBOOT_MAGIC) + return -1; + memset(img, 0, sizeof(*img)); + img->hdr = image; + memcpy(&fw_size, image + sizeof(uint32_t), sizeof(fw_size)); + img->fw_size = fw_size; + img->fw_base = image + IMAGE_HEADER_SIZE; + img->hdr_ok = 1; + return 0; +} + +int wolfBoot_verify_integrity(struct wolfBoot_image* img) +{ + img->sha_ok = (mock_verify_integrity_ret == 0) ? 1 : 0; + return mock_verify_integrity_ret; +} + +int wolfBoot_verify_authenticity(struct wolfBoot_image* img) +{ + img->signature_ok = (mock_verify_authenticity_ret == 0) ? 1 : 0; + return mock_verify_authenticity_ret; +} + +int wolfBoot_get_dts_size(void *dts_addr) +{ + (void)dts_addr; + return -1; +} + +void hal_prepare_boot(void) +{ +} + +void do_boot(const uint32_t *address) +{ + mock_do_boot_called++; + mock_boot_address = address; +} + +int hal_flash_protect(haladdr_t address, int len) +{ + mock_flash_protect_called++; + mock_flash_protect_addr = address; + mock_flash_protect_len = len; + return 0; +} + +#include "update_disk.c" + +/* A header that declares fw_size larger than the RAM load region must not be + * copied into RAM: the write would overrun the load buffer before the image + * is ever authenticated. The loader must refuse it (panic, no boot) and leave + * the guard band untouched. */ +START_TEST(test_update_disk_oob_rejects_oversize_fw_size) +{ + reset_mocks(); + build_part_a(7, ATTACKER_FW_SIZE, 0xEE); + /* partition B left blank so A is the only candidate */ + + wolfBoot_start(); + + ck_assert_msg(canary_intact(), + "fw_size=%u overran the %u-byte load region into the guard band", + (unsigned)ATTACKER_FW_SIZE, (unsigned)DISK_LOAD_MAX); + ck_assert_int_eq(mock_do_boot_called, 0); + ck_assert_int_gt(wolfBoot_panicked, 0); +} +END_TEST + +/* Boundary companion: an image exactly the size of the load region is valid + * and must still boot. Guards the fix against an off-by-one (it must reject + * strictly-greater-than, not greater-or-equal). */ +START_TEST(test_update_disk_accepts_exact_max_fw_size) +{ + reset_mocks(); + build_part_a(7, DISK_LOAD_MAX, 0xEE); + + wolfBoot_start(); + + ck_assert_msg(canary_intact(), + "exact-max fw_size=%u overran the load region", + (unsigned)DISK_LOAD_MAX); + ck_assert_int_eq(wolfBoot_panicked, 0); + ck_assert_int_eq(mock_do_boot_called, 1); + ck_assert_ptr_eq(mock_boot_address, (const uint32_t *)WOLFBOOT_LOAD_ADDRESS); +} +END_TEST + +Suite *wolfboot_suite(void) +{ + Suite *s = suite_create("wolfBoot"); + TCase *tc = tcase_create("update-disk-oob"); + + tcase_add_test(tc, test_update_disk_oob_rejects_oversize_fw_size); + tcase_add_test(tc, test_update_disk_accepts_exact_max_fw_size); + suite_add_tcase(s, tc); + + return s; +} + +int main(void) +{ + int fails; + Suite *s = wolfboot_suite(); + SRunner *sr = srunner_create(s); + + srunner_run_all(sr, CK_NORMAL); + fails = srunner_ntests_failed(sr); + srunner_free(sr); + return fails; +}