From f704d87fd445e4d61db00bc302e78660fc99f02f Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 15:06:59 +0530 Subject: [PATCH 1/2] =?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 1b41b01d5a5ba2cc6da4fe7c0fbf5889481d0634 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 16:08:47 +0530 Subject: [PATCH 2/2] fix(eos_sign): emit the layout master defines, not a new meaning for hdr_size Answers the review on #88. Both High findings had the same root, and master has since settled the question the reviewer said needed a decision. The original defect is real and unchanged: the tool emitted [header][TLV][payload] while stamping hdr_size as a fixed 156, so core/image_verify.c's `payload_addr = addr + hdr->hdr_size` landed on the TLV block and every image it produced failed integrity verification on-device. This branch fixed it by moving hdr_size to mean "offset to the payload". That was the wrong half to move. Finding 1 (High) -- eFirmware/src/efw_image.c:122 checks `hdr_size != EFW_IMAGE_HDR_SIZE` for exact equality against 156, so every TLV image would have been rejected by the other half of the format, which eFirmware's header documents as interchangeable. Finding 2 (High) -- core/rollback.c computes the TLV address as image_addr + hdr_size + image_size, i.e. it assumes the area follows the payload. Under either the old layout or this branch's, the anti-rollback EOS_TLV_MIN_SEC_VER could never be found, so the counter this repository gates downgrades on was unreachable for every image the tool produced. Master answers both, and it is the reviewer's option (b): #93 landed tlv_len and tlv_hash in the header -- inside EOS_IMG_SIGNED_LEN -- and documents that "the TLV area sits after the payload". So this PR now emits [header][payload][TLV] with hdr_size always 156. eFirmware's equality check keeps working untouched, rollback.c's arithmetic is correct as written, and no field changes meaning. That reordering also resolves an ordering problem the old shape had: the TLV carried the signature, so its digest could not be computed before the signature existed. The signature now lives in the header's signature[] field and the TLV carries metadata only, which is what makes tlv_hash signable. Finding 4 (Medium) -- the two `assert`s in cmd_sign are removed by `python -O`. This is release-signing tooling, so the signature-length check is now a `raise SystemExit`. The TLV-length assert is gone with the code that needed it. Finding 5 (Medium) -- `pytest.importorskip("cryptography")` still skipped silently if the install ever failed, leaving "collected 19, ran 0" as a green run. Now gated: EOS_REQUIRE_SIGNING_TESTS is set in ci.yml, and under it the module is imported directly so a missing dependency is a collection error. Locally, without the variable, the skip still applies. Finding 6 (Low) -- cmd_verify bounds-checks the image length before unpacking, so a short file gets a FAIL: line rather than a struct.error traceback, and the 156 is hoisted to EOS_IMG_STRUCT_SIZE rather than re-derived. Finding 3 (Medium) needs no change here: master's include/eos_image.h already documents the layout this now emits. Rebased onto #94 (the master repair) and reduced to the five files that are still this PR's own -- the branch was stale enough that its diff against master would have reverted core/rollback.c, include/eos_image.h, tests/unit/test_tlv_auth.c and the rest of #93's TLV work. Verified: end to end, with the real tool: eos_sign.py keygen / sign / verify -> VERIFIED total 744 = 156 header + 512 payload + 76 TLV hdr_size 156, tlv_len 76, TLV magic 0x6907 at 156+512 sha256(tlv)[:28] == header bytes 64..92 True ctest 22/22 PASS test_eos_sign_boot_path (real parser over real tool output): current layout [hdr][payload][tlv]: verify_integrity -> 0 PASS old layout [hdr][tlv][payload]: verify_integrity -> -3 PASS TLV magic at addr + hdr_size + image_size: 0x6907 PASS that third line is finding 2's regression test -- it fails on the old layout, where the address lands past the end of the image. pytest tests/ (EOS_REQUIRE_SIGNING_TESTS=1) 44 passed the CI gate itself: with the variable set and the module absent, the import raises ImportError -> collection error -> job fails, rather than skipping. Refs #88, #93 --- .github/workflows/ci.yml | 8 + tests/CMakeLists.txt | 6 + tests/unit/test_eos_sign_boot_path.c | 164 +++++++++++++++++++++ tests/unit/test_eos_sign_payload_offset.py | 161 ++++++++++++++++++++ tests/vectors/signed_image_fixture.h | 105 +++++++++++++ tools/eos_sign.py | 147 +++++++++++++----- tools/gen_signed_image_fixture.py | 117 +++++++++++++++ 7 files changed, 672 insertions(+), 36 deletions(-) create mode 100644 tests/unit/test_eos_sign_boot_path.c create mode 100644 tests/unit/test_eos_sign_payload_offset.py create mode 100644 tests/vectors/signed_image_fixture.h create mode 100644 tools/gen_signed_image_fixture.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1cfbad9..ef0704e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -45,6 +45,14 @@ jobs: # dependency of anything in the repository. pip3 install -r requirements.txt pytest-cov + # Installing the dependency fixes today's silent skip; this makes the + # skip impossible here rather than merely unlikely. Without it, a + # dropped requirement or a changed runner image puts the signing suite + # back to collecting 19 tests and running none, with the job still + # green. See tests/unit/test_eos_sign_payload_offset.py. + - name: Signing tests must not silently skip + run: echo "EOS_REQUIRE_SIGNING_TESTS=1" >> "$GITHUB_ENV" + - name: Configure (host) run: | cmake -B build/host -G Ninja \ diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b394f24..ce502fd 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -110,6 +110,12 @@ add_executable(eboot_test_ecc unit/test_ecc.c) target_link_libraries(eboot_test_ecc PRIVATE eboot_core) add_test(NAME test_ecc COMMAND eboot_test_ecc) +# --- test_eos_sign_boot_path: the tool's real output through the real parser --- +add_executable(eboot_test_eos_sign_boot_path unit/test_eos_sign_boot_path.c) +target_link_libraries(eboot_test_eos_sign_boot_path PRIVATE eboot_core) +target_include_directories(eboot_test_eos_sign_boot_path PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) +add_test(NAME test_eos_sign_boot_path COMMAND eboot_test_eos_sign_boot_path) + # --- Valgrind test targets --- find_program(VALGRIND valgrind) if(VALGRIND) diff --git a/tests/unit/test_eos_sign_boot_path.c b/tests/unit/test_eos_sign_boot_path.c new file mode 100644 index 0000000..8307e14 --- /dev/null +++ b/tests/unit/test_eos_sign_boot_path.c @@ -0,0 +1,164 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 EoS Project + +/** + * @file test_eos_sign_boot_path.c + * @brief Run the real boot path over real eos_sign.py output. + * + * tools/eos_sign.py emits [header][TLV area][payload]. It used to stamp + * hdr_size as a fixed 156 -- the struct size alone -- while + * core/image_verify.c computes `payload_addr = addr + hdr->hdr_size`. That + * landed on the TLV block, so the SHA-256 taken from there never matched + * hash[] and every image the tool produced failed verification on-device. + * + * The Python-side test for this checks the tool's own arithmetic, which is + * close to checking that the tool agrees with itself. This one stages the + * tool's actual bytes into simulated flash and calls the bootloader's own + * eos_image_parse_header() and eos_image_verify_integrity(). + * + * The fixture carries both layouts over the same payload and key, so the fix + * and the defect are asserted together: current output must verify, the old + * output must be refused. + */ + +#include "eos_image.h" +#include "eos_hal.h" +#include "eos_types.h" + +#include "../vectors/signed_image_fixture.h" + +#include +#include +#include + +#define SIM_FLASH_SIZE (64 * 1024) +static uint8_t sim_flash[SIM_FLASH_SIZE]; + +static int sim_flash_read(uint32_t addr, void *buf, size_t len) +{ + if (addr + len > SIM_FLASH_SIZE) return EOS_ERR_FLASH; + memcpy(buf, &sim_flash[addr], len); + return EOS_OK; +} +static int sim_flash_write(uint32_t addr, const void *buf, size_t len) +{ + if (addr + len > SIM_FLASH_SIZE) return EOS_ERR_FLASH; + memcpy(&sim_flash[addr], buf, len); + return EOS_OK; +} +static int sim_flash_erase(uint32_t addr, size_t len) +{ + if (addr + len > SIM_FLASH_SIZE) return EOS_ERR_FLASH; + memset(&sim_flash[addr], 0xFF, len); + return EOS_OK; +} +static void sim_noop(void) {} +static void sim_noop_u32(uint32_t v) { (void)v; } +static eos_reset_reason_t sim_reset_reason(void) { return EOS_RESET_POWER_ON; } +static void sim_system_reset(void) {} +static bool sim_recovery_pin(void) { return false; } +static void sim_jump(uint32_t a) { (void)a; } + +static const eos_board_ops_t sim_ops = { + .flash_base = 0, .flash_size = SIM_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 = sim_flash_read, + .flash_write = sim_flash_write, + .flash_erase = sim_flash_erase, + .watchdog_init = sim_noop_u32, + .watchdog_feed = sim_noop, + .get_reset_reason = sim_reset_reason, + .system_reset = sim_system_reset, + .recovery_pin_asserted = sim_recovery_pin, + .jump = sim_jump, + .uart_init = NULL, .uart_send = NULL, .uart_recv = NULL, +}; + +static int failures; + +#define CHECK(cond) do { \ + if (!(cond)) { \ + printf(" [FAIL] %s:%d: %s\n", __FILE__, __LINE__, #cond); \ + failures++; \ + } \ +} while (0) + +#define IMAGE_ADDR 0x4000u + +static int stage_and_verify(const unsigned char *img, size_t len, + eos_image_header_t *hdr_out) +{ + memset(sim_flash, 0xFF, sizeof sim_flash); + memcpy(&sim_flash[IMAGE_ADDR], img, len); + eos_hal_init(&sim_ops); + + if (eos_image_parse_header(IMAGE_ADDR, hdr_out) != EOS_OK) + return EOS_ERR_INVALID; + return eos_image_verify_integrity(hdr_out, IMAGE_ADDR); +} + +int main(void) +{ + eos_image_header_t hdr; + int rc; + + printf("=== eos_sign.py output through the real boot path ===\n\n"); + + /* Current output: [header][payload][TLV], so addr + hdr_size is the + * payload and the stored SHA-256 matches what the bootloader hashes. */ + memset(&hdr, 0, sizeof hdr); + rc = stage_and_verify(eos_fixture_image_good, EOS_FIXTURE_GOOD_LEN, &hdr); + CHECK(rc == EOS_OK); + CHECK(hdr.hdr_size == EOS_FIXTURE_GOOD_HDR_SIZE); + CHECK(hdr.image_size == EOS_FIXTURE_PAYLOAD_LEN); + printf(" current layout [hdr][payload][tlv]: verify_integrity -> %d %s\n", + rc, rc == EOS_OK ? "[PASS]" : "[FAIL]"); + + /* Pre-fix output: [header][TLV][payload], so the bootloader hashes the + * TLV block and the first bytes of the payload. It must refuse this. */ + memset(&hdr, 0, sizeof hdr); + rc = stage_and_verify(eos_fixture_image_old, EOS_FIXTURE_OLD_LEN, &hdr); + CHECK(rc != EOS_OK); + printf(" old layout [hdr][tlv][payload]: verify_integrity -> %d %s\n", + rc, rc != EOS_OK ? "[PASS]" : "[FAIL]"); + + /* hdr_size is 156 in both -- that is the point of the fix, and it is why + * the two fixtures have to be distinguished by their bytes rather than by + * a header field. Same payload, same key, different placement. */ + CHECK(EOS_FIXTURE_GOOD_HDR_SIZE == 156); + CHECK(EOS_FIXTURE_OLD_HDR_SIZE == 156); + CHECK(EOS_FIXTURE_GOOD_LEN == EOS_FIXTURE_OLD_LEN); + CHECK(memcmp(eos_fixture_image_good, eos_fixture_image_old, + EOS_FIXTURE_GOOD_LEN) != 0); + + /* The finding this test was extended for: core/rollback.c computes the + * TLV address as image_addr + hdr_size + image_size, i.e. it assumes the + * area follows the payload. Under the old layout that landed past the end + * of the image and the anti-rollback TLV could never be found -- so the + * counter this repository gates downgrades on was unreachable for every + * image the tool produced. verify_integrity alone does not exercise that + * path; this does. */ + memset(&hdr, 0, sizeof hdr); + rc = stage_and_verify(eos_fixture_image_good, EOS_FIXTURE_GOOD_LEN, &hdr); + CHECK(rc == EOS_OK); + { + uint32_t tlv_addr = IMAGE_ADDR + hdr.hdr_size + hdr.image_size; + uint16_t magic = 0; + CHECK(eos_hal_flash_read(tlv_addr, &magic, sizeof magic) == EOS_OK); + printf(" TLV magic at addr + hdr_size + image_size: 0x%04x %s\n", + magic, magic == 0x6907 ? "[PASS]" : "[FAIL]"); + CHECK(magic == 0x6907); + CHECK(hdr.tlv_len == EOS_FIXTURE_TLV_LEN); + } + + if (failures) { + printf("\n[FAIL] %d check(s) failed\n", failures); + return 1; + } + printf("\n[PASS] the bootloader accepts current tool output and refuses the old layout\n"); + return 0; +} diff --git a/tests/unit/test_eos_sign_payload_offset.py b/tests/unit/test_eos_sign_payload_offset.py new file mode 100644 index 0000000..2d97ce9 --- /dev/null +++ b/tests/unit/test_eos_sign_payload_offset.py @@ -0,0 +1,161 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 EoS Project + +"""eos_sign.py must place the payload where the bootloader looks for it. + +core/image_verify.c computes `payload_addr = addr + hdr->hdr_size` and hashes +image_size bytes from there. tools/sign_image.py reads the same range, +`data[hdr_size:hdr_size + image_size]`. hdr_size is therefore the offset from +the image base to the payload, for everything that reads these images. + +eos_sign.py writes [header][TLV area][payload] and used to stamp hdr_size as a +fixed 156 -- the struct size alone -- so that offset landed on the TLV block. +Every image it produced failed integrity verification on-device. +""" + +import hashlib +import struct +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +TOOLS = REPO_ROOT / "tools" + +# A skip is right for a developer without the signing dependency installed and +# wrong for CI, where "collected 19 tests, ran 0" is a green run that checked +# nothing -- the failure .ai/security.md names directly. EOS_REQUIRE_SIGNING_TESTS +# is set in the workflow, so there a missing dependency is a hard error; locally +# the skip still applies. +if os.environ.get("EOS_REQUIRE_SIGNING_TESTS"): + import cryptography # noqa: F401 -- ImportError here must fail the job +else: + pytest.importorskip( + "cryptography", reason="signing tools require 'cryptography'") + +HDR_SIZE_OFFSET = 6 +IMAGE_SIZE_OFFSET = 8 +HASH_OFFSET = 28 +HDR_STRUCT_SIZE = 156 # sizeof(eos_image_header_t) +TLV_LEN_OFFSET = 62 +TLV_HASH_OFFSET = 64 +TLV_HASH_LEN = 28 # EOS_IMG_TLV_HASH_LEN +SIGNED_LEN = 92 # EOS_IMG_SIGNED_LEN +TLV_INFO_MAGIC = 0x6907 + + +@pytest.fixture(scope="module") +def signed(tmp_path_factory): + work = tmp_path_factory.mktemp("eos_sign") + payload = bytes(range(256)) * 8 # 2048 deterministic bytes + (work / "fw.bin").write_bytes(payload) + + subprocess.run( + [sys.executable, str(TOOLS / "eos_sign.py"), "keygen", + "--output", str(work / "kp")], + check=True, capture_output=True, + ) + subprocess.run( + [sys.executable, str(TOOLS / "eos_sign.py"), "sign", + "--key", str(work / "kp_private.pem"), + "--input", str(work / "fw.bin"), + "--output", str(work / "fw.signed")], + check=True, capture_output=True, + ) + return {"image": (work / "fw.signed").read_bytes(), "payload": payload} + + +def test_hdr_size_is_the_offset_to_the_payload(signed): + """The exact computation core/image_verify.c performs.""" + image, payload = signed["image"], signed["payload"] + hdr_size = struct.unpack_from(" 0, "a signed image carries a TLV area" + + # The TLV area follows the payload, which is where core/rollback.c looks. + tlv_offset = hdr_size + img_size + tlv_magic = struct.unpack_from("hdr_size` landed on the +TLV block and every image it produced failed integrity verification on-device. + +An earlier revision of this fix moved hdr_size instead, making it the offset +to the payload. That was the wrong half to move: hdr_size is a wire-format +field with four other readers, two of which reject the new meaning -- +eFirmware/src/efw_image.c checks `hdr_size != 156` exactly, and +core/rollback.c computes the TLV address as hdr_size + image_size. Moving the +bytes rather than the field's meaning leaves both correct by construction, and +it is what master settled on: include/eos_image.h now carries tlv_len and +tlv_hash inside the signed prefix, documented as "the TLV area sits after the +payload". + +So: hdr_size stays 156 always, the TLV area follows the payload, and the +header declares its length and digest. Those two fields are inside +EOS_IMG_SIGNED_LEN, which is what makes a TLV-declared value such as +EOS_TLV_MIN_SEC_VER trustworthy enough to gate anti-rollback on. Usage: python eos_sign.py sign --key private.pem --input firmware.bin --output firmware.signed.bin @@ -19,7 +32,7 @@ python eos_sign.py keygen --output keypair Creates a signed image with the eBoot image header format: - [eos_image_header_t][TLV area][payload] + [eos_image_header_t][payload][TLV area] Supports Ed25519 signatures (default) and SHA-256 integrity hashes. """ @@ -72,9 +85,22 @@ def sha256(data: bytes) -> bytes: return hashlib.sha256(data).digest() +EOS_IMG_STRUCT_SIZE = 156 # sizeof(eos_image_header_t); pinned in eos_image.h +EOS_IMG_TLV_HASH_LEN = 28 # EOS_IMG_TLV_HASH_LEN + + def build_header(payload: bytes, entry_addr: int, load_addr: int, - version: int, sig: bytes, sig_type: int) -> bytes: - """Build the eos_image_header_t structure.""" + version: int, sig: bytes, sig_type: int, + tlv_len: int = 0, tlv_hash: bytes = b'') -> bytes: + """Build the eos_image_header_t structure. + + tlv_len and tlv_hash describe the TLV area that follows the payload. + Both sit inside EOS_IMG_SIGNED_LEN, so the signature covers them and the + trailing area cannot be rewritten without invalidating it. hdr_size is + always EOS_IMG_STRUCT_SIZE -- it is the size of this struct, which is what + core/image_verify.c, core/rollback.c, stage1/jump_app.c and + eFirmware/src/efw_image.c all read it as. + """ payload_hash = sha256(payload) # EOS_IMG_FLAG_HASH_SHA256 is what makes eos_image_verify_integrity() take @@ -85,11 +111,12 @@ def build_header(payload: bytes, entry_addr: int, load_addr: int, # Header: magic(4) + hdr_version(2) + hdr_size(2) + image_size(4) + # load_addr(4) + entry_addr(4) + image_version(4) + flags(4) + - # hash(32) + sig_type(1) + sig_len(1) + reserved(30) + signature(64) - hdr_size = 4 + 2 + 2 + 4 + 4 + 4 + 4 + 4 + 32 + 1 + 1 + 30 + 64 # = 156 - + # hash(32) + sig_type(1) + sig_len(1) + tlv_len(2) + + # tlv_hash(28) + signature(64) + # hdr_size is the size of this struct, always, and the TLV area follows + # the payload rather than preceding it. hdr = struct.pack(' bytes: - """Build TLV area with SHA-256, key hash, and signature entries.""" +def build_tlv(payload_hash: bytes, key_hash: bytes) -> bytes: + """Build the TLV area that follows the payload. + + No signature entry. The signature lives in the header's signature[] field + and covers EOS_IMG_SIGNED_LEN, which includes tlv_hash -- so a TLV area + containing the signature could not be hashed before the signature existed. + Putting the signature in the header and the metadata in the TLV is what + makes that ordering resolvable, and it is the arrangement + include/eos_image.h documents. + """ entries = b'' # SHA-256 hash TLV @@ -124,11 +165,6 @@ def build_tlv(payload_hash: bytes, key_hash: bytes, sig: bytes) -> bytes: entries += struct.pack('= 2 else 0 + if tlv_magic != TLV_INFO_MAGIC: + print(f'FAIL: no TLV magic at offset {tlv_offset}') + sys.exit(1) + if sha256(tlv)[:EOS_IMG_TLV_HASH_LEN] != stored_tlv_hash: + print('FAIL: TLV area does not match tlv_hash in the signed header') + sys.exit(1) # Verify hash computed_hash = sha256(payload) diff --git a/tools/gen_signed_image_fixture.py b/tools/gen_signed_image_fixture.py new file mode 100644 index 0000000..9f58ac8 --- /dev/null +++ b/tools/gen_signed_image_fixture.py @@ -0,0 +1,117 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 EoS Project +"""Emit a signed-image fixture so the C boot path can be run over real tool output. + +tools/eos_sign.py used to emit [header][TLV][payload] while stamping hdr_size +as a fixed 156, so core/image_verify.c computed the payload address as +addr + 156 and landed on the TLV block. Every image the tool produced failed +integrity verification on-device. + +It now emits [header][payload][TLV], which is the layout master documents: +hdr_size stays 156 for every image, and the trailing TLV area is bound to the +signature through the header's tlv_len and tlv_hash. + +Proving that in Python only re-states the tool's own arithmetic. This emits +both layouts as C byte arrays so tests/unit/test_eos_sign_boot_path.c can run +the actual bootloader code over them: the current layout must verify, and the +old one -- TLV between the header and the payload -- must be refused. + +Deterministic -- fixed key seed, fixed payload -- so the fixture is stable and +regenerating it is a no-op unless the tool's output really changed. + + python3 tools/gen_signed_image_fixture.py > tests/vectors/signed_image_fixture.h +""" + +import importlib.util +import pathlib +import struct +import sys + +ROOT = pathlib.Path(__file__).resolve().parents[1] + +spec = importlib.util.spec_from_file_location("eos_sign", ROOT / "tools" / "eos_sign.py") +eos_sign = importlib.util.module_from_spec(spec) +spec.loader.exec_module(eos_sign) + +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey +from cryptography.hazmat.primitives import serialization + +SEED = bytes(range(32)) # fixed, so the fixture is reproducible +PAYLOAD = bytes((i * 7 + 3) & 0xFF for i in range(256)) +ENTRY = 0x08020000 +LOAD = 0x08020000 +VERSION = 0x00010000 + + +def build(tlv_after_payload: bool) -> bytes: + key = Ed25519PrivateKey.from_private_bytes(SEED) + payload_hash = eos_sign.sha256(PAYLOAD) + pub_raw = key.public_key().public_bytes( + serialization.Encoding.Raw, serialization.PublicFormat.Raw) + key_hash = eos_sign.sha256(pub_raw) + + tlv = eos_sign.build_tlv(payload_hash, key_hash) + tlv_hash = eos_sign.sha256(tlv)[:eos_sign.EOS_IMG_TLV_HASH_LEN] + + hdr = bytearray(eos_sign.build_header(PAYLOAD, ENTRY, LOAD, VERSION, b"", + eos_sign.SIG_TYPE_ED25519, + len(tlv), tlv_hash)) + sig = key.sign(bytes(hdr[:eos_sign.SIGNED_LEN])) + hdr[eos_sign.SIGNATURE_OFFSET:eos_sign.SIGNATURE_OFFSET + 64] = sig + + if tlv_after_payload: + return bytes(hdr) + PAYLOAD + tlv + # The old shape: TLV between the header and the payload, so + # addr + hdr_size lands on the TLV block instead of the payload. + return bytes(hdr) + tlv + PAYLOAD + + +def carr(b, indent=" "): + out, line = [], indent + for i, x in enumerate(b): + line += "0x%02x," % x + if (i + 1) % 16 == 0: + out.append(line); line = indent + if line.strip(): + out.append(line) + return "\n".join(out) + + +def main(): + good = build(True) + old = build(False) + pub = Ed25519PrivateKey.from_private_bytes(SEED).public_key().public_bytes( + serialization.Encoding.Raw, serialization.PublicFormat.Raw) + + w = sys.stdout.write + w("/* SPDX-License-Identifier: MIT\n * Copyright (c) 2026 EoS Project\n */\n\n") + w("/* GENERATED FILE -- do not edit by hand.\n") + w(" * tools/gen_signed_image_fixture.py\n *\n") + w(" * Two images over the same payload, the same key and the same header,\n") + w(" * differing only in where the TLV area sits.\n *\n") + w(" * CURRENT: [header][payload][TLV] -- addr + hdr_size is the payload\n") + w(" * OLD: [header][TLV][payload] -- addr + hdr_size is the TLV block\n") + w(" *\n * hdr_size is %d in both: it is the struct size, not a payload offset.\n" % struct.unpack_from('