Fixes 20260408

[danielinux]

F/2273 - sign: scrub key buffers before free (1c824539)
F/2274 - Scrub RSA keygen private material (87d5a7fc)
F/2275 - Zero ECC keygen private buffers (114140c2)
F/2276 - Zero EdDSA keygen private data (6f40afae)
F/2277 - zero ML-DSA private key buffer (32783034)
F/2258 - Add restricted key mask authenticity tests (8399e3ed)
F/1892 - Propagate encrypt key flash errors (afef21fe)
F/1893 - Propagate erase encrypt key write failures (13370613)
F/1894 - Fix policy_create PCR digest validation (6b8ade6b)
F/1895 - Fix sign encrypted output open failure (34438999)
F/1898 - Check image reopen failures in sign tool (5fd09eed)
F/2247 - Use constant-time RSA hash comparison (9a7930dd)
F/2255 - Protect bootloader before application boot (f32c275d)
F/2259 - Add auth type coverage for unit-image (644e70e7)
F/2260 - Add auth-only invalid update test (ff60286e)
F/2261 - Add RAM_CODE self-update unit coverage (3659d210)
F/2262 - Strengthen same-version RAM update test (1353e933)
F/2266 - Fix sign header TLV overflow sizing (ea6700f1)
F/2267 - Reject oversized delta source offsets (fefd74e0)
F/1897 - fix memmove large-length backward copy (7526fd23)
F/2248 - Use constant-time TPM secret checks (78de4a79)
F/2249 - Use constant-time encryption key validation (56c46be7)
F/2252 - Use fixed-length erased-key check (830869c3)
F/2256 - enforce skip-verify prerequisites (a09babb5)
F/2264 - Add equal-version update-disk regression test (395202bb)
F/2268 - Reject valid zero-size delta images (29367f81)
F/2269 - Fix total size type in update flash (fffd8543)
F/1888 - zeroize update key material (05823677)
F/1889 - zero custom encrypt stack buffers (7c1c8631)
F/1890 - zeroize swap trailer key buffer (a870af20)
F/1891 - Scrub sign-tool encryption material (3f1906fa)
F/2253 - Use constant-time delta base hash compare (10035dbc)
F/2257 - Warn when DISABLE_BACKUP is enabled (13d232c4)
F/2270 - Add sign/parser roundtrip tests (fb9faffc)
F/2271 - Add delta roundtrip edge-case coverage (08d39cf4)

[Copilot]

Pull request overview

This PR is a security- and robustness-focused sweep across wolfBoot tooling and update paths, adding constant-time comparisons, zeroization of sensitive material, stricter bounds/validation, and expanding unit-test coverage for regressions and edge cases.

Changes:

• Add zeroization/scrubbing for signing/keygen/encryption-key material and convert several comparisons to constant-time.
• Harden update/self-update/delta flows (bounds checks, error propagation, bootloader flash protection hook).
• Add/extend unit tests for header sizing, encrypted-sign output error handling, PCR digest validation, delta/update edge cases, and keymask/auth-type behavior.

Reviewed changes

Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tools/unit-tests/unit-update-ram.c	Adds an assertion validating the RAM-loaded image size metadata.
tools/unit-tests/unit-update-flash.c	Adds self-update tests, auth-type/size edge-case tests, and helpers for image payload construction.
tools/unit-tests/unit-update-disk.c	Adds monolithic/self-header config and a regression test for equal-version partition selection.
tools/unit-tests/unit-string.c	Adds a Linux-only regression test targeting large-length overlapping memmove.
tools/unit-tests/unit-sign-header-size.c	New unit test validating sign-tool manifest header-size enforcement logic.
tools/unit-tests/unit-sign-encrypted-output.mkfrag	Adds a source list fragment to build the encrypted-output sign-tool unit test.
tools/unit-tests/unit-sign-encrypted-output.c	New unit test exercising sign tool encrypted output error-handling and TLV/header roundtrips.
tools/unit-tests/unit-policy-create.c	New unit test covering PCR digest argument validation in policy_create tool.
tools/unit-tests/unit-keystore.c	Adjusts keystore slot mask to a restricted “app-only” verify mask for tests.
tools/unit-tests/unit-image.c	Adds authenticity tests for mismatched auth type and key mask restrictions.
tools/unit-tests/unit-enc-nvm.c	Adds tests ensuring flash write failures propagate for encrypt-key set/erase APIs.
tools/unit-tests/unit-delta.c	Adds delta offset-limit test and significantly expands roundtrip/diff/patch edge coverage.
tools/unit-tests/Makefile	Adds new unit-test targets and specialized build variants (delta/self-update/sign tests).
tools/tpm/policy_create.c	Fixes PCR digest length validation by correctly treating the size as signed for the check.
tools/keytools/sign.c	Enforces header-size match, adds header overflow checks, improves error handling, and scrubs key buffers.
tools/keytools/Makefile	Links header_size.o into the sign tool build.
tools/keytools/keygen.c	Scrubs keygen private buffers (RSA/ECC/EdDSA/ML-DSA) and improves cleanup paths.
tools/keytools/header_size.h	New helper API for enforcing manifest header-size alignment with compiled bootloader config.
tools/keytools/header_size.c	Implements manifest header-size enforcement helper.
tools/delta/bmdiff.c	Adds maximum input size validation to prevent oversized file processing.
src/x86/ahci.c	Uses constant-time comparison for TPM-sealed secret verification.
src/update_ram.c	Calls hal_flash_protect() before handoff (outside TrustZone) to protect bootloader region.
src/update_flash.c	Zeroizes encryption buffers, tightens delta edge-case handling, adds constant-time hash/secret checks, and adjusts total-size type.
src/update_flash_hwswap.c	Calls hal_flash_protect() before hal_prepare_boot() to protect bootloader region.
src/update_disk.c	Calls hal_flash_protect() before hal_prepare_boot() to protect bootloader region.
src/tpm.c	Exposes constant-time compare for TPM use-cases.
src/string.c	Fixes memmove backward-copy loop to correctly handle large size_t lengths.
src/libwolfboot.c	Improves encrypt-key validity/erased checks, propagates flash errors, and scrubs key/nonce buffers in init paths.
src/image.c	Exposes image_CT_compare() (noinline) for constant-time digest comparisons.
src/delta.c	Rejects delta match offsets beyond 24-bit limit.
options.mk	Enforces WOLFBOOT_SKIP_BOOT_VERIFY prerequisites and warns when DISABLE_BACKUP=1.
include/wolfboot/wolfboot.h	Adds compile-time enforcement of WOLFBOOT_SKIP_BOOT_VERIFY prerequisites.
include/tpm.h	Declares wolfBoot_constant_compare() when TPM seal/keystore is enabled.
include/image.h	Switches RSA/digest verification macros to use image_CT_compare() and adds its prototype.
include/hal.h	Adds hal_flash_protect() API to HAL interface.
hal/skeleton.c	Documents that hal_flash_protect() is invoked before hal_prepare_boot().
hal/nrf5340.c	Removes duplicated bootloader flash-protect call now handled centrally.
hal/hal.c	Provides a weak default hal_flash_protect() implementation (no-op).
docs/compile.md	Documents unrecoverable power-loss implications when DISABLE_BACKUP=1.

---

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

[dgarske]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 8 total — 8 posted, 0 skipped

Posted findings

• [High] int ret declared after statements — C89 violation in embedded target code — src/update_flash.c:501
• [High] aes_init() does not check wolfBoot_get_encrypt_key() return — inconsistent with chacha_init() — src/libwolfboot.c:1791
• [Medium] header_required_size() uses CMD.policy_sz before it is initialized — tools/keytools/sign.c:1244-1248
• [Medium] keygen_ecc and keygen_ml_dsa not refactored to zero private material on error paths — tools/keytools/keygen.c:620-707
• [Medium] Output file f not closed on failure path in make_header_ex — tools/keytools/sign.c:2100-2103
• [Medium] Missing wolfBoot_get_encrypt_key() return check in wolfBoot_swap_and_final_erase — src/update_flash.c:488
• [Low] update_flash_hwswap.c uses boot_panic() without error message — inconsistent — src/update_flash_hwswap.c:110-111
• [Info] memmove word-copy fast path not gated behind FAST_MEMCPY — src/string.c:296-310

Review generated by Skoll via openclaw

[dgarske]

🟠 [High] int ret declared after statements — C89 violation in embedded target code
🚫 BLOCK bug

The new int ret; declaration at line 501 appears after executable statements (lines 441-498: wolfBoot_open_image, wolfBoot_get_partition_state, memcpy, wb_flash_erase, etc.). This is valid in C99+ but not in C89/C90. wolfBoot targets embedded compilers including CCRX (this same file has #pragma section FRAM for CCRX at line 799), which may enforce C89 rules. The #ifdef EXT_ENCRYPTED preprocessor guard does NOT create a C block scope, so the declaration is mid-function from the C compiler's perspective.

Suggestion:

Suggested change

	int ret;
	Move `int ret;` to the top of the function alongside other locals (around line 439), still guarded by `#ifdef EXT_ENCRYPTED`:
	```c
	uint32_t tmpBuffer[TRAILER_OFFSET_WORDS + 1];
	#ifdef EXT_ENCRYPTED
	int ret;
	#endif

[dgarske]

🟠 [High] aes_init() does not check wolfBoot_get_encrypt_key() return — inconsistent with chacha_init()
🚫 BLOCK bug

In chacha_init() (lines 1724-1726), the return value of wolfBoot_get_encrypt_key(key, stored_nonce) is checked and on failure the function jumps to exit. In aes_init() at line 1791, the same call is made but the return value is silently discarded. If the custom key retrieval fails, aes_init will proceed with uninitialized stack-allocated key[] and stored_nonce[] buffers. While encrypt_key_is_valid() at line 1810 may catch all-zero buffers, uninitialized stack data could pass the validation. This is a clear inconsistency between the two init functions introduced by this PR.

Suggestion:

Suggested change

	enc_key =(wrap_enc_key_t*)RENESAS_TSIP_INSTALLEDENCKEY_ADDR;
	```c
	#elif defined(CUSTOM_ENCRYPT_KEY)
	ret = wolfBoot_get_encrypt_key(key, stored_nonce);
	if (ret != 0)
	goto exit;

[dgarske]

🟡 [Medium] header_required_size() uses CMD.policy_sz before it is initialized
💡 SUGGEST bug

header_required_size() is called at line 1292 to determine if the header needs to grow for a cert chain. At that point, CMD.policy_sz is still 0 (it gets set to CMD.signature_sz at line 1746, deep inside make_header_ex). When CMD.policy_sign is true, the calculation at line 1247 computes 0 + sizeof(uint32_t) = 4 bytes for the policy TLV instead of CMD.signature_sz + sizeof(uint32_t) (e.g., 260 bytes for RSA2048). This undercounting means the header might not be grown sufficiently when both --policy and --cert-chain are used together. The header_append_tag() overflow check at line 232 would catch the overflow at runtime and exit(1), so no corrupt image is produced, but the tool would fail unnecessarily.

Suggestion:

Suggested change

	if (CMD.policy_sign) {
	Use `CMD.signature_sz` directly (since `policy_sz` is always set to `signature_sz`):
	```c
	if (CMD.policy_sign) {
	header_size_align_8(&idx);
	header_size_append_tag(&idx,
	CMD.signature_sz + (uint32_t)sizeof(uint32_t));
	}

[dgarske]

🟡 [Medium] keygen_ecc and keygen_ml_dsa not refactored to zero private material on error paths
💡 SUGGEST convention

The PR adds goto-cleanup patterns with wc_ForceZero() for RSA, Ed25519, and Ed448 keygen functions, but keygen_ecc (line 620) and keygen_ml_dsa (line ~1040) still use direct exit() calls on error paths. For keygen_ecc, if any error occurs after the private scalar d[] is populated (line 633) but before the cleanup code at line 696, the exit() bypasses wc_ForceZero(d, ...). Similarly for keygen_ml_dsa, if any error occurs after priv = malloc(...) (line 1102) but before line 1208, the free(priv) with zeroing is skipped. While exit() terminates the process (so memory is reclaimed by the OS), this is inconsistent with the hardening treatment given to the other keygen functions in this PR.

Suggestion:

Suggested change

	#define MAX_ECC_KEY_SIZE 66
	Refactor `keygen_ecc` and `keygen_ml_dsa` with the same `exit_code` + `goto cleanup` pattern used for RSA/Ed25519/Ed448.

[dgarske]

🟡 [Medium] Output file f not closed on failure path in make_header_ex
💡 SUGGEST bug

The output file handle f is opened at line 1973 and closed on the success path at lines 2100-2101, but the failure: cleanup block (line 2103) does not close f. If any of the new goto failure paths introduced by this PR fire after f is opened (e.g., encryption key read failure at lines 2028-2047), f is leaked. The PR already correctly manages fek and fef in the failure block with NULL checks. This is partly pre-existing but is expanded by the PR's conversion of exit(1) calls to goto failure.

Suggestion:

Suggested change

	fclose(f);
	Add `f` cleanup to the failure block (set `f = NULL` after closing on the success path):
	```c
	if (f) {
	fclose(f);
	f = NULL;
	}
	failure:
	...
	if (f)
	fclose(f);

[dgarske]

🟡 [Medium] Missing wolfBoot_get_encrypt_key() return check in wolfBoot_swap_and_final_erase
💡 SUGGEST bug

At line 488, wolfBoot_get_encrypt_key() is called to retrieve the encryption key into tmpBuffer, but its return value is not checked. If the key retrieval fails, tmpBuffer may contain invalid data, which is then staged into the boot partition (line 494) and later used to reinitialize encryption (line 503). The PR added a return check for wolfBoot_set_encrypt_key (line 503) but not for the get call. This is an inconsistency with the PR's theme of propagating encrypt key errors.

Suggestion:

Suggested change

	wb_flash_erase(boot, WOLFBOOT_PARTITION_SIZE - eraseLen, eraseLen);
	```c
	ret = wolfBoot_get_encrypt_key((uint8_t*)tmpBuffer,
	(uint8_t*)&tmpBuffer[ENCRYPT_KEY_SIZE/sizeof(uint32_t)]);
	if (ret != 0) {
	/* handle error */
	}

[dgarske]

⚪ [Info] memmove word-copy fast path not gated behind FAST_MEMCPY
🔧 NIT convention

The new memmove backward copy adds a word-aligned fast path (lines 296-310), but this code is unconditionally compiled. The analogous memcpy forward word-copy (line 263) is gated behind #ifdef FAST_MEMCPY. For consistency and to keep code size minimal on constrained targets that don't opt in, consider gating the memmove word path similarly. The memmove logic itself is correct — both byte-by-byte and word-at-a-time paths are verified.

Suggestion:

Suggested change

	char *d = (char *)dst;
	Gate the optimization behind `#ifdef FAST_MEMCPY` for consistency.

[Copilot]

Pull request overview

Copilot reviewed 36 out of 36 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

tools/unit-tests/unit-sign-encrypted-output.c:1

• PATH_MAX and errno/EACCES are used later in this file, but the required headers are not included here. This can cause compilation failures on platforms where PATH_MAX is not indirectly defined. Add the appropriate includes (<limits.h> for PATH_MAX and <errno.h> for errno/EACCES).
  tools/unit-tests/unit-string.c:1
• This test maps ~2GB+ of virtual address space and will be flaky or fail outright on some 32-bit userspaces (where SIZE_MAX > INT_MAX can still be true) or constrained CI environments. Consider restricting it to 64-bit processes (e.g., UINTPTR_MAX > 0xFFFFFFFF) and/or gracefully skipping the test when mmap fails instead of asserting, so the suite remains reliable across environments.
  src/libwolfboot.c:1
• In the non-MMU/non-fixed-partitions path, the function returns ret before the #endif, making the final return 0; unreachable in that configuration (and likely generating warnings). Consider using a single ret variable across all preprocessor branches and returning once at the end, or moving the return 0; into only the branches that need it.

/* libwolfboot.c

---

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

tools/unit-tests/unit-sign-encrypted-output.c:1

• PATH_MAX is used but this file doesn’t include <limits.h>, which can cause build failures on platforms/toolchains where PATH_MAX is not implicitly available. Add an explicit #include <limits.h> (or another portable approach used in the repo) near the other standard includes.
  tools/unit-tests/unit-string.c:1
• This test requires mapping >2GiB and will fail (hard) on systems without sufficient virtual memory/overcommit, making CI potentially flaky across environments. Consider turning the MAP_FAILED case into a test skip (or gating behind an additional env flag/capability check) rather than asserting, so the suite remains reliable on constrained runners.

---

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

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

tools/unit-tests/unit-sign-encrypted-output.c:1

• This file uses errno and EACCES but does not include <errno.h>, which can fail compilation (or at least produce implicit-declaration warnings promoted to errors). Add the missing header include.
  tools/unit-tests/unit-sign-encrypted-output.c:1
• PATH_MAX is used but there is no include guaranteeing it is defined (commonly <limits.h>). Add <limits.h> (or otherwise avoid PATH_MAX) so this unit test builds portably.

---

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