Fix/authenticate tlv anti rollback

[vignesh917]

Summary

Type of Change

• eat — New feature
• ix — Bug fix
• docs — Documentation only
• style — Formatting, no code change
• [ ]
  efactor — Code restructuring without behavior change
• est — Add or fix tests
• �uild — Build system or dependency changes
• ci — CI/CD pipeline changes
• perf — Performance improvement

Changes

Testing

• Unit tests pass (ctest --test-dir build --output-on-failure)
• Integration tests pass
• Manual testing performed
• New tests added for new functionality

Pre-Submission Checklist

• Code compiles without warnings (-Wall -Wextra -Werror for C)
• All existing tests pass
• New tests added for new functionality
• Documentation updated if API changed
• Commit messages follow (): convention
• Branch is rebased on latest master

Related Issues

Screenshots / Logs

Additional Notes

[srpatcha]

Review — eBoot#93 "Fix/authenticate tlv anti rollback"

head: 02a0a6e author: vignesh917 ci: none reported (checks.txt is 0 bytes)

Verdict: The security analysis is correct and the core fix is the right one — the anti-rollback counter really is read from bytes that nothing signs or hashes, and binding the TLV area through the signed prefix is the right shape of answer. I extracted 13a7a02, applied this patch, built it and ran the suite: it compiles clean and 21/21 pass. But the binding has a hole I could exploit in the built tree, the format change consumes a signed field without a version bump, and the PR itself is unusable as submitted — an empty template body, no CI run at all, and 484 lines of hiring-assessment paperwork plus a proof-of-concept exploit committed into the repository root.

Findings

#	Severity	File:line	Finding	Recommended fix
1	High	core/rollback.c (the eos_tlv_parse() call, after the new binding check)	The binding covers hdr.tlv_len bytes; the parser is steered by a different length that is never compared against it, and the downgrade survives in the gap. tlv_area_matches_header() hashes exactly hdr.tlv_len bytes. eos_tlv_parse() then reads info.tlv_total_len out of the area and bounds it only against sizeof(eos_tlv_info_t) and EOS_TLV_MAX_SIZE — never against hdr.tlv_len. So any byte between hdr.tlv_len and info.tlv_total_len is parsed as a TLV entry while sitting outside everything the header vouches for. Executed against this patch applied to 13a7a02 and linked to the built libeboot_core.a + libeboot_hal.a, with the device floor at 9 and a signed image declaring counter 3: with tlv_len = 12 (the whole area) tampering is caught, rc=-2, eos_rollback_verify returns -14 — the fix works. With tlv_len = 4 (sizeof(eos_tlv_info_t), which passes the new tlv_len < sizeof(eos_tlv_info_t) guard) and the area's own tlv_total_len still 12, rewriting the same four counter bytes gives rc=0 counter=9 verify=0 — the exact forgery PROPOSAL.md §1 demonstrates against master, intact. In fairness this is not attacker-reachable on its own: hdr.tlv_len is inside the signed prefix, so a short binding has to come from the signer, not the attacker. That is precisely why it matters now — finding 5 says no signing tool emits these fields yet, so the invariant is entirely unpinned, and whoever writes that tool can produce images that pass every test in test_tlv_auth.c with the vulnerability still live. Severity is not rounded down for unreachability, per .ai/security.md.	One comparison, after eos_tlv_parse() succeeds: if (ctx.total_len > hdr.tlv_len) return EOS_ERR_INVALID;. The stricter != is better still — the header and the area should agree exactly, and there is no legitimate producer of a mismatch. Add the case to test_tlv_auth.c alongside oversized_tlv_len_is_rejected; my probe is 80 lines and reuses that file's simulated-flash harness almost verbatim.
2	High	— (checks.txt is empty; PR body)	1005 added lines across the image header ABI, core/rollback.c and the signing tools, with zero executed evidence attached to the pull request. No check of any kind reported on this head — checks.txt is 0 bytes, while eBoot#92 in the same batch carries 27. createdAt and updatedAt are 2026-09-02T12:18:02Z / 12:18:03Z, opened and never touched, which fits a first-time contributor whose workflow runs need maintainer approval. Separately, the PR body is the unmodified template: every "Type of Change" box unchecked, "Changes" left as two empty bullets, "Related Issues" empty, and the entire Testing and Pre-Submission checklists unchecked — including "All existing tests pass" and "Code compiles without warnings", on a change that redefines a signed header field. The evidence does exist and it is good, but it is inside PROPOSAL.md, a document written for a hiring screen rather than for this repository, and none of it is where a reviewer or the merge record will look. I verified the substance independently rather than taking it on trust: see the verified-clean list below.	A maintainer approves the workflow runs so the checks execute against this head. The author rewrites the PR body from PROPOSAL.md §§1–4 — the finding, the fix, the seven test cases and the ctest / run_all_tests.py numbers — and fills in the checklist honestly, including the limitations in §5 rather than leaving them only in a file that is not going to be merged.
3	Medium	PROPOSAL.md:1, RUN.md:1, poc/README.md:1, poc/tlv_downgrade_poc.c:1	484 lines of assessment paperwork and a working exploit committed to the repository root. PROPOSAL.md is addressed to a hiring panel — it opens "eBoot — Technical Assessment Submission", carries the author's name and personal email address, closes with "I used AI assistance while working through this", and states plainly: "Per the screening instructions I have not opened a pull request against embeddedos-org/eBoot." This PR therefore contradicts its own contents; either it was opened by someone else or the instruction changed, and neither is recorded anywhere. RUN.md is reproduction instructions for that submission. The root of this repo already carries HANDOFF.md, MODES.md, PR_DESCRIPTION.md, TASKS.md and ORCHESTRATION.md, so the bar here is low, but a third party's email address in a public repository is not something to merge by inattention. The PoC is a different question from the other three: it is genuinely useful and it belongs in the tree — just not at the root under a name that reads like product code.	Drop PROPOSAL.md and RUN.md from the branch entirely; their content belongs in the PR body (finding 2) and in the commit messages. Move poc/tlv_downgrade_poc.c to tests/ — it is a regression demonstration, and tests/ is where .ai/architect.md's target layout puts "unit, functional, fuzz, performance, simulation". If the author wants the write-up preserved, docs/ with the personal details removed.
4	Medium	include/eos_image.h:37-54	A signed field is redefined without a header-version bump. reserved[30] becomes tlv_len + tlv_hash[28] at the same offsets, EOS_IMAGE_HDR_VERSION stays at 2, and the header stays 156 bytes — so an old bootloader reading a new image and a new bootloader reading an old image are both well-defined, which is the argument PROPOSAL.md §3 makes and it is a fair one. What it does not give you is any way for a consumer to tell the two apart. After this change, tlv_len == 0 means both "this image predates the binding" and "this image has no TLV area", and a device cannot distinguish an image that was never bound from one whose binding was legitimately empty. §23.2 makes firmware format a versioned compatibility contract, and eFirmware/src/efw_image.c is a second consumer of this struct that this PR does not touch. PROPOSAL.md §5.2 raises this itself and asks for a maintainer's call, correctly — it is a format decision, not a bug fix, and .ai/architect.md requires stating reason, migration impact and safest path before a structural change.	This is the maintainer's call, not the reviewer's, and it should be made before the code lands rather than after. If the reserved-byte route is chosen, bump EOS_IMAGE_HDR_VERSION to 3 anyway so that "carries a TLV binding" is expressible, and record the decision as an ADR under §38's ADR-007. Audit eFirmware's copy of the struct in the same change.
5	Medium	tools/eos_sign.py:105, tools/imgpack.py:74	The fix ships inert: no producer in the tree sets tlv_len, so after this change the anti-rollback counter can never be read at all. Both tool hunks change a comment and nothing else — each still emits b'\x00' * 30. With tlv_len always 0, the new early return fires before eos_tlv_parse(), *counter_out stays 0, eos_rollback_stage(0) is what reaches eos_rollback_commit(), 0 <= current clears the staged value, and the monotonic floor can never advance. PROPOSAL.md §5.1 discloses this honestly and calls it a deliberate fail-closed default pending the format decision in finding 4, which is a defensible position. Two things still need saying. First, none of it appears in the PR body, so the merge record will show "Fix/authenticate tlv anti rollback" against a change that leaves anti-rollback unusable. Second, "nothing regresses" is narrower than it sounds: it holds for images the in-tree tools produce, because those carry no TLV area and read 0 on master too — I confirmed eos_tlv_parse() returns EOS_ERR_NOT_FOUND on erased flash at core/image_tlv.c:29. It does not hold for an image built by an external tool that appends a conformant TLV area. Such an image reads its real counter on master and reads 0 here, and on a device whose floor has already advanced past 0, eos_rollback_verify(0) returns EOS_ERR_ANTI_ROLLBACK — the device stops accepting it.	Say it in the PR body and in the commit message, in the same words §5.1 uses. Then land the producer side as the immediate follow-up, gated on finding 4's decision, and add finding 1's cross-check before the tool exists rather than after — a signing tool written against the current code has no way to learn that tlv_len must cover the whole area.
6	Low	core/rollback.c (the hashing loop)	The TLV read is still unbounded against slot capacity, and now there is a hashing loop walking it. verify_slot() bounds hdr_size + image_size against eos_hal_slot_size(); nothing bounds the area that follows, so tlv_area_matches_header() can stream up to EOS_TLV_MAX_SIZE (512, include/eos_image_tlv.h:40) bytes past the end of a slot an image exactly fills. PROPOSAL.md §5.5 raises this and is right that it is not a regression — eos_tlv_parse() already read the same region — and right that the HAL flash_read implementations reject out-of-range addresses, which is what makes it Low rather than higher. It is worth recording because this PR is the change that makes the region security-relevant.	Bound tlv_addr + hdr.tlv_len against eos_hal_slot_size() next to the existing capacity check in slot_manager.c, as §5.5 suggests. Separate PR.
7	Low	core/sha512.c:1, tests/production_test_suite.py:902,1205	Two unrelated fixes ride along: a missing SPDX header, and /home/ubuntu/eBoot hard-coded in two places in the production suite so it raised FileNotFoundError for anyone but the author. Both are real improvements and both are one-liners, so this is not worth splitting — but neither is described by the PR title or body, and .ai/architect.md asks that restructuring and behaviour changes not share a commit. The path fix in particular is the reason the author could run the suite at all, which is worth one line in the body rather than being invisible.	Mention them in the body. No code change.

Verified clean, executed rather than reasoned, because a 1005-line TCB change with no CI deserves it. Extracted 13a7a02 with git archive into /tmp/eboot93, applied diff.patch (git apply --check clean), configured -DCMAKE_BUILD_TYPE=Debug -DEBLDR_BUILD_TESTS=ON:

• It builds, and adds no warnings. make exits 0. The only warnings are pre-existing: two -Wunused-parameter in core/boot_menu.c and the deliberate #warning about the test-vector trust anchor in core/keystore.c:29. None come from this diff.
• ctest --no-tests=error is 21/21, including the new test_tlv_auth, up from 20 on master. That is consistent with PROPOSAL.md's 38/38 once the 17 valgrind targets it also ran are added, which I did not run.
• The core fix works in the case it was written for. With a full-area binding, rewriting the four counter bytes gives rc=-2 (EOS_ERR_INVALID) and the downgrade is refused. That is the behaviour §1 of the proposal claims, reproduced independently.
• It compiles because every reserved reference was updated. grep finds the field used in test_image_abi.c:74, test_image_verify.c:331 and test_slot_manager.c:149-150, and all four sites are in the diff. The test_slot_manager.c mock now stashes its slot index in tlv_hash[0], which is ugly but no worse than what it replaced and does not collide with anything the mock exercises.
• The streaming digest API it depends on exists. eos_sha256_init / _update / _final and eos_sha256_ctx_t are declared at include/eos_crypto_boot.h:26-30, so the chunked hashing is not aspirational.
• core/rollback.c is the only TLV consumer in the tree. grep for eos_tlv_parse and eos_tlv_find outside image_tlv.c returns three lines, all in rollback.c. So the binding check does not need to be replicated anywhere else — for now.
• The constant-time comparison is genuine — volatile uint8_t diff accumulated with |= over all 28 bytes, no early return — and matches the secure_compare() pattern the proposal cites.

Architecture conformance

Conforms on layering, deviates on format governance. §21 Tier 1 — Foundation. core/rollback.c and include/ are the right homes; stage0/, hal/ and boards/ are untouched, and the new dependency is on eos_crypto_boot.h, which is already inside eboot_core — no #include, link line or target_link_libraries entry points up a tier, and §5.1's minimal-and-auditable TCB holds. §8.1's "rollback protection where hardware/policy supports it, with anti-rollback counters actually checked" is the requirement at issue, and the honest reading is that master does not meet it (the counter is forgeable) and this PR does not yet meet it either (the counter is unreadable) — findings 1 and 5.

The deviation is §23.2, "Firmware format — versioned .efw manifest and boot compatibility": the meaning of 30 signed bytes changes while EOS_IMAGE_HDR_VERSION stays at 2 (finding 4).

No proposal appended. The design gap is already on record and already covers this case exactly — .ai/autoreview/proposals/2026-09.md, "§8.1 requires signed images but never defines the image container" (§8.1, triggered by eBoot#88 and eBoot#87). Its proposed §8.2 states: "Redefining the meaning of an existing field is a breaking change to the format, even when no byte moves and no struct member changes. It requires a header-version increment, not a producer-side edit" and "Fields inside the signed prefix are authenticated, so their meaning is part of what a signature attests to." Those two sentences are finding 4 in full, written before this PR arrived. This is the third trigger for that proposal rather than a new one, and the existing text needs no amendment to cover it. The same proposal's point about eFirmware/src/efw_image.c as a second consumer of this struct is what makes finding 4's audit item concrete.

Proposed changes

Smallest sequence that keeps the tree working, in order:

1. Get the workflow runs approved and executed (finding 2). Nothing below should be judged final until they report.
2. Add if (ctx.total_len > hdr.tlv_len) return EOS_ERR_INVALID; after the parse, with a test case (finding 1). This is a two-line change and it is the only one I would call a merge blocker on the code itself.
3. Drop PROPOSAL.md and RUN.md; move poc/tlv_downgrade_poc.c under tests/ (finding 3). Rewrite the PR body from the proposal's own §§1–5 (findings 2 and 5).
4. Settle the format question — reserved bytes versus hdr_version 3 versus moving the TLV area under hash[] — as an ADR, and audit eFirmware's copy of the struct (finding 4). This is the one decision that should precede the merge rather than follow it, because it is the one that is expensive to reverse once images exist in the field.
5. Then the producer side in sign_image.py (finding 5), and the slot-capacity bound (finding 6), as separate PRs.

Steps 2 and 3 are independent of each other and of step 4. Steps 5 and 6 depend on step 4.

Not checked

• Nothing ran on hardware and nothing was cross-compiled. Host gcc, x86-64, Debug, out of tree under /tmp. The author states the same limitation in PROPOSAL.md §4 and I am in no better position: I did not build any target in boards/, so the stack-usage argument for the streaming digest is unverified, as is any claim about behaviour on a device with a real monotonic counter.
• I did not run the valgrind sweep, run_all_tests.py, or production_test_suite.py. So 38/38, 33/33 and the "one pre-existing SA-6.9 failure" are uncorroborated; my 21/21 is the plain ctest subset only. The SA-6.9 claim in particular — that the sanitizer-job failure is pre-existing and out of scope — I did not check against master.
• Finding 1 was demonstrated, its exploitability was not. I proved the parser walks past the authenticated region and returns a forged counter. I did not establish that any realistic signing tool would emit tlv_len < tlv_total_len, and I did not trace whether eos_secure_boot() step 5b would actually boot the resulting image end to end — my probe called eos_rollback_read_image_counter() and eos_rollback_verify() directly, not the full boot path.
• The 224-bit truncation argument was accepted, not analysed. PROPOSAL.md §3 argues second-preimage resistance on a ~64-byte blob is far beyond reach at 224 bits. That is almost certainly right and I did not examine it further, nor did I check whether truncated SHA-256 interacts badly with anything else in the tree.
• eFirmware was not opened. Finding 4 asserts it is a second consumer of this struct on the strength of the existing proposal's citation of eFirmware/src/efw_image.c:122, not on my own reading of that repository at its current head.
• The claim that PROPOSAL.md reflects an actual hiring screen is taken from the document itself. I have no other source for it, and finding 3 does not depend on it being true — the files do not belong at the repository root either way.
• mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED. No merge attempted. tests/CMakeLists.txt will collide with eBoot#84, #85, #88, #89 and #91, all of which touch it. Nothing in the repository was checked out, stashed or modified; the patched tree lives entirely under /tmp and was produced with git archive.

---

Automated architecture review of 02a0a6e03804 — 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.