ci(fuzz): compile the harnesses, and refuse the extern that hid the bugs - #101
ci(fuzz): compile the harnesses, and refuse the extern that hid the bugs#101Kartikey1306 wants to merge 4 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❌ Patch coverage is
📢 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.
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.
d8e0bbf to
e2bb1f9
Compare
srpatcha
left a comment
There was a problem hiding this comment.
Review — eBoot#101 "ci(fuzz): compile the harnesses, and refuse the extern that hid the bugs"
head: e2bb1f9 author: Kartikey1306 ci: pass (25 checks, Fuzz Harness Build green)
Verdict: This is the right shape for the problem — a job that compiles what was never compiled, plus a static guard so the pattern that hid it cannot come back. Both work. I ran the guard against master's tests/fuzz/ and it fails naming all five hand-written extern declarations; against this head all five of its tests pass. Two things to fix: the guard has one obvious bypass, and half of fuzz_fw_update's inputs are discarded by the sim-flash geometry before any code under test runs.
This PR is #99 plus one commit (e2bb1f9f); the harness rewrites are reviewed under #99.
Findings
| # | Severity | File:line | Finding | Recommended fix |
|---|---|---|---|---|
| 1 | Medium | tests/unit/test_fuzz_harnesses.py:38–40 |
EXTERN_FUNCTION_RE anchors on ^\s*extern\s+, so it only catches declarations that spell extern. A file-scope function declaration has external linkage without it, and the compiler cannot check either one across translation units — they are the same defect. Verified: I dropped a harness containing int eos_image_parse_header(const void *flash_base, size_t flash_len); (the exact wrong signature from the original bug, minus the keyword) into tests/fuzz/ and test_no_harness_declares_its_own_prototypes passed. The module docstring at :20 claims "these tests refuse the pattern, so the sixth cannot be written", which is stronger than what the regex does. |
Drop extern\s+ from the pattern and exclude static and LLVMFuzzerTestOneInput instead — i.e. reject any file-scope function declaration ending in ; that is not static and not the entry point. Then re-run against master's tree to confirm it still catches all five, and against the bare-prototype probe. |
| 2 | Medium | tests/fuzz/fuzz_sim_flash.h:23,55 |
Slot B does not exist. FUZZ_FLASH_SIZE is 0x10000 while .slot_b_addr = 0xC000, .slot_b_size = 0x8000 spans 0xC000–0x14000, i.e. 32 KiB past the end of fuzz_flash[], so every erase or write covering it is rejected by fuzz_flash_erase/fuzz_flash_write at :35,:41. fuzz_fw_update.c:42 picks the slot from the input (selector & 1), so half the corpus dies at eos_fw_update_begin. Measured over 20,000 inputs: slot A tried 10,071, began 10,071; slot B tried 9,929, began 0. Half the fuzzing budget of that harness buys nothing, and the A/B asymmetry the harness exists to explore is never explored. |
#define FUZZ_FLASH_SIZE (128 * 1024), or shrink the slots to 0x2000 each. Either way the Fuzz Harness Build job should not report an execution count as if it were coverage — a per-harness "reached the code N times" counter would have caught this and would have caught #100's two dead harnesses too. |
| 3 | Low | tests/unit/test_fuzz_harnesses.py:89–95 |
test_every_harness_defines_the_entry_point asserts the string LLVMFuzzerTestOneInput appears anywhere in the file, comments included. Every harness in the tree mentions it in prose, so a file that only discusses the entry point without defining it would pass. |
Match a definition: re.search(r'\bLLVMFuzzerTestOneInput\s*\([^)]*\)\s*\{', text). |
| 4 | Low | .github/workflows/ci.yml (fuzz-build, smoke-run step) |
-max_total_time=5 with no -seed makes this a nondeterministic required check: whether it goes red depends on what coverage-guided search finds in five seconds on that runner. That is exactly how this job earned its place — it found the real off-by-one in #99's recovery oracle in six seconds, which 500k random local inputs never hit — but as a per-PR gate it means an unrelated PR can be blocked by a pre-existing defect discovered that day, with no reproducible run behind it. |
Keep the build on every PR; pin the smoke run (-seed=1 -runs=100000) and put the open-ended campaign in nightly.yml. Also add fuzz-build to #90's CI Gate needs: list — #90's test_gate_covers_every_job_that_runs_on_a_pull_request will fail loudly otherwise, which is the guard working, but whichever of #90/#101 lands second has to do it. |
Worth keeping exactly as written, because these are the two ways this job could have become a check that cannot fail and both are closed: EBLDR_BUILD_TESTS=ON alongside EBLDR_BUILD_FUZZ=ON (without it tests/fuzz/ is never added from tests/CMakeLists.txt and the job passes having compiled nothing), and the explicit ${#harnesses[@]} -eq 0 failure with nullglob. test_every_harness_is_built and test_every_harness_links_the_library_under_test cover the CMake side of the same question statically, and test_there_are_harnesses_to_check stops the glob-matched-nothing vacuity — that last one is the check most suites in this position forget.
Architecture conformance
Conforms. Tier 1 Foundation (§21); the change is .github/, tests/fuzz/ and tests/unit/, with no runtime dependency and no include pointing up a tier (§5.1). This is the concrete mechanism behind .ai/security.md's "every externally reachable parser … gets fuzz coverage, not just unit tests" and its ctest --no-tests=error rule generalised: a suite that silently collected nothing is a failed check reported as green, and this PR closes that for tests/fuzz/ in two independent ways (a job that compiles, and a static guard that runs without clang or cmake). §8's "trusted computing base minimal and auditable" is the reason it belongs in eBoot rather than in a shared CI template.
Proposed changes
- Widen
EXTERN_FUNCTION_REto any non-staticfile-scope function declaration, and re-run it against master'stests/fuzz/and against a bare-prototype probe. FUZZ_FLASH_SIZE (128 * 1024)so slot B is real, then re-measurefuzz_fw_update's per-slotbegincounts.- Match a definition, not a mention, in
test_every_harness_defines_the_entry_point. - Pin the smoke-run seed; add
fuzz-buildto #90's gate. - Land with #99, and close #100 (its
fuzz_bootctlandfuzz_fw_updateread no input at all — see that review).
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. TheFuzz Harness Buildcheck is green pergh pr checks 101, but I did not fetch the job log, so the claim that these harnesses have now been linked and run under libFuzzer rests on the check status and the author's comment, not on output I read. - The reported red-then-green history of this job: not reproduced. The off-by-one analysis on #99 is consistent with the code I read (
core/recovery.c:270bounds the last written byte; the oracle now matches atfuzz_recovery_protocol.c:50), but I did not run the crashing input under libFuzzer. - My reach-through numbers are not libFuzzer numbers. They come from a plain
gccdriver callingLLVMFuzzerTestOneInputin a loop over pseudo-random inputs. That is sound for finding 2 — a rejected erase is rejected regardless of how the input was chosen — and it is not a statement about what coverage-guided search would find. test_fuzz_harnesses.pywas run without pytest. No pytest on this host; I imported the module and called eachtest_*function directly. Same assertions, but I did not verify it under the runner CI uses, and codecov reports 4 lines/partials in that file uncovered.ctest: NOT RUN on this head. Thecore/ed25519_verify.c,include/eos_image.handtests/unit/test_ed25519.cparts of the diff are #94's commit (f704d87f), reviewed under #94/#98.- The local clone does not have
e2bb1f9f; everything above is from agh api tarballsnapshot at that sha.
Automated architecture review of e2bb1f9f5f6e — 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.
…resynchronising on garbage Answers the third review on embeddedos-org#84. Finding 3 (Low, and the one that matters on hardware) -- the header was read through a cast fdt_header_t* and direct member loads, while every struct-block read goes through the fdt_read_u32() memcpy helper precisely because the blob may be unaligned. Fuzz input, a buffer inside a larger message, a copy at an odd offset: nothing promises 4-byte alignment, and on a strict-alignment cross target a direct member load is the same fault class this parser exists to avoid. The unit suite could never catch it -- blob_t.bytes happens to be aligned. All header fields now go through fdt_hdr_u32() (memcpy at offsetof), one rule for the whole blob, in validate(), load() and get_prop(). FUZZ_FLAGS gains `undefined` so the fuzz job checks alignment too. Finding 4 (Low) -- `default: break;` resynchronised on unrecognised tags: the walk skipped 4 bytes and treated whatever followed as the next token, so a struct block of arbitrary bytes parsed to a clean "not found". Bounded, but a TCB parser that walks garbage to completion is accepting input it does not understand. FDT_NOP -- the one legal unknown, padding the spec allows between tokens -- is now named in the header and passes; anything else returns -6. Findings 1, 2 and the -8 line: the fuzz-build job's single home is embeddedos-org#101 (nothing added here; the harness is inert until that job lands and then compiled by it -- said on the thread, not just here), and the header no longer documents return code -8, which nothing at this head returns. embeddedos-org#85 adds the -8 return and re-documents it together with the FDT_MAX_PATH_DEPTH move. Verified: ctest 22/22 PASS ctest -DEBLDR_SANITIZE=ON (ASan+UBSan) 22/22 PASS pytest tests/ 38 passed test_fdt_loader 16 tests PASS (was 13) discrimination, each fix in isolation: default: break restored -> test_a_garbage_tag_is_refused_not_skipped FAILS (expects -6, gets "not found") direct member load restored -> under UBSan the new unaligned-blob test reports "load of misaligned address 0x...671 for type 'const uint32_t'" at the exact line -- and cannot fire on the fixed code, which runs the same test clean NOP counter-check: interleaved FDT_NOP tokens still resolve, so the stricter default does not reject real trees. Refs embeddedos-org#84
Stacked on #99. #99 repairs the five harnesses in
tests/fuzz/; this stops thesame class coming back, in the two places it can.
Why it came back-able
Nothing compiles them.
tests/fuzz/is behindEBLDR_BUILD_FUZZ, whichdefaults
OFF, and no job set it. That is the whole reason three harnessescould name functions that exist nowhere in the repository —
eos_bootctl_parse,eos_recovery_parse_packet,eos_fw_update_initandfriends — and sit there unnoticed. A harness that is not built cannot fail to
build.
The
externis still legal. All four defects came from a hand-writtenprototype standing in for an
#include. Anexternis a promise the compileris obliged to believe and has no way to check, which is why the fourth was
invisible even to a compiler:
fuzz_image_verifydeclaredagainst a real
int eos_image_parse_header(uint32_t addr, eos_image_header_t *out).It linked. It ran. It fuzzed nothing.
What this adds
fuzz-buildjob (Fuzz Harness Build)EBLDR_BUILD_FUZZ=ON, then smoke-runs each for five seconds.EBLDR_BUILD_TESTS=ONtoo —tests/fuzzis added fromtests/CMakeLists.txt, so the fuzz option alone builds nothing and the job would pass having compiled no harness at all. It does not fuzz; a campaign belongs in a scheduled workflow.tests/unit/test_fuzz_harnesses.pyeboot_core, definesLLVMFuzzerTestOneInput, and declares no prototypes of its own.The two halves catch different failures and neither subsumes the other: the
guard is static and stops the pattern being written at all; the job is a
compiler, and catches a signature that drifts under a harness that already
includes the right header.
Mutation-checked:
externprototypetest_no_harness_declares_its_own_prototypesadd_executabletest_every_harness_is_builtThe job paid for itself on its first run
Not a hypothetical. The first time these harnesses ran under real libFuzzer —
this job, on this PR —
fuzz_recovery_protocoltrapped in six seconds ona write covering
[0xFFFFDBDB, 0xFFFFFFFF]: 9253 bytes ending on the lastaddressable byte, every one of them inside 32 bits, so accepting it is correct.
The harness's oracle was asserting on
base + offset + len > 0xFFFFFFFF— theaddress one past the end — where
eos_recovery_write_in_range()correctlybounds the last byte written, exactly as its own final line says:
if ((uint32_t)len - 1u > UINT32_MAX - (base + offset)). The function wasright and the harness reported the opposite. Fixed in #99 as
bbf4e89.That is the case for keeping both halves. The static guard could never have
found an off-by-one inside an oracle — it is a real
#includecalling a realfunction with the right types. Only executing it under a coverage-driven fuzzer
could, and until this job existed nothing did.
Verification
pytest tests/unitctestOn this head the
Fuzz Harness Buildjob is green in CI, along with theother 24 checks.
Locally the job could not be run: Apple clang ships no
libclang_rt.fuzzer, so-fsanitize=fuzzerwill not link on this host. Each of #99's five harnesses wasinstead compiled and linked against
eboot_corewith a stand-in driver under-fsanitize=address,undefinedand run over ~20k pseudo-random inputs plus everylength from 0 to 200 — all five clean. That covers symbol resolution and harness
logic but not the libFuzzer link or its coverage-guided mutation, which is
precisely the gap that found the oracle bug above. CI is the only place that
runs.
Merge order
After #99 (head
bbf4e89), which is after #94 —masterdoes not compilewithout it. Nothing here touches
tests/fuzz/*.c, so there is no overlap with#99's diff.