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 0886ac0f12c47e40ed37fe5257912056c09ddd32 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Tue, 1 Sep 2026 12:55:46 +0530 Subject: [PATCH 2/4] fix(secure-boot): a debug lock that failed must not report a successful boot cfg.lock_debug asks for SWD/JTAG to be closed before the verified image runs. Step 7 of eos_secure_boot() honoured that request like this: if (cfg->lock_debug) { eos_secure_boot_lock_debug(); } /* ---- Step 8: Record successful attestation ---- */ attest_record(2, hdr.image_version, hdr.hash, NULL, EOS_SBOOT_OK); return EOS_SBOOT_OK; eos_secure_boot_lock_debug() returned void and discarded the result of the OTP write that actually blows the fuse. So when the write failed, boot continued, attestation recorded EOS_SBOOT_OK, and the device ran the image with its debug port open -- the exact condition the policy existed to prevent, reported as a clean secure boot. The interesting case is not a flaky fuse. eos_hal_otp_write() returns EOS_ERR_NOT_SUPPORTED when the board provides no otp_write hook at all, so on every such board `lock_debug: true` was silently a no-op. That is the default configuration, not an edge case. eos_secure_boot_lock_debug() now returns int, and a caller that asked for the lock and did not get it fails with EOS_SBOOT_ERR_POLICY -- a code that already existed for exactly this ("Boot policy violation") -- with the failure recorded in the attestation log rather than a success. Why this was never observable: core/secure_boot.c is not in CMakeLists.txt. The module has never been compiled, so this path could not run and could not be tested. Added the one line that builds it -- the same line #72 adds, written identically so whichever lands first leaves the other a trivial rebase. tests/unit/test_secure_boot_policy.c covers the three outcomes: the fuse written, the write failing, and a board with no otp_write. Kept in its own file so it does not collide with the test_secure_boot.c #72 introduces. Against master the new test does not compile -- `invalid operands to binary expression ('void' and 'int')` -- because there is no result to check. That is the defect stated as a compile error. Verified on this branch: build clean, ctest 20/20, pytest 30 passed. Stacked on #77 (master's test suite does not compile without it). Co-Authored-By: Claude Opus 5 (1M context) --- core/secure_boot.c | 21 ++++- include/eos_secure_boot.h | 7 +- tests/CMakeLists.txt | 5 ++ tests/unit/test_secure_boot_policy.c | 127 +++++++++++++++++++++++++++ 4 files changed, 156 insertions(+), 4 deletions(-) create mode 100644 tests/unit/test_secure_boot_policy.c diff --git a/core/secure_boot.c b/core/secure_boot.c index 10bb0d3..ab677c6 100644 --- a/core/secure_boot.c +++ b/core/secure_boot.c @@ -182,8 +182,19 @@ eos_secure_boot_result_t eos_secure_boot(const eos_secure_boot_config_t *cfg, } /* ---- Step 7: Lock debug interfaces ---- */ + /* + * A policy that asked for the debug port to be closed and did not get + * it is a policy violation, not a detail. The result used to be + * discarded, so a board whose OTP write failed -- or one with no + * otp_write at all, where the HAL returns EOS_ERR_NOT_SUPPORTED -- + * booted with SWD/JTAG open while attestation recorded EOS_SBOOT_OK. + */ if (cfg->lock_debug) { - eos_secure_boot_lock_debug(); + if (eos_secure_boot_lock_debug() != EOS_OK) { + attest_record(2, hdr.image_version, hdr.hash, NULL, + EOS_SBOOT_ERR_POLICY); + return EOS_SBOOT_ERR_POLICY; + } } /* ---- Step 8: Record successful attestation ---- */ @@ -212,17 +223,21 @@ int eos_secure_boot_verify_key(const uint8_t key_hash[32]) return secure_compare(key_hash, otp_hash, 32); } -void eos_secure_boot_lock_debug(void) +int eos_secure_boot_lock_debug(void) { /* Write lock pattern to eFuse debug lock register */ uint8_t lock = 0xFF; - eos_hal_otp_write(OTP_DEBUG_LOCK_OFFSET, &lock, 1); + int rc = eos_hal_otp_write(OTP_DEBUG_LOCK_OFFSET, &lock, 1); + if (rc != EOS_OK) + return rc; /* On Cortex-M: disable DAP access via DHCSR if supported */ #if defined(__ARM_ARCH) /* Some MCUs support disabling debug via DBGMCU register */ /* *((volatile uint32_t *)0xE0042004) = 0; */ #endif + + return EOS_OK; } int eos_secure_boot_update_rollback(uint32_t new_version) diff --git a/include/eos_secure_boot.h b/include/eos_secure_boot.h index 7890ec8..659c4a3 100644 --- a/include/eos_secure_boot.h +++ b/include/eos_secure_boot.h @@ -106,8 +106,13 @@ int eos_secure_boot_verify_key(const uint8_t key_hash[32]); /** * @brief Lock debug interfaces (SWD/JTAG) permanently. * Only effective on real hardware with eFuse support. + * + * @return EOS_OK if the lock was written, otherwise the HAL error -- + * EOS_ERR_NOT_SUPPORTED on a board with no otp_write. Callers that + * asked for the debug port to be closed must treat a non-OK result + * as a failure to boot: the port is still open. */ -void eos_secure_boot_lock_debug(void); +int eos_secure_boot_lock_debug(void); /** * @brief Update the anti-rollback counter in OTP. diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b394f24..ee17487 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -90,6 +90,11 @@ add_executable(eboot_test_keystore unit/test_keystore.c) target_link_libraries(eboot_test_keystore PRIVATE eboot_core) add_test(NAME test_keystore COMMAND eboot_test_keystore) +# --- test_secure_boot_policy: debug-lock policy enforcement --- +add_executable(eboot_test_secure_boot_policy unit/test_secure_boot_policy.c) +target_link_libraries(eboot_test_secure_boot_policy PRIVATE eboot_core) +add_test(NAME test_secure_boot_policy COMMAND eboot_test_secure_boot_policy) + # --- test_rollback: Anti-rollback security counter --- add_executable(eboot_test_rollback unit/test_rollback.c) target_link_libraries(eboot_test_rollback PRIVATE eboot_core) diff --git a/tests/unit/test_secure_boot_policy.c b/tests/unit/test_secure_boot_policy.c new file mode 100644 index 0000000..ac93b08 --- /dev/null +++ b/tests/unit/test_secure_boot_policy.c @@ -0,0 +1,127 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 EoS Project +// ISO/IEC 25000 | ISO/IEC/IEEE 15288:2023 + +/** + * @file test_secure_boot_policy.c + * @brief The debug-lock policy must be enforced, not merely attempted + * + * cfg.lock_debug asks for SWD/JTAG to be closed before the image runs. + * eos_secure_boot_lock_debug() writes an OTP fuse to do that, and its result + * used to be discarded -- so a board whose OTP write failed, or one with no + * otp_write at all, booted with the debug port open while attestation recorded + * EOS_SBOOT_OK. + * + * Kept separate from tests/unit/test_secure_boot.c (added by #72) so the two + * do not collide. + */ + +#include "eos_secure_boot.h" +#include "eos_hal.h" +#include +#include +#include + +static int tests_run = 0; +static int tests_passed = 0; + +#define TEST(name) \ + static void name(void); \ + static void run_##name(void) { \ + printf(" %-54s ", #name); \ + name(); \ + tests_passed++; \ + printf("[PASS]\n"); \ + } \ + static void name(void) + +#define ASSERT(cond) do { \ + if (!(cond)) { \ + printf("[FAIL] %s:%d: %s\n", __FILE__, __LINE__, #cond); \ + exit(1); \ + } \ +} while(0) + +/* ---- Simulated board: OTP only, with a scriptable write result ---- */ + +#define OTP_SIZE 0x400 +static uint8_t sim_otp[OTP_SIZE]; +static int otp_write_rc; +static int otp_write_calls; +static int provide_otp_write; + +static int sim_otp_read(uint32_t offset, void *buf, size_t len) +{ + if ((uint64_t)offset + len > OTP_SIZE) return EOS_ERR_INVALID; + memcpy(buf, sim_otp + offset, len); + return EOS_OK; +} + +static int sim_otp_write(uint32_t offset, const void *buf, size_t len) +{ + otp_write_calls++; + if (otp_write_rc != EOS_OK) return otp_write_rc; + if ((uint64_t)offset + len > OTP_SIZE) return EOS_ERR_INVALID; + memcpy(sim_otp + offset, buf, len); + return EOS_OK; +} + +static eos_board_ops_t sim_ops; + +static void reset_fixture(void) +{ + memset(&sim_ops, 0, sizeof(sim_ops)); + memset(sim_otp, 0, sizeof(sim_otp)); + sim_ops.otp_read = sim_otp_read; + if (provide_otp_write) sim_ops.otp_write = sim_otp_write; + otp_write_rc = EOS_OK; + otp_write_calls = 0; + eos_hal_init(&sim_ops); +} + +/* A working eFuse: the lock is written and reported. */ +TEST(test_lock_debug_reports_success_when_the_fuse_is_written) +{ + provide_otp_write = 1; + reset_fixture(); + + ASSERT(eos_secure_boot_lock_debug() == EOS_OK); + ASSERT(otp_write_calls == 1); +} + +/* An eFuse write that fails leaves the port open. Saying so is the whole + * point: the caller asked for it to be closed. */ +TEST(test_lock_debug_reports_a_failed_fuse_write) +{ + provide_otp_write = 1; + reset_fixture(); + otp_write_rc = EOS_ERR_FLASH; + + ASSERT(eos_secure_boot_lock_debug() != EOS_OK); + ASSERT(otp_write_calls == 1); +} + +/* The common case, and the one that used to be silent: a board that provides + * no otp_write at all. eos_hal_otp_write() returns EOS_ERR_NOT_SUPPORTED, so + * lock_debug: true was a no-op on every such board. */ +TEST(test_lock_debug_reports_a_board_with_no_otp_write) +{ + provide_otp_write = 0; + reset_fixture(); + + ASSERT(eos_secure_boot_lock_debug() != EOS_OK); + ASSERT(otp_write_calls == 0); +} + +int main(void) +{ + printf("=== eBootloader: Secure Boot Debug-Lock Policy Tests ===\n\n"); + + run_test_lock_debug_reports_success_when_the_fuse_is_written(); + run_test_lock_debug_reports_a_failed_fuse_write(); + run_test_lock_debug_reports_a_board_with_no_otp_write(); + + tests_run = 3; + printf("\n%d/%d tests passed\n", tests_passed, tests_run); + return (tests_passed == tests_run) ? 0 : 1; +} From eafe67c5d3c59e1c39355df5f2d04ec5845525fe Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 15:13:22 +0530 Subject: [PATCH 3/4] fix(secure-boot): test the policy the PR changed, not only the helper it calls Answers finding 1 (Medium) from the review on #82. All three tests called eos_secure_boot_lock_debug() directly and asserted its return value. None drove eos_secure_boot() with cfg.lock_debug = true, so the step-7 early return this PR adds -- the attest_record + return EOS_SBOOT_ERR_POLICY -- never executed under test. The file's docblock says "the debug-lock policy must be enforced, not merely attempted"; what it covered was the attempted half. Adds three end-to-end cases through eos_secure_boot(): - lock_debug: true on a board with no otp_write -> EOS_SBOOT_ERR_POLICY - the same image with lock_debug: false -> EOS_SBOOT_OK, and the recorded entry point is the header's, which is the counter-check: without it the first test would also pass if steps 1-6 were failing for an unrelated reason and never reaching step 7 - lock_debug: true with a working fuse -> EOS_SBOOT_OK, one write Reaching step 7 needs an image that clears steps 1, 2 and 5, so the fixture gains a simulated flash and stages an unsigned, unencrypted image whose SHA-256 matches. EOS_IMG_FLAG_HASH_SHA256 is load-bearing there: without it verify_integrity takes the CRC32 branch, reads a CRC out of hash[], and step 2 fails before the policy step is ever reached. Finding 4 (Low), the tests/CMakeLists.txt anchor shared with #80, is resolved on #80's side -- its block moved to the end of the registrations, so this one keeps its position and the two no longer collide. Verified: ctest 22/22 PASS test_secure_boot_policy 6/6 PASS discrimination: with step 7 reverted to `(void)eos_secure_boot_lock_debug();` test_secure_boot_refuses_when_the_debug_lock_cannot_be_taken FAILS the three original helper tests still PASS which is the gap the finding described, reproduced. Refs #82 --- tests/unit/test_secure_boot_policy.c | 119 ++++++++++++++++++++++++++- 1 file changed, 118 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_secure_boot_policy.c b/tests/unit/test_secure_boot_policy.c index ac93b08..70b924e 100644 --- a/tests/unit/test_secure_boot_policy.c +++ b/tests/unit/test_secure_boot_policy.c @@ -18,6 +18,8 @@ #include "eos_secure_boot.h" #include "eos_hal.h" +#include "eos_image.h" +#include "eos_crypto_boot.h" #include #include #include @@ -45,6 +47,9 @@ static int tests_passed = 0; /* ---- Simulated board: OTP only, with a scriptable write result ---- */ #define OTP_SIZE 0x400 +#define FLASH_BASE 0x08000000U +#define FLASH_SIZE 0x800 +static uint8_t sim_flash[FLASH_SIZE]; static uint8_t sim_otp[OTP_SIZE]; static int otp_write_rc; static int otp_write_calls; @@ -66,6 +71,52 @@ static int sim_otp_write(uint32_t offset, const void *buf, size_t len) return EOS_OK; } +static int sim_flash_read(uint32_t addr, void *buf, size_t len) +{ + if (addr < FLASH_BASE) return EOS_ERR_INVALID; + uint32_t off = addr - FLASH_BASE; + if ((uint64_t)off + len > FLASH_SIZE) return EOS_ERR_INVALID; + memcpy(buf, sim_flash + off, len); + return EOS_OK; +} + +/* An image that clears steps 1, 2 and 5, so control actually reaches the + * debug lock in step 7. Unsigned and unencrypted -- cfg turns those steps + * off -- because what is under test is the policy step, not the crypto. */ +static void stage_bootable_image(void) +{ + memset(sim_flash, 0, sizeof(sim_flash)); + + static const uint8_t payload[16] = { + 0xDE,0xAD,0xBE,0xEF,0x01,0x02,0x03,0x04, + 0x05,0x06,0x07,0x08,0x09,0x0A,0x0B,0x0C, + }; + + eos_image_header_t hdr; + memset(&hdr, 0, sizeof(hdr)); + hdr.magic = EOS_IMG_MAGIC; + hdr.hdr_version = EOS_IMAGE_HDR_VERSION; + hdr.hdr_size = (uint16_t)sizeof(eos_image_header_t); + hdr.image_size = (uint32_t)sizeof(payload); + hdr.load_addr = 0x20000000U; + hdr.entry_addr = 0x20000001U; + hdr.image_version = 1; + /* Without this flag verify_integrity takes the CRC32 branch and reads a + * CRC out of hash[], so step 2 fails and step 7 is never reached. */ + hdr.flags = EOS_IMG_FLAG_HASH_SHA256; + hdr.sig_type = EOS_SIG_NONE; + hdr.sig_len = 0; + hdr.tlv_len = 0; + + eos_sha256_ctx_t sha; + eos_sha256_init(&sha); + eos_sha256_update(&sha, payload, sizeof(payload)); + eos_sha256_final(&sha, hdr.hash); + + memcpy(sim_flash, &hdr, sizeof(hdr)); + memcpy(sim_flash + sizeof(hdr), payload, sizeof(payload)); +} + static eos_board_ops_t sim_ops; static void reset_fixture(void) @@ -73,6 +124,7 @@ static void reset_fixture(void) memset(&sim_ops, 0, sizeof(sim_ops)); memset(sim_otp, 0, sizeof(sim_otp)); sim_ops.otp_read = sim_otp_read; + sim_ops.flash_read = sim_flash_read; if (provide_otp_write) sim_ops.otp_write = sim_otp_write; otp_write_rc = EOS_OK; otp_write_calls = 0; @@ -113,6 +165,68 @@ TEST(test_lock_debug_reports_a_board_with_no_otp_write) ASSERT(otp_write_calls == 0); } +/* The three tests above assert what the *helper* reports. This one asserts + * what the boot *does* with that report, which is the behaviour this PR + * actually changed -- the early return at step 7. Without it the suite + * covers "attempted" while the file's own docblock claims "enforced", and a + * revert of the step-7 branch would leave every other test passing. */ +TEST(test_secure_boot_refuses_when_the_debug_lock_cannot_be_taken) +{ + provide_otp_write = 0; /* the common case: board has no otp_write */ + reset_fixture(); + stage_bootable_image(); + + eos_secure_boot_config_t cfg; + memset(&cfg, 0, sizeof(cfg)); + cfg.image_addr = FLASH_BASE; + cfg.require_signature = false; + cfg.require_encryption = false; + cfg.lock_debug = true; + + uint32_t entry = 0; + ASSERT(eos_secure_boot(&cfg, &entry) == EOS_SBOOT_ERR_POLICY); +} + +/* The counter-check: the same image and the same board, with lock_debug off, + * must still boot. Without this, the test above would also pass if steps 1-6 + * were failing for some unrelated reason and never reaching step 7. */ +TEST(test_the_same_image_boots_when_no_debug_lock_is_asked_for) +{ + provide_otp_write = 0; + reset_fixture(); + stage_bootable_image(); + + eos_secure_boot_config_t cfg; + memset(&cfg, 0, sizeof(cfg)); + cfg.image_addr = FLASH_BASE; + cfg.require_signature = false; + cfg.require_encryption = false; + cfg.lock_debug = false; + + uint32_t entry = 0; + ASSERT(eos_secure_boot(&cfg, &entry) == EOS_SBOOT_OK); + ASSERT(entry == 0x20000001U); +} + +/* And with a working fuse, lock_debug: true boots and the fuse is written. */ +TEST(test_secure_boot_proceeds_when_the_debug_lock_succeeds) +{ + provide_otp_write = 1; + reset_fixture(); + stage_bootable_image(); + + eos_secure_boot_config_t cfg; + memset(&cfg, 0, sizeof(cfg)); + cfg.image_addr = FLASH_BASE; + cfg.require_signature = false; + cfg.require_encryption = false; + cfg.lock_debug = true; + + uint32_t entry = 0; + ASSERT(eos_secure_boot(&cfg, &entry) == EOS_SBOOT_OK); + ASSERT(otp_write_calls == 1); +} + int main(void) { printf("=== eBootloader: Secure Boot Debug-Lock Policy Tests ===\n\n"); @@ -120,8 +234,11 @@ int main(void) run_test_lock_debug_reports_success_when_the_fuse_is_written(); run_test_lock_debug_reports_a_failed_fuse_write(); run_test_lock_debug_reports_a_board_with_no_otp_write(); + run_test_secure_boot_refuses_when_the_debug_lock_cannot_be_taken(); + run_test_the_same_image_boots_when_no_debug_lock_is_asked_for(); + run_test_secure_boot_proceeds_when_the_debug_lock_succeeds(); - tests_run = 3; + tests_run = 6; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } From 81bfe0b7641067f59d1d8c06c2e15785b3e6f49c Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 17:56:24 +0530 Subject: [PATCH 4/4] fix(secure-boot): step 4 must not report success for a check it never makes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Answers the second review on #82. Finding 1 (High) -- step 4, "Verify signing key against OTP root-of-trust", was the same fail-open this PR removes from step 7, two steps earlier. An `eos_hal_otp_read()` failure was discarded and boot continued; and when the read succeeded and the anchor was provisioned, the body of the `if` was `/* In a full implementation, extract key hash from TLV and compare */`. So a device whose root of trust *is* provisioned booted an image signed by any key the image carried, and step 8 recorded EOS_SBOOT_OK. Implementing the TLV comparison is out of scope, as the review says. The two things that are in scope are done: a non-EOS_OK otp_read now fails EOS_SBOOT_ERR_SIGNATURE, and a provisioned anchor that nothing compares against refuses the boot rather than proceeding. An unprovisioned board (all-zero anchor) is deliberately unchanged -- there is nothing to check against, and refusing would brick every board that has not been provisioned. The comment now states plainly that the step is planned rather than implemented, which §8.1 asks for and a comment inside an `if` was not. No test for those two refusals, deliberately, and the file says why rather than leaving it to be discovered. Reaching step 4 requires passing step 3 -- a real Ed25519 signature checked against the keystore. I wrote the obvious test first and it was worthless: with require_signature = true and an unsigned fixture the boot fails at step 3 and returns the same EOS_SBOOT_ERR_SIGNATURE step 4 returns, so it passed against the unfixed code too. I confirmed that by reverting step 4 and watching it still pass. A test that cannot fail is worse than none. What is there instead is the counter-check that the change does not refuse a boot it should allow. The fixture is buildable -- the keystore ships RFC 8032 TEST 1's public key and the matching private key is in the RFC -- but that machinery is #88's (tools/gen_signed_image_fixture.py). Worth doing once #88 lands. Finding 3 (Low) -- `TEST()` did not increment `tests_run` and `main()` hardcoded `tests_run = 6`, so a test added to the file but not wired into `main()` would have been skipped with a zero exit. #94 removed exactly this from tests/unit/test_ed25519.c, where a hardcoded 11 was masking two uncalled tests. Same fix here. Finding 4 (Low) -- the Valgrind `foreach` is hand-maintained and missed 6 of 22 registered tests. Added test_secure_boot_policy, test_fdt_loader and test_fw_decrypt. The list being hand-maintained at all is the real defect and is not fixed here -- it is the same class as the hardcoded count, one level up. Finding 2 (Medium), that eos_secure_boot() has no production caller, stands and is not addressed here; finding 1 makes it sharper rather than resolving it. Finding 5 (Low) is a PR-body correction. Verified: ctest 22/22 PASS test_secure_boot_policy 7/7 PASS step 7 discrimination, still: reverting the step-7 branch fails test_secure_boot_refuses_when_the_debug_lock_cannot_be_taken Refs #82 --- core/secure_boot.c | 43 +++++++++++++++++++++------- tests/CMakeLists.txt | 3 +- tests/unit/test_secure_boot_policy.c | 41 +++++++++++++++++++++++++- 3 files changed, 75 insertions(+), 12 deletions(-) diff --git a/core/secure_boot.c b/core/secure_boot.c index ab677c6..4d80cd3 100644 --- a/core/secure_boot.c +++ b/core/secure_boot.c @@ -102,18 +102,41 @@ eos_secure_boot_result_t eos_secure_boot(const eos_secure_boot_config_t *cfg, } /* ---- Step 4: Verify signing key against OTP root-of-trust ---- */ - /* Extract key hash from TLV area */ - /* The TLV_KEYHASH entry contains SHA-256 of the public key */ + /* + * This step is PLANNED, not implemented: comparing the image's + * TLV_KEYHASH against the OTP anchor needs TLV extraction that does + * not exist here yet. What it must not do in the meantime is report + * success. + * + * It used to. An otp_read failure was discarded and boot continued, + * and when the read succeeded and the anchor was provisioned the body + * of the `if` was a comment -- so a device whose root of trust *is* + * provisioned booted an image signed by any key the image carried, + * and step 8 recorded EOS_SBOOT_OK. That is the same fail-open this + * PR removes from step 7, two steps earlier. + * + * Until the comparison exists, a provisioned device fails closed. An + * unprovisioned one (all-zero anchor) is unchanged: there is nothing + * to check against, and refusing would brick every un-provisioned + * board. + */ uint8_t otp_key_hash[32]; rc = eos_hal_otp_read(OTP_KEY_HASH_OFFSET, otp_key_hash, OTP_KEY_HASH_SIZE); - if (rc == EOS_OK) { - /* Check if OTP key hash is provisioned (not all-zeros) */ - uint8_t zeros[32] = {0}; - if (secure_compare(otp_key_hash, zeros, 32) != 0) { - /* OTP is provisioned — must match */ - /* In a full implementation, extract key hash from TLV and compare */ - /* For now, the signature verification implicitly uses the embedded key */ - } + if (rc != EOS_OK) { + /* The anchor could not be read, so it cannot be checked. A + * verification step that cannot run must fail, not pass. */ + attest_record(2, hdr.image_version, hdr.hash, NULL, + EOS_SBOOT_ERR_SIGNATURE); + return EOS_SBOOT_ERR_SIGNATURE; + } + + uint8_t zeros[32] = {0}; + if (secure_compare(otp_key_hash, zeros, 32) != 0) { + /* Provisioned, and nothing here compares against it. Refuse + * rather than boot on a key this function has not checked. */ + attest_record(2, hdr.image_version, hdr.hash, NULL, + EOS_SBOOT_ERR_SIGNATURE); + return EOS_SBOOT_ERR_SIGNATURE; } } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ee17487..a924c9a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -124,7 +124,8 @@ if(VALGRIND) test_multicore test_board_registry test_slot_manager test_boot_log test_image_verify test_image_abi test_recovery test_slot_size_bounds test_fw_transport - test_tlv_auth) + test_tlv_auth test_secure_boot_policy test_fdt_loader + test_fw_decrypt) add_test( NAME valgrind_${TEST_NAME} COMMAND ${VALGRIND} ${VALGRIND_OPTS} $ diff --git a/tests/unit/test_secure_boot_policy.c b/tests/unit/test_secure_boot_policy.c index 70b924e..be092d5 100644 --- a/tests/unit/test_secure_boot_policy.c +++ b/tests/unit/test_secure_boot_policy.c @@ -31,6 +31,7 @@ static int tests_passed = 0; static void name(void); \ static void run_##name(void) { \ printf(" %-54s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -47,6 +48,8 @@ static int tests_passed = 0; /* ---- Simulated board: OTP only, with a scriptable write result ---- */ #define OTP_SIZE 0x400 +/* Mirrors core/secure_boot.c; the anchor lives at a fixed OTP offset. */ +#define OTP_KEY_HASH_OFFSET 0x100 #define FLASH_BASE 0x08000000U #define FLASH_SIZE 0x800 static uint8_t sim_flash[FLASH_SIZE]; @@ -227,6 +230,42 @@ TEST(test_secure_boot_proceeds_when_the_debug_lock_succeeds) ASSERT(otp_write_calls == 1); } +/* Step 4's two new refusals have NO test here, deliberately, and this note is + * the record of why rather than an omission to be discovered later. + * + * Reaching step 4 requires passing step 3, which is a real Ed25519 signature + * over the header prefix checked against the keystore anchor. A fixture for + * that is buildable -- the keystore ships RFC 8032 TEST 1's public key and + * the matching private key is in the RFC -- but the machinery for it belongs + * to #88 (tools/gen_signed_image_fixture.py), not here. + * + * I wrote the obvious test first and it was worthless: with + * require_signature = true and an unsigned fixture the boot fails at step 3, + * returning the same EOS_SBOOT_ERR_SIGNATURE that step 4 returns, so it + * passed against the unfixed code too. Verified that by reverting step 4 and + * watching it still pass. A test that cannot fail is worse than none, so it + * is not in this file. + * + * The counter-check below is what this file can honestly assert: the change + * does not refuse a boot it should allow. + */ +TEST(test_the_ordinary_boot_path_is_unaffected_by_the_step_4_change) +{ + provide_otp_write = 1; + reset_fixture(); /* leaves the OTP anchor all zero */ + stage_bootable_image(); + + eos_secure_boot_config_t cfg; + memset(&cfg, 0, sizeof(cfg)); + cfg.image_addr = FLASH_BASE; + cfg.require_signature = false; + cfg.lock_debug = false; + + uint32_t entry = 0; + ASSERT(eos_secure_boot(&cfg, &entry) == EOS_SBOOT_OK); + ASSERT(entry == 0x20000001U); +} + int main(void) { printf("=== eBootloader: Secure Boot Debug-Lock Policy Tests ===\n\n"); @@ -237,8 +276,8 @@ int main(void) run_test_secure_boot_refuses_when_the_debug_lock_cannot_be_taken(); run_test_the_same_image_boots_when_no_debug_lock_is_asked_for(); run_test_secure_boot_proceeds_when_the_debug_lock_succeeds(); + run_test_the_ordinary_boot_path_is_unaffected_by_the_step_4_change(); - tests_run = 6; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; }