Merge pull request #806 from rizlik/oob_update_disk

update_disk.c: enforce bound over memcpy

Author: dgarske

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.

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
 

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;
         }

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)