fix: bound unauthenticated image size before RAM load

[rizlik]

ZD#21988

[Copilot]

Pull request overview

This PR addresses a security-hardening issue in wolfBoot’s RAM-load paths by bounding an unauthenticated image length (read from a decrypted-but-not-yet-authenticated header) before it can drive RAM copies/decryption. This is especially important for EXT_ENCRYPTED stream-cipher configurations where ciphertext malleability can attacker-influence the decrypted length field.

Changes:

• Introduces a shared wolfBoot_ramboot_check_size() helper to validate payload size against the RAM load region.
• Uses the helper to reject oversized images in both wolfBoot_ramboot() (RAM load) and wolfBoot_ram_decrypt() (MMU ramboot decrypt path) before copying/decrypt loops run.
• Adds new unit tests (and Makefile targets) to regress oversized length, wraparound length, and ciphertext bit-flip manipulation of the length field.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
tools/unit-tests/unit-update-ram-enc.c	Adds regression tests for encrypted/MMU RAM decrypt size-bounding (including wrap and malleation cases).
tools/unit-tests/Makefile	Adds build/run targets for the new encrypted RAM decrypt unit tests (fixed partitions + nopart/RAMBOOT_MAX_SIZE variants).
src/update_ram.c	Switches RAM-load size validation to the shared wolfBoot_ramboot_check_size() helper.
src/libwolfboot.c	Adds bounds checking in wolfBoot_ram_decrypt() before the decrypt/copy loop.
include/image.h	Adds the wolfBoot_ramboot_check_size() inline helper for consistent bounds validation.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[dgarske]

Skoll Multi-Scan Review

Modes: review + review-securityOverall recommendation: COMMENT
Findings: 4 total — 4 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)

Posted findings

• [Medium] [review] Bound-check edge cases not exercised: exact max-valid length and FIXED underflow guard — tools/unit-tests/unit-update-ram-enc.c:118-300
• [Low] [review+review-security] WOLFBOOT_RAMBOOT_MAX_SIZE silently ignored when WOLFBOOT_FIXED_PARTITIONS is also defined — src/libwolfboot.c:2418-2429
• [Low] [review-security] New length bound does not account for ENCRYPT_BLOCK_SIZE rounding of the decrypt copy loop — src/libwolfboot.c:2418-2439
• [Low] [review] Overflow-length tcase is missing its tcase_set_timeout call — tools/unit-tests/unit-update-ram-enc.c:323-326

Review generated by Skoll

[dgarske]

🟠 [Medium] Bound-check edge cases not exercised: exact max-valid length and FIXED underflow guard · Test Coverage

The new tests bracket the limit well (valid len=1024, oversize len=WOLFBOOT_PARTITION_SIZE, the 32-bit wrap value, and a bit-flip-forged length), but nothing pins the exact accept/reject boundary of the comparison len > (WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE). An off-by-one in that expression (> vs >=, or +/- IMAGE_HEADER_SIZE) would not be caught. The new FIXED-partition underflow guard WOLFBOOT_PARTITION_SIZE <= IMAGE_HEADER_SIZE is also untested (it only triggers for a sub-256-byte partition, which is an extreme config and admittedly hard to fixture).

Fix: Add a boundary pair: one image with len == (WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE) (or RAMBOOT_MAX_SIZE for the nopart build) that must return 0, and one with len one block larger that must return -1, to lock the comparison's off-by-one behavior; the underflow-guard branch can be left untested or noted as defensive-only.

[dgarske]

🔵 [Low] WOLFBOOT_RAMBOOT_MAX_SIZE silently ignored when WOLFBOOT_FIXED_PARTITIONS is also defined · Logic

Both new bounds checks use #ifdef WOLFBOOT_FIXED_PARTITIONS ... #elif defined(WOLFBOOT_RAMBOOT_MAX_SIZE). If a configuration defines BOTH macros, the partition-size branch wins and WOLFBOOT_RAMBOOT_MAX_SIZE is silently ignored, inverting the priority of the BEFORE code in update_ram.c (which preferred the NO_PARTITIONS / RAMBOOT branch first). If someone intentionally set WOLFBOOT_RAMBOOT_MAX_SIZE smaller than the partition to constrain a RAM load region under fixed partitions, the looser partition-size bound would be used instead, potentially overrunning the smaller RAM region. NOT a regression today: every shipping config that sets WOLFBOOT_RAMBOOT_MAX_SIZE (polarfire, raspi3, raspi3-encrypted, versal) also sets WOLFBOOT_NO_PARTITIONS=1, so WOLFBOOT_FIXED_PARTITIONS is undefined and the two branches are mutually exclusive. The same pattern is duplicated in src/update_ram.c:101-112. Severity views: review rated this Low (SUGGEST/question), review-security rated it Info; stricter severity (Low) kept. Reported as a latent maintenance hazard.

Fix: Confirm the intended precedence. If supporting both macros simultaneously is ever intended, apply the tighter (minimum) of the two bounds rather than letting the partition branch shadow RAMBOOT_MAX_SIZE. Otherwise document that RAMBOOT_MAX_SIZE is a NO_PARTITIONS-only knob (a comment noting the macros are mutually exclusive). Note: the #error text ("...required when WOLFBOOT_NO_PARTITIONS=1") is also slightly stale relative to the new WOLFBOOT_FIXED_PARTITIONS condition.

[dgarske]

🔵 [Low] New length bound does not account for ENCRYPT_BLOCK_SIZE rounding of the decrypt copy loop · Security

The new bound limits the unauthenticated len to WOLFBOOT_PARTITION_SIZE - IMAGE_HEADER_SIZE (or WOLFBOOT_RAMBOOT_MAX_SIZE), so the intended copy region is at most WOLFBOOT_PARTITION_SIZE bytes. However, the unchanged decrypt loop immediately below writes in fixed ENCRYPT_BLOCK_SIZE granules (64 bytes for ChaCha, 16 for AES): while (dst_offset < (len + IMAGE_HEADER_SIZE)) { ... XMEMCPY(dst + dst_offset, dec_block, ENCRYPT_BLOCK_SIZE); dst_offset += ENCRYPT_BLOCK_SIZE; }. When len + IMAGE_HEADER_SIZE is not a multiple of ENCRYPT_BLOCK_SIZE, the final iteration writes up to ENCRYPT_BLOCK_SIZE - 1 bytes PAST len + IMAGE_HEADER_SIZE, i.e. up to roundup(len + IMAGE_HEADER_SIZE, ENCRYPT_BLOCK_SIZE). With len at its maximum, this is roundup(WOLFBOOT_PARTITION_SIZE, ENCRYPT_BLOCK_SIZE). This is safe ONLY because WOLFBOOT_PARTITION_SIZE / WOLFBOOT_RAMBOOT_MAX_SIZE (and IMAGE_HEADER_SIZE) are block-aligned in every shipping config, so the rounded-up value equals the bound and the destination RAM staging region is exactly WOLFBOOT_PARTITION_SIZE. If a target ever defines a non-block-aligned partition or ramboot-max size, the last block decrypt would write past the bound the check is meant to enforce. Defense-in-depth observation about the tightness of the newly introduced bound, not an exploitable overflow in current configurations.

Fix: Optionally tighten the bound to account for block rounding, e.g. compare against WOLFBOOT_PARTITION_SIZE after rounding len + IMAGE_HEADER_SIZE up to ENCRYPT_BLOCK_SIZE, or add a compile-time assertion that WOLFBOOT_PARTITION_SIZE % ENCRYPT_BLOCK_SIZE == 0 and WOLFBOOT_RAMBOOT_MAX_SIZE % ENCRYPT_BLOCK_SIZE == 0. No action required for the currently supported (block-aligned) targets.

[dgarske]

🔵 [Low] Overflow-length tcase is missing its tcase_set_timeout call · Test Consistency

Three of the four new tcases get an explicit 5s timeout (bitflip, valid, oversize), but overflow (test_ram_decrypt_overflow_len_rejected) is created and added to the suite without a matching tcase_set_timeout(overflow, 5). It falls back to the libcheck default timeout. Purely cosmetic/consistency - all four reject-path tests are fast - but the omission looks accidental given the other three are set right next to it.

Fix: Add tcase_set_timeout(overflow, 5); so all four tcases set their timeout consistently.

[dgarske]

#802 (review)