fix(fw_decrypt): drop a HW-crypto shortcut that cannot express streaming GCM

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];

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"
@@ -192,16 +195,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

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);

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");
 

tests/CMakeLists.txt

@@ -110,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)

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;
 }