enhance the data content checking on the boot update logic
What the developer wrote
Authored by aaron
enhance the data content checking on the boot update logic
What changed, and why it matters
This commit adds safety checks to the bootloader update code in a cryptocurrency hardware wallet. Before the change, the firmware could trust attacker-controlled length values read from a staged update image, potentially allowing a malicious or malformed update to copy too much data, loop too many times, or write past a fixed-size buffer. The patch validates the boot image length and caps the per-loop copy size, which reduces the risk of memory corruption or unauthorized flash writes during a boot update.
Treat this as a security-hardening commit with likely bug-fix relevance. Review the full boot update flow for additional missing validations: magic number verification, hash/signature verification before any flash write, alignment checks on baseAddr, and validation of all fields parsed from the staged image. Ensure the len variable cannot become negative or exceed SECTOR_SIZE through any code path, and consider fuzzing UpdateBootFromFlash() with malformed headers.
Security signals we found
Length value from untrusted staged image used as loop bound and copy size before validation
Fixed-size 4 KB buffer g_fileUnit copied into with a length derived from external input
Addition of bootLen bounds check against header size and partition size
Addition of defensive len > SECTOR_SIZE guard before memcpy
Code operates in bootloader update path with direct flash erase/write primitives
Evidence from the diff
In UpdateBootFromFlash(), the function reads a 32-bit bootLen from a staged firmware image at g_fileUnit[MAGIC_NUMBER_SIZE + 4] and later uses it to drive a loop that copies sectors from APP_ADDR into a 4 KB g_fileUnit buffer before writing to QSPI flash. The patch adds three checks: (1) bootLen must be at least 0x134 bytes (header size), (2) bootLen must not exceed the physical boot partition size (APP_ADDR - BOOT_ADDR), and (3) the remaining payload after the header must align to sector granularity with at least 4 bytes. It also adds a defensive check that len never exceeds SECTOR_SIZE before the memcpy into g_fileUnit. These changes mitigate out-of-bounds reads/writes and integer underflow in the loop bound calculation, but the patch is partial: it does not validate the hash, magic number, or other metadata, and the len > SECTOR_SIZE check is defensive rather than a complete fix for all length derivations.
Changed components
src/boot_update.cUpdateBootFromFlash()bootloader update pathQSPI flash write pathInspect captured patch +13 / −0
diff --git a/src/boot_update.c b/src/boot_update.c
index d89ea03..6d5bf1e 100644
--- a/src/boot_update.c
+++ b/src/boot_update.c
@@ -80,6 +80,15 @@ int32_t UpdateBootFromFlash(void)
printf("bootLen = %d\n", bootLen);
memcpy(hash, &g_fileUnit[MAGIC_NUMBER_SIZE + 4], 32);
+ // Validate the staged length before it drives any loop bound or copy size.
+ // bootLen must cover the 0x134 header and fit within the boot partition
+ if (bootLen < 0x134 || bootLen > (uint32_t)(APP_ADDR - BOOT_ADDR) ||
+ (bootLen - 0x134) % SECTOR_SIZE < 4) {
+ printf("invalid bootLen = %u\n", bootLen);
+ osKernelUnlock();
+ return -1;
+ }
+
memset(g_fileUnit, 0xFF, sizeof(g_fileUnit));
memcpy(g_fileUnit, (uint32_t *)(baseAddr + 4 + 32 + 0x30 + MAGIC_NUMBER_SIZE), BOOT_HEAD_SIZE);
QspiFlashEraseAndWrite(0x01000000, g_fileUnit, SECTOR_SIZE);
@@ -98,6 +107,10 @@ int32_t UpdateBootFromFlash(void)
len = SECTOR_SIZE;
sha256_update(&ctx, (uint32_t *)(APP_ADDR + i * SECTOR_SIZE), len);
}
+ if (len > SECTOR_SIZE) { // defensive: never copy past the 4 KB g_fileUnit buffer
+ osKernelUnlock();
+ return -1;
+ }
memcpy(g_fileUnit, (uint32_t *)(APP_ADDR + i * SECTOR_SIZE), len);
crcCalc = crc32_ieee(crcCalc, (uint32_t *)(APP_ADDR + i * SECTOR_SIZE), len);
printf("writeAddr = %#x\n", writeAddr);
Why this scored 71/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.