Merge pull request #802 from rizlik/fix_oob_ramboot

fix: bound unauthenticated image size before RAM load

Author: dgarske

src/libwolfboot.c

@@ -2376,6 +2376,17 @@ int RAMFUNCTION ext_flash_decrypt_read(uintptr_t address, uint8_t *data, int len
 #endif /* __WOLFBOOT */
 
 #if defined(MMU)
+/* The decrypt loop in wolfBoot_ram_decrypt() copies in ENCRYPT_BLOCK_SIZE
+ * granules, rounding the final write up to a block boundary. The size bound is
+ * only safe if the RAM load region is itself block-aligned; assert that where
+ * the region size is a compile-time constant (a macro in every such config). */
+#if defined(WOLFBOOT_FIXED_PARTITIONS) && defined(WOLFBOOT_PARTITION_SIZE)
+typedef char wolfBoot_ramboot_blockalign_check[
+    ((WOLFBOOT_PARTITION_SIZE) % ENCRYPT_BLOCK_SIZE == 0) ? 1 : -1];
+#elif defined(WOLFBOOT_RAMBOOT_MAX_SIZE)
+typedef char wolfBoot_ramboot_blockalign_check[
+    ((WOLFBOOT_RAMBOOT_MAX_SIZE) % ENCRYPT_BLOCK_SIZE == 0) ? 1 : -1];
+#endif
 /**
  * @brief Decrypt data from RAM.
  *
@@ -2406,7 +2417,32 @@ int wolfBoot_ram_decrypt(uint8_t *src, uint8_t *dst)
         wolfBoot_printf("Error decrypting header at %p!\n", src);
         return -1;
     }
-    len = *((uint32_t*)(dec_hdr + sizeof(uint32_t)));
+    /* dec_hdr is a byte buffer: copy the little-endian length field without an
+     * unaligned cast, then convert to native byte order. */
+    XMEMCPY(&len, dec_hdr + sizeof(uint32_t), sizeof(len));
+    len = im2n(len);
+
+#if !defined(WOLFBOOT_FIXED_PARTITIONS) && !defined(WOLFBOOT_RAMBOOT_MAX_SIZE)
+#  error "WOLFBOOT_FIXED_PARTITIONS or WOLFBOOT_RAMBOOT_MAX_SIZE required to bound the RAM load"
+#endif
+    /* Bound the UNAUTHENTICATED image length before it drives the copy into the
+     * RAM load region: the image is loaded to RAM before its signature is
+     * verified, so this length (read from the not-yet-authenticated header) is
+     * attacker-influenceable and must be range checked first. When both are
+     * configured, WOLFBOOT_RAMBOOT_MAX_SIZE takes precedence: it is the explicit
+     * cap on the RAM load region and may be tighter than the partition size. */
+#if defined(WOLFBOOT_RAMBOOT_MAX_SIZE)
+    if (len > WOLFBOOT_RAMBOOT_MAX_SIZE) {
+        wolfBoot_printf("Invalid encrypted image size %u at %p\n", len, src);
+        return -1;
+    }
+#elif defined(WOLFBOOT_FIXED_PARTITIONS)
+    if (WOLFBOOT_PARTITION_SIZE <= IMAGE_HEADER_SIZE ||
+            len > (uint32_t)(WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE)) {
+        wolfBoot_printf("Invalid encrypted image size %u at %p\n", len, src);
+        return -1;
+    }
+#endif
 
     /* decrypt content */
     while (dst_offset < (len + IMAGE_HEADER_SIZE)) {

src/update_ram.c

@@ -91,16 +91,23 @@ int wolfBoot_ramboot(struct wolfBoot_image *img, uint8_t *src, uint8_t *dst)
 
     /* determine size of partition */
     img_size = wolfBoot_image_size((uint8_t*)dst);
-#if defined(WOLFBOOT_NO_PARTITIONS)
-#  ifndef WOLFBOOT_RAMBOOT_MAX_SIZE
-#    error "WOLFBOOT_RAMBOOT_MAX_SIZE required when WOLFBOOT_NO_PARTITIONS=1"
-#  endif
+#if !defined(WOLFBOOT_FIXED_PARTITIONS) && !defined(WOLFBOOT_RAMBOOT_MAX_SIZE)
+#  error "WOLFBOOT_FIXED_PARTITIONS or WOLFBOOT_RAMBOOT_MAX_SIZE required to bound the RAM load"
+#endif
+    /* Bound the UNAUTHENTICATED image length before it drives the copy into the
+     * RAM load region: the image is loaded to RAM before its signature is
+     * verified, so this length (read from the not-yet-authenticated header) is
+     * attacker-influenceable and must be range checked first. When both are
+     * configured, WOLFBOOT_RAMBOOT_MAX_SIZE takes precedence: it is the explicit
+     * cap on the RAM load region and may be tighter than the partition size. */
+#if defined(WOLFBOOT_RAMBOOT_MAX_SIZE)
     if (img_size > WOLFBOOT_RAMBOOT_MAX_SIZE) {
         wolfBoot_printf("Invalid image size %u at %p\n", img_size, src);
         return -1;
     }
-#elif defined(WOLFBOOT_PARTITION_SIZE)
-    if (img_size > (WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE)) {
+#elif defined(WOLFBOOT_FIXED_PARTITIONS)
+    if (WOLFBOOT_PARTITION_SIZE <= IMAGE_HEADER_SIZE ||
+            img_size > (uint32_t)(WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE)) {
         wolfBoot_printf("Invalid image size %u at %p\n", img_size, src);
         return -1;
     }

tools/unit-tests/Makefile

@@ -51,7 +51,7 @@ TESTS:=unit-parser unit-fdt unit-extflash unit-string unit-spi-flash unit-aes128
        unit-enc-nvm-flagshome unit-delta unit-gzip unit-update-flash unit-update-flash-delta \
        unit-update-flash-hook \
        unit-update-flash-self-update \
-       unit-update-flash-enc unit-update-ram unit-update-ram-nofixed unit-update-ram-noramboot unit-update-flash-hwswap unit-pkcs11_store unit-psa_store unit-disk \
+       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-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 \
@@ -165,6 +165,20 @@ unit-update-ram-noramboot:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN -DUNIT_TE
 	-DWOLFBOOT_HASH_SHA256 -DPRINTF_ENABLED -DEXT_FLASH -DPART_UPDATE_EXT \
 	-DPART_SWAP_EXT -DPART_BOOT_EXT -DWOLFBOOT_DUALBOOT -DNO_XIP -DWOLFBOOT_NO_RAMBOOT \
 	-DWOLFBOOT_ORIGIN=MOCK_ADDRESS_BOOT -DBOOTLOADER_PARTITION_SIZE=WOLFBOOT_PARTITION_SIZE
+unit-update-ram-enc:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN -DUNIT_TEST_AUTH \
+	-DWOLFBOOT_HASH_SHA256 -DPRINTF_ENABLED -DEXT_FLASH -DPART_UPDATE_EXT \
+	-DPART_SWAP_EXT -DPART_BOOT_EXT -DWOLFBOOT_DUALBOOT -DNO_XIP -DMMU \
+	-DEXT_ENCRYPTED -DENCRYPT_WITH_CHACHA -DHAVE_CHACHA \
+	-DWOLFBOOT_ORIGIN=MOCK_ADDRESS_BOOT -DBOOTLOADER_PARTITION_SIZE=WOLFBOOT_PARTITION_SIZE
+# Same source as unit-update-ram-enc, but with a WOLFBOOT_RAMBOOT_MAX_SIZE that
+# is tighter than the partition cap (NO_PARTITIONS / RAM-load-region flavour),
+# so the bound check is exercised through its RAMBOOT_MAX_SIZE branch.
+unit-update-ram-enc-nopart:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN -DUNIT_TEST_AUTH \
+	-DWOLFBOOT_HASH_SHA256 -DPRINTF_ENABLED -DEXT_FLASH -DPART_UPDATE_EXT \
+	-DPART_SWAP_EXT -DPART_BOOT_EXT -DWOLFBOOT_DUALBOOT -DNO_XIP -DMMU \
+	-DEXT_ENCRYPTED -DENCRYPT_WITH_CHACHA -DHAVE_CHACHA \
+	-DWOLFBOOT_NO_PARTITIONS -DWOLFBOOT_RAMBOOT_MAX_SIZE=0x1000 \
+	-DWOLFBOOT_ORIGIN=MOCK_ADDRESS_BOOT -DBOOTLOADER_PARTITION_SIZE=WOLFBOOT_PARTITION_SIZE
 unit-update-ram-nofixed:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN \
 	-DUNIT_TEST_AUTH -DWOLFBOOT_HASH_SHA256 -DPRINTF_ENABLED -DEXT_FLASH \
 	-DPART_UPDATE_EXT -DPART_SWAP_EXT -DPART_BOOT_EXT -DWOLFBOOT_DUALBOOT \
@@ -473,6 +487,18 @@ unit-update-flash-enc: ../../include/target.h unit-update-flash.c
 unit-update-ram: ../../include/target.h unit-update-ram.c
 	gcc -o $@ unit-update-ram.c ../../src/image.c $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sha256.c $(CFLAGS) $(LDFLAGS)
 
+unit-update-ram-enc: ../../include/target.h unit-update-ram-enc.c
+	gcc -o $@ unit-update-ram-enc.c \
+		$(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sha256.c \
+		$(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/chacha.c \
+		$(CFLAGS) $(LDFLAGS)
+
+unit-update-ram-enc-nopart: ../../include/target.h unit-update-ram-enc.c
+	gcc -o $@ unit-update-ram-enc.c \
+		$(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sha256.c \
+		$(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/chacha.c \
+		$(CFLAGS) $(LDFLAGS)
+
 unit-update-ram-nofixed: ../../include/target.h unit-update-ram-nofixed.c
 	gcc -o $@ unit-update-ram-nofixed.c ../../src/image.c $(WOLFBOOT_LIB_WOLFSSL)/wolfcrypt/src/sha256.c $(CFLAGS) $(LDFLAGS)