fix(eos_sign): put the payload where the bootloader looks for it - #88
fix(eos_sign): put the payload where the bootloader looks for it#88Kartikey1306 wants to merge 2 commits into
Conversation
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#88 "fix(eos_sign): put the payload where the bootloader looks for it"
head: 0eb2332 author: Kartikey1306 ci: pass
Verdict: The defect is real, the diagnosis is right, and test_eos_sign_boot_path.c is the correct shape for proving it — real tool output staged through the real eos_image_parse_header() and eos_image_verify_integrity(), both layouts asserted together. But the fix redefines a wire-format field that sits inside the signed prefix, and the PR body's survey of who reads it is incomplete. Two consumers were missed, one of them in another repo, and one of them rejects the new value outright. The CI fix is a genuine and separate win.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | High | eFirmware/src/efw_image.c:121-122 |
efw_image_parse() reads h.hdr_size = get_u16(buf + 6); and then if (h.hdr_size != (uint16_t)EFW_IMAGE_HDR_SIZE) return EFW_ERR_INVALID; — an exact-equality check against 156u (eFirmware/include/efw/efw_image.h:59). After this PR, every image eos_sign.py produces with a TLV area carries hdr_size = 300 and is rejected by eFirmware's parser as malformed. eFirmware's header documents the two formats as interchangeable — "an image packed here is consumable by eos_image_parse_header() without translation" (eFirmware/include/efw/efw_image.h:10-11) — and this change breaks the reverse direction. The PR body states "Nothing else in the system reads hdr_size that way" on the strength of two call sites; this is a third, in the repo that is the other half of the format. |
Decide the convention once and change both sides in one go, or do not change the meaning. If hdr_size is to be the payload offset, eFirmware/src/efw_image.c:122 has to become a range check (>= EFW_IMAGE_HDR_SIZE && <= 4096) matching core/image_verify.c:92, and eFirmware's offset table needs the same note. That is a cross-repo change and should not be inferred from an eBoot-only PR — at minimum this PR must name the break, and it is a strong argument for the alternative the docstring originally proposed (move the TLV after the payload, leaving hdr_size == 156 always). |
| 2 | High | core/rollback.c:30-34 |
The second missed consumer, and it is in eBoot. eos_rollback_read_image_counter() computes tlv_addr = image_addr; tlv_addr += hdr.hdr_size; tlv_addr += hdr.image_size; under the comment "The TLV area follows the header and payload" — i.e. it assumes the layout [header][payload][TLV], the opposite of the [header][TLV][payload] that eos_sign.py emits. Before this PR that computation landed image_size bytes past the start of a 144-byte TLV area, i.e. inside or past the payload. After it, with hdr_size now including the TLV, it lands exactly tlv_len bytes past the end of the image and hands that address to eos_tlv_parse(). Either way the anti-rollback minimum-security-version TLV can never be found in an eos_sign.py image, and eos_tlv_parse() reads out of the image in the TCB. §8.1 requires "Rollback protection where hardware/policy supports it" and §15.1 "Rollback and recovery"; .ai/security.md requires anti-rollback counters to be "actually checked". test_eos_sign_boot_path.c exercises verify_integrity only, so the suite confirms the path the author surveyed and is silent on this one. |
This PR changes the field the disagreement hinges on, so it should not land without resolving it. Either fix core/rollback.c to read the TLV at image_addr + sizeof(eos_image_header_t) under the new convention and correct its comment, or state plainly in the PR that eos_sign.py images carry no reachable EOS_TLV_MIN_SEC_VER and open an issue. Add a case to test_eos_sign_boot_path.c that calls eos_rollback_read_image_counter() on eos_fixture_image_good and asserts the counter it should find — that is the test that would have caught this. |
| 3 | Medium | include/eos_image.h:28 and :96-97 |
The authoritative declaration still documents the old meaning, in two mutually inconsistent ways. Line 28: uint16_t hdr_size; /* Size of this header in bytes */ — the definition this PR abandons. Lines 96-97: "Payload starts after hdr_size and an optional TLV area, matching eos_sign.py" — which reads as payload_offset = hdr_size + tlv_len and double-counts the TLV under the new convention, while core/image_verify.c:122 computes addr + hdr->hdr_size with no TLV term. So the header describes two things, neither of which is what the tool now writes. #87 pins this field's offset with a static assert and tests/unit/test_image_abi.c:65 pins it again, but nothing pins or states its semantics — which is exactly how a field acquires three meanings. |
Rewrite line 28 as /* Offset from the image base to the payload: this struct plus any TLV area */ and rewrite lines 96-97 to "Payload starts at addr + hdr_size, which spans this header and any TLV area." Header-only, no code change, and it is what makes the convention discoverable to the next reader. |
| 4 | Medium | tools/eos_sign.py, assert len(tlv) == tlv_len and the adjacent assert len(sig) == EOS_SIG_MAX_SIZE |
Both are removed by the interpreter under python -O or PYTHONOPTIMIZE=1, leaving no check at all. The PR body argues this one is load-bearing — "If those ever diverge, hdr_size points at something that is not the payload and the image fails silently on-device, so it is worth the assert" — and that is the right judgement, which makes assert the wrong mechanism for it. .ai/security.md: "A security step whose result is discarded is a finding even when the happy path is correct." This is release-signing tooling, so the stricter reading in §14.1 and the run brief applies. |
Raise instead: if len(tlv) != tlv_len: raise SystemExit(f'TLV length changed after signing: {len(tlv)} != {tlv_len}'), and the same for the signature length. Pre-existing for the sig assert; both are one line. |
| 5 | Medium | .github/workflows/ci.yml:39-43 |
Adding cryptography fixes today's silent skip and finding it is good work — 19 signing-toolchain tests collected and never run is precisely the failure mode .ai/security.md calls out ("ctest exits 0 when it finds no tests at all … a security suite that silently collected nothing is a failed check reported as green"). But the fix removes the symptom, not the mechanism: pytest.importorskip("cryptography") still silently skips if the install fails, if the package is dropped from the list, or if a future runner image changes. The job stays green either way, and nobody learns. |
Make the skip impossible in CI rather than merely unlikely. Simplest: gate importorskip on an environment variable — if os.environ.get('EOS_REQUIRE_SIGNING_TESTS'): import cryptography / else: pytest.importorskip('cryptography') — and set EOS_REQUIRE_SIGNING_TESTS=1 in the workflow so a missing dependency is a hard error there while a local run without it still skips. A pytest --strict-markers -W error or an assertion on the collected count would also work; the property to secure is that CI cannot report green having run zero signing tests. |
| 6 | Low | tools/eos_sign.py, cmd_verify |
Two small things in the rewritten payload-offset block. struct.unpack('<H', image[struct_size:struct_size + 2]) raises an unhandled struct.error traceback rather than a clean FAIL: line for an image shorter than 158 bytes, unlike every other failure in the command; and struct_size = 156 is a bare local literal, re-derived here while build_header() computes the same 156 from its field-width sum a few lines above. A verify command is the thing pointed at untrusted files, so its malformed-input path should be as tidy as its happy path. |
Hoist the struct size to a module constant (EOS_IMG_STRUCT_SIZE = 156) used by both functions, and bounds-check before the unpack: if len(image) < struct_size + 2: print('FAIL: image too short for a TLV area'); sys.exit(1). |
Architecture conformance
Deviates, on the strength of findings 1 and 2 — not in where the code lives, but in the dependency the change creates.
Placement is correct. §21 Tier 1 — Foundation; tools/, tests/unit/ and tests/vectors/ are the right homes, core/ and stage1/ are untouched, and no C source changes at all, so §5.1's minimal-TCB requirement is untroubled. No new include, link line or target_link_libraries entry points up a tier; tests/CMakeLists.txt adds one executable linking eboot_core, which is downward. §8.1 "Signed manifests and images" and §15.1 "Signed metadata and package provenance" are the requirements this PR serves, and it serves them better than master did.
The deviation is §5.1's underlying rule that a lower layer must not acquire a silent dependency on a higher one. hdr_size is a wire-format field written by two producers (tools/eos_sign.py, eFirmware/src/efw_image.c:56) and read by four consumers (core/image_verify.c:122, core/rollback.c:32, stage1/jump_app.c:80, eFirmware/src/efw_image.c:122) plus tools/sign_image.py:88. Changing its meaning in one producer makes correctness depend on all six agreeing, and two of them do not. stage1/jump_app.c:80 — jump_addr = addr + hdr.hdr_size — is a third consumer that happens to survive, because for an XIP image the entry point is at the payload; worth confirming rather than assuming, and I did not.
The root cause is a gap in the master design, not in this PR. §8.1 requires "Signed manifests and images" but defines no image container: no field list, no field semantics, no owning repository, no compatibility rule. §10.1 defines the component manifest contract in detail and says nothing about the boot image header. So hdr_size has acquired three readings — struct size, payload offset, and "header plus a separately-counted TLV area" — across two repos, and there is no document that adjudicates. Proposal appended to .ai/autoreview/proposals/2026-09.md.
Proposed changes
Smallest sequence that keeps every consumer correct:
- Resolve the convention before the semantics change lands. Two coherent options, and the PR should pick one explicitly rather than leave the tree in the state findings 1 and 2 describe:
- (a) Keep this PR's direction. Then
eFirmware/src/efw_image.c:122becomes a range check andcore/rollback.c:30-34is corrected toimage_addr + sizeof(eos_image_header_t), both in this change set or a companion PR that merges with it. - (b) Take the docstring's original option — TLV after the payload.
hdr_sizestays 156 forever,eFirmware's equality check keeps working untouched, andcore/rollback.c's existinghdr_size + image_sizearithmetic becomes correct as written. More bytes move, but no field changes meaning and no cross-repo coordination is needed. Given that (b) leavescore/rollback.cright by construction and (a) requires editing it, (b) looks like the smaller total change despite being the larger diff. Worth the maintainers' call, not mine.
- (a) Keep this PR's direction. Then
- Update
include/eos_image.h:28and:96-97to state whichever convention wins (finding 3). - Replace both
asserts incmd_signwith raises (finding 4) — independent, land any time. - Harden the CI skip so it cannot silently return (finding 5) — independent, and worth landing on its own even if the format question takes longer. The
cryptographyaddition should not wait behind the format decision. - Tidy
cmd_verify's short-image path and hoist the 156 (finding 6). - Extend
test_eos_sign_boot_path.cwith aeos_rollback_read_image_counter()case (finding 2).
Items 3-5 are independent of the format decision. Item 1 blocks items 2 and 6.
Not checked
- Nothing was executed. No
pytest, noctest, no signing run. Reproducing needs the PR head checked out, which the run brief forbids. Every number in the body —hdr_size156→300, TLV magic0x6907ataddr + 156,19 passed / 2 modules skippedvs38 passed,3 of 5 failagainst the unfixed tool — is the author's and unverified by me. Findings 1 and 2 are from readingeFirmware/src/efw_image.candcore/rollback.con the working tree; I did not observe either failure, and confirming them means building aneos_sign.pyimage with a TLV area and feeding it toefw_image_parse()and toeos_rollback_read_image_counter(). Both should be done before acting on them. stage1/jump_app.c:80not analysed.jump_addr = addr + hdr.hdr_sizeis a fourthhdr_sizeconsumer. Under the new convention it points at the payload start, which is plausibly what a jump wants, but I did not trace whether it is correct for both XIP and copy-to-RAM targets, and no test in the bundle covers it.- The
eFirmwareworking tree is at whatever commit the sync step left it. I did not check its branch, or whether an unmerged PR there relaxes thehdr_sizeequality check atefw_image.c:122. If one does, finding 1's severity drops. - Test key handling, checked with a caveat.
tools/gen_signed_image_fixture.pyembedsSEED = bytes(range(32))— a fixed Ed25519 private seed — andtests/vectors/signed_image_fixture.hcommits only the derived public key (eos_fixture_pubkey[32]), not the private key..ai/security.mdrequires that test keys be "structurally incapable of signing a release artifact" and that "the release path cannot reach them". I found no reference togen_signed_image_fixture.py,signed_image_fixture.horeos_fixture_pubkeyin any workflow, CMake file or C source, and no trusted-key store incore/,stage1/orinclude/for the fixture public key to leak into. But that grep ran againstmaster, which does not contain the new files — so it establishes that the release path does not reach the fixture today, not that this PR keeps it that way. Someone should confirm the release workflow after merge. Nothing structural prevents a release from being pointed at the seed; it is convention only. - 21 of 22 checks pass;
Create GitHub Releasereportsskipping, expected on a PR. No required check failing. Note the checks that pass here were run with thecryptographyaddition, so the green result already reflects 38 tests rather than 19 — I could not confirm that fromchecks.txt, which reports no per-test counts. mergeStateStatus: BLOCKED,mergeable: MERGEABLE. No merge attempted.tests/CMakeLists.txttouches the same region as #84/#85 and #90; expect conflicts.
Automated architecture review of 0eb2332274d2 — 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.
…rged broken master (22d8f8b) does not compile. Two independent double-merges, both the same shape: two PRs fixing adjacent things landed on stale bases, each was green on its own branch, and the result was never rebuilt. 1. include/eos_image.h — embeddedos-org#93 replaced reserved[30] with tlv_len (2) + tlv_hash[28], preserving every offset. embeddedos-org#87 merged afterwards carrying asserts written against the older struct: error: no member named 'reserved' in 'eos_image_header_t' (x2) embeddedos-org#93 already asserts tlv_len at 62 and tlv_hash at 64, so the offset assert was a duplicate; the width assert had no replacement and is restored as two asserts covering both halves of the same 30-byte span. No offset moves and the wire format is unchanged. 2. core/ed25519_verify.c — embeddedos-org#86 and embeddedos-org#57 both landed a subgroup guard, so the file carried two byte-identical point_is_identity() definitions: error: redefinition of 'point_is_identity' Only embeddedos-org#57's public_key_is_valid_subgroup() is wired to the call site, so embeddedos-org#86's key_has_prime_order() was dead. Kept the live function, folded embeddedos-org#86's fuller rationale onto it, deleted the duplicate. 3. tests/unit/test_ed25519.c — collateral from the same merge. Two copies of test_ed25519_identity_key_forgery_rejected, main() calling it twice and two tests not at all, and test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery referencing k_low_order[] and messages[] that the merge had dropped. While restoring the corpus, corrected it (review finding on embeddedos-org#86): the array claimed to hold "the eight low-order point encodings" and held five. Every order here was computed rather than copied — decode y, recover x, add the point to itself until it reaches the identity — giving 1, 2, 4, 4, 8, 8, 8, 8. Missing before: y=0 with the sign bit set, and both sign-flipped order-8 encodings. D9FF..FF was in the array and is not a low-order point at all — no x satisfies the curve equation for that y — so it moves to a separate k_non_canonical[], with EDFF..FF7F (y=p) and EEFF..FF7F (y=p+1). tests_run was assigned a literal (11) in main() and never incremented, which is how the duplicate call and the two unregistered tests went unnoticed. The TEST macro now increments it, so the total cannot drift. Verified: cmake -DEBLDR_BUILD_TESTS=ON on master FAILS to build, 3 errors same with this commit builds clean ctest 21/21 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 21/21 PASS pytest tests/ 24 passed, 1 skipped test_ed25519 14/14 PASS (was 11 claimed, 12 run) discrimination, with `public_key_is_valid_subgroup` disabled: test_ed25519_low_order_keys_rejected FAILS, as it must test_ed25519_non_canonical_... still PASSES — those are refused by unpackneg() on canonicality, a different mechanism, which is the reason they are held in a separate array rather than counted among the eight.
…hdr_size Answers the review on embeddedos-org#88. Both High findings had the same root, and master has since settled the question the reviewer said needed a decision. The original defect is real and unchanged: the tool emitted [header][TLV][payload] while stamping hdr_size as a fixed 156, so core/image_verify.c's `payload_addr = addr + hdr->hdr_size` landed on the TLV block and every image it produced failed integrity verification on-device. This branch fixed it by moving hdr_size to mean "offset to the payload". That was the wrong half to move. Finding 1 (High) -- eFirmware/src/efw_image.c:122 checks `hdr_size != EFW_IMAGE_HDR_SIZE` for exact equality against 156, so every TLV image would have been rejected by the other half of the format, which eFirmware's header documents as interchangeable. Finding 2 (High) -- core/rollback.c computes the TLV address as image_addr + hdr_size + image_size, i.e. it assumes the area follows the payload. Under either the old layout or this branch's, the anti-rollback EOS_TLV_MIN_SEC_VER could never be found, so the counter this repository gates downgrades on was unreachable for every image the tool produced. Master answers both, and it is the reviewer's option (b): embeddedos-org#93 landed tlv_len and tlv_hash in the header -- inside EOS_IMG_SIGNED_LEN -- and documents that "the TLV area sits after the payload". So this PR now emits [header][payload][TLV] with hdr_size always 156. eFirmware's equality check keeps working untouched, rollback.c's arithmetic is correct as written, and no field changes meaning. That reordering also resolves an ordering problem the old shape had: the TLV carried the signature, so its digest could not be computed before the signature existed. The signature now lives in the header's signature[] field and the TLV carries metadata only, which is what makes tlv_hash signable. Finding 4 (Medium) -- the two `assert`s in cmd_sign are removed by `python -O`. This is release-signing tooling, so the signature-length check is now a `raise SystemExit`. The TLV-length assert is gone with the code that needed it. Finding 5 (Medium) -- `pytest.importorskip("cryptography")` still skipped silently if the install ever failed, leaving "collected 19, ran 0" as a green run. Now gated: EOS_REQUIRE_SIGNING_TESTS is set in ci.yml, and under it the module is imported directly so a missing dependency is a collection error. Locally, without the variable, the skip still applies. Finding 6 (Low) -- cmd_verify bounds-checks the image length before unpacking, so a short file gets a FAIL: line rather than a struct.error traceback, and the 156 is hoisted to EOS_IMG_STRUCT_SIZE rather than re-derived. Finding 3 (Medium) needs no change here: master's include/eos_image.h already documents the layout this now emits. Rebased onto embeddedos-org#94 (the master repair) and reduced to the five files that are still this PR's own -- the branch was stale enough that its diff against master would have reverted core/rollback.c, include/eos_image.h, tests/unit/test_tlv_auth.c and the rest of embeddedos-org#93's TLV work. Verified: end to end, with the real tool: eos_sign.py keygen / sign / verify -> VERIFIED total 744 = 156 header + 512 payload + 76 TLV hdr_size 156, tlv_len 76, TLV magic 0x6907 at 156+512 sha256(tlv)[:28] == header bytes 64..92 True ctest 22/22 PASS test_eos_sign_boot_path (real parser over real tool output): current layout [hdr][payload][tlv]: verify_integrity -> 0 PASS old layout [hdr][tlv][payload]: verify_integrity -> -3 PASS TLV magic at addr + hdr_size + image_size: 0x6907 PASS that third line is finding 2's regression test -- it fails on the old layout, where the address lands past the end of the image. pytest tests/ (EOS_REQUIRE_SIGNING_TESTS=1) 44 passed the CI gate itself: with the variable set and the module absent, the import raises ImportError -> collection error -> job fails, rather than skipping. Refs embeddedos-org#88, embeddedos-org#93
0eb2332 to
1b41b01
Compare
…gate that failed open Answers the review on embeddedos-org#90. Finding 1 (Medium) -- `CI Gate` gates three jobs in ci.yml. This repository has 16 workflow files and a PR head reports 26 checks across five runs, so the maintainer action in the body -- require one name -- would have left CodeQL, every EoSim platform leg, all three Cross-Platform legs and a second host build unrequired, and `master` could still go red from any of them. The rot-guard had the same boundary: WORKFLOW was hardcoded to ci.yml, so a job added to build.yml or codeql.yml was neither covered nor noticed. Adds two tests over every workflow with a `pull_request` trigger: REQUIRED_CHECKS names what a maintainer must actually require, NO_GATE excuses the rest with a reason about the workflow itself, and a third test asserts each gated workflow really has a job displaying under the name given. Finding 2 (Medium) -- `Simulation Gate` had the exact fail-open shape this PR removes from ci.yml: `needs: [simulate, cross-platform]`, then it printed both results and branched on `simulate` alone before printing "All simulation checks passed". A red or skipped `cross-platform` -- three OS legs -- passed it. Ported the `toJSON(needs)` + `jq` body, which is dependency-list-agnostic and cannot fall out of step with `needs:`. And made it a rule rather than a one-off: test_no_aggregating_gate_ignores_part_of_its_needs walks every gate in every pull-request workflow and fails if a declared dependency is never compared. Two refinements were needed to make it mean something: - printing a result is not testing it. The first version grepped for `needs.X.result` anywhere in the script, which Simulation Gate satisfied with its echo line. Only a line that compares counts. - book-build.yml's `summary` writes a step summary and claims no verdict. It is reporting, not gating, so the check applies only to jobs that either `exit 1` or assert that everything passed. Finding 3 (Low) -- this branch had replaced master's `pip3 install -r requirements.txt pytest-cov` with a hand-maintained list to add pyyaml, which regressed the guard in tests/unit/test_requirements.py: these jobs run pytest over tests/ but never install from requirements.txt ... ['ci.yml:test'] Restored the requirements.txt install; pyyaml was already declared there, so the hand list was not needed at all. That also removes the collision with embeddedos-org#88, which edits the same line. Verified: pytest tests/ 47 passed ctest 21/21 PASS yaml.safe_load of ci.yml, simulation-test.yml, book-build.yml all parse discrimination, both ways: - reverting Simulation Gate to its fail-open body gives simulation-test.yml:sanity-gate declares needs ['simulate', 'cross-platform'] but never tests ['cross-platform'] - with the toJSON(needs) body in place, 9 passed Refs embeddedos-org#90
… makes Answers the second review on embeddedos-org#82. Finding 1 (High) -- step 4, "Verify signing key against OTP root-of-trust", was the same fail-open this PR removes from step 7, two steps earlier. An `eos_hal_otp_read()` failure was discarded and boot continued; and when the read succeeded and the anchor was provisioned, the body of the `if` was `/* In a full implementation, extract key hash from TLV and compare */`. So a device whose root of trust *is* provisioned booted an image signed by any key the image carried, and step 8 recorded EOS_SBOOT_OK. Implementing the TLV comparison is out of scope, as the review says. The two things that are in scope are done: a non-EOS_OK otp_read now fails EOS_SBOOT_ERR_SIGNATURE, and a provisioned anchor that nothing compares against refuses the boot rather than proceeding. An unprovisioned board (all-zero anchor) is deliberately unchanged -- there is nothing to check against, and refusing would brick every board that has not been provisioned. The comment now states plainly that the step is planned rather than implemented, which §8.1 asks for and a comment inside an `if` was not. No test for those two refusals, deliberately, and the file says why rather than leaving it to be discovered. Reaching step 4 requires passing step 3 -- a real Ed25519 signature checked against the keystore. I wrote the obvious test first and it was worthless: with require_signature = true and an unsigned fixture the boot fails at step 3 and returns the same EOS_SBOOT_ERR_SIGNATURE step 4 returns, so it passed against the unfixed code too. I confirmed that by reverting step 4 and watching it still pass. A test that cannot fail is worse than none. What is there instead is the counter-check that the change does not refuse a boot it should allow. The fixture is buildable -- the keystore ships RFC 8032 TEST 1's public key and the matching private key is in the RFC -- but that machinery is embeddedos-org#88's (tools/gen_signed_image_fixture.py). Worth doing once embeddedos-org#88 lands. Finding 3 (Low) -- `TEST()` did not increment `tests_run` and `main()` hardcoded `tests_run = 6`, so a test added to the file but not wired into `main()` would have been skipped with a zero exit. embeddedos-org#94 removed exactly this from tests/unit/test_ed25519.c, where a hardcoded 11 was masking two uncalled tests. Same fix here. Finding 4 (Low) -- the Valgrind `foreach` is hand-maintained and missed 6 of 22 registered tests. Added test_secure_boot_policy, test_fdt_loader and test_fw_decrypt. The list being hand-maintained at all is the real defect and is not fixed here -- it is the same class as the hardcoded count, one level up. Finding 2 (Medium), that eos_secure_boot() has no production caller, stands and is not addressed here; finding 1 makes it sharper rather than resolving it. Finding 5 (Low) is a PR-body correction. Verified: ctest 22/22 PASS test_secure_boot_policy 7/7 PASS step 7 discrimination, still: reverting the step-7 branch fails test_secure_boot_refuses_when_the_debug_lock_cannot_be_taken Refs embeddedos-org#82
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#88 "fix(eos_sign): put the payload where the bootloader looks for it"
head: 1b41b01 author: Kartikey1306 ci: pass
Verdict: The fix is the right one and the C fixture test proves it — I built and ran it.
[header][payload][TLV] with hdr_size fixed at 156 is exactly what core/rollback.c:70-74
and include/eos_image.h:26-30 already assume, so the two are correct by construction. Two
problems: one of the six new Python tests cannot fail — it exits argparse before reaching the
code it claims to exercise, which I reproduced — and the PR body describes a superseded
revision of the fix and a CI change that is not in the diff.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | High | tests/unit/test_eos_sign_payload_offset.py:152-161 |
test_the_tools_own_verify_still_accepts_its_output never runs cmd_verify. verify declares --key as required=True (tools/eos_sign.py:404, unchanged by this PR) and the subprocess omits it, so argparse exits 2 with the usage message on stderr and stdout empty — and assert "FAIL" not in r.stdout is then trivially true. Reproduced verbatim: returncode = 2, stdout = '', assertion True. The one test that covers the new tlv_hash checks, the hdr_size != 156 rejection and the short-image guard added at :305-313/:334-337 is the one test that runs none of them. .ai/reviewer.md names this shape directly: a verification whose result is discarded. |
Pass the key and assert the exit code: subprocess.run([sys.executable, TOOL, "verify", "--key", str(work/"kp_public.pem"), "--input", str(img)], …) then assert r.returncode == 0, r.stdout + r.stderr. With the key supplied the tool does pass — Hash: OK, Signature: OK, VERIFIED, rc=0 — so this is a test defect, not a tool defect. While there, add the negative cases the new code paths deserve: a 100-byte file, an image with hdr_size overwritten to 300, and one with a byte flipped inside the TLV area (must hit FAIL: TLV area does not match tlv_hash). |
| 2 | Medium | pr body | The body describes a revision the code no longer implements. Its "Measured, before and after" table gives hdr_size 156 → 300 and "what sits at addr + hdr_size"; the diff keeps hdr_size at 156 permanently (tools/eos_sign.py:88, 117) and moves the TLV area instead. The docstring at :14-22 explains why that reversal happened; the body was never updated to match. The body also says "ci.yml installs pytest pytest-cov but not cryptography … Added cryptography to the install" — requirements.txt:4 already declares cryptography>=41.0 and ci.yml:47 already installs -r requirements.txt, both on master. The diff adds an EOS_REQUIRE_SIGNING_TESTS step, which is a different and better thing, and the body does not mention it. Per the brief, a PR body whose measurements do not describe the head is itself the finding. |
Rewrite the table for the layout actually implemented (hdr_size 156 in both; TLV at hdr_size + image_size vs. at hdr_size), and describe the CI change that is in the diff. The 19 passed / 38 passed figure is still worth keeping if it was measured — say against which commit. |
| 3 | Medium | .github/workflows/ci.yml:48-55; tests/unit/test_sign_image.py |
The anti-silent-skip guard is half-installed. EOS_REQUIRE_SIGNING_TESTS is honoured only by the new module (test_eos_sign_payload_offset.py:36-42). tests/unit/test_sign_image.py is not in this PR and still calls bare pytest.importorskip("cryptography") — so the 14 cases that "pin the signed header format", named in the ci.yml:41-44 comment as the reason the guard exists, can still collect and skip with the job green. The step's own comment claims it "makes the skip impossible here"; it makes it impossible for 5 of 19 cases. |
Move the guard into a tests/conftest.py so it applies to every module that needs cryptography, or repeat the four-line pattern in test_sign_image.py. Then the comment is true. |
| 4 | Low | tools/gen_signed_image_fixture.py:41 |
SEED = bytes(range(32)) is a hardcoded Ed25519 private seed living in tools/, one directory entry away from tools/eos_sign.py, the release signing tool. .ai/security.md asks that development keys be structurally incapable of signing a release artifact. Nothing structural separates them here — only the fact that this script writes a C header to stdout. I confirmed the derived public key (03a107bf…) is not in core/keystore.c and that no workflow, CMake file or script invokes the generator, so the release path does not reach it today. core/keystore.c:29 shows the pattern this repo already uses for the same hazard: a #warning plus an EBLDR_PRODUCTION_KEY gate. |
Move the generator to tests/vectors/gen_signed_image_fixture.py so it is not in the tool directory, and rename the constant to something that cannot be misread — INSECURE_FIXTURE_SEED. Optionally assert in test_keystore.c that eos_fixture_pubkey is not a trusted slot, which makes the separation checkable rather than conventional. |
| 5 | Low | tools/eos_sign.py:81; include/eos_image_tlv.h:43 |
TLV_ED25519 = 0x24 is now unreferenced — removing the signature TLV entry from build_tlv() left its only user behind. EOS_TLV_ED25519 on the C side has no consumer either, on master or here. A wire-format tag that nothing writes and nothing reads will eventually be reused for something else. |
Delete TLV_ED25519 from eos_sign.py. For the C constant, either keep it with a comment saying it is reserved-and-unused, or drop it — a maintainer call, worth an issue rather than this PR. |
| 6 | Low | tools/eos_sign.py:340-355 (cmd_verify) |
The new malformed-input guards are a real improvement, but two gaps remain in a command aimed at untrusted files: tlv_len is not bounded (core/rollback.c:92 rejects > EOS_TLV_MAX_SIZE = 512; the tool accepts up to 65535), and trailing bytes past hdr_size + image_size + tlv_len are not rejected, so an image with appended data still reports VERIFIED. |
Add if tlv_len > 512: FAIL to match the parser the image will actually meet, and if len(image) != EOS_IMG_STRUCT_SIZE + img_size + tlv_len: FAIL for trailing data. |
| 7 | Low | tests/unit/test_eos_sign_payload_offset.py:145-150 |
test_hdr_size_stays_within_the_parsers_bound asserts 156 <= hdr_size <= 4096, but test_hdr_size_is_the_struct_size_and_the_tlv_follows_the_payload:110 already asserts hdr_size == 156. It cannot fail unless the stronger test fails first. |
Either drop it, or make it earn its place by writing an out-of-range hdr_size into a copy of the image and asserting cmd_verify rejects it — which is the bound that actually matters. |
Architecture conformance
Master design §8.1 (signed manifests and images; explicit separation of implemented vs.
planned), §14.1 (key management integrated across eBoot, eSec, eOTA and release signing),
§15.1 (signed metadata and package provenance), §5.1 (eBoot keeps the TCB minimal and
auditable). Conforms.
The direction is settled by the code already on master, not by preference:
core/rollback.c:70-74 tlv_addr = image_addr + hdr.hdr_size; tlv_addr += hdr.image_size;
include/eos_image.h:26 "The TLV area sits after the payload, so neither the signature
… nor hash[] … reaches it. tlv_len and tlv_hash do"
So hdr_size is the struct size and the TLV area trails the payload. The docstring at
tools/eos_sign.py:14-22 reaches the same conclusion and cites the two readers that would
have broken under the hdr_size = 300 alternative. Removing the TLV_ED25519 entry from
build_tlv() resolves the ordering problem honestly — tlv_hash is inside
EOS_IMG_SIGNED_LEN (offset 64+28 = 92), so a TLV area containing the signature could not
be hashed before the signature existed. Nothing in the diff points up a tier; this is
Tier-1 tooling writing a Tier-1 format.
Verified by running, on master + this patch:
cmake -B build/host -G Ninja -DEBLDR_BUILD_TESTS=ON && cmake --build build/host
ctest --output-on-failure --no-tests=error -j4
-> 100% tests passed, 0 failed out of 22 (test_eos_sign_boot_path included)
./tests/eboot_test_eos_sign_boot_path
current layout [hdr][payload][tlv]: verify_integrity -> 0 [PASS]
old layout [hdr][tlv][payload]: verify_integrity -> -3 [PASS]
TLV magic at addr + hdr_size + image_size: 0x6907 [PASS]
python3 tools/gen_signed_image_fixture.py | diff - tests/vectors/signed_image_fixture.h
-> identical; the committed fixture reproduces from the tool
eos_sign.py keygen && sign && verify --key … --input …
-> Hash: OK / Signature: OK / VERIFIED, rc=0
The C test is the strongest thing in this PR. It is the difference between checking the tool
agrees with itself and checking the bootloader agrees with the tool, and holding both
layouts over one payload and key means the fix and the defect are pinned together. Worth
saying explicitly: the fixture carries only the public key, and eos_fixture_pubkey is not
in core/keystore.c — I checked.
Proposed changes
- Fix finding 1 — pass
--key, assertreturncode == 0. Without it the PR ships a test
that will keep passing after the code it names is deleted. - Add the three negative cases in finding 1 so the new guards in
cmd_verifyhave coverage. - Move the
cryptographyguard intotests/conftest.py(finding 3). - Rewrite the body's measurement table for the implemented design (finding 2).
- Findings 4–7 are small and can travel with the above or follow separately.
Not checked
- No on-hardware or cross-compiled run. Host
x86_64gccandctestonly; the ARM
Cortex-M4 and STM32F4 legs inchecks.txtwere read, not reproduced. pytestis not installed in this environment, so the six new Python tests were not run as
a suite. Finding 1 was verified by executing the test's exactsubprocess.runinvocation
directly and observing rc/stdout, not by running pytest.- The body's
19 passed → 38 passedfigure was not reproduced (nopytest), and I could not
determine which commit it was measured against —masteralready installscryptography. eFirmware/src/efw_image.c, cited in the docstring and in the test as checking
hdr_size != 156exactly, is in another repository and was not inspected.EOS_TLV_MIN_SEC_VERend-to-end anti-rollback behaviour was not exercised beyond
test_rollbackandtest_tlv_authpassing in the suite above; the new C test checks the
TLV area is findable athdr_size + image_size, not that a downgrade is refused.mergeStateStatus: BLOCKED,reviewDecision: REVIEW_REQUIRED— the branch protection rule
behind that was not inspected.- This PR also carries the
core/ed25519_verify.c,include/eos_image.hand
tests/unit/test_ed25519.cchanges that repairmaster's broken host build (identical to
eBoot#81, reviewed there). They are the reason this branch builds at all —
origin/master@22d8f8bdoes not compile. Reviewed under #81; not re-litigated here.
Automated architecture review of 1b41b01d5a5b — 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.
eos_sign.pywrites[header][TLV area][payload]and stampedhdr_sizeas a fixed 156 — the size ofeos_image_header_talone. Nothing else in the system readshdr_sizethat way:For both,
hdr_sizeis the offset from the image base to the payload. Soaddr + 156landed on the TLV block, the SHA-256 taken overimage_sizebytes from there did not matchhash[], and every image this tool produced failed integrity verification on-device. The tool's own docstring recorded this as a known limitation and pointed users atsign_image.pyinstead.Measured, before and after
Signing 2048 deterministic bytes:
hdr_sizeaddr + hdr_size0x6907image[hdr_size:hdr_size+image_size] == payloadsha256(payload@hdr_size) == hash[]Why this direction
The docstring framed the fix as a format decision — move the TLV after the payload, or teach the boot path to parse it. There is a third option, and
sign_image.pyalready settles which convention is the established one: keep the layout and makehdr_sizemean what every reader already assumes.hdr_sizenow spans the header struct and the TLV area.This is the least invasive of the three: no C changes, no boot-path change, and no change to the bytes an image without a TLV area produces (
tlv_lenis 0,hdr_sizeis 156, byte for byte what it was).hdr_size = 300sits well inside the boundcore/image_verify.calready enforces (>= sizeof(header),<= 4096).hdr_sizeis at offset 6, inside the signed prefix, so the TLV length has to be final before the prefix is signed. It is deterministic — payload hash, key hash and an Ed25519 signature, all fixed-width — so it is computed up front, with an assert afterwards that the real TLV came out the same length. If those ever diverge,hdr_sizepoints at something that is not the payload and the image fails silently on-device, so it is worth the assert.cmd_verifyupdated to match: the TLV area is insidehdr_sizenow, so it starts at 156 and the payload starts athdr_size.These tests have never run in CI
ci.ymlinstallspytest pytest-covbut notcryptography, and bothtests/unit/test_sign_image.pyand the new fileimportorskipon it. 19 signing-toolchain tests were being collected and silently skipped — the signing path has never been exercised in CI. Addedcryptographyto the install.Validation
🤖 Generated with Claude Code