[KhajaRaheelAhmedMohiuddin]

fix(fw-update): verify the image signature unconditionally at install

Summary

eos_fw_update_finalize() — the last step of installing a firmware image
streamed over the UART update transports (raw / XMODEM / YMODEM) — decided
whether to check the image's signature based on a field inside the image
itself:

/* Verify digital signature if present */
if (ctx->header.sig_type >= EOS_SIG_ED25519) {
    int sig_rc = eos_image_verify_signature(&ctx->header);
    ...
}

sig_type is attacker-controlled — it travels in the image header that the
sender supplies. EOS_SIG_NONE = 0, EOS_SIG_CRC32 = 1, EOS_SIG_SHA256 = 2,
EOS_SIG_ED25519 = 3. An image that declares sig_type = 0/1/2 fails the
>= EOS_SIG_ED25519 test, so signature verification is skipped entirely and
the image is written to the target slot and marked bootable with no
authentication.

The integrity check that still runs (SHA-256 or CRC32) proves only that the
payload matches a hash the same sender placed in the header. It is not a
signature. So an attacker who can talk to the update transport can install an
arbitrary, unsigned image.

Why this is the wrong call site, specifically

eos_image_verify_signature() is the intended single policy point. It rejects
NONE/CRC32/SHA256 and unknown types with EOS_ERR_SIGNATURE, validates the
Ed25519 signature length, resolves the key through the keystore (applying
revocation), and verifies over the signed header prefix. Every other
verification path calls it unconditionally:

Call site	Guard
core/slot_manager.c	unconditional
core/recovery.c	unconditional
stage1/jump_app.c	unconditional
core/secure_boot.c	gated on the trusted cfg->require_signature policy
core/fw_update.c	gated on the untrusted header.sig_type ← the bug

fw_update.c was the only place keying the decision off untrusted input. There
is no EBLDR_REQUIRE_SIGNATURES opt-out involved — that option gates nothing in
the current tree, and the boot-side paths reject unsigned images regardless, so
an unsigned image could never boot anyway. Accepting one at install is pure
downside (an attacker can overwrite an A/B slot and have finalize report
success).

The fix

Call eos_image_verify_signature() unconditionally in finalize and fail
closed on anything but EOS_OK, matching the rest of the trusted computing
base. Signed images are unaffected — they run the exact check the boot path
already applies.

Regression test

tests/unit/test_fw_update_sig.c drives the real begin → write → finalize
pipeline over a simulated flash (same harness style as test_fw_transport.c):

• test_unsigned_image_with_valid_crc_is_rejected — an EOS_SIG_NONE image
  with a correct CRC must return EOS_ERR_SIGNATURE. On the unpatched code
  finalize returns EOS_OK (the image is committed) — this is the exploit,
  and the test fails before the fix and passes after.
• test_sha256_sigtype_is_still_unsigned_and_rejected — sig_type = EOS_SIG_SHA256 (2) took the same bypass; the whole NONE/CRC32/SHA256 class
  is now refused.
• test_corrupt_image_is_rejected_at_integrity_stage — a control: the same
  image with a corrupted CRC is rejected earlier with EOS_ERR_CRC, proving
  the rejection above is the signature gate acting on an otherwise-valid
  image, not the integrity gate.

Verification

Important: master (22d8f8b) does not compile — an unrelated merge left
include/eos_image.h, core/ed25519_verify.c and tests/unit/test_ed25519.c
broken (12 errors), fixed in #94/#95. This PR touches none of those files, so
the numbers below were not produced from this head alone. They were produced
by applying the #94 build-fix on top of this branch first (verification only —
not part of this commit). GCC 13, C11:

# branch head + the #94 build-fix applied locally on top:
$ cmake -B build -DEBLDR_BUILD_TESTS=ON && cmake --build build
   0 errors (one pre-existing #warning from core/keystore.c:29, no new diagnostics)

$ ./build/tests/eboot_test_fw_update_sig
   3/3 tests passed

$ ctest --test-dir build -E valgrind
   100% tests passed, 22/22

# Regression proof — revert only the core/fw_update.c hunk, keep the test:
$ ./build/tests/eboot_test_fw_update_sig
   [FAIL] test_unsigned_image_with_valid_crc_is_rejected  (finalize returned 0/EOS_OK)

The fw_update.c hunk and the new test are self-contained C that do not depend
on the broken files. Once a build-fix lands on master, I will rebase this and
replace the block above with output produced directly from the PR head (with CI
confirming it).

Scope and independence

This change touches only core/fw_update.c, a new test, and one line of
tests/CMakeLists.txt. It is independent of the master build breakage and
shares no files with #94/#95/#98/#99–101 — so it rebases cleanly onto
whichever build-fix lands. Because master does not currently compile, CI here
will be red until then; that is the pre-existing breakage, not this change.

The new test covers the rejection property (unsigned images refused). The
accept path — a validly signed image still installing — is not exercised
here because it needs a real Ed25519 signature under the keystore's active key;
that path is unchanged by this PR and is the same
eos_image_verify_signature() already covered on the boot side by
slot_manager, recovery and jump_app. I'm happy to add a signed-image
happy-path case as a follow-up if you'd like it in this suite.

[srpatcha]

Review — eBoot#104 "fix(fw-update): verify the image signature unconditionally at install"

head: 3553670 author: KhajaRaheelAhmedMohiuddin ci: pending (no checks reported on the branch)

Verdict: Correct, in-scope TCB fix — the install path was keying signature verification off header.sig_type, an attacker-supplied field, and this makes it unconditional and fail-closed, matching every other verify call site. Conforms to master design §8.1 and §14.1. I reproduced both the vulnerability and the fix locally; the findings below are about the verification claim and about pre-existing gaps around the code being changed, not about the change itself.

Findings

#	Severity	File:line	Finding	Recommended fix
1	Medium	core/fw_update.c (whole PR)	The head as pushed does not compile, so the ctest --test-dir build -E valgrind → 22/22 and cmake --build build → 0 errors results in the PR body cannot be reproduced from this commit. Verified: include/eos_image.h:135 and :142 assert on eos_image_header_t.reserved, which no longer exists in the struct (include/eos_image.h:53-54 replaced it with tlv_len/tlv_hash); core/ed25519_verify.c:338 redefines point_is_identity; tests/unit/test_ed25519.c:31,263 redefine run_test_ed25519_identity_key_forgery_rejected / test_ed25519_identity_key_forgery_rejected. All three are inherited from origin/master (22d8f8b), which fails identically — 12 errors — so none is introduced here.	Do not fix it here. State in the Verification block which commits or patches were applied on top of this head to get those numbers (the body discloses stacking on a build fix further down, but the command block reads as run against this head). Rebase once #94 — "repair master — the ABI asserts and the Ed25519 verifier both merged broken" — lands, then re-post the real output.
2	Medium	tests/fuzz/fuzz_fw_update.c:18-20	Pre-existing, directly adjacent: the fuzz harness for the very parser this PR hardens declares eos_fw_update_init(), eos_fw_update_process_chunk() and eos_fw_update_finalize(void). Verified: none of the first two exists anywhere in the tree, and the real eos_fw_update_finalize takes two arguments (include/eos_fw_update.h:100). The target cannot link, and EBLDR_BUILD_FUZZ defaults to OFF (CMakeLists.txt:29), so nothing notices. .ai/security.md ("Input validation") requires fuzz coverage, not just unit tests, for externally reachable parsers — the UART update stream is one.	Already covered by #99 / #100 / #101 — do not duplicate. Reference one of them here so the unit test added in this PR is not read as closing the fuzz gap.
3	Low	core/fw_update.c:199-204	Pre-existing, same function, one gate below the change: eos_image_check_rollback() returning EOS_ERR_NOT_SUPPORTED is accepted as a pass. .ai/security.md ("Fail closed") states a HAL returning EOS_ERR_NOT_SUPPORTED is not success; master design §8.1 only requires rollback protection "where hardware/policy supports it" and does not require a target that lacks it to declare so. The two do not agree.	Out of scope for this PR. Adjacent to #103 (anti-rollback via authenticated TLV counter). Raised as a design proposal (see below) rather than as a change request here.
4	Low	tests/unit/test_fw_update_sig.c:252	tests_run = 3 is a hand-maintained literal. Adding a TEST() and forgetting the matching run_*() call in main() leaves tests_passed == tests_run == 3 and the suite green with a case that never ran — the same silent-drop failure mode tests/unit/test_cmake_test_registration.py exists to catch one level up. Forgetting to bump the literal after adding a run_*() call fails closed, so only this direction leaks.	Increment tests_run inside the TEST macro's run_##name alongside tests_passed, so the count is derived from the calls rather than restated. (#95 is doing the same de-duplication for suite totals repo-wide — align with whatever lands there.)

Nothing Critical or High against this diff. The vulnerability it closes would have been Critical.

Verification I ran

Detached worktree at the PR head, host GCC, C11:

• origin/master (22d8f8b) alone: cmake --build → 12 errors, first at include/eos_image.h:135. Master's host build is broken independently of this PR.
• PR head with those pre-existing merge artifacts patched out locally (not committed, verification only):
  • cmake --build build --target eboot_test_fw_update_sig → 0 errors
  • ./build/tests/eboot_test_fw_update_sig → 3/3 passed, exit 0
  • Reverting only the core/fw_update.c hunk and rerunning → [FAIL] test_unsigned_image_with_valid_crc_is_rejected at test_fw_update_sig.c:212, exit 1. The regression test is genuine and ctest would catch it.
• The bypass is real: core/fw_update.c:175 on master gated on ctx->header.sig_type >= EOS_SIG_ED25519, and sig_type is read straight out of the streamed header (core/fw_update.c, EOS_FW_STATE_HEADER path).
• eos_image_verify_signature() (core/image_verify.c:157-219) does fail closed for NONE/CRC32/SHA256 (line 163) and for unknown types (line 219), and resolves the key through the keystore so revocation applies (lines 182-190). The PR's claim that it is the single policy point is accurate.
• The call-site table in the PR body checks out: core/slot_manager.c:68, core/recovery.c:328, stage1/jump_app.c:43 are unconditional; core/secure_boot.c:97-98 gates on the trusted cfg->require_signature. fw_update.c was the only one keyed off untrusted input.
• The EBLDR_REQUIRE_SIGNATURES claim checks out: CMakeLists.txt:24,57-58 defines the macro, and no .c/.h under stage0/ stage1/ core/ hal/ include/ tests/ references it. Making verification unconditional removes no working opt-out.
• No existing suite regresses: core/fw_transport_uart.c:43 is the only non-test caller of finalize, and tests/unit/test_fw_transport.c exercises begin/write only — it never reaches the signature gate.
• The new suite is registered in tests/CMakeLists.txt:43-46, so test_cmake_test_registration.py is satisfied and the target will actually be built and run.

Architecture conformance

Conforms.

• §8.1 "Required boot concepts" — "Signed manifests and images." The install path was committing an unsigned image to an A/B slot and marking it bootable; this restores the requirement at the last step that can still refuse it.
• §14.1 "Security principles" — "Integrate key management across eBoot, eSec, eOTA and release signing." Routing through eos_image_verify_signature() rather than a local decision keeps keystore selection and revocation in one place, as §14.1 requires them to be reviewed as one system.
• §5.1 "Architectural law" — no new dependency in either direction. core/fw_update.c already included eos_fw_update.h/eos_hal.h/eos_fwsvc.h; the change adds no include, no link entry, no manifest edit. eBoot's TCB gets smaller in surface, not larger: one attacker-controlled input is removed from a trust decision.
• §21 tier placement — Tier 1 Foundation, eBoot, boot/update trust. Correct repo; the code is in core/ (shared boot logic) per the .ai/architect.md target layout, and the test is under tests/unit/.
• Split policy §21.1 — not engaged; nothing moves between repos.

Proposed changes

Smallest sequence that keeps things working:

1. Land #94 to restore the host build on master.
2. Rebase this branch onto it (no shared files — verified: this PR touches core/fw_update.c, tests/CMakeLists.txt, tests/unit/test_fw_update_sig.c; #94 touches include/eos_image.h and core/ed25519_verify.c), then replace the Verification block with output actually produced by this head.
3. Optional, in this PR: derive tests_run in the TEST macro (finding 4) — two lines, no behaviour change.
4. Separately: nothing to do about findings 2 and 3 here; they belong to #99/#100/#101 and #103.

CI cannot confirm any of this until step 1: gh pr checks 104 reports no checks at all on this branch, and mergeStateStatus is BLOCKED. A required-check gate is what #90 is for; while there is none, a red or absent build does not block a merge, which is worth saying out loud on a TCB change.

Not checked

• CI. checks.txt in the bundle is empty and gh pr checks 104 reports no checks on the branch. I have no CI evidence for or against this PR; the local build result above is mine, not CI's.
• The full ctest suite (22 tests). I could not run it: tests/unit/test_ed25519.c does not compile at this head for the pre-existing reason in finding 1, and I would not commit a patch to it to find out. Only eboot_test_fw_update_sig was built and run. The PR's "100% tests passed, 22/22" is unconfirmed.
• The valgrind suite. Not run (the PR excludes it too).
• Cross builds / on-target. No cross toolchain and no hardware exercised. Whether finalize still behaves on a real target after this change is untested by me; the change is target-independent C, so the risk is low but not zero.
• Signed-image happy path. Neither the PR nor I test that a validly signed image still installs through finalize — the three new cases are all rejections. eos_image_verify_signature() needs a keystore key and a real Ed25519 signature to exercise that, which the simulated-flash harness does not set up. This is the one coverage gap in an otherwise good test: the fix could in principle reject everything and all three tests would still pass. I confirmed by reading core/image_verify.c:175-215 that the accept path is unchanged and shared with slot_manager/recovery/jump_app, which do have coverage — but it is inference, not a test.
• Whether unsigned images are legitimately installed anywhere today (factory provisioning, developer flashing, a recovery flow). I checked every in-tree caller — core/fw_transport_uart.c:43 is the only one — but not any out-of-tree or vendor flow, and not tools/. If an in-house provisioning script relies on installing unsigned images over UART, this change breaks it. That would be the correct outcome, but it should be a conscious one.

---

Automated architecture review of 355367099a11 — scheduled, model claude-opus-5, checked against the EmbeddedOS Master Design v2.0. Advisory only: this reviewer never approves, requests changes, or merges. Reply here to discuss or push back — a wrong finding is a bug worth reporting.

[KhajaRaheelAhmedMohiuddin]

Thanks @srpatcha — really appreciate the depth here, especially reproducing both the bypass and the fix and checking it against §8.1/§14.1. Point by point:

1 — Verification block reads as if run against this head. Fair, and my wording was misleading. The build/test numbers were produced by applying the #94 build-fix on top of this branch (this PR touches none of the broken files), not from this head, which inherits master's 12 errors. I've reworded the Verification section to say that explicitly. Once a build-fix lands on master I'll rebase and replace that block with output produced directly from the head, with CI confirming it.

2 — fuzz_fw_update.c declares functions that don't exist. Agreed, and it's pre-existing — not touched here. That belongs to the fuzz work in #99/#100/#101, so I'm not duplicating it. To be clear: the unit test I added is a targeted regression for the signature gate, not a claim to close the fuzz-coverage requirement in .ai/security.md — the UART stream still needs the fuzz target those PRs are fixing.

3 — rollback EOS_ERR_NOT_SUPPORTED accepted as pass. Also pre-existing, one gate below my change, and it's a real "fail closed" vs §8.1 tension. Out of scope here; it's adjacent to the authenticated-counter work in #103, so I'll leave it to that line rather than fold a design change into this fix.

4 — tests_run = 3 is a hand-maintained literal. Fixed — tests_run is now incremented inside the TEST macro's run_##name wrapper, so the total is derived from the calls rather than restated (same direction as #95's repo-wide de-dup). It's in my local branch and will land with the rebase onto #94.

On the accept path (your "not checked" note). Correct that all three cases are rejections, so in principle the gate could reject everything and still pass. The accept path is unchanged by this PR and is the same eos_image_verify_signature() already exercised on the boot side via slot_manager/recovery/jump_app — but that's inference, as you say, not a test in this suite. Happy to add a signed-image happy-path case (real Ed25519 signature under the keystore's active key) if you'd like it here; say the word and I'll include it in the rebase.

Sequence I'll follow: land #94 → rebase this onto it (no shared files) → repost real, CI-backed output with items 1 and 4 folded in. Thanks again.