fix(fuzz): point every harness at the API it claims to fuzz - #99
fix(fuzz): point every harness at the API it claims to fuzz#99Kartikey1306 wants to merge 3 commits into
Conversation
…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.
Four of the five fuzz harnesses did not test what they name, and the
build could not say so because not one of them includes a project
header. Each declares its target itself:
fuzz_recovery_protocol.c extern int eos_recovery_parse_packet(...)
fuzz_fw_update.c extern int eos_fw_update_init(void)
extern int eos_fw_update_process_chunk(...)
extern int eos_fw_update_finalize(void)
fuzz_bootctl.c extern int eos_bootctl_parse(...)
fuzz_image_verify.c extern int eos_image_parse_header(const void *, size_t)
eos_recovery_parse_packet, eos_fw_update_init,
eos_fw_update_process_chunk and eos_bootctl_parse have never existed —
those names appear nowhere outside the harness that declares them, so
three of the five targets have never linked.
The other two are worse, because they do link:
- eos_fw_update_finalize exists but takes (ctx, mode), not (void).
- eos_image_parse_header takes (uint32_t addr, eos_image_header_t *),
not (const void *, size_t). fuzz_image_verify has been passing a
pointer where an address is expected and a length where an output
struct is expected, on every input, for as long as it has run.
A local extern is why none of this was caught: it tells the compiler the
symbol exists with whatever shape the harness asserts, and the mismatch
survives to link time or past it.
All five now include the real header and drive the real entry points:
fuzz_image_verify parse_header -> verify_integrity, boot-path order
fuzz_bootctl bootctl_load, then trap if load() accepted a
block validate() rejects
fuzz_fw_update begin -> write in fuzz-chosen chunk widths ->
finalize or abort
fuzz_recovery_protocol write_in_range, trapping on any accepted write
that leaves the slot or wraps, checked in wider
types that cannot
fuzz_crypto already correct; switched to the header so it
stays that way
Adds tests/fuzz/fuzz_sim_flash.h: the three harnesses that reach flash
need board ops installed, and without them they fuzz a null op table.
That the compiler now checks this is not theoretical — writing this, it
rejected EOS_UPGRADE_MODE_TEST for EOS_UPGRADE_TEST immediately, which
is exactly the error class the externs were hiding.
Verified. libFuzzer is unavailable here (libclang_rt.fuzzer_osx.a not
found), so each harness was linked against eboot_core and driven by a
standalone seed generator under ASan+UBSan:
all five compile -Wall -Wextra 5/5, 0 failures
20,000 inputs each 100,000 total, no reports
ctest 21/21 passed
Third instance of this class across these repos, after eos#50
(fuzz_devicetree naming eos_dtb_parse) and eos#110 (fuzz_ota_header
naming eos_ota_parse_header). The common factor every time is a fuzz
target declaring its own subject.
Stacked on embeddedos-org#94, without which include/eos_image.h does not compile.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The restored Fuzz Harness Build job on embeddedos-org#101 ran these harnesses under real libFuzzer for the first time and fuzz_recovery_protocol trapped within seconds. That was my bug, not the code's. The oracle computed the one-past-the-end address: end = base + offset + len; if (end > 0xFFFFFFFF) trap; The last byte a write of len bytes touches is base + offset + len - 1. eos_recovery_write_in_range() checks exactly that: if ((uint32_t)len - 1u > UINT32_MAX - (base + offset)) reject; So an accepted write whose final byte lands exactly on 0xFFFFFFFF -- base=0xFFFFFF00, offset=0, len=256 -- is legal, the function accepts it, and the old oracle trapped on it. A harness that traps on valid input reports the opposite of the truth. Verified both directions: the boundary case, fed directly -> no trap a fail-open stub accepting all input -> trap fires (exit 133) 500,000 random inputs, ASan+UBSan -> no reports libFuzzer doing precisely what it is for -- driving inputs into the one corner a hand-written oracle got wrong -- is the strongest argument yet for embeddedos-org#101's job. The guard could never have caught this; only execution under a coverage-driven fuzzer did.
|
The Crashing unit, from job Reproduced locally against The write covers The oracle asserts on end - 1 > 0xFFFFFFFFULL /* or, equivalently, end > 0x100000000ULL */
if ((uint32_t)len - 1u > UINT32_MAX - (base + offset))
return EOS_ERR_INVALID;So the harness is currently stricter than the contract, and it fails the fuzz - end > 0xFFFFFFFFULL) {
+ end - 1 > 0xFFFFFFFFULL) {The other three clauses are right, and the choice to give this harness an oracle I have not pushed anything to this branch. #101 is red for this and nothing |
embeddedos-org#99 repairs the five harnesses in tests/fuzz/. Neither it nor anything else stops the same thing happening again, because two of the conditions that produced it are still in place. **Nothing compiles them.** tests/fuzz/ is behind EBLDR_BUILD_FUZZ, which defaults OFF, and no job set it. That is the whole reason three harnesses could name functions that exist nowhere -- eos_bootctl_parse, eos_recovery_parse_packet, eos_fw_update_init and friends -- and sit there for as long as they did. A harness that is not built cannot fail to build. The fuzz-build job restores that: clang, EBLDR_BUILD_FUZZ=ON (and EBLDR_BUILD_TESTS=ON, since tests/fuzz is added from tests/CMakeLists.txt, so the option alone would build nothing and the job would pass having compiled no harness at all), then a five-second smoke run of each target. It does not fuzz; a real campaign belongs in a scheduled workflow. **The `extern` is still legal.** Every one of the four defects came from a hand-written prototype standing in for an #include -- a promise the compiler is obliged to believe and has no way to check. It is what made the fourth defect invisible even to a compiler: fuzz_image_verify declared eos_image_parse_header as (const void *, size_t) when it is (uint32_t, eos_image_header_t *), so that harness linked, ran, and fuzzed nothing. tests/unit/test_fuzz_harnesses.py refuses the pattern, and also checks that every harness is built by a CMake target, links eboot_core, and defines LLVMFuzzerTestOneInput. The two halves cover different failures: the guard is static and stops the pattern being written, the job is a compiler and catches a signature that drifts under a harness that includes the right header. Neither subsumes the other. Mutation-checked: restoring one extern prototype fails test_no_harness_declares_its_own_prototypes; deleting one add_executable fails test_every_harness_is_built. Verified: pytest tests/unit 26 passed 1 skipped, ctest 21/21. The fuzz job itself could not be run here -- Apple clang ships no libclang_rt.fuzzer, so -fsanitize=fuzzer will not link on this host. Instead each of embeddedos-org#99's five harnesses was compiled and linked against eboot_core with a stand-in driver under -fsanitize=address,undefined and run over ~20k pseudo-random inputs plus every length from 0 to 200; all five clean. That covers the symbol resolution and the harness logic, which is what the job's build step checks; it does not prove the libFuzzer link itself, which only CI can.
|
Fixed in Verified three ways on the new head:
Worth recording why the job caught this when local testing did not: 500k random inputs never hit it, because random 32-bit values essentially never land #101 rebasing onto |
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#99 "fix(fuzz): point every harness at the API it claims to fuzz"
head: bbf4e89 author: Kartikey1306 ci: pass (24 checks, none of them a fuzz check)
Verdict: The diagnosis is right and I confirmed it mechanically rather than by reading. Against libeboot_core.a built from master, nm -g --defined-only reports eos_bootctl_parse, eos_recovery_parse_packet, eos_fw_update_init and eos_fw_update_process_chunk absent — those three harnesses could never have linked. eos_image_parse_header is present, and its real signature is (uint32_t addr, eos_image_header_t *out) (include/eos_image.h:180) against the harness's (const void *, size_t), so that one linked and ran with a pointer where an address goes. The replacements are pointed at real entry points and the shared fuzz_sim_flash.h backend is the right call — I measured it: 200,000 inputs through the new fuzz_bootctl produce 200,000 flash reads, none rejected, all of them reading fuzzer bytes. Compare the #100 rewrite of the same file, which reads zero (see that review).
The corrected recovery oracle is right. eos_recovery_write_in_range (core/recovery.c:260–273) bounds the last written byte via len - 1u > UINT32_MAX - (base + offset), and recovery_handle_write calls eos_hal_flash_write(base + offset, buf, len) — so len - 1 is the correct displacement and the harness's last > 0xFFFFFFFFULL at :50 now mirrors the implementation clause for clause.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | — (CI) | This head still builds none of the harnesses. EBLDR_BUILD_FUZZ defaults OFF (CMakeLists.txt:29) and no workflow sets it — I grepped all 16 files under .github/workflows at bbf4e89c for fuzz/FUZZ: zero hits — and checks.txt lists 24 checks with no fuzz check. So the repair is correct and unenforced: the condition that hid four broken harnesses is untouched by this PR. The job is in #101, one commit on top of this one. |
Land #99 and #101 together, or say in the body that #99 alone leaves the harnesses unbuilt. As it stands the PR title's promise ("point every harness at the API it claims to fuzz") is only compiler-checked once #101 lands. |
| 2 | Medium | — (duplicate) | #100 is an independent single-commit implementation of the same repair by the same author, opened the same day, and it also adds the CI job. #99+#101 and #100 cannot both land. #99/#101 is the better of the two — it factors the flash backend into tests/fuzz/fuzz_sim_flash.h instead of copying it into each harness, and #100's own fuzz_bootctl does not reach the code it fuzzes. |
Close #100 in favour of #99+#101, and say so on #100 so its green fuzz job is not read as a reason to prefer it. |
| 3 | Medium | tests/fuzz/fuzz_sim_flash.h:78 |
memcpy(&fuzz_flash[addr], data, n) is reached with data == NULL, from fuzz_fw_update.c:39 (fuzz_flash_load(0x4000, NULL, 0)). memcpy declares both pointers non-null regardless of n, so this is undefined behaviour in a test helper. Reproduced: gcc -fsanitize=undefined -fno-sanitize-recover=all, one 8-byte input → tests/fuzz/fuzz_sim_flash.h:78: runtime error: null pointer passed as argument 2, which is declared to never be null, exit 1. It is invisible in the new job because FUZZ_FLAGS is fuzzer,address with no undefined (tests/fuzz/CMakeLists.txt:17) — which means the obvious next hardening step, adding undefined, would trip on every single input of that harness. |
if (n) memcpy(&fuzz_flash[addr], data, n); in fuzz_flash_load, then add undefined to FUZZ_FLAGS. |
| 4 | Low | tests/fuzz/fuzz_recovery_protocol.c:6, header comment |
The file is named for the recovery protocol and fuzzes one pure predicate inside it. recovery_handle_write (core/recovery.c:275) is the function that takes the wire fields, and eos_recovery_enter is the command loop; both remain unfuzzed, and recovery_handle_write is static so no harness can reach it. .ai/security.md puts OTA payloads and IPC frames among the parsers that "get fuzz coverage, not just unit tests". The other two rewritten harnesses state honestly what they drive; this one's name implies more than it does. |
Say in the header comment that only the bounds predicate is covered and that the command loop is not. If protocol coverage is wanted, that needs a non-static seam, and it is a separate PR. |
| 5 | Low | tests/fuzz/fuzz_crypto.c:16 |
/* Forward-declare crypto APIs */ is still there, immediately above the #include "eos_crypto_boot.h" that replaced the forward declarations it describes. |
Delete the comment. |
The oracle in fuzz_recovery_protocol.c traps only on a write it wrongly accepts, never on one it wrongly rejects. That is deliberate and stated at :38–:40, and it is the right asymmetry for a bounds check — noting it so the one-sidedness is on the record, not as a finding.
Architecture conformance
Conforms. Tier 1 Foundation (§21); everything is under tests/, no runtime dependency added, no include pointing up a tier (§5.1). fuzz_sim_flash.h installs board ops through eos_hal_init rather than defining eos_hal_flash_read itself, which keeps the harnesses on the same HAL seam the unit tests use — correct under .ai/architect.md ("hal/ contains no board specifics"; a test fake belongs behind the interface, not as a duplicate symbol). §8 wants the eBoot TCB auditable, and a harness that names a function which does not exist is the opposite of auditable.
Proposed changes
if (n) memcpy(...)infuzz_flash_load; then addundefinedtoFUZZ_FLAGS.- Delete the stale comment in
fuzz_crypto.c. - Bound the claim in
fuzz_recovery_protocol.c's header comment. - Resolve #99+#101 vs #100 before any of them lands.
Not checked
- libFuzzer link and run: NOT RUN here. All six harnesses compile at this head; this host has no libFuzzer/ASan runtime (
libclang_rt.fuzzer.a,libclang_rt.asan.aabsent), so none links. The coverage numbers I quote come from a plaingccdriver of my own callingLLVMFuzzerTestOneInputin a loop over pseudo-random inputs, not from libFuzzer. That measures whether the harness reaches the code; it does not measure what coverage-guided search would find. - The 20.5M-execution run reported on #100, and #101's
Fuzz Harness Build: not reproduced. I read the check states viagh pr checks; I did not fetch or verify the job logs. fuzz_crypto,fuzz_image_verify,fuzz_fw_updatereach-through: NOT measured. I instrumentedfuzz_bootctlonly.fuzz_fw_updatereturns early whenevereos_fw_update_beginfails and I did not check how often that is — worth measuring before trusting its execution count as coverage.cteston this head: NOT RUN. Thecore/ed25519_verify.c,include/eos_image.handtests/unit/test_ed25519.cparts of this diff are #94's commit (f704d87f), reviewed under #94/#98; I verified separately that master does not compile without them.- The local clone does not have
bbf4e89c; everything above is from agh api tarballsnapshot at that sha.
Automated architecture review of bbf4e89c22d9 — 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.
3 participants
Four of the five fuzz harnesses did not test what they name, and nothing in the build could say so, because not one of them includes a project header. Each declares its own target:
eos_recovery_parse_packet,eos_fw_update_init,eos_fw_update_process_chunkandeos_bootctl_parsehave never existed — each name appears nowhere outside the harness declaring it. Those three targets have never linked, which is what failsFuzz Harness Buildon #85.The two that do link are worse
eos_fw_update_finalizeexists, but takes(ctx, mode)— not(void).eos_image_parse_headertakes(uint32_t addr, eos_image_header_t *out)— not(const void *, size_t). Same name, different function; C never checks across translation units.fuzz_image_verifyhas therefore been passing the fuzzer's pointer as a flash address and its length as an output struct, on every input, for as long as it has run. Measured over 20,000 inputs:EOS_ERR_FLASH(-6) — first flash read failedEOS_ERR_NO_IMAGE(-5) — magic check reachedEOS_ERR_INVALID(-2) — version/hdr_size reachedEvery single input bailed two lines in. The target ran, reported nothing, and covered none of the parser.
What each harness drives now
fuzz_image_verifyparse_header→verify_integrity, in boot-path orderfuzz_bootctlbootctl_load, then trap ifload()accepted a blockvalidate()rejectsfuzz_fw_updatebegin→writein fuzz-chosen chunk widths →finalize/abortfuzz_recovery_protocolwrite_in_range, trapping on any accepted write that leaves the slot or wraps — rechecked in 64-bit types that cannotfuzz_cryptoNew
tests/fuzz/fuzz_sim_flash.h: the three harnesses that reach flash need board ops installed, and without them they fuzz a null op table.That the compiler now checks this is not theoretical — writing it, it rejected
EOS_UPGRADE_MODE_TESTforEOS_UPGRADE_TESTimmediately. That is precisely the class theexterns were hiding.Validation
libFuzzer is unavailable here (
libclang_rt.fuzzer_osx.aabsent), so each harness was linked againsteboot_core+eboot_haland driven by a standalone seed generator under-fsanitize=address,undefined:Credit
@task-13's session independently found and fixed the same three harnesses, and reached the identical before/after conclusion on
fuzz_image_verify— its parked branch isfix/fuzz-harnesses-that-link. It deliberately did not open a PR to avoid duplicating this one. This PR is the superset: it also coversfuzz_bootctl(whoseeos_bootctl_parseis equally non-existent) andfuzz_crypto, so noexternis left intests/fuzz/. Its deeperfuzz_fw_updatebody is worth layering on top of this if you want it.The pattern
Third instance across these repos, after eos#50 (
fuzz_devicetreenamingeos_dtb_parse) and eos#110 (fuzz_ota_headernamingeos_ota_parse_header). The common factor every time is a fuzz target declaring its own subject. Removing the lastexternfromtests/fuzz/is what stops the fourth.🤖 Generated with Claude Code