Skip to content

fix(fuzz): point the harnesses at functions that exist, and build them in CI - #100

Closed
Kartikey1306 wants to merge 2 commits into
embeddedos-org:masterfrom
Kartikey1306:fix/fuzz-harnesses-that-never-built
Closed

fix(fuzz): point the harnesses at functions that exist, and build them in CI#100
Kartikey1306 wants to merge 2 commits into
embeddedos-org:masterfrom
Kartikey1306:fix/fuzz-harnesses-that-never-built

Conversation

@Kartikey1306

Copy link
Copy Markdown
Contributor

Three of the five fuzz harnesses name functions this tree has never had, and a fourth declares the wrong signature for one it does. They compile — a declaration costs nothing — and have never linked, because EBLDR_BUILD_FUZZ defaults OFF and no CI job set it.

This is the third instance of the pattern. eos#50 found fuzz_devicetree declaring an eos_dtb_parse() that did not exist, and found it the same way: by making something build it.

Stacked on #94, which repairs master.

How it surfaced

I added the fuzz-build job below while answering a review finding on #84. It went red immediately, on harnesses #84 had not touched:

undefined reference to `eos_fw_update_init'
undefined reference to `eos_fw_update_process_chunk'
undefined reference to `eos_recovery_parse_packet'

I moved the job out of #84 rather than leave that PR red on a pre-existing defect, and this is where both live.

What was wrong

harness declared reality
fuzz_fw_update eos_fw_update_init(), _process_chunk(), _finalize(void) none exist; the API is begin(ctx, slot) / write(ctx, data, len) / finalize(ctx, mode)
fuzz_recovery_protocol eos_recovery_parse_packet() does not exist
fuzz_bootctl eos_bootctl_parse() does not exist; eos_bootctl_load() is the entry point
fuzz_image_verify (const void *, size_t) real: (uint32_t addr, eos_image_header_t *out)
fuzz_crypto correct, unchanged

fuzz_image_verify is the one worth pausing on. C has no overloading, so that symbol matched. It would have linked and run, writing a parsed header through whatever size happened to be. A wrong declaration is worse than a missing one, because it is the case that does not fail loudly.

What they do now

  • fuzz_fw_update — real begin → write → finalize over a simulated flash, with chunk widths taken from the input so a header straddling two writes is reachable. That boundary is where a streaming parser goes wrong.
  • fuzz_recovery_protocoleos_recovery_write_in_range() with all four arguments fuzzed, including the slot geometry, so every wrap case the function documents is reachable. It traps if the function accepts a write that does not fit — a harness that only calls a predicate and discards the answer tests nothing.
  • fuzz_bootctleos_bootctl_load() over fuzzer bytes staged as flash, then the state transitions a boot makes on whatever it accepted.
  • fuzz_image_verify — includes the header so the compiler checks the call, and parses from an address as the real caller does.

eos_recovery_enter() is deliberately not driven: a command loop over a real UART that does not return on success is not a shape libFuzzer can use. The range check is the part of recovery that takes untrusted numbers and answers a safety question.

The job

Builds every harness and runs each briefly over its own generated inputs. Not a campaign — enough to catch a harness that no longer compiles, no longer links, or crashes at once, which is the failure this repo has actually had three times. A real campaign belongs in a scheduled workflow.

It needs EBLDR_BUILD_TESTS alongside EBLDR_BUILD_FUZZ: tests/fuzz/ is added from tests/CMakeLists.txt, so the fuzz flag alone configures cleanly and builds nothing — the job would have passed having compiled no harness. Found that before writing the job rather than after.

Verification

check result
ctest 22/22 PASS
pytest tests/ 38 passed
all five harnesses link against eboot_core (stub main) 5/5
each under -fsanitize=address,undefined, 300 random inputs no report, all five

NOT RUN: the libFuzzer link itself. This host has no libclang_rt.fuzzer_osx.a in its Xcode toolchain, so I linked each harness against a stub driver instead to prove the symbols resolve, and drove them under ASan+UBSan with random input to prove they run. The libFuzzer build is what this job exists to demonstrate, and it is CI's to show.

Coordination: #90 adds a CI Gate with needs: [test, build-arm, static-analysis]. Whichever of this and #90 lands second must add fuzz-build to that list — and #90's own test_gate_covers_every_job_that_runs_on_a_pull_request fails loudly if it is not.

Refs #84, #50

…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.
…m in CI

Three of the five fuzz harnesses named functions this tree has never had, and
a fourth declared the wrong signature for one it does. They compiled -- a
declaration costs nothing -- and never linked, because EBLDR_BUILD_FUZZ
defaults OFF and no CI job set it. This is the third instance of the pattern:
eos#50 found fuzz_devicetree declaring an eos_dtb_parse() that did not exist,
and it was found the same way, by making something build it.

Found by the fuzz-build job below, which was written for eBoot#84 and turned
red immediately on harnesses that PR had not touched:

    undefined reference to `eos_fw_update_init'
    undefined reference to `eos_fw_update_process_chunk'
    undefined reference to `eos_recovery_parse_packet'

| harness | was | now |
|---|---|---|
| fuzz_fw_update | `eos_fw_update_init/_process_chunk/_finalize(void)` -- none exist | real begin -> write -> finalize over a simulated flash, chunk widths from the input so a header can straddle two writes |
| fuzz_recovery_protocol | `eos_recovery_parse_packet()` -- does not exist | `eos_recovery_write_in_range()` with all four arguments fuzzed, and a trap if it accepts a write that does not fit |
| fuzz_bootctl | `eos_bootctl_parse()` -- does not exist | `eos_bootctl_load()` over fuzzer bytes staged as flash, then the transitions a boot makes on what it accepted |
| fuzz_image_verify | `(const void *, size_t)` for a function that takes `(uint32_t, eos_image_header_t *)` | includes the header so the compiler checks the call; parses from an address, as the real caller does |
| fuzz_crypto | correct | unchanged |

The image_verify one is worth naming separately: C has no overloading, so that
symbol matched and it would have linked and run, writing a parsed header
through whatever `size` happened to be. A wrong declaration is worse than a
missing one, because it is the case that does not fail loudly.

eos_recovery_enter() is deliberately not driven -- a command loop over a real
UART that does not return on success is not a shape libFuzzer can use. The
range check is the part of recovery that takes untrusted numbers and answers a
safety question.

The CI job builds every harness and runs each briefly over its own generated
inputs. Not a campaign -- enough to catch a harness that no longer compiles,
no longer links, or crashes at once, which is the failure this repo has
actually had, three times. It needs EBLDR_BUILD_TESTS alongside
EBLDR_BUILD_FUZZ: tests/fuzz/ is added from tests/CMakeLists.txt, so the fuzz
flag alone configures cleanly and builds nothing, and the job would pass
having compiled no harness. Found that before writing the job rather than
after.

Verified:
  ctest                                    22/22 PASS
  pytest tests/                            38 passed
  all five harnesses link against eboot_core, checked with a stub main
    (this host has no libFuzzer runtime -- libclang_rt.fuzzer_osx.a is absent
    from the Xcode toolchain -- so the libFuzzer link itself is CI's to show)
  each harness under -fsanitize=address,undefined over 300 random inputs:
    fuzz_bootctl / fuzz_crypto / fuzz_fw_update / fuzz_image_verify /
    fuzz_recovery_protocol -- 300 inputs, no report, all five

Refs embeddedos-org#84, embeddedos-org#50
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

CI produced the evidence I could not

Fuzz Harness Build is green, and it is the first time any of these harnesses has been linked, let alone run:

built 5 harnesses
=== fuzz_bootctl ===            Done 3,114,242 runs in 6 seconds
=== fuzz_crypto ===             Done    55,610 runs in 6 seconds
=== fuzz_fw_update ===          Done 3,673,098 runs in 6 seconds
=== fuzz_image_verify ===       Done 6,744,891 runs in 6 seconds
=== fuzz_recovery_protocol ===  Done 6,977,285 runs in 6 seconds

20.5 million executions, no crashes. My local runs were 300 random inputs each under ASan+UBSan against a stub driver, because this host has no libclang_rt.fuzzer_osx.a. The libFuzzer link and the coverage-guided run are exactly what the job was added to demonstrate, and they are what it demonstrated.

Two things worth reading out of those numbers rather than just the green tick:

  • fuzz_crypto is 55,610 against 3–7 million elsewhere — about sixty times slower per execution. That is Ed25519 doing real scalar multiplication, so it is expected, but it means a fixed time budget buys it far less coverage than the others. If a scheduled campaign is added later, that harness wants its own longer slot rather than an equal share.
  • fuzz_recovery_protocol at 6.9M is checking the write_in_range contract on every one of them, including the trap for a write it accepts that does not fit. It never fired.

The five counts also confirm the harnesses are reaching real code rather than returning early — a harness that bailed on its length check would show a far flatter number.

fuzz_fdt is not in this list because it belongs to #84; this PR is based on master. Once both land the job builds six.

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — eBoot#100 "fix(fuzz): point the harnesses at functions that exist, and build them in CI"

head: 0ca7698 author: Kartikey1306 ci: pass (26 checks, Fuzz Harness Build green)

Verdict: The CI job is the right idea and it works. Two of the four rewritten harnesses still fuzz nothing, and the new job reports them green — which is the failure mode .ai/security.md singles out, now with a passing check in front of it. This also duplicates #99+#101 by the same author, and those versions do reach the code.

I measured reach-through rather than trusting execution counts, by driving LLVMFuzzerTestOneInput from a plain gcc harness of my own over 100k–200k pseudo-random inputs and counting where control actually got to.

Findings

# Severity File:line Finding Recommended fix
1 High tests/fuzz/fuzz_bootctl.c:6279 The harness never reads a single byte of fuzzer input. memset(&ops, 0, sizeof ops) at :63 leaves bootctl_addr and bootctl_backup_addr at 0, and sim_flash_read at :40 rejects any addr < SIM_FLASH_BASE (0x08000000). eos_bootctl_load (core/bootctl.c:74,79) reads at exactly those two addresses, so both reads fail before touching sim_flash. Measured over 200,000 inputs: 200,000 flash reads, 200,000 rejected, 0 that saw fuzzer bytes, and eos_bootctl_load returned EOS_OK 0 times — so :80:84 (increment_attempts, set_pending, clear_pending, reset_attempts, save) and the whole BCB parse are unreachable. The 3,114,242 executions the job reports are 3.1M round trips through two rejected reads. The PR fixes the name and re-creates the defect through the addresses. Set the addresses to something sim_flash_read accepts: ops.bootctl_addr = SIM_FLASH_BASE; ops.bootctl_backup_addr = SIM_FLASH_BASE + 0x1000; — or take #99's tests/fuzz/fuzz_sim_flash.h, where flash_base is 0 and the same measurement gives 200,000 reads with 0 rejected.
2 High tests/fuzz/fuzz_fw_update.c:6778 Same cause, same result. ops is zeroed, so slot_b_addr and slot_b_size are 0, and eos_fw_update_begin(&ctx, EOS_SLOT_B) at :77 can never succeed. Measured over 100,000 inputs: begin succeeded 0 times, eos_fw_update_write was called 0 times. The comment at the write loop — "Chunk widths come from the input, so a header straddling two writes is reachable" — is not true at this head; nothing is reachable. #99's version of this file gives 50,107 successful begin calls and 198,021 successful write calls on the same input stream. Populate slot_b_addr/slot_b_size (and slot_a_*), or use fuzz_sim_flash.h. Then add an assertion or a counter so a harness that stops reaching the code fails instead of passing quietly.
3 Medium — (duplicate) This is a competing implementation of #99 (harness repair) + #101 (CI job + static guard), all three by the same author on the same base commit f704d87f. #99+#101 is the better line of work: it factors the flash backend into one header instead of three inline copies, its fuzz_bootctl and fuzz_fw_update actually reach the code under test, and #101 adds tests/unit/test_fuzz_harnesses.py, which refuses the hand-written extern that hid the original bugs — I ran that guard against master's tests/fuzz/ and it correctly names all five offending declarations. Close this in favour of #99+#101. If anything here is wanted, it is the runs-on/step layout, which is nearly identical anyway.
4 Low .github/workflows/ci.yml:126127 The new job is inserted between the # ── Static Analysis ── banner and the static-analysis: job it labels, so the banner now heads fuzz-build and static-analysis has none. #101 gives the job its own banner and leaves the existing one attached. Move the banner back below the new job, or add one for fuzz-build.
5 Low tests/fuzz/fuzz_image_verify.c:62 This is the one harness here that does reach the code — it passes SIM_FLASH_BASE explicitly rather than relying on ops. It stops at eos_image_parse_header and never calls eos_image_verify_integrity, so header parsing is covered and verification is not. #101's version chains them in boot order (parse → on EOS_OKverify_integrity), which is the sequence .ai/security.md cares about. Chain the two calls.
6 Low .github/workflows/ci.yml (fuzz-build) -max_total_time=5 with no -seed makes this a nondeterministic required check: whether it goes red depends on what coverage-guided search happens to find in five seconds on that runner. That is how #101's job found the real off-by-one in #99's recovery oracle, so the value is not in question — but as a per-PR gate it means an unrelated PR can be blocked by a pre-existing defect found that day, with no way to reproduce the run. Both #100 and #101 have this. Keep the build on every PR; pin the smoke run (-seed=1 -runs=100000) and move the open-ended campaign to nightly.yml. Whichever fuzz job lands must also be added to #90's CI Gate needs: list.

The job's own guards are good and worth keeping in whichever version lands: EBLDR_BUILD_TESTS=ON alongside EBLDR_BUILD_FUZZ=ON (without it tests/fuzz/ is never added and the job would pass having compiled nothing), and the explicit ${#harnesses[@]} -eq 0 failure. Those are exactly the two ways this job could have become a check that cannot fail, and both are closed.

Architecture conformance

Conforms. Tier 1 Foundation (§21). Everything is under tests/ and .github/, no runtime dependency and no include pointing up a tier (§5.1). One note under .ai/architect.md: three copies of the same RAM-backed board-ops fake is the duplication that file tells reviewers to collapse to a single call site, and #99 already has the collapsed form.

Proposed changes

  1. Close in favour of #99+#101, or fix findings 1 and 2 first — a green Fuzz Harness Build over two harnesses that read no input is worse than no job, because it converts an invisible gap into a documented pass.
  2. If kept: populate the board-ops addresses in both harnesses, then re-check reach-through with a counter rather than an execution count.
  3. Restore the Static Analysis banner.
  4. Pin the smoke-run seed and add the job to #90's CI Gate.

Not checked

  • libFuzzer link and run: NOT RUN. This host has no libFuzzer/ASan runtime (libclang_rt.fuzzer.a absent), so no harness links here. My reach-through numbers come from a plain gcc driver calling LLVMFuzzerTestOneInput over pseudo-random inputs, not from libFuzzer. Coverage-guided search reaches inputs random bytes do not — but it cannot reach code behind a flash read that is rejected on address alone, which is what findings 1 and 2 turn on.
  • The job's own log: NOT read. I took the check status from gh pr checks 100; the 20.5M-execution figures in the author's comment come from the job output and I did not fetch it.
  • fuzz_crypto and fuzz_recovery_protocol reach-through: NOT measured on this head. Both are pure-function harnesses with no board ops, so the defect in findings 1 and 2 does not apply to them.
  • ctest: NOT RUN on this head. The core/ed25519_verify.c, include/eos_image.h and tests/unit/test_ed25519.c parts of this diff are #94's commit (f704d87f) and are reviewed under #94/#98.
  • Cross builds: NOT RUN locally; checks.txt reports them green.
  • The local clone does not have 0ca7698f; everything above is from a gh api tarball snapshot at that sha.

Automated architecture review of 0ca7698f37ef — 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.

@Kartikey1306

Copy link
Copy Markdown
Contributor Author

Closing in favour of #99 + #101, which together are a strict superset of this PR — same consolidation rule the review on #86 spelled out for the Ed25519 triplicate: pick one, close the rest explicitly so a fourth does not appear. All three came from my account working the same finding in parallel; this is me deduplicating my own work.

What decided it, having compared the three head-to-head:

  • ci(fuzz): compile the harnesses, and refuse the extern that hid the bugs #101 carries a guard this PR lacks: test_fuzz_harnesses.py refuses a hand-written extern prototype inside tests/fuzz/ — the root cause of all four defects, including the one the compiler could never see (fuzz_image_verify's wrong-signature declaration, which linked). My version fixed the instances; theirs also fences the class, which is the difference the reviews here keep pointing at.
  • fix(fuzz): point every harness at the API it claims to fuzz #99's oracle is stronger than mine. Its recovery harness asserts the last-byte bound (base + offset + len − 1 ≤ 0xFFFFFFFF); mine checked base + offset wrap only, so a function accepting a write whose tail wrapped past the top of address space would have gotten past my trap. Theirs also went through a real crash-fix cycle proving the trap fires — CI run 33786556555 trapped on base=0xffffdbdb, len=0x2425, a write ending exactly at 0xFFFFFFFF; I decoded it against the implementation, confirmed eos_recovery_write_in_range() was right to accept it, and the off-by-one was in the harness's one-past-the-end oracle — fixed on fix(fuzz): point every harness at the API it claims to fuzz #99's head, green since.
  • Shared fuzz_sim_flash.h instead of three copies of the simulated flash.

Two things from this PR worth carrying forward rather than losing:

  1. The 20.5M-execution evidence (comment above): all five harnesses linked and fuzzed for the first time — 3.1M/55k/3.6M/6.7M/6.9M executions in 6s each, no crashes. Note fuzz_crypto at 55k vs millions elsewhere: real Ed25519 scalarmult, so a fixed time budget buys it ~60× less coverage. If a scheduled campaign is ever added, that harness wants its own longer slot.
  2. The gate coordination: ci: add one job branch protection can require #90 adds CI Gate with needs: [test, build-arm, static-analysis]. Whichever of ci(fuzz): compile the harnesses, and refuse the extern that hid the bugs #101 and ci: add one job branch protection can require #90 lands second must add fuzz-build to that list — and ci: add one job branch protection can require #90's test_gate_covers_every_job_that_runs_on_a_pull_request fails loudly if it is not.

Nothing else here is lost — the harness rewrites converge on the same real APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants