Fenrir fixes 2026 06 10#792
Merged
mattia-moffa merged 37 commits intoJun 11, 2026
Merged
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #792
Scan targets checked: wolfboot-bugs, wolfboot-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
…ted vault A power fault during cache_commit(0) can leave a node header in the PSA keyvault with valid magic/tok/obj/type but pos left as erased flash (WOLFPSA_INVALID_ID). find_object_buffer() detects the data-sector mismatch and calls delete_object(), which reaches bitmap_put(0xFFFFFFFF, 0). bitmap_put computed octet = pos/8 and indexed cached_sector[4 + octet] with no bounds check, writing ~512 MB past the static sector buffer. Reject pos >= KEYVAULT_MAX_ITEMS so a corrupted header can no longer turn into an out-of-bounds write. This is the psa_store.c sibling of the pkcs11_store.c fix in F-4965. Adds a unit test that seeds a node with pos=WOLFPSA_INVALID_ID and confirms delete_object() no longer faults.
…sor wrap pci_enum_next_aligned32 only validates the aligned start address against the pool limit, not the end of the allocated region. A malformed PCI device advertising a 2GB non-prefetchable 32-bit MMIO BAR (bar_align 0x80000000, length 0x80000000) gets an aligned start of 0x80000000 that passes the start-only limit check, but the cursor update *base = bar_value + length wraps to 0 in uint32_t. The wrapped cursor lands subsequent BARs at address 0 (IVT/BIOS region) and makes pci_program_bridge program a spurious 2GB MMIO window via (info->mem - 1) >> 16 = 0xFFFF. Add an explicit check in pci_program_bar that the whole region [bar_value, bar_value + length) fits below the pool limit without overflowing 32 bits, mirroring the guards already present for the zero-align and oversized-IO cases. Add a unit test that fails before the fix (the oversized BAR is accepted and the cursor wraps to 0).
…argument disk_read and disk_write divided the 64-bit byte offset by the block size into a uint32_t block_addr, silently truncating addresses at or above 2 TB (LBA >= 2^32). A crafted GPT partition whose first LBA is >= 2^32 passes the 64-bit bounds check in disk_part_read but wraps the SD command argument, redirecting reads/writes to an unintended in-range sector (e.g. sector 0). The SD/SDHC command argument register (SDHCI_SRS02) is inherently 32-bit, so such addresses are not hardware-addressable; reject them instead of wrapping.
…open disk_open() computed bytes_left = n_part * array_sz from the GPT header and scanned the whole declared partition-entry array (one disk_read per 512-byte chunk) to compute its CRC32 *before* comparing against ptable.part_crc. Both n_part and array_sz are taken verbatim from the GPT header, whose only gate is a header CRC32 the attacker can freely recompute. A crafted header with e.g. n_part=0xFFFFFFFF forces ~10^9 disk reads before the mismatch is detected: a pre-auth denial of service that can trip a watchdog and block boot. Reject the header when n_part * array_sz exceeds GPT_MAX_PART_ENTRIES (128, the UEFI default) * GPT_PART_ENTRY_SIZE before entering the scan loop. The bound is generous enough for any standard table (128 * 128 = 16 KiB) and for the existing oversized-array test cases, but caps the scan at 64 sectors. Add a regression test that crafts a header with a valid header CRC and an 8 MB declared array and asserts disk_open performs no partition-array reads.
…flow The protective-MBR lba_first field (attacker-controlled uint32_t, no range check) was multiplied by GPT_SECTOR_SIZE (int 512) in disk_open. C's usual arithmetic conversions made the product a 32-bit unsigned, which wraps for gpt_lba >= 0x800000 (512 * 0x800001 -> 0x200), silently redirecting the GPT header read back to LBA 1 instead of the out-of-range LBA named. Cast the constant to uint64_t so the byte offset is computed in 64 bits. Add a unit test that points lba_first at an overflowing LBA and confirms disk_open now rejects it.
…flow The alignment round-up current_offset = (current_offset + p_align - 1) & ~(p_align - 1) used p_align straight from a (possibly crafted) program header without bounds. For ELF64 a p_align near UINT64_MAX wraps the sum to a tiny value (e.g. 0x78 -> 0); for ELF32 a value that rounds the offset past 2^32 is silently truncated when stored into the uint32_t offset field. Either way segment data is written at a wrong (often zero) file offset, clobbering the output ELF header while squashelf still reports success. Reject such inputs: bound the round-up against UINT64_MAX before applying it (both classes) and reject an ELF32 offset that exceeds UINT32_MAX. Normal page-aligned segments are unaffected. Adds test-align-overflow.py covering the ELF64 wrap, the ELF32 truncation, and the normal-segment regression.
mb2_build_boot_info_header only rejected header_length below the size of struct mb2_header; an oversized value (e.g. 0xFFFFFFFF) made tags_len wrap to ~4GB so mb2_find_tag_by_type's end pointer was inflated far past the image, defeating its size guards and allowing an out-of-bounds read on 64-bit builds. Per the Multiboot2 spec the header must lie within the first 32 KiB of the image, so reject header_length > MB2_HEADER_MAX_OFF.
…2_t wrap uart_flash_erase/read/write read attacker-controlled address and len as raw 4-byte words from the UART peer and guard the mmap region with `address + len > FIRMWARE_PARTITION_SIZE + SWAP_SIZE`. The addition is done in uint32_t, so a large len (e.g. 0xFFFFFFFF) wraps the sum below 0x21000 and the guard passes, after which the loop walks far past the 0x21000-byte mapping — SIGSEGV on a 64-bit host, or wrapped writes over the firmware header on 32-bit. Compute address + len in uint64_t before the comparison so the sum cannot wrap. Add a pty-driven regression test (test_overflow) and a `make test` target that sends an ERASE with a wrapping address+len and asserts the server survives.
A crafted GPT entry with pe->last = UINT64_MAX passes the existing geometry guards (first>last, last==0), but the byte-offset conversion ((pe->last + 1) * GPT_SECTOR_SIZE) - 1 wraps to UINT64_MAX. The bogus part->end then defeats the 'start > p->end' bound in disk_part_read(), neutralising partition isolation. Reject any pe->last >= UINT64_MAX / GPT_SECTOR_SIZE before the multiply; since pe->first <= pe->last this also bounds the part->start computation.
…rings+size_dt_strings uint32_t overflow fdt_data_size_() added the two FDT header uint32_t fields without overflow protection; a crafted FDT with off_dt_strings+size_dt_strings>=2^32 caused the sum to wrap to zero. On 32-bit MMU targets (Cortex-M, RV32, PPC32) the pointer arithmetic in fdt_splice_string_ and fdt_find_add_string_ also wraps, placing 'p' and 'new' at the start of the FDT buffer. All bounds checks in fdt_splice_ then pass (p==end==fdt, oldlen==0, newlen<totalsize), and the subsequent memcpy writes the property-name string directly over the FDT header, corrupting magic, totalsize, and struct offsets. Fix by computing the sum in 64-bit in fdt_data_size_ and returning -FDT_ERR_BADOFFSET on overflow; add a symmetric early-return overflow check in fdt_splice_string_ before the pointer is formed; and propagate the error through fdt_shrink so it does not silently store a zero totalsize.
…%PAGE_SIZE!=0 The loop condition `p < end_address` excluded the final page whenever len % FLASH_PAGE_SIZE != 0: end_address landed exactly on that page's start offset and the strict less-than guard skipped it. Change to `p <= end_address` to match the intended inclusive-end semantics. Add a unit test (test_erase_unaligned_len_covers_last_page) to the existing unit-flash-erase-h7 harness that verifies len=PAGE_SIZE+1 erases two pages, not one.
…y before every return Add hal_uds_zeroize (volatile byte-loop, same pattern as hal_dice_zeroize in the WOLFBOOT_DICE_HW block) and call it to clear the SHA digest and uuid_be stack arrays on both error and success paths, preventing UDS key material and UUID bytes from remaining on the stack after the function returns.
The short-circuit `(match_id < 0) &&` caused the constant-time comparison to be skipped for all slots after the first match, leaking which trust-anchor hash matched via timing despite the function's comment promising otherwise. Evaluate the CT comparison unconditionally each iteration and update match_id only when no prior match was found.
Three related defects: - panic() halted with a single hlt instruction (no loop), so any resumable interrupt (LAPIC timer via iretq) caused it to return, allowing callers to continue executing. Add while(1) and declare __attribute__((noreturn)) in both definition and header. - x86_paging_setup_ptp guarded with == WOLFBOOT_PTP_NUM instead of >=, so if the counter ever exceeded that value (after a panic() return) the guard was permanently bypassed. - The ptp pointer was computed before the bounds check, creating an out-of-bounds pointer for one-past-end indices; move the assignment to after the guard so no invalid pointer is ever formed. Add unit-x86-paging-oob test that sets page_table_page_used to WOLFBOOT_PTP_NUM and verifies that every subsequent call to x86_paging_setup_ptp triggers panic (via longjmp stub) rather than silently proceeding with an out-of-bounds memset.
Replace offset > WCS_FWTPM_NV_SIZE with offset >= WCS_FWTPM_NV_SIZE in all three NV backend helpers so that offset == NV_SIZE returns BAD_FUNC_ARG for any size, including 0. Add unit-fwtpm-nv-oob to prove the boundary.
wc_MlDsaKey_Free does not zero the struct; add wc_ForceZero(&key, sizeof(key)) immediately after, matching the explicit pattern already used in the adjacent keygen_xmss function.
wc_ed448_free does not zero the struct; add wc_ForceZero(&k, sizeof(k)) immediately after, matching the pattern used for ml_dsa (F-4972) and the priv[] zeroing already present on the preceding line.
wc_ecc_free does not zero the struct; add wc_ForceZero(&k, sizeof(k)) immediately after, matching the pattern used for ed448 (F-4971), ml_dsa (F-4972), and the d[] / priv_der[] zeroing already present on the following lines.
When hal_flash_write is called with len=1 on a FLASH_CFI_WIDTH=16 device, nwords = 1/2 = 0 by integer truncation. The AMD CFI write-buffer protocol then receives (uint16_t)(0-1)=0xFFFF as the word count, the data loop runs zero times, and the confirm is issued with no data, silently dropping the write and potentially leaving the device in an invalid state. Add a read-modify-write path for the nwords==0 case: read the containing 16-bit word, update the target byte (preserving the other byte), and issue a correct single-word write-buffer sequence (word count 0 = 1 word).
…hosts sign_tool_find_header returns a raw pointer into LE-encoded TLV bytes. Aliasing it as uint32_t* and dereferencing produces the wrong value on big-endian build hosts (e.g. version 5 read as 0x05000000). Decode explicitly byte-by-byte like header_store_u32_le does for writes.
…ual key material When an existing object is reopened in write mode and the new payload is shorter than the old one, bytes beyond the new payload end were never erased. wolfPSA_Store_Write only commits sectors covered by the new write, so the trailing bytes from the previous (longer) key persisted in flash and were recoverable via raw flash read. Add an erase pass over every sector of the KEYVAULT_OBJ_SIZE region in wolfPSA_Store_Open when opening an existing object for writing. Only the tok/obj-id prefix of the first sector is restored; the rest is left erased (0xFF) so a subsequent wolfPSA_Store_Write starts from a clean slate. New objects created by create_object are already clean and are skipped via the is_new flag. Add unit test test_shorter_overwrite_clears_tail that confirms the old 0xAA pattern is absent from tail bytes after a 200-byte key is overwritten with a 32-byte key.
… residual key material When an existing object was reopened in write mode, only hdr->size was reset; the data sectors were never erased. A subsequent shorter write left the prior key bytes in flash beyond hdr->size (physically readable via JTAG/SWD). Mirror the fix applied to psa_store in F-4784: track is_new, and when opening a pre-existing object in write mode erase every sector of the object region via cache_commit (0xFF fill, tok/obj id re-written in sector 0) before the size record is truncated.
security_command_passphrase used strlen(passphrase) on a 32-byte binary buffer from TPM unsealing that carries no null-terminator guarantee, causing an OOB stack read whenever none of the 32 key bytes is zero. Replace strlen with strnlen(passphrase, ATA_SECURITY_PASSWORD_LEN) using a new constant (32, matching the ATA-8 ACS password field size) defined in ata.h. Also add a size check in sata_unlock_disk after sata_get_unlock_secret so a short or malformed unseal result is rejected before reaching the ATA command path.
Replace bare exit(1) calls with goto cleanup / exit_code pattern matching keygen_ml_dsa; wc_XmssKey_Free + wc_ForceZero now run on every error path after wc_XmssKey_Init succeeds, preventing XMSS private-state exposure.
Replace bare exit(1) calls with goto cleanup / exit_code pattern matching keygen_xmss; wc_LmsKey_Free + wc_ForceZero now run on every error path after wc_LmsKey_Init succeeds, preventing LMS private-state exposure.
…_size The collision guard in elf_load_image_mmu compared vaddr+file_size against the next unread program header, but the subsequent BSS-zero memset writes up to vaddr+mem_size. When mem_size > file_size and headers are not stack-cached (entry_count > ELF_MAX_PH), the memset could corrupt unread program headers. Change the guard to use mem_size so the full write range is checked.
…on 32-bit-long platforms strtol saturates to LONG_MAX (INT32_MAX) and sets errno=ERANGE for version strings above 2147483647 on Windows LLP64 and 32-bit hosts, silently encoding the wrong version. strtoul covers the full uint32_t range on all platforms (ULONG_MAX >= UINT32_MAX). Add explicit out-of-range error to match existing pattern (lines 2063-2071).
Adds test_emergency_rollback_equal_versions which exercises the cur_ver == upd_ver path in the rollback_needed gate (update_flash.c:952). The existing test only used cur_ver=2/upd_ver=1 (strictly greater), so the >= → > mutation survived; this test catches it by asserting IMG_STATE_SUCCESS after a same-version rollback.
danielinux
force-pushed
the
fenrir-fixes-2026-06-10
branch
from
87c5b02 to
82142f8
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #792
Scan targets checked: wolfboot-bugs, wolfboot-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
F-4647 added an exact-size guard in sata_unlock_disk():
if (secret_size != ATA_UNLOCK_DISK_KEY_SZ) { r = -1; goto cleanup; }
on the assumption that the disk unlock secret is a fixed
ATA_UNLOCK_DISK_KEY_SZ (32) byte key. That assumption is wrong: the
secret is a variable-length base64 string produced from
ATA_SECRET_RANDOM_BYTES (21) random bytes, i.e. 28 encoded chars plus a
NUL = 29 bytes. The guard therefore rejected every valid unseal result
(29 != 32) and panicked on every boot, failing the fsp_qemu_test CI job
("Secret 29 bytes" -> "wolfBoot: PANIC!").
The Fenrir F-4647 finding was only partly off: the real issue it
flagged -- an out-of-bounds read from an unbounded copy of a
non-NUL-terminated passphrase buffer -- is genuine and remains fixed in
security_command_passphrase() (the passphrase copy is length-bounded to
ATA_SECURITY_PASSWORD_LEN). Only the additional sata_unlock_disk size
guard was based on a wrong premise.
Remove the guard and document why none belongs there: the secret is a
variable-length string (so an equality check is wrong), and a size guard
is unnecessary because sata_get_unlock_secret() passes sizeof(secret) as
the wolfBoot_unseal() capacity, so secret_size can never exceed the
buffer.
mattia-moffa
approved these changes
Jun 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
82142f8 F-4412: add equal-version emergency rollback test to pin >= operator
9e390e8 F-4414: replace strtol with strtoul for fw_version to fix saturation on 32-bit-long platforms
ad3bc88 F-4418: fix BSS-clear collision guard to use mem_size instead of file_size
aebecba F-4419: zeroize LmsKey on all error paths in keygen_lms
ef16945 F-4420: zeroize XmssKey on all error paths in keygen_xmss
63b58c5 F-4647: bound strlen to ATA_SECURITY_PASSWORD_LEN in passphrase path
a24f107 F-4649: erase pkcs11_store object sectors on write-mode open to clear residual key material
2a3cbf2 F-4784: erase psa_store object data on write-mode open to clear residual key material
051c41c F-4786: fix delta_base_version LE decode in base_diff for big-endian hosts
5958185 F-4788: zeroize stack ed25519_key after Free in keygen_ed25519 cleanup
3349bef F-4966: fix nwords=0 truncation on sub-word write to 16-bit NOR flash
793cec4 F-4969: zeroize stack RsaKey after FreeRsaKey in keygen_rsa cleanup
ca3e76a F-4970: zeroize stack ecc_key after Free in keygen_ecc cleanup
652febd F-4971: zeroize stack ed448_key after Free in keygen_ed448 cleanup
f20cd30 F-4972: zeroize stack wc_MlDsaKey after Free in keygen_ml_dsa cleanup
87fc929 F-5092: fix off-by-one in fwtpm_nv_read/write/erase offset guard
286581c F-5093: fix OOB memset in x86_paging_setup_ptp and non-looping panic
712f389 F-5348: always evaluate keyslot_CT_hint_matches for every slot
d3bd09f F-5351: test wolfBoot_open_self_address rejects bad magic with hdr_ok cleared
c165905 F-5357: zeroize digest and uuid_be stack buffers in hal_uds_derive_key before every return
2b0fde4 F-5745: fix off-by-one in hal_flash_erase skipping last page when len%PAGE_SIZE!=0
f7e7733 F-5747: guard fdt_data_size_ and fdt_splice_string_ against off_dt_strings+size_dt_strings uint32_t overflow
3423173 F-4258: reject GPT partitions whose LBA extent overflows uint64_t
71545e9 F-4339: widen uart-flash-server bounds checks to 64-bit to stop uint32_t wrap
6a60ea3 F-4651: bound multiboot2 header_length to prevent OOB tag walk
0f01a9d F-4692: guard squashelf segment-offset alignment against integer overflow
93a8fe8 F-4714: compute GPT header offset in 64 bits to prevent uint32_t overflow
e0f271b F-4715: bound GPT partition-entry array size before CRC scan in disk_open
306a304 F-4716: reject SDHCI block addresses that overflow 32-bit SD command argument
b3cfb9e F-4717: reject PCI BARs whose region overruns the pool to prevent cursor wrap
55e13b5 F-4782: bound hdr->pos in bitmap_put to prevent OOB write from corrupted vault