From f704d87fd445e4d61db00bc302e78660fc99f02f Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 15:06:59 +0530 Subject: [PATCH 1/4] =?UTF-8?q?fix:=20repair=20master=20=E2=80=94=20the=20?= =?UTF-8?q?ABI=20asserts=20and=20the=20Ed25519=20verifier=20both=20merged?= =?UTF-8?q?=20broken?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 — #93 replaced reserved[30] with tlv_len (2) + tlv_hash[28], preserving every offset. #87 merged afterwards carrying asserts written against the older struct: error: no member named 'reserved' in 'eos_image_header_t' (x2) #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 — #86 and #57 both landed a subgroup guard, so the file carried two byte-identical point_is_identity() definitions: error: redefinition of 'point_is_identity' Only #57's public_key_is_valid_subgroup() is wired to the call site, so #86's key_has_prime_order() was dead. Kept the live function, folded #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 #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. --- core/ed25519_verify.c | 56 ++++------------- include/eos_image.h | 16 +++-- tests/unit/test_ed25519.c | 126 ++++++++++++++++++++++++++++---------- 3 files changed, 116 insertions(+), 82 deletions(-) diff --git a/core/ed25519_verify.c b/core/ed25519_verify.c index d36cc7c..34aca77 100644 --- a/core/ed25519_verify.c +++ b/core/ed25519_verify.c @@ -289,6 +289,16 @@ static int point_is_identity(gf p[4]) return diff == 0; } +static void scalarbase(gf r[4], const uint8_t *s) +{ + gf q[4]; + fe_copy16(q[0], BX); + fe_copy16(q[1], BY); + fe_copy16(q[2], gf1); + fe_mul(q[3], BX, BY); + scalarmult(r, q, s); +} + /* Reject a public key outside the prime-order subgroup. * * Decoding a point is not enough. Ed25519 has eight points of low order, and @@ -304,51 +314,9 @@ static int point_is_identity(gf p[4]) * so there is no separate constant to transcribe wrongly: a mistyped L would * reject valid keys, and only in the field. * - * A arrives negated from unpackneg(). [L](-A) = -[L]A and the identity is its - * own negation, so neither condition is affected by the sign. - * - * Formulation taken from eBoot#57 by @muhammadburhandevv-hub, which reached - * this before I did and states both conditions in one expression. + * The key arrives negated from unpackneg(). [L](-A) = -[L]A and the identity + * is its own negation, so neither condition is affected by the sign. */ -static int key_has_prime_order(gf A[4]) -{ - uint8_t order_l[32]; - gf q[4], multiple[4]; - int i; - - for (i = 0; i < 32; i++) - order_l[i] = (uint8_t)ORDER_L[i]; - for (i = 0; i < 4; i++) - fe_copy16(q[i], A[i]); - - scalarmult(multiple, q, order_l); - return point_is_identity(multiple) && !point_is_identity(A); -} - -static void scalarbase(gf r[4], const uint8_t *s) -{ - gf q[4]; - fe_copy16(q[0], BX); - fe_copy16(q[1], BY); - fe_copy16(q[2], gf1); - fe_mul(q[3], BX, BY); - scalarmult(r, q, s); -} - -static int point_is_identity(gf p[4]) -{ - uint8_t encoded[32]; - point_pack(encoded, p); - - uint8_t diff = (uint8_t)(encoded[0] ^ 1U); - for (int i = 1; i < 32; i++) - diff |= encoded[i]; - return diff == 0; -} - -/* Public keys must be non-identity points in Ed25519's prime-order subgroup. - * Merely decoding a point is insufficient: an identity or torsion key can - * make the verification equation true without knowledge of a private key. */ static int public_key_is_valid_subgroup(gf public_key[4]) { uint8_t order_l[32]; diff --git a/include/eos_image.h b/include/eos_image.h index 62744d0..24cbb16 100644 --- a/include/eos_image.h +++ b/include/eos_image.h @@ -108,7 +108,8 @@ EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, tlv_hash) + /* Every remaining field, pinned. * - * Four of the fourteen fields were asserted. Transposing two adjacent + * Three of the thirteen field offsets were asserted (the fourth pre-existing + * assert is sizeof, which is not a field). Transposing two adjacent * same-width fields moves neither sizeof nor any of those four offsets, so it * compiled clean: with load_addr and entry_addr swapped, all four existing * asserts still passed and the bootloader would load an image at its entry @@ -132,15 +133,18 @@ EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, flags) == 24, "flags must stay at offset 24"); EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, sig_len) == 61, "sig_len must stay at offset 61"); -EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, reserved) == 62, - "reserved[] must stay at offset 62"); +/* tlv_len and tlv_hash are asserted above, where #93 introduced them; the + * 30 bytes they occupy are the ones this block used to pin as reserved[]. */ /* Field widths. An offset assert cannot see a field growing into padding that - * happens to keep every later offset -- reserved[] absorbs exactly that. */ + * happens to keep every later offset -- the 30 bytes at 62 absorb exactly + * that, which is why both halves of that span carry a width assert. */ EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->hash) == 32, "hash[] is 32 bytes on the wire"); -EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->reserved) == 30, - "reserved[] is 30 bytes on the wire"); +EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->tlv_len) == 2, + "tlv_len is 2 bytes on the wire"); +EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->tlv_hash) == 28, + "tlv_hash is 28 bytes on the wire"); EOS_IMG_STATIC_ASSERT(sizeof(((eos_image_header_t *)0)->signature) == 64, "signature[] is 64 bytes on the wire"); diff --git a/tests/unit/test_ed25519.c b/tests/unit/test_ed25519.c index f78012d..9ea837c 100644 --- a/tests/unit/test_ed25519.c +++ b/tests/unit/test_ed25519.c @@ -30,6 +30,7 @@ static int tests_passed = 0; static void name(void); \ static void run_##name(void) { \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -211,29 +212,105 @@ TEST(test_ed25519_identity_key_forgery_rejected) msg, sizeof(msg) - 1) != EOS_OK); } +/* The eight canonical low-order point encodings. + * + * Every order was computed rather than copied: decoding each y, recovering x, + * and repeatedly adding the point until it reached the identity gives + * 1, 2, 4, 4, 8, 8, 8, 8 for the entries below in order. An earlier revision + * of this array held only five of them -- it omitted y=0 with the sign bit + * set and both sign-flipped order-8 encodings -- while its comment claimed to + * hold "the eight". [L](-A) = -[L]A, so the guard rejects a sign variant + * whether or not it is listed; the reason to list them is that this is the + * regression record for a secure-boot bypass, and a claimed class has to be + * the class it claims. */ +static const uint8_t k_low_order[8][32] = { + /* order 1: the identity, y = 1 */ + {0x01}, + /* order 2: y = -1 */ + {0xEC,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x7F}, + /* order 4: y = 0, sign bit clear */ + {0x00}, + /* order 4: y = 0, sign bit set -- the encoding the earlier array missed */ + {0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00, + 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x80}, + /* order 8 */ + {0x26,0xE8,0x95,0x8F,0xC2,0xB2,0x27,0xB0,0x45,0xC3,0xF4,0x89,0xF2,0xEF,0x98,0xF0, + 0xD5,0xDF,0xAC,0x05,0xD3,0xC6,0x33,0x39,0xB1,0x38,0x02,0x88,0x6D,0x53,0xFC,0x05}, + /* order 8 */ + {0xC7,0x17,0x6A,0x70,0x3D,0x4D,0xD8,0x4F,0xBA,0x3C,0x0B,0x76,0x0D,0x10,0x67,0x0F, + 0x2A,0x20,0x53,0xFA,0x2C,0x39,0xCC,0xC6,0x4E,0xC7,0xFD,0x77,0x92,0xAC,0x03,0x7A}, + /* order 8: sign flip of the first order-8 entry -- also missing before */ + {0x26,0xE8,0x95,0x8F,0xC2,0xB2,0x27,0xB0,0x45,0xC3,0xF4,0x89,0xF2,0xEF,0x98,0xF0, + 0xD5,0xDF,0xAC,0x05,0xD3,0xC6,0x33,0x39,0xB1,0x38,0x02,0x88,0x6D,0x53,0xFC,0x85}, + /* order 8: sign flip of the second -- also missing before */ + {0xC7,0x17,0x6A,0x70,0x3D,0x4D,0xD8,0x4F,0xBA,0x3C,0x0B,0x76,0x0D,0x10,0x67,0x0F, + 0x2A,0x20,0x53,0xFA,0x2C,0x39,0xCC,0xC6,0x4E,0xC7,0xFD,0x77,0x92,0xAC,0x03,0xFA}, +}; + +/* Not low-order points, and refused earlier and by a different mechanism: + * unpackneg() rejects them on canonicality or because no x exists. Kept + * separate so the array above means what its name says -- an earlier revision + * spent one of its eight slots on D9FF..FF, which does not decode at all. */ +static const uint8_t k_non_canonical[3][32] = { + /* y = p: reduces to 0, decodes as an order-4 point but is not canonical */ + {0xED,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x7F}, + /* y = p + 1: reduces to the identity, likewise not canonical */ + {0xEE,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0x7F}, + /* no x satisfies the curve equation for this y: unpackneg() fails */ + {0xD9,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF, + 0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF,0xFF}, +}; + +/* A low-order key forges for roughly one message in n, where n is its order, + * so a single fixed message would let a real bypass pass this suite. */ +static const char *const messages[] = { + "untrusted firmware", "v1.0.0", "", "a", "boot", "eos", "1234", "payload", +}; + TEST(test_ed25519_low_order_keys_rejected) { /* zero_pubkey covers one encoding; Ed25519 has eight low-order points and * the family is what matters. A subgroup test alone is not enough either: * the identity has order 1, which divides L, so [L]identity = identity and - * it passes. Both checks are required. */ - static const uint8_t low_order[4][32] = { - {0}, - {1}, - {0x26,0xe8,0x95,0x8f,0xc2,0xb2,0x27,0xb0,0x45,0xc3,0xf4,0x89,0xf2,0xef,0x98,0xf0, - 0xd5,0xdf,0xac,0x05,0xd3,0xc6,0x33,0x39,0xb1,0x38,0x02,0x88,0x6d,0x53,0xfc,0x05}, - {0xc7,0x17,0x6a,0x70,0x3d,0x4d,0xd8,0x4f,0xba,0x3c,0x0b,0x76,0x0d,0x10,0x67,0x0f, - 0x2a,0x20,0x53,0xfa,0x2c,0x39,0xcc,0xc6,0x4e,0xc7,0xfd,0x77,0x92,0xac,0x03,0x7a}, - }; - const uint8_t msg[] = "untrusted firmware"; + * it passes. Both checks are required. + * + * The sweep is every low-order encoding as the key against every one as R, + * over eight messages, because a low-order key of order n forges for + * roughly one message in n -- a single fixed message would let a genuine + * bypass through this test. Measured against 13a7a02, the last commit + * before the subgroup check: 16 of the 64 (key, R) pairs were accepted by + * at least one message. Here: none. */ + for (size_t k = 0; k < sizeof(k_low_order) / sizeof(k_low_order[0]); k++) { + for (size_t r = 0; r < sizeof(k_low_order) / sizeof(k_low_order[0]); r++) { + for (size_t m = 0; m < sizeof(messages) / sizeof(messages[0]); m++) { + uint8_t sig[64]; + memset(sig, 0, sizeof(sig)); + memcpy(sig, k_low_order[r], 32); + ASSERT(eos_ed25519_verify(sig, k_low_order[k], + (const uint8_t *)messages[m], + strlen(messages[m])) != EOS_OK); + } + } + } +} - for (int k = 0; k < 4; k++) { - for (int r = 0; r < 4; r++) { +TEST(test_ed25519_non_canonical_encodings_rejected) +{ + /* These are refused before the subgroup check ever runs -- unpackneg() + * rejects them on canonicality, or because no x satisfies the curve + * equation. Pinned separately so that nobody deletes that path on the + * grounds that the subgroup test now covers it. It does not. */ + for (size_t k = 0; k < sizeof(k_non_canonical) / sizeof(k_non_canonical[0]); k++) { + for (size_t m = 0; m < sizeof(messages) / sizeof(messages[0]); m++) { uint8_t sig[64]; memset(sig, 0, sizeof(sig)); - memcpy(sig, low_order[r], 32); - ASSERT(eos_ed25519_verify(sig, low_order[k], - msg, sizeof(msg) - 1) != EOS_OK); + memcpy(sig, k_non_canonical[k], 32); + ASSERT(eos_ed25519_verify(sig, k_non_canonical[k], + (const uint8_t *)messages[m], + strlen(messages[m])) != EOS_OK); } } } @@ -260,21 +337,6 @@ TEST(test_ed25519_zero_signature_rejected) ASSERT(eos_ed25519_verify(sig, pk, msg, 1) != EOS_OK); } -TEST(test_ed25519_identity_key_forgery_rejected) -{ - /* The identity point has compressed encoding 01 00...00. With both the - * public key and R set to the identity and S set to zero, the verification - * equation is true for every message unless low-order keys are rejected. */ - uint8_t identity_pub[32] = {1}; - uint8_t identity_sig[64] = {1}; - const uint8_t msg[] = "untrusted firmware"; - - ASSERT(eos_ed25519_verify(identity_sig, identity_pub, - msg, sizeof(msg) - 1) != EOS_OK); -} - -/* ---- SHA-512, the hash Ed25519 is defined over (FIPS 180-4) ---- */ - TEST(test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery) { /* The subgroup check guards the public key, not R, and that is @@ -367,13 +429,13 @@ int main(void) run_test_ed25519_null_args(); run_test_ed25519_identity_key_forgery_rejected(); run_test_ed25519_low_order_keys_rejected(); + run_test_ed25519_non_canonical_encodings_rejected(); + run_test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery(); run_test_ed25519_zero_pubkey_rejected(); run_test_ed25519_zero_signature_rejected(); - run_test_ed25519_identity_key_forgery_rejected(); run_test_sha512_known_answers(); run_test_sha512_streaming_matches_one_shot(); - tests_run = 11; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } From d81375b55dedb1011db5a8e0f94cd405181b5f3c Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 23:11:51 +0530 Subject: [PATCH 2/4] fix(fuzz): point every harness at the API it claims to fuzz MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #94, without which include/eos_image.h does not compile. --- tests/fuzz/fuzz_bootctl.c | 42 ++++++++++----- tests/fuzz/fuzz_crypto.c | 6 +-- tests/fuzz/fuzz_fw_update.c | 64 +++++++++++++--------- tests/fuzz/fuzz_image_verify.c | 44 ++++++++------- tests/fuzz/fuzz_recovery_protocol.c | 46 ++++++++++++---- tests/fuzz/fuzz_sim_flash.h | 83 +++++++++++++++++++++++++++++ 6 files changed, 215 insertions(+), 70 deletions(-) create mode 100644 tests/fuzz/fuzz_sim_flash.h diff --git a/tests/fuzz/fuzz_bootctl.c b/tests/fuzz/fuzz_bootctl.c index c52ab54..3a23000 100644 --- a/tests/fuzz/fuzz_bootctl.c +++ b/tests/fuzz/fuzz_bootctl.c @@ -3,25 +3,43 @@ /** * @file fuzz_bootctl.c - * @brief libFuzzer harness for boot control block (BCB) parsing + * @brief libFuzzer harness for boot control block loading and validation. * - * Feeds arbitrary data into the boot control block parser, exercising - * magic-number validation, slot metadata decoding, retry counters, - * and CRC integrity checks. + * This harness used to declare and call `eos_bootctl_parse()`. No such + * function exists in this repository -- the name appears nowhere outside this + * file -- so the target never linked, and a fuzz target that does not link + * proves nothing about the code it names. + * + * The real untrusted-input path is eos_bootctl_load(), which reads the block + * out of flash and is where a corrupt or hostile BCB arrives. The fuzz input + * is written to the primary and backup BCB addresses and loaded from there. */ -#include +#include "eos_bootctl.h" +#include "fuzz_sim_flash.h" + #include +#include -/* Forward-declare boot control block parser */ -extern int eos_bootctl_parse(const void *bcb_data, size_t bcb_len); +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) +{ + eos_bootctl_t bctl; -int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { - if (size < 8) { - return 0; - } + if (size < 8) return 0; + + /* bootctl_addr is 0 and bootctl_backup_addr is 0x1000 in fuzz_sim_ops, so + * one load can see fuzzer bytes in both the primary and the backup. */ + fuzz_flash_load(0, data, size); + fuzz_flash_write(0x1000, data, size < 0x1000 ? size : 0x1000); - eos_bootctl_parse(data, size); + memset(&bctl, 0, sizeof bctl); + if (eos_bootctl_load(&bctl) == EOS_OK) { + /* Anything load() accepted must also pass validate(): the two + * disagreeing is how a corrupt block reaches the boot decision. */ + if (!eos_bootctl_validate(&bctl)) __builtin_trap(); + (void)eos_bootctl_increment_attempts(&bctl); + (void)eos_bootctl_clear_pending(&bctl); + } return 0; } diff --git a/tests/fuzz/fuzz_crypto.c b/tests/fuzz/fuzz_crypto.c index 41a7830..fe53d01 100644 --- a/tests/fuzz/fuzz_crypto.c +++ b/tests/fuzz/fuzz_crypto.c @@ -14,11 +14,7 @@ #include /* Forward-declare crypto APIs */ -extern int eos_crypto_hash(const uint8_t *data, size_t len, uint8_t digest[32]); -extern int eos_ed25519_verify(const uint8_t signature[64], - const uint8_t public_key[32], - const uint8_t *message, - size_t msg_len); +#include "eos_crypto_boot.h" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { uint8_t digest[32]; diff --git a/tests/fuzz/fuzz_fw_update.c b/tests/fuzz/fuzz_fw_update.c index 7269a79..1ed754d 100644 --- a/tests/fuzz/fuzz_fw_update.c +++ b/tests/fuzz/fuzz_fw_update.c @@ -3,41 +3,57 @@ /** * @file fuzz_fw_update.c - * @brief libFuzzer harness for firmware update stream processing + * @brief libFuzzer harness for the firmware update ingest path. * - * Simulates a chunked firmware-update data stream, feeding arbitrary data - * into the update parser to exercise header validation, chunk sequencing, - * checksum verification, and boundary conditions. + * This harness used to declare three functions of its own: + * + * eos_fw_update_init(), eos_fw_update_process_chunk(), eos_fw_update_finalize(void) + * + * The first two have never existed. The third does exist but takes + * (ctx, mode), not (void) -- so had the other two ever resolved, this would + * have called it through a wrong prototype. The real ingest path is + * begin -> write -> finalize/abort over a context the caller owns. + * + * Chunk widths come from the input rather than being fixed, so the fuzzer can + * split the same payload across write() calls differently -- which is where + * state carried between chunks goes wrong. */ -#include +#include "eos_fw_update.h" +#include "fuzz_sim_flash.h" + #include -#include +#include -/* Forward-declare firmware update APIs */ -extern int eos_fw_update_init(void); -extern int eos_fw_update_process_chunk(const uint8_t *chunk, size_t chunk_len); -extern int eos_fw_update_finalize(void); +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) +{ + eos_fw_update_ctx_t ctx; + size_t offset = 0; + uint8_t selector; -int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { - if (size < 4) { - return 0; - } + if (size < 2) return 0; - eos_fw_update_init(); + selector = data[0]; + data += 1; size -= 1; + + fuzz_flash_load(0x4000, NULL, 0); + + memset(&ctx, 0, sizeof ctx); + if (eos_fw_update_begin(&ctx, (selector & 1) ? EOS_SLOT_B : EOS_SLOT_A) != EOS_OK) + return 0; - /* Feed data in variable-sized chunks derived from the fuzzer input */ - size_t offset = 0; while (offset < size) { - size_t chunk_sz = (data[offset] % 64) + 1; - if (offset + chunk_sz > size) { - chunk_sz = size - offset; - } - eos_fw_update_process_chunk(data + offset, chunk_sz); - offset += chunk_sz; + /* 1..64 bytes, chosen by the data itself. */ + size_t chunk = (size_t)(data[offset] & 0x3F) + 1; + if (chunk > size - offset) chunk = size - offset; + if (eos_fw_update_write(&ctx, data + offset, chunk) != EOS_OK) break; + offset += chunk; } - eos_fw_update_finalize(); + if (selector & 2) + (void)eos_fw_update_finalize(&ctx, EOS_UPGRADE_TEST); + else + eos_fw_update_abort(&ctx); return 0; } diff --git a/tests/fuzz/fuzz_image_verify.c b/tests/fuzz/fuzz_image_verify.c index 514f15f..ea82367 100644 --- a/tests/fuzz/fuzz_image_verify.c +++ b/tests/fuzz/fuzz_image_verify.c @@ -3,31 +3,39 @@ /** * @file fuzz_image_verify.c - * @brief libFuzzer harness for image header parsing and verification + * @brief libFuzzer harness for image header parsing and verification. * - * Feeds fuzzer-generated data into the image header parser via a simulated - * flash-backed buffer, exercising bounds checks, magic-number validation, - * and field-range assertions. + * This harness used to declare the parser itself: + * + * extern int eos_image_parse_header(const void *flash_base, size_t flash_len); + * + * The real one is `int eos_image_parse_header(uint32_t addr, + * eos_image_header_t *out)`. The names matched so it linked, and every call + * passed a pointer where an address was expected and a size where an output + * struct was expected. It fuzzed nothing and was undefined behaviour doing it. + * Including the header instead means the compiler checks this from now on. */ -#include +#include "eos_image.h" +#include "fuzz_sim_flash.h" + #include -#include +#include -/* Forward-declare the image header parser */ -extern int eos_image_parse_header(const void *flash_base, size_t flash_len); +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) +{ + const uint32_t addr = 0x4000; /* slot A */ + eos_image_header_t hdr; -/** - * Simulated flash read-back: the fuzzer data is treated as raw flash content - * starting at offset 0. This lets the parser exercise its flash-pointer - * arithmetic on arbitrary byte sequences. - */ -int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { - if (size < 4) { - return 0; - } + if (size < sizeof(eos_image_header_t)) return 0; - eos_image_parse_header(data, size); + fuzz_flash_load(addr, data, size); + memset(&hdr, 0, sizeof hdr); + if (eos_image_parse_header(addr, &hdr) == EOS_OK) { + /* Only a header the parser accepted reaches verification, which is + * the same order the boot path uses. */ + (void)eos_image_verify_integrity(&hdr, addr); + } return 0; } diff --git a/tests/fuzz/fuzz_recovery_protocol.c b/tests/fuzz/fuzz_recovery_protocol.c index 77b4cbc..d8e4964 100644 --- a/tests/fuzz/fuzz_recovery_protocol.c +++ b/tests/fuzz/fuzz_recovery_protocol.c @@ -3,24 +3,48 @@ /** * @file fuzz_recovery_protocol.c - * @brief libFuzzer harness for recovery protocol packet parsing + * @brief libFuzzer harness for the recovery write bounds check. * - * Exercises the recovery-mode packet parser with arbitrary byte streams, - * targeting framing, command dispatch, and length-field validation. + * This harness used to declare and call `eos_recovery_parse_packet()`. No such + * function has ever existed in this repository -- the name appears nowhere + * outside this file -- so the target has never linked, and a fuzz target that + * does not link is a coverage claim with nothing behind it. + * + * eos_recovery_write_in_range() is the function in this module that actually + * takes untrusted numbers: it decides whether a recovery write stays inside + * its slot, and it is the check standing between a malformed recovery command + * and a write outside the slot. Its four parameters are driven from the input. */ -#include +#include "eos_recovery.h" +#include "eos_types.h" + #include +#include +#include -/* Forward-declare recovery protocol handler */ -extern int eos_recovery_parse_packet(const uint8_t *pkt, size_t pkt_len); +int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) +{ + uint32_t base, slot_size, offset; + uint16_t len; -int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { - if (size < 2) { - return 0; - } + if (size < 14) return 0; - eos_recovery_parse_packet(data, size); + memcpy(&base, data + 0, 4); + memcpy(&slot_size, data + 4, 4); + memcpy(&offset, data + 8, 4); + memcpy(&len, data + 12, 2); + /* The contract is a pure predicate: it must return, and must never accept + * a write that leaves the slot or wraps. Anything it accepts is asserted + * against the same arithmetic, in wider types that cannot wrap. */ + if (eos_recovery_write_in_range(base, slot_size, offset, len) == EOS_OK) { + uint64_t end = (uint64_t)base + (uint64_t)offset + (uint64_t)len; + if (base == 0 || slot_size == 0 || len == 0 || + (uint64_t)offset + (uint64_t)len > (uint64_t)slot_size || + end > 0xFFFFFFFFULL) { + __builtin_trap(); /* accepted a write it had to reject */ + } + } return 0; } diff --git a/tests/fuzz/fuzz_sim_flash.h b/tests/fuzz/fuzz_sim_flash.h new file mode 100644 index 0000000..01b9466 --- /dev/null +++ b/tests/fuzz/fuzz_sim_flash.h @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 EoS Project + +/** + * @file fuzz_sim_flash.h + * @brief RAM-backed board ops, shared by the harnesses that touch flash. + * + * eos_image_parse_header(), eos_bootctl_* and eos_fw_update_* all read and + * write through the HAL, so a harness must install board ops before calling + * them. Without this every one of those harnesses either dereferences a null + * op table or fuzzes nothing. + */ + +#ifndef EOS_FUZZ_SIM_FLASH_H +#define EOS_FUZZ_SIM_FLASH_H + +#include "eos_hal.h" +#include "eos_types.h" + +#include +#include + +#define FUZZ_FLASH_SIZE (64 * 1024) + +static uint8_t fuzz_flash[FUZZ_FLASH_SIZE]; + +static int fuzz_flash_read(uint32_t addr, void *buf, size_t len) +{ + if (addr > FUZZ_FLASH_SIZE || len > FUZZ_FLASH_SIZE - addr) return EOS_ERR_FLASH; + memcpy(buf, &fuzz_flash[addr], len); + return EOS_OK; +} +static int fuzz_flash_write(uint32_t addr, const void *buf, size_t len) +{ + if (addr > FUZZ_FLASH_SIZE || len > FUZZ_FLASH_SIZE - addr) return EOS_ERR_FLASH; + memcpy(&fuzz_flash[addr], buf, len); + return EOS_OK; +} +static int fuzz_flash_erase(uint32_t addr, size_t len) +{ + if (addr > FUZZ_FLASH_SIZE || len > FUZZ_FLASH_SIZE - addr) return EOS_ERR_FLASH; + memset(&fuzz_flash[addr], 0xFF, len); + return EOS_OK; +} +static void fuzz_noop(void) {} +static void fuzz_noop_u32(uint32_t v) { (void)v; } +static eos_reset_reason_t fuzz_reset_reason(void) { return EOS_RESET_POWER_ON; } +static void fuzz_system_reset(void) {} +static bool fuzz_recovery_pin(void) { return false; } +static void fuzz_jump(uint32_t a) { (void)a; } + +static const eos_board_ops_t fuzz_sim_ops = { + .flash_base = 0, .flash_size = FUZZ_FLASH_SIZE, + .slot_a_addr = 0x4000, .slot_a_size = 0x8000, + .slot_b_addr = 0xC000, .slot_b_size = 0x8000, + .recovery_addr = 0, .recovery_size = 0, + .bootctl_addr = 0, .bootctl_backup_addr = 0x1000, + .log_addr = 0x2000, .app_vector_offset = 0, + .flash_read = fuzz_flash_read, + .flash_write = fuzz_flash_write, + .flash_erase = fuzz_flash_erase, + .watchdog_init = fuzz_noop_u32, + .watchdog_feed = fuzz_noop, + .get_reset_reason = fuzz_reset_reason, + .system_reset = fuzz_system_reset, + .recovery_pin_asserted = fuzz_recovery_pin, + .jump = fuzz_jump, + .uart_init = NULL, .uart_send = NULL, .uart_recv = NULL, +}; + +/** Load fuzz input as flash contents at @p addr and install the ops. */ +static inline void fuzz_flash_load(uint32_t addr, const uint8_t *data, size_t size) +{ + memset(fuzz_flash, 0xFF, sizeof fuzz_flash); + if (addr < FUZZ_FLASH_SIZE) { + size_t n = size; + if (n > FUZZ_FLASH_SIZE - addr) n = FUZZ_FLASH_SIZE - addr; + memcpy(&fuzz_flash[addr], data, n); + } + eos_hal_init(&fuzz_sim_ops); +} + +#endif /* EOS_FUZZ_SIM_FLASH_H */ From bbf4e89c22d966bf8acc7f0579a7d2d16f8af709 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 23:20:06 +0530 Subject: [PATCH 3/4] fix(fuzz): the recovery oracle was off by one, and CI proved it The restored Fuzz Harness Build job on #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 #101's job. The guard could never have caught this; only execution under a coverage-driven fuzzer did. --- tests/fuzz/fuzz_recovery_protocol.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/fuzz/fuzz_recovery_protocol.c b/tests/fuzz/fuzz_recovery_protocol.c index d8e4964..eb62d14 100644 --- a/tests/fuzz/fuzz_recovery_protocol.c +++ b/tests/fuzz/fuzz_recovery_protocol.c @@ -39,10 +39,15 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) * a write that leaves the slot or wraps. Anything it accepts is asserted * against the same arithmetic, in wider types that cannot wrap. */ if (eos_recovery_write_in_range(base, slot_size, offset, len) == EOS_OK) { - uint64_t end = (uint64_t)base + (uint64_t)offset + (uint64_t)len; + /* The last byte written is base + offset + len - 1, not + * base + offset + len. An oracle using the one-past-the-end address + * rejects a write whose final byte lands exactly on 0xFFFFFFFF -- + * legal, and accepted by the function under test. That off-by-one + * made this harness trap on valid input rather than find a defect. */ + uint64_t last = (uint64_t)base + (uint64_t)offset + (uint64_t)len - 1u; if (base == 0 || slot_size == 0 || len == 0 || (uint64_t)offset + (uint64_t)len > (uint64_t)slot_size || - end > 0xFFFFFFFFULL) { + last > 0xFFFFFFFFULL) { __builtin_trap(); /* accepted a write it had to reject */ } } From e2bb1f9f5f6e606fa816d52ccf8b1eee1178d4c4 Mon Sep 17 00:00:00 2001 From: Kartikey1306 Date: Thu, 3 Sep 2026 23:16:09 +0530 Subject: [PATCH 4/4] ci(fuzz): compile the harnesses, and refuse the extern that hid the bugs #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 #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. --- .github/workflows/ci.yml | 45 ++++++++++++ tests/unit/test_fuzz_harnesses.py | 110 ++++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+) create mode 100644 tests/unit/test_fuzz_harnesses.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1cfbad9..b51c315 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -123,6 +123,51 @@ jobs: build/arm/**/*.hex retention-days: 90 + # ── Fuzz Harness Build ──────────────────────────────────────────────────── + # tests/fuzz/ is behind EBLDR_BUILD_FUZZ, which defaults OFF, and no job set + # it -- so the harnesses were never compiled anywhere. A harness that is not + # built cannot fail to build, and #99 is what that hid: three of the five + # named functions that exist nowhere in the repository, and a fourth declared + # eos_image_parse_header with the wrong signature, so it linked, ran, and + # fuzzed nothing. + # + # #99 repairs the harnesses and tests/unit/test_fuzz_harnesses.py refuses the + # hand-written `extern` that made the mismatch invisible, but neither + # compiles anything: a signature that drifts after this lands still needs a + # compiler to notice. This is that compiler. + # + # It does not fuzz. It builds the targets with clang and runs each for a few + # seconds over its own generated inputs, which is enough to catch a harness + # that no longer compiles or crashes immediately. A real campaign belongs in + # a scheduled workflow, not on every pull request. + fuzz-build: + name: Fuzz Harness Build + runs-on: ubuntu-22.04 + steps: + - uses: actions/checkout@v4 + - name: Build fuzz targets + run: | + set -euo pipefail + # EBLDR_BUILD_TESTS too: tests/fuzz/ is added from + # tests/CMakeLists.txt, so EBLDR_BUILD_FUZZ alone builds nothing and + # the job would pass having compiled no harness at all. + cmake -B build/fuzz -DEBLDR_BUILD_TESTS=ON -DEBLDR_BUILD_FUZZ=ON \ + -DCMAKE_C_COMPILER=clang -DCMAKE_BUILD_TYPE=Debug + cmake --build build/fuzz -j"$(nproc)" + - name: Smoke-run each harness + run: | + set -euo pipefail + shopt -s nullglob + harnesses=(build/fuzz/tests/fuzz/fuzz_*) + if [ ${#harnesses[@]} -eq 0 ]; then + echo "::error::EBLDR_BUILD_FUZZ=ON produced no harnesses" + exit 1 + fi + for f in "${harnesses[@]}"; do + echo "=== $(basename "$f") ===" + "$f" -max_total_time=5 -print_final_stats=1 + done + # ── Static Analysis ─────────────────────────────────────────────────────── static-analysis: name: Static Analysis (cppcheck + clang-tidy) diff --git a/tests/unit/test_fuzz_harnesses.py b/tests/unit/test_fuzz_harnesses.py new file mode 100644 index 0000000..9684ca6 --- /dev/null +++ b/tests/unit/test_fuzz_harnesses.py @@ -0,0 +1,110 @@ +"""Static guards for the libFuzzer harnesses under tests/fuzz/. + +A fuzz harness fails quietly in a way a unit test does not. It has no +assertions to go red, so the only signals it can give are "did not build" and +"crashed" -- and until the fuzz-build job was added, nothing built them: +EBLDR_BUILD_FUZZ defaults OFF and no workflow turned it on. + +What that hid, and what #99 repaired: four of the five harnesses did not call +the code they claimed to. Three declared functions that exist nowhere -- +eos_bootctl_parse, eos_recovery_parse_packet, eos_fw_update_init and friends -- +so those targets could never link. The fourth declared a real function, +eos_image_parse_header, with the wrong signature: a pointer and a length where +the function takes a flash address and an output struct. That one *did* link, +and ran, and fuzzed nothing -- 20,000 inputs, every one of them bailing at the +first flash read. + +The common cause is a hand-written `extern` prototype in the harness instead of +an #include of the real header. An `extern` is a promise the compiler is +obliged to believe and has no way to check. #99 removed all five instances; +these tests refuse the pattern, so the sixth cannot be written. The compiler +then sees both declarations and a mismatch is a build error rather than a +harness that reports success having tested nothing. + +Static: they parse the sources and CMakeLists, so no clang, cmake or libFuzzer +is needed to run them. +""" + +import re +from pathlib import Path + +FUZZ_DIR = Path(__file__).resolve().parents[1] / "fuzz" +FUZZ_CMAKE = FUZZ_DIR / "CMakeLists.txt" + +ADD_EXECUTABLE_RE = re.compile(r"add_executable\(\s*(\w+)\s+([^)]*?)\)", re.S) + +# `extern "C"` and an extern *variable* are not the problem; an extern function +# prototype standing in for a header is. +EXTERN_FUNCTION_RE = re.compile( + r'^\s*extern\s+(?!"C")[^;{]*\w+\s*\([^;{]*\)\s*;', re.M +) + + +def _harnesses(): + return sorted(FUZZ_DIR.glob("fuzz_*.c")) + + +def _cmake_text(): + return FUZZ_CMAKE.read_text(encoding="utf-8") + + +def test_there_are_harnesses_to_check(): + """A glob that matches nothing would make every test below vacuous.""" + assert _harnesses(), f"no fuzz_*.c under {FUZZ_DIR}" + + +def test_every_harness_is_built(): + """A harness with no add_executable() is never compiled, so never checked.""" + registered = set() + for _target, sources in ADD_EXECUTABLE_RE.findall(_cmake_text()): + for source in sources.split(): + registered.add(Path(source).name) + + missing = [p.name for p in _harnesses() if p.name not in registered] + assert not missing, ( + f"these harnesses exist but no add_executable() in " + f"tests/fuzz/CMakeLists.txt builds them: {missing}. An unbuilt harness " + f"cannot fail, which is how three of them came to name functions that " + f"do not exist." + ) + + +def test_no_harness_declares_its_own_prototypes(): + """Include the header; an `extern` here is an unchecked promise.""" + offenders = {} + for path in _harnesses(): + found = EXTERN_FUNCTION_RE.findall(path.read_text(encoding="utf-8")) + if found: + offenders[path.name] = [line.strip() for line in found] + + assert not offenders, ( + f"these harnesses declare function prototypes instead of including the " + f"header that declares them: {offenders}. The compiler then never sees " + f"the real declaration beside the call, so a wrong signature does not " + f"even warn -- eos_image_parse_header was called with a pointer and a " + f"length for exactly this reason." + ) + + +def test_every_harness_defines_the_entry_point(): + """Without it the target links against libFuzzer's main and does nothing.""" + for path in _harnesses(): + text = path.read_text(encoding="utf-8") + assert "LLVMFuzzerTestOneInput" in text, ( + f"{path.name} defines no LLVMFuzzerTestOneInput" + ) + + +def test_every_harness_links_the_library_under_test(): + """A harness that links nothing would build and fuzz an empty program.""" + text = _cmake_text() + for target, _sources in ADD_EXECUTABLE_RE.findall(text): + if not target.startswith("fuzz_"): + continue + link = re.search( + r"target_link_libraries\(\s*" + re.escape(target) + r"\s+[^)]*\)", + text, + ) + assert link and "eboot_core" in link.group(0), ( + f"{target} does not link eboot_core, so it fuzzes nothing" + )