security: harden application slot boundary validation - #76
Conversation
srpatcha
left a comment
There was a problem hiding this comment.
Approving. The gap is real and the arithmetic is right.
The gap
eboot_jump_to_app() parses the header and then calls
eos_image_verify_integrity(), which streams hdr.image_size bytes from
addr + hdr.hdr_size. Before this, nothing bounded that against the slot.
The only existing limit is a global one:
image_verify.c:95
if (out->image_size == 0 || out->image_size > 16 * 1024 * 1024)A fixed 16 MB ceiling, not the slot's actual size. On a part where a slot is
128 KB, a header claiming 15 MB passes that check and the verifier reads far past
the slot into whatever follows it in the address map.
The update path already has the equivalent guard —
fw_update.c:90-91
ctx->slot_size < sizeof(eos_image_header_t) ||
ctx->header.image_size > ctx->slot_size - sizeof(eos_image_header_t)— so the boot path was the one place a header could describe an image larger than
the slot holding it. Adding it here makes the two paths agree.
The arithmetic
if (slot_size == 0 ||
hdr.hdr_size > slot_size ||
hdr.image_size > slot_size - hdr.hdr_size)Ordered correctly. hdr.hdr_size > slot_size is tested before the
subtraction, so slot_size - hdr.hdr_size cannot wrap — and hdr_size is
attacker-controlled, so that ordering is load-bearing rather than stylistic.
Written the other way round it would be the same unsigned-wrap defect found in
eos#95 (p + len > cap) and eos#87 (bytes_written + len).
Using hdr.hdr_size rather than sizeof(eos_image_header_t) is also the better
choice here: it is the value actually used as the payload offset two calls later,
so the check bounds the same quantity the read uses. It is safe precisely because
the line above it bounds hdr_size first.
Placing it before verification rather than after matters too — the point is to
stop the streaming read from starting, not to notice afterwards.
Verified
Applied on current master:
0 build errors
100% tests passed, 0 tests failed out of 18
One suggestion, not blocking
There is no test for it. The EOS_LOG_IMAGE_INVALID path is observable, and
eBoot's suite already has the machinery — test_slot_size_bounds.c exists and
test_image_verify.c has a simulated flash with configurable slot geometry. A
case with image_size just over slot_size - hdr_size would pin this against a
future refactor that reorders the comparison and reintroduces the wrap.
Approving as is; the check is correct and the absence of a test is not a reason
to hold a bounds fix on the boot path.
Note for the rebase, if one is needed: test targets here are namespaced eboot_*
since #71, with a configure-time guard that will tell you if a new one misses it.
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#76 "security: harden application slot boundary validation"
head: 42c3f22 author: Mohammed18-19 ci: unstable (bundle has no check list)
Verdict: The check is correct and the arithmetic is safe. It arrives with no test
covering the function it changes, and it is the third verbatim copy of the same four
lines in this repo.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | stage1/jump_app.c:34-41 |
No test exercises this code path. git grep -l eboot_jump_to_app -- tests/ returns nothing on master or on this head. The PR cites test_slot_size_bounds as covering it, but that file's own header (tests/unit/test_slot_size_bounds.c:5-27) states it tests verify_slot(), and its body calls eos_slot_scan_all() — core/slot_manager.c, a different call path. A security hardening change whose rejection branch has never been executed is not evidenced. |
Add a test_jump_app_bounds alongside test_slot_size_bounds, reusing its simulated-flash fixture: an oversized image_size must return EOS_ERR_INVALID with payload_bytes_read == 0, and an in-slot image must be let through. |
| 2 | Medium | stage1/jump_app.c:35-38 |
Third copy of the same idiom. core/slot_manager.c:54-56 and core/recovery.c:318-320 already carry it character-for-character. Three call sites now have to be kept in step by hand, and the next boundary path added will be a fourth. |
Extract bool eos_image_fits_in_slot(eos_slot_t slot, const eos_image_header_t *hdr) next to eos_image_parse_header() and call it from all three. Doing it in this PR is fine — it is a pure move — but a separate refactor PR is cleaner given #55/#57 are already conflicting in this repo. |
| 3 | Low | PR body, "Validation" | ctest — 18/18 tests passed is true for this branch's base (038f624), which registers 18 add_test entries. Current master registers 20. The claim reads as full-suite coverage and is two tests short of it. |
Rebase and re-run; state the base commit alongside the count. |
| 4 | Low | stage1/jump_app.c:81 |
The comment says "Use the validated entry point, not load_addr" — nothing in this function validates hdr.entry_addr, including after this change. It is in fact safe, because eos_image_verify_signature() signs the header prefix rather than hash[] alone (core/image_verify.c:192-198), so entry_addr is authenticated. The comment attributes that to a validation step that does not exist. |
Reword to say the entry point is authenticated by the header signature, which is the actual property. |
What the change gets right
eos_hal_slot_size(slot) returning 0 is rejected first, so the later subtraction cannot
be taken on a bad slot. hdr.hdr_size > slot_size is tested before
slot_size - hdr.hdr_size, so the uint32_t subtraction cannot wrap — the ordering is
load-bearing and it is right. The placement is right too: it sits after
eos_image_parse_header() and before eos_image_verify_integrity(), which is exactly
where the body claims it prevents a read past the slot, and
eos_image_verify_integrity() streams image_size bytes from addr + hdr_size, so
the bound the check establishes is the bound that matters. The failure is logged through
the same eos_boot_log_append(EOS_LOG_IMAGE_INVALID, ...) the neighbouring failures use.
Architecture conformance
Conforms. §21 Tier 1 (eBoot, Foundation) — a Stage-1 handoff bounds check belongs here
and nowhere else. §5.1: no new dependency, nothing pointing up a tier, and the change
narrows rather than widens what the TCB will accept, which is the direction "minimal and
auditable" asks for. §8.1 lists a "Factory/recovery image strategy"; note the function
still returns an error to stage1/main.c:75 rather than routing to recovery, which is
pre-existing and out of scope here.
Proposed changes
1. Add tests/unit/test_jump_app_bounds.c (fixture copied from
test_slot_size_bounds.c) + 3 lines in tests/CMakeLists.txt.
2. Reword the stage1/jump_app.c:81 comment.
3. Follow-up PR: extract eos_image_fits_in_slot() and collapse the three
copies. Do not fold this into the same commit as the behaviour change.
Not checked
- Nothing was built or run; the brief forbids checking out, and
eBoot's tree is on
another branch. Test-coverage claims above come fromgit grep, not from a run. - CI is unverified.
ghreportsmergeStateStatus: UNSTABLE, meaning at least one
check is failing, but this bundle'schecks.txtis empty so I cannot say which. Given
the sibling PRs,test_image_verify.c's lost line continuation (#77, since merged as
b7e07d4) is the likely cause, but that is a guess and I have not confirmed it. reviewDecision: APPROVEDis recorded; I did not check against which head.
Automated architecture review of 42c3f2262ad0 — 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
Harden the Stage-1 application handoff by validating the image size against the actual slot capacity before integrity verification.
Security impact
eboot_jump_to_app()previously proceeded from header parsing directly to payload integrity verification.This change adds defense-in-depth validation before any payload bytes can be streamed:
Validation
cmake --build build— passedctest --test-dir build --output-on-failure— 18/18 tests passedgit diff --check— passedtest_slot_size_boundsregression test passes.The change is intentionally small and localized to the Stage-1 application handoff path.