From f704d87fd445e4d61db00bc302e78660fc99f02f Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 15:06:59 +0530 Subject: [PATCH 1/3] =?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 fb3afdb127fcbc417562135d78287bac035bb259 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Tue, 1 Sep 2026 10:31:13 +0530 Subject: [PATCH 2/3] fix(fw_decrypt): drop a HW-crypto shortcut that cannot express streaming GCM core/fw_decrypt.c is a hand-written AES-256-GCM sitting in the secure boot path with no tests. It has an opt-in fast path: if (ops && ops->hw_aes_decrypt) { int rc = ops->hw_aes_decrypt(ctx->key, EOS_AES_KEY_SIZE, ctx->iv, data, data, len); if (rc == EOS_OK) { ctx->bytes_processed += len; return EOS_OK; } } The hook is (key, key_len, iv, in, out, len). That signature cannot carry streaming GCM, and taking the path broke decryption two ways: 1. No counter position. ctx->iv is passed unchanged on every call, so a second chunk restarts the CTR keystream at block 0 and is decrypted against the same keystream as the first. Reusing a CTR keystream across two plaintexts is the one thing the mode must never do. 2. No GHASH. The hook returns plaintext only, so ctx->ghash_acc is never fed. eos_fw_decrypt_final() computes the tag over an empty accumulator and rejects the image -- a board with an AES engine could not install a correctly encrypted firmware update at all. (2) is why this was never noticed: it fails closed, and no board in-tree implements the hook yet. (1) is why it cannot be patched by also feeding GHASH: the plaintext would still be wrong past the first chunk. Re-enabling needs a hook that takes a block offset and either exposes GHASH state or does the whole GCM operation including the tag. Removed, with that written down where the next person will look. Also adds tests/unit/test_fw_decrypt.c -- the first tests this file has had. Vectors come from an independent implementation (Python cryptography, i.e. OpenSSL) rather than from this code, so they pin behaviour rather than recording it: - whole blocks, and a 20-byte payload with a partial trailing block - the same ciphertext split [32] / [16,16] / [10,22] / [1,31] / [5,15]: GCM is a stream, so the result must not depend on how the caller sliced it - a board advertising a working AES engine must reach the same answer as one without -- this is the case that fails on master - every single-bit flip in the tag (128 of them), and in each ciphertext byte, must be rejected - unprovisioned/unreadable OTP keys and uninitialised contexts are refused Verified: 8/8 pass with this change; against the unmodified fw_decrypt.c the suite fails on test_board_with_aes_engine_still_accepts_a_genuine_image, the assertion it exists to make. The software GCM itself is correct -- I checked it against the reference vectors before changing anything, including every chunk split above. Note: the full test build on master is currently broken by test_image_verify.c (fixed in #77), so this target was built directly. Co-Authored-By: Claude Opus 5 (1M context) --- core/fw_decrypt.c | 36 +++-- tests/CMakeLists.txt | 5 + tests/unit/test_fw_decrypt.c | 271 +++++++++++++++++++++++++++++++++++ 3 files changed, 302 insertions(+), 10 deletions(-) create mode 100644 tests/unit/test_fw_decrypt.c diff --git a/core/fw_decrypt.c b/core/fw_decrypt.c index da09321..c61857c 100644 --- a/core/fw_decrypt.c +++ b/core/fw_decrypt.c @@ -192,16 +192,32 @@ int eos_fw_decrypt_update(eos_fw_decrypt_ctx_t *ctx, uint8_t *data, size_t len) if (!ctx || !data) return EOS_ERR_INVALID; if (!ctx->initialized) return EOS_ERR_INVALID; - /* Try HW-accelerated decryption via HAL */ - const eos_board_ops_t *ops = eos_hal_get_ops(); - if (ops && ops->hw_aes_decrypt) { - int rc = ops->hw_aes_decrypt(ctx->key, EOS_AES_KEY_SIZE, - ctx->iv, data, data, len); - if (rc == EOS_OK) { - ctx->bytes_processed += (uint32_t)len; - return EOS_OK; - } - } + /* No HW-accelerated shortcut here, deliberately. + * + * eos_board_ops_t::hw_aes_decrypt is (key, key_len, iv, in, out, len). + * That signature cannot express streaming GCM, and taking it broke this + * function two ways: + * + * 1. No counter position. Every call passed ctx->iv unchanged, so a + * second chunk restarted the CTR keystream at block 0 and decrypted + * against the same keystream as the first -- the one thing CTR mode + * must never do. + * 2. No GHASH. The hook returns plaintext only, so ctx->ghash_acc was + * never fed. eos_fw_decrypt_final() then computed the tag over an + * empty accumulator and rejected the image, so a board with an AES + * engine could not install a correctly encrypted update at all. + * + * The second is why this was never noticed: it fails closed, and no + * board in-tree implements the hook yet. The first is why it cannot be + * patched by also feeding GHASH -- the plaintext would still be wrong + * past the first chunk. + * + * Re-enabling this needs a hook that takes a block offset and either + * exposes the GHASH state or performs the whole GCM operation including + * the tag. Until then the software path below is the only correct one; + * it is checked against reference vectors in + * tests/unit/test_fw_decrypt.c. + */ /* Software AES-256-CTR decryption. * AES-256 in CTR mode: encrypt the counter block with AES, then XOR diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b394f24..e388a2a 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_fw_decrypt: Streaming AES-256-GCM firmware decryption --- +add_executable(eboot_test_fw_decrypt unit/test_fw_decrypt.c) +target_link_libraries(eboot_test_fw_decrypt PRIVATE eboot_core) +add_test(NAME test_fw_decrypt COMMAND eboot_test_fw_decrypt) + # --- 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_fw_decrypt.c b/tests/unit/test_fw_decrypt.c new file mode 100644 index 0000000..ca3c358 --- /dev/null +++ b/tests/unit/test_fw_decrypt.c @@ -0,0 +1,271 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 EoS Project +// ISO/IEC 25000 | ISO/IEC/IEEE 15288:2023 + +/** + * @file test_fw_decrypt.c + * @brief Unit tests for streaming AES-256-GCM firmware decryption + * + * core/fw_decrypt.c is a hand-written AES-256-GCM in the secure boot path and + * had no tests at all. These pin it against reference vectors produced by an + * independent implementation (Python `cryptography`, i.e. OpenSSL), for the + * cases a streaming decryptor actually meets: whole blocks, a partial tail, + * and callers that split the stream on boundaries that are not multiples of + * 16 -- GCM is a stream, so the result must not depend on how it was sliced. + */ + +#include "eos_fw_decrypt.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(" %-52s ", #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) + +/* ---- Reference vectors (AES-256-GCM, 96-bit IV, no AAD) ---- */ +static const uint8_t vec_key[32] = {0x00,0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08,0x09,0x0a,0x0b,0x0c,0x0d,0x0e,0x0f,0x10,0x11,0x12,0x13,0x14,0x15,0x16,0x17,0x18,0x19,0x1a,0x1b,0x1c,0x1d,0x1e,0x1f}; +static const uint8_t vec_iv[12] = {0x00,0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08,0x09,0x0a,0x0b}; +/* v16: 16 bytes */ +static const uint8_t v16_ct[] = {0x47,0x03,0xd4,0x18,0xc1,0xe0,0xc4,0x1c,0x85,0x48,0x9d,0x80,0xbd,0xe4,0x76,0x62}; +static const uint8_t v16_pt[] = {0x00,0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08,0x09,0x0a,0x0b,0x0c,0x0d,0x0e,0x0f}; +static const uint8_t v16_tag[] = {0xed,0x39,0x55,0x08,0x27,0x6f,0xf6,0x60,0x85,0x0d,0x12,0xd3,0xe7,0x55,0xeb,0xa5}; +/* v32: 32 bytes */ +static const uint8_t v32_ct[] = {0x47,0x03,0xd4,0x18,0xc1,0xe0,0xc4,0x1c,0x85,0x48,0x9d,0x80,0xbd,0xe4,0x76,0x62,0x93,0xc7,0x95,0x27,0xe4,0x6e,0x49,0x6b,0x20,0x7e,0xff,0x9e,0x01,0x74,0x1e,0xad}; +static const uint8_t v32_pt[] = {0x00,0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08,0x09,0x0a,0x0b,0x0c,0x0d,0x0e,0x0f,0x10,0x11,0x12,0x13,0x14,0x15,0x16,0x17,0x18,0x19,0x1a,0x1b,0x1c,0x1d,0x1e,0x1f}; +static const uint8_t v32_tag[] = {0x5e,0xdd,0xdc,0x50,0x74,0x04,0x4e,0x22,0x82,0xb4,0x32,0xb3,0xf2,0xd8,0xf6,0x73}; +/* v20: 20 bytes */ +static const uint8_t v20_ct[] = {0x47,0x03,0xd4,0x18,0xc1,0xe0,0xc4,0x1c,0x85,0x48,0x9d,0x80,0xbd,0xe4,0x76,0x62,0x93,0xc7,0x95,0x27}; +static const uint8_t v20_pt[] = {0x00,0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08,0x09,0x0a,0x0b,0x0c,0x0d,0x0e,0x0f,0x10,0x11,0x12,0x13}; +static const uint8_t v20_tag[] = {0x47,0x60,0x36,0x9f,0x8e,0x14,0x73,0x21,0x58,0x5e,0x06,0x47,0x37,0x46,0x3f,0x95}; + +/* ---- Simulated board ---- + * Only OTP is provided: eos_fw_decrypt_init() reads the key from it. */ + +#define OTP_FW_KEY_OFFSET 0x200 + +static int otp_rc = EOS_OK; +static uint8_t otp_key[EOS_AES_KEY_SIZE]; + +static int sim_otp_read(uint32_t offset, void *buf, size_t len) +{ + if (otp_rc != EOS_OK) return otp_rc; + if (offset != OTP_FW_KEY_OFFSET || len != EOS_AES_KEY_SIZE) + return EOS_ERR_INVALID; + memcpy(buf, otp_key, EOS_AES_KEY_SIZE); + return EOS_OK; +} + +/* A board whose AES engine works: it returns the correct plaintext and reports + * success. That matters -- an engine that returns an error is not exercising + * anything, because the caller just falls through to the software path. The + * bug this pins only appears when the engine SUCCEEDS: the old code then + * returned early, having skipped GHASH entirely, and eos_fw_decrypt_final() + * refused a genuine image. + * + * Only the reference vectors are ever passed here, so returning their known + * plaintext is a faithful stand-in for a working engine. */ +static int sim_hw_aes_decrypt(const void *key, size_t key_len, const void *iv, + const void *in, void *out, size_t len) +{ + (void)key; (void)key_len; (void)iv; (void)in; + + if (len == sizeof v32_pt) memcpy(out, v32_pt, len); + else if (len == sizeof v16_pt) memcpy(out, v16_pt, len); + else if (len == sizeof v20_pt) memcpy(out, v20_pt, len); + else return EOS_ERR_NOT_SUPPORTED; + + return EOS_OK; +} + +static eos_board_ops_t sim_ops; + +static void reset_fixture(int with_aes_engine) +{ + memset(&sim_ops, 0, sizeof(sim_ops)); + sim_ops.otp_read = sim_otp_read; + if (with_aes_engine) sim_ops.hw_aes_decrypt = sim_hw_aes_decrypt; + memcpy(otp_key, vec_key, sizeof(otp_key)); + otp_rc = EOS_OK; + eos_hal_init(&sim_ops); +} + +/* Decrypt `ct` in `chunk`-sized calls and check plaintext and tag. */ +static void decrypt_and_check(const uint8_t *ct, size_t ctlen, + const uint8_t *pt, const uint8_t *tag, + const size_t *splits, int nsplits) +{ + uint8_t buf[64]; + ASSERT(ctlen <= sizeof(buf)); + memcpy(buf, ct, ctlen); + + eos_fw_decrypt_ctx_t ctx; + ASSERT(eos_fw_decrypt_init(&ctx, vec_iv) == EOS_OK); + + size_t off = 0; + for (int i = 0; i < nsplits && off < ctlen; i++) { + size_t n = splits[i]; + if (off + n > ctlen) n = ctlen - off; + ASSERT(eos_fw_decrypt_update(&ctx, buf + off, n) == EOS_OK); + off += n; + } + ASSERT(off == ctlen); + + ASSERT(memcmp(buf, pt, ctlen) == 0); + ASSERT(eos_fw_decrypt_final(&ctx, tag) == EOS_OK); +} + +TEST(test_single_block_matches_reference) +{ + reset_fixture(0); + const size_t one[] = { 64 }; + decrypt_and_check(v16_ct, sizeof v16_ct, v16_pt, v16_tag, one, 1); +} + +TEST(test_partial_trailing_block_matches_reference) +{ + reset_fixture(0); + const size_t one[] = { 64 }; + decrypt_and_check(v20_ct, sizeof v20_ct, v20_pt, v20_tag, one, 1); +} + +/* GCM is a stream. Feeding the same ciphertext in different chunk sizes must + * give the same plaintext and the same tag, including splits that land inside + * a block. */ +TEST(test_result_is_independent_of_chunk_boundaries) +{ + reset_fixture(0); + const size_t whole[] = { 32 }; + const size_t aligned[] = { 16, 16 }; + const size_t inside[] = { 10, 22 }; + const size_t byte1[] = { 1, 31 }; + decrypt_and_check(v32_ct, sizeof v32_ct, v32_pt, v32_tag, whole, 1); + decrypt_and_check(v32_ct, sizeof v32_ct, v32_pt, v32_tag, aligned, 2); + decrypt_and_check(v32_ct, sizeof v32_ct, v32_pt, v32_tag, inside, 2); + decrypt_and_check(v32_ct, sizeof v32_ct, v32_pt, v32_tag, byte1, 2); + + const size_t tail[] = { 5, 15 }; + decrypt_and_check(v20_ct, sizeof v20_ct, v20_pt, v20_tag, tail, 2); +} + +/* A board advertising an AES engine must get the same answer as one without. + * The HW shortcut skipped GHASH, so eos_fw_decrypt_final() computed the tag + * over an empty accumulator and refused a correctly encrypted image. */ +TEST(test_board_with_aes_engine_still_accepts_a_genuine_image) +{ + reset_fixture(1); + const size_t one[] = { 64 }; + decrypt_and_check(v32_ct, sizeof v32_ct, v32_pt, v32_tag, one, 1); + decrypt_and_check(v16_ct, sizeof v16_ct, v16_pt, v16_tag, one, 1); + decrypt_and_check(v20_ct, sizeof v20_ct, v20_pt, v20_tag, one, 1); +} + +TEST(test_tampered_tag_is_rejected) +{ + reset_fixture(0); + uint8_t buf[32]; + eos_fw_decrypt_ctx_t ctx; + + /* Every single-bit flip in the tag must be caught. */ + for (int bit = 0; bit < 8 * (int)EOS_AES_TAG_SIZE; bit++) { + uint8_t bad[EOS_AES_TAG_SIZE]; + memcpy(bad, v32_tag, sizeof(bad)); + bad[bit / 8] ^= (uint8_t)(1u << (bit % 8)); + + memcpy(buf, v32_ct, sizeof(buf)); + ASSERT(eos_fw_decrypt_init(&ctx, vec_iv) == EOS_OK); + ASSERT(eos_fw_decrypt_update(&ctx, buf, sizeof(buf)) == EOS_OK); + ASSERT(eos_fw_decrypt_final(&ctx, bad) != EOS_OK); + } + + uint8_t zeros[EOS_AES_TAG_SIZE] = {0}; + memcpy(buf, v32_ct, sizeof(buf)); + ASSERT(eos_fw_decrypt_init(&ctx, vec_iv) == EOS_OK); + ASSERT(eos_fw_decrypt_update(&ctx, buf, sizeof(buf)) == EOS_OK); + ASSERT(eos_fw_decrypt_final(&ctx, zeros) != EOS_OK); +} + +/* Tampering with the ciphertext must change the tag. */ +TEST(test_tampered_ciphertext_is_rejected) +{ + reset_fixture(0); + for (size_t i = 0; i < sizeof v32_ct; i++) { + uint8_t buf[32]; + memcpy(buf, v32_ct, sizeof(buf)); + buf[i] ^= 0x01; + + eos_fw_decrypt_ctx_t ctx; + ASSERT(eos_fw_decrypt_init(&ctx, vec_iv) == EOS_OK); + ASSERT(eos_fw_decrypt_update(&ctx, buf, sizeof(buf)) == EOS_OK); + ASSERT(eos_fw_decrypt_final(&ctx, v32_tag) != EOS_OK); + } +} + +TEST(test_init_rejects_bad_arguments_and_unprovisioned_keys) +{ + reset_fixture(0); + eos_fw_decrypt_ctx_t ctx; + + ASSERT(eos_fw_decrypt_init(NULL, vec_iv) != EOS_OK); + ASSERT(eos_fw_decrypt_init(&ctx, NULL) != EOS_OK); + + /* An all-zero OTP word means the key was never provisioned. */ + memset(otp_key, 0, sizeof(otp_key)); + ASSERT(eos_fw_decrypt_init(&ctx, vec_iv) != EOS_OK); + + /* An OTP that cannot be read is not a reason to decrypt with garbage. */ + memcpy(otp_key, vec_key, sizeof(otp_key)); + otp_rc = EOS_ERR_FLASH; + ASSERT(eos_fw_decrypt_init(&ctx, vec_iv) != EOS_OK); + otp_rc = EOS_OK; +} + +TEST(test_update_and_final_reject_uninitialised_contexts) +{ + reset_fixture(0); + eos_fw_decrypt_ctx_t ctx; + memset(&ctx, 0, sizeof(ctx)); /* initialized == false */ + + uint8_t buf[16] = {0}; + ASSERT(eos_fw_decrypt_update(&ctx, buf, sizeof(buf)) != EOS_OK); + ASSERT(eos_fw_decrypt_final(&ctx, v16_tag) != EOS_OK); + + ASSERT(eos_fw_decrypt_init(&ctx, vec_iv) == EOS_OK); + ASSERT(eos_fw_decrypt_update(&ctx, NULL, 16) != EOS_OK); + ASSERT(eos_fw_decrypt_final(&ctx, NULL) != EOS_OK); +} + +int main(void) +{ + printf("=== eBootloader: Firmware Decryption (AES-256-GCM) Tests ===\n\n"); + + run_test_single_block_matches_reference(); + run_test_partial_trailing_block_matches_reference(); + run_test_result_is_independent_of_chunk_boundaries(); + run_test_board_with_aes_engine_still_accepts_a_genuine_image(); + run_test_tampered_tag_is_rejected(); + run_test_tampered_ciphertext_is_rejected(); + run_test_init_rejects_bad_arguments_and_unprovisioned_keys(); + run_test_update_and_final_reject_uninitialised_contexts(); + + tests_run = 8; + printf("\n%d/%d tests passed\n", tests_passed, tests_run); + return (tests_passed == tests_run) ? 0 : 1; +} From 04b7b74c75ed8a558025e903e5a7c3e849afd59c Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 15:10:16 +0530 Subject: [PATCH 3/3] =?UTF-8?q?fix(fw=5Fdecrypt):=20answer=20the=20review?= =?UTF-8?q?=20=E2=80=94=20stale=20doc,=20orphaned=20hook,=20anchor=20clash?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the review on #80, all in files this branch already owns. Finding 2 (Medium) -- core/fw_decrypt.c's file header still read "Falls back to HAL hw_aes_decrypt if available." The diff started at line 192, so that line survived and directly contradicted the twenty-line rationale this PR installs below it. A self-contradictory file is worse than the original. Finding 1 (Medium) -- after the removal, hw_aes_decrypt has zero call sites while eos_hal.h still advertises it under "software fallback used if NULL", which is now false in the other direction: a board author who implements the hook gets nothing, silently, with no diagnostic. That is the same class of failure this PR is fixing. Kept the member rather than deleting it -- removing it from a public struct is an ABI change for out-of-tree boards, and the hook is worth having once it can express the operation -- and documented it as reserved and currently unconsumed, with the reason and with what a streaming-capable replacement would need. Finding 3 (Low) -- this PR and #82 both inserted their add_executable/add_test triple immediately after add_test(NAME test_keystore ...), so whichever landed second would have conflicted for no reason but placement. Re-anchored this one to the end of the registrations, with a comment saying why. Also rebased: this branch was 7 commits behind and its diff against current master would have reverted the test_recovery link-line fix. It is now stacked on #94, which repairs master -- without that, every PR that builds the test suite is red on include/eos_image.h and core/ed25519_verify.c. Verified: cmake --build (EBLDR_BUILD_TESTS=ON) clean ctest 22/22 PASS test_fw_decrypt 8/8 PASS git grep hw_aes_decrypt header + this file's comment only Refs #80 --- core/fw_decrypt.c | 5 ++++- include/eos_hal.h | 15 +++++++++++++++ tests/CMakeLists.txt | 13 ++++++++----- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/core/fw_decrypt.c b/core/fw_decrypt.c index c61857c..865f322 100644 --- a/core/fw_decrypt.c +++ b/core/fw_decrypt.c @@ -8,7 +8,10 @@ * * Provides decrypt-in-place for encrypted firmware updates. * Decryption key is retrieved from OTP/eFuse via the HAL. - * Falls back to HAL hw_aes_decrypt if available. + * + * Software-only. The HAL's hw_aes_decrypt hook is deliberately not used -- + * see the rationale above eos_fw_decrypt_update() for why its one-shot + * signature cannot express a streaming AEAD. */ #include "eos_fw_decrypt.h" diff --git a/include/eos_hal.h b/include/eos_hal.h index 156fc44..46f2998 100644 --- a/include/eos_hal.h +++ b/include/eos_hal.h @@ -89,6 +89,21 @@ typedef struct { /* HW-accelerated crypto (optional, software fallback used if NULL) */ int (*hw_sha256)(const void *data, size_t len, void *digest); + + /* Reserved, and currently consumed by nothing. + * + * This signature cannot express a streaming AEAD: it carries no counter + * position, so every chunk restarts the CTR keystream at block 0, and it + * returns plaintext only, so there is no way to feed GHASH. core/ + * fw_decrypt.c used to call it and produced both keystream reuse and a + * tag computed over an empty accumulator; the call site was removed + * rather than patched, because feeding GHASH alone still leaves the + * plaintext wrong past the first chunk. + * + * A board that implements this hook today gets nothing, silently. Do not + * add a caller: define a streaming-capable contract first (init/update/ + * final, or an explicit block offset plus an AAD/tag path), which is a + * change to this struct rather than to its consumers. */ int (*hw_aes_decrypt)(const void *key, size_t key_len, const void *iv, const void *in, void *out, size_t len); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index e388a2a..ac11778 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -90,11 +90,6 @@ 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_fw_decrypt: Streaming AES-256-GCM firmware decryption --- -add_executable(eboot_test_fw_decrypt unit/test_fw_decrypt.c) -target_link_libraries(eboot_test_fw_decrypt PRIVATE eboot_core) -add_test(NAME test_fw_decrypt COMMAND eboot_test_fw_decrypt) - # --- test_rollback: Anti-rollback security counter --- add_executable(eboot_test_rollback unit/test_rollback.c) target_link_libraries(eboot_test_rollback PRIVATE eboot_core) @@ -115,6 +110,14 @@ 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_fw_decrypt: Streaming AES-256-GCM firmware decryption --- +# Anchored here rather than after test_keystore: #82 inserts its own +# triple at that anchor, and whichever of the two landed second would have +# conflicted for no reason other than where the block was placed. +add_executable(eboot_test_fw_decrypt unit/test_fw_decrypt.c) +target_link_libraries(eboot_test_fw_decrypt PRIVATE eboot_core) +add_test(NAME test_fw_decrypt COMMAND eboot_test_fw_decrypt) + # --- Valgrind test targets --- find_program(VALGRIND valgrind) if(VALGRIND)