Fenrir fixes 2026 09 30 - #917
Merged
Merged
Conversation
nvm_select_fresh_sector() gets the PART_UPDATE flag base from PART_UPDATE_ENDFLAGS (UPDATE size) but computed addrErase with the BOOT size, a copy-paste from the BOOT branch. The two are equal in every compilable write-once build today (asymmetric sizes are #error'd without SELF_UPDATE_MONOLITHIC, which is incompatible with write-once), so align the erase sector with the base.
wolfSSL-Fenrir-bot
previously requested changes
Sep 30, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #917
Scan targets checked: wolfboot-src, wolfboot-bugs
Coverage: 12 of 14 in-scope changed file(s) opened by the reviewer; not opened: src/libwolfboot.c, tools/tpm/policy_sign.c
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Lite
The parser read the SEQUENCE length without checking it, skipped the AlgorithmIdentifier without comparing it, and ignored trailing bytes, so only the digest length was pinned. Any free byte in the message is forgeable with a low-exponent key (e=3) via Bleichenbacher 2006, and keygen -i imports exponents unchecked. Require the SEQUENCE to span the whole payload, the AlgorithmIdentifier to match the configured hash OID byte for byte, and the OCTET STRING to end at the payload end. Test pins the exact wc_EncodeSignature() encoding and rejects trailing bytes and the empty-algoid forge shape.
wolfBoot_get_partition_state() only rejected PART_NONE, so any other part byte (SWAP, SELF, unknown) passed through the NSC veneer to a NULL dereference of get_partition_magic() in secure code. Reject every part without a trailer at the shared entry; the NSC veneer delegates here, so a non-secure caller can no longer crash the secure world. The mock get_trailer_at() now matches the real one (NULL for parts without a trailer), which is what lets the test prove the deref.
wolfBoot_record_verify_failure() read the image version from img->hdr, which for an external-flash image is a device offset, not a CPU address: the persistent record stored a garbage version, and the read can fault where the offset is unmapped. Read the version through wolfBoot_get_image_version(part), which fetches external headers via ext_flash_check_read. No unit test compiles update_flash.c with WOLFBOOT_PERSIST_FAILURE_STATUS, so add a compile check for that ext-flash configuration to the gate.
qspi_quad_enable() memset the status register it had just read before setting the QE bit, so the write-back zeroed every other writable bit (SRL/CMP in SR2, BP/SRWD on the SR1 path) on the first probe of a part with QE clear. Drop the memset so the write is a read-modify-write. Test pins the preserved-bits write and the no-write sunny path.
mb2_build_boot_info_header() rejected a valid OS image header when the optional information-request tag was missing, and rejected any request type it did not implement without checking the tag's optional flag (bit 0) the spec defines for skipping. A well-formed header without the tag now requests nothing, optional unsupported requests are skipped, and only a mandatory unsupported one rejects the image. The finder returns NULL for a missing tag and for a malformed tag list alike, so the absent case walks the list to tell the two apart: any corrupt tag rejects the header. Tests updated to the spec semantics (optionality driven by the tag flags) plus malformed-list cases.
nsc_mech_prepare() read the non-secure legacy ulPasswordLen pointer twice: once to size the secure password copy, once again (via nsc_in) to fill the secure length word the library dereferences. A concurrent non-secure write between the two reads could desync the length from the password buffer size. Take a single snapshot and write it into the secure length word directly. Add the missing test for the legacy branch: the stub now consumes the length the way wolfPKCS11 does (through the bounced pointer) and checks scrubbing.
loadFile() in the TPM policy-signing tool read the private key through a fully buffered stdio stream, leaving a libc-owned copy of the key material that outlives the call. Open the file unbuffered (setvbuf _IONBF), the same pattern sign.c already uses for its key reads.
The secure-mode TRNG seed helper copied each 24-byte CryptoCell EHR batch into a stack array and returned without clearing it, leaving raw TRNG output in the retired secure frame. Force-zero the batch at the end of each iteration, mirroring the existing scrub in hal/cm4.c.
The secure-mode TRNG seed helper (shared by the STM32L5/U5/H5 TrustZone ports) staged each TRNG_DR word in a stack local and returned without clearing it. Force-zero the local before the return, mirroring the existing scrub in hal/cm4.c.
danielinux
force-pushed
the
fenrir-fixes-2026-09-30
branch
from
September 30, 2026 14:34
fa88dee to
ce814dd
Compare
F-14487/F-14488 scrubbed the TRNG staging buffers with wc_ForceZero, which links the hal objects to wolfCrypt's memory.c. The OTP keystore primer links the hal without wolfCrypt and failed to build (undefined reference to wc_ForceZero) in the stm32h5_tz_dualbank_otp and trustzone-emulator configs. Scrub with a local volatile loop, the same pattern nvm_cache_scrub uses for the same reason.
F-14470 corrected the request-list entry width to the spec's 16-bit, which doubled the entry count. Intel FSP pads the list so the last entry is zero (the end-tag type), and the old 32-bit width never reached it. A zero entry is padding, not a mandatory request: skip it, so FSP images keep booting. New test covers a zero-padded list.
The F-14470 fix changed the info-request entry width to 16-bit on a
misreading of the spec: Multiboot2 v2.0 defines mbi_tag_types as
u32[n] ("an array of u32's"), and the original code already divided
by sizeof(uint32_t). A 32-bit request {4} was misread as {4, 0}, so
the follow-up zero-skip was only papering over the misparse; real
FSP images with a mandatory request tag stopped booting (fsp_qemu
CI). Restore the u32 width, drop the zero-skip, and rebuild the
test fixtures with 32-bit entries.
wolfSSL-Fenrir-bot
previously requested changes
Sep 30, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #917
Scan targets checked: wolfboot-src, wolfboot-bugs
Coverage: 6 of 6 in-scope changed file(s) opened by the reviewer
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Lite
mb2_parse_info_request_tag() (DEBUG_MB2) still divided by sizeof(uint16_t) after the u32 revert, doubling the entry count and reading (size-8) bytes past the tag. Same width, one more call site.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #917
Scan targets checked: wolfboot-src, wolfboot-bugs
Coverage: 1 of 1 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
wolfSSL-Fenrir-bot
dismissed stale reviews from themself
September 30, 2026 20:00
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
dgarske
approved these changes
Sep 30, 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
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.
ce814dd F-14488: scrub the STM32 TZ TRNG word in hal_trng_get_entropy
e280cf1 F-14487: scrub the nRF5340 TRNG staging batch in hal_trng_get_entropy
7896d2d F-14486: read policy_sign key files unbuffered
2ac990b F-14485: snapshot the legacy PBKDF2 password length once
4eb6305 F-14470: accept optional/missing multiboot2 info requests
45e35dd F-14469: quad-enable writes back the status register it read
7069f98 F-14468: record verify-failure version via get_image_version
7913b91 F-14484: reject non-BOOT/UPDATE parts in get_partition_state
3cd94aa F-14483: pin the RSA DigestInfo in RsaDecodeSignature
11b7e78 F-14467: use UPDATE size for write-once trailer erase sector