Fix anti-rollback to use authenticated TLV counter - #103
Conversation
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#103 "Fix anti-rollback to use authenticated TLV counter"
head: c9fe84c author: Tanisha1723 ci: none reported
Verdict: Correct and valuable fix to a real anti-rollback bypass on the boot path that
actually runs. The core change conforms to §8.1 and .ai/security.md. What is missing is
coverage of the two production paths it changes, and cleanup of the defective function it
leaves exported.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | core/image_verify.c:230 |
After this PR eos_image_check_rollback() has no production caller — git grep at this head finds only the new test. It remains exported, and it is exactly the defect being removed: it compares a firmware image_version against the OTP monotonic floor. Any future caller reintroduces the bypass, silently and with a plausible-looking name. |
Delete it. If the regression test needs to demonstrate the old behaviour, move the comparison into the test file under a name that says it is wrong (legacy_broken_version_floor_check), so no production translation unit can reach it. |
| 2 | Medium | tests/unit/test_tlv_auth.c:304 |
The new regression test does not exercise either changed path. It calls eos_rollback_read_image_counter() and eos_rollback_verify() directly. Neither eboot_jump_to_app() (stage1/jump_app.c:60) nor eos_fw_update_finalize() (core/fw_update.c:191) is invoked. I confirmed this by reading the test: reverting either production hunk leaves the suite green. The bug being fixed lives in the wiring, and the wiring is untested. |
Add one test that drives eos_fw_update_finalize() to completion with an image carrying image_version = 0x00010000 and MIN_SEC_VER = 3 against sim_counter = 9, asserting EOS_ERR_ANTI_ROLLBACK. test_fw_update.c already has the context construction needed. |
| 3 | Medium | stage1/jump_app.c:60, core/secure_boot.c:148 |
Two independent implementations of the same TCB sequence now exist: eboot_main() → eboot_jump_to_app() (stage1/main.c:75, the path stage 1 actually takes) and eos_secure_boot(), which nothing in stage1/ calls. The PR comment acknowledges this — "Same sequence as eos_secure_boot() step 5b" — and duplication is how the two paths diverged in the first place: one got the TLV counter, the other kept image_version. |
Extract the read → verify → stage sequence into one function in core/rollback.c (e.g. eos_rollback_gate_image(addr, bool stage)) and have both call sites use it, so the next change cannot land in one path only. Out of scope for this PR; open a follow-up. |
| 4 | Low | tests/unit/test_tlv_auth.c:339 |
tests_run is still a hand-maintained literal. This PR corrects it to 8, but the next test added under-reports unless someone remembers, and tests_passed == tests_run then passes with a test that never ran. |
Already addressed org-wide by open PR eBoot#95 ("derive the suite totals … instead of restating them"). No change needed here; rebase after #95 lands. |
| 5 | Low | PR body, "Testing" | The body reports the C suite as unrunnable due to "the current MSVC build". The blocking errors are not MSVC-specific and not a Windows artifact — they are two merge defects on master itself that break the gcc host build too (see Not checked). Worth correcting so the next reader does not go looking for a toolchain problem. |
Restate as: blocked by broken master, tracked by eBoot#94 / #98. |
Note on the "Testing" section: the body states plainly that the suite could not be run,
rather than asserting an unsupported PASS. That is the right call and is not a finding.
Architecture conformance
Conforms. Master design §8.1 requires "rollback protection where hardware/policy supports
it"; .ai/security.md ("Boot and update chain") requires "anti-rollback counters actually
checked". Before this PR the counter was not actually checked on the live boot path —
hdr.image_version (scale 0x00MMmmpp) was compared against the OTP monotonic floor
(small integers), so an old image with a high version number cleared a high floor. The
change substitutes the authenticated EOS_TLV_MIN_SEC_VER, which core/rollback.c binds
to the signed header via tlv_len/tlv_hash. That is the value §8.1 intends.
Two secondary points, both correct as written:
- Dropping the
rb_rc != EOS_ERR_NOT_SUPPORTEDtolerance is a genuine tightening and does
not brick counter-less boards:eos_rollback_verify()(core/rollback.c:129) maps
EOS_ERR_NOT_SUPPORTEDfrom the HAL toEOS_OKitself. The now-fatal cases are a
malformed header or a TLV area that does not match its signed hash, which must fail
closed per.ai/security.md("Fail closed"). - Verifying but not staging in
eos_fw_update_finalize()is right. Staging on install
would raise the floor for an image that has not booted, removing the fallback — the
separationinclude/eos_rollback.hdocuments.
No layering violation. eos_rollback.h is within eBoot; nothing points up a tier (§5.1).
The TCB does not grow: core/rollback.c was already linked into eboot_core.
Proposed changes
- Delete
eos_image_check_rollback()fromcore/image_verify.cand its declaration;
move the demonstration of the old comparison intotest_tlv_auth.cunder an explicitly
wrong name. (Finding 1.) - Add the
eos_fw_update_finalize()regression test described in finding 2. This is the
one change I would ask for before merge — as it stands the fix is correct but unguarded. - Leave finding 3 to a follow-up; folding it in here would mix a structural change with a
behavioural one, which.ai/architect.mdprohibits.
Not checked
-
CI:
checks.txtis empty. No check runs were reported for this head at bundle time.
I did not determine whether the workflows had not started, are not required on this
branch, or did not trigger. Read the finding below as the reason it matters, not as a
claim about what CI did. -
masterdoes not compile, and this is not caused by this PR. Verified: I extracted
origin/master(22d8f8b) to a scratch directory and ran the CI's own host
configuration. It fails with two independent, pre-existing defects:
include/eos_image.h:135,142still assert on areservedmember thattlv_len/
tlv_hashreplaced, andcore/ed25519_verify.c:338redefinespoint_is_identity
(a merge artifact — the same function appears at :281). The PR head inherits both.
Consequence for this PR: TCB code is landing with no green build behind it.
This is already covered by open PRs eBoot#94 and eBoot#98, so per the review policy I
have opened nothing. #94 or #98 should merge before this. -
I did run this PR's new test, with those two master defects patched out in a scratch
copy only (git archiveto/tmp; the repository was not touched). Result:$ cmake -B bld2 -DEBLDR_BUILD_TESTS=ON && cmake --build bld2 --target eboot_test_tlv_auth $ ./bld2/tests/eboot_test_tlv_auth test_hw_floor_uses_tlv_counter_not_image_version [PASS] 8/8 passed (exit 0)So the new test does pass and does demonstrate the stated bug. It is not evidence that
the suite passes onmaster, becausemasterdoes not build; the fullctestrun and
the ARM cross build were not run —tests/unit/test_ed25519.c:31,263has its own
duplicate-definition merge artifact that blocks the rest of the test target. -
Not verified on hardware. Whether
eos_hal_monotonic_read/incrementbehave as
assumed on a real OTP-backed board is untested here; the run above uses the simulated
HAL intest_tlv_auth.c. §28.1 would require a named board before calling T-202
mitigated on hardware. -
Not examined: whether
eos_bootctl_confirm()(core/bootctl.c:159, which calls
eos_rollback_commit()) executes in the same address space as theeboot_jump_to_app()
that staged the counter.g_staged_*is file-static RAM state; if confirm runs in the
application rather than in the bootloader image, the staged value is not visible and the
floor never advances. That is pre-existing design, not introduced here, but it decides
whether this fix ends up enforcing anything, and I could not settle it from the tree.
Automated architecture review of c9fe84c4b26c — 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.
Summary
Fixes the production anti-rollback check to use the authenticated
EOS_TLV_MIN_SEC_VER security counter instead of the firmware
image_version field.
Changes
without staging it, since finalize installs an image rather than
booting it.
and the authenticated TLV security counter.
Testing
build is blocked by pre-existing errors unrelated to this change.
Commit
28700bc Fix anti-rollback to use authenticated TLV counter