fix(ed25519): reject public keys outside the prime-order subgroup

core/ed25519_verify.c

@@ -276,6 +276,55 @@ static void scalarmult(gf r[4], gf q[4], const uint8_t *s)
     }
 }
 
+/* The identity encodes as y = 1 with the sign bit clear. Takes a point rather
+ * than an encoding so both callers below can pass one directly. */
+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;
+}
+
+/* Reject a public key outside the prime-order subgroup.
+ *
+ * Decoding a point is not enough. Ed25519 has eight points of low order, and
+ * for any of them the verification equation can hold regardless of the
+ * message: an all-zero key with an all-zero signature verified against any
+ * content at all, which is not a weak signature but no signature.
+ *
+ * Two conditions, because neither alone is sufficient. [L]A = identity holds
+ * for every point whose order divides L -- including the identity itself,
+ * whose order is 1 -- so the identity must also be excluded explicitly.
+ *
+ * The scalar is derived from ORDER_L rather than written out a second time,
+ * 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.
+ */
+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];
@@ -417,6 +466,10 @@ int eos_ed25519_verify(const uint8_t signature[64],
     if (unpackneg(A, public_key) != EOS_OK)
         return EOS_ERR_SIGNATURE;
 
+    /* On the curve is not enough: a low-order key verifies anything. */
+    if (!key_has_prime_order(A))
+        return EOS_ERR_SIGNATURE;
+
     /* k = SHA-512(R || A || M) mod L */
     eos_sha512_ctx_t ctx;
     uint8_t k[64];

tests/unit/test_ed25519.c

@@ -219,8 +219,126 @@ TEST(test_ed25519_zero_signature_rejected)
     ASSERT(eos_ed25519_verify(sig, pk, msg, 1) != EOS_OK);
 }
 
+/* The eight low-order point encodings. A public key must be none of them, and
+ * so must the R half of a signature. */
+/* Messages the low-order sweeps run each candidate pair against. Which pair
+ * forges depends on k = SHA-512(R || A || M), so the message matters: the
+ * count below is for exactly this list. */
+static const char *const messages[] = {
+    "untrusted firmware", "malicious package payload",
+    "AB", "ABC", "boot this image", "", "A", "0123456789",
+};
+
+static const uint8_t k_low_order[8][32] = {
+    /* y = 0, order 4 */
+    {0},
+    /* the identity, order 1 */
+    {1},
+    /* 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},
+    /* p - 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},
+    /* p, which reduces to y = 0 */
+    {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},
+    /* p + 1, which reduces to the identity */
+    {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},
+    /* non-canonical, above p */
+    {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},
+};
+
+TEST(test_ed25519_low_order_forgeries_rejected)
+{
+    /* All 64 combinations of the eight encodings as the public key and as R,
+     * with S = 0, against several messages.
+     *
+     * The sweep is not thoroughness for its own sake. Which combination
+     * forges depends on k = SHA-512(R || A || M) mod L, so for the order-4
+     * and order-8 points it depends on the *message*: roughly one message in
+     * four takes for an order-4 key. A test pinned to one pair and one
+     * message can therefore pass against unfixed code and look like a
+     * regression test without being one.
+     *
+     * That is not hypothetical. Measured against this file's parent commit,
+     * 16 of these 64 pairs are accepted for at least one of the messages
+     * below -- and the pair (identity key, R = 0) is not among them, so an
+     * identity test written with an all-zero signature passes on the unfixed
+     * code and proves nothing. The pair that does forge with an identity key
+     * is R = identity, not R = 0.
+     */
+    unsigned k, r, m;
+
+    for (k = 0; k < 8; k++) {
+        for (r = 0; r < 8; r++) {
+            uint8_t sig[64];
+            memset(sig, 0, sizeof(sig));
+            memcpy(sig, k_low_order[r], 32);   /* R = low order, S = 0 */
+
+            for (m = 0; m < sizeof(messages) / sizeof(messages[0]); m++) {
+                ASSERT(eos_ed25519_verify(sig, k_low_order[k],
+                                          (const uint8_t *)messages[m],
+                                          strlen(messages[m])) != EOS_OK);
+            }
+        }
+    }
+}
+
+TEST(test_ed25519_low_order_key_with_real_signature_rejected)
+{
+    /* A low-order key paired with a genuine signature over a real message,
+     * rather than with S = 0. The key alone is disqualifying: the subgroup
+     * test must not depend on the signature being degenerate too. */
+    uint8_t sig[64], msg[1];
+    unsigned k;
+
+    hex2bin(k_vectors[1].sig_hex, sig, 64);
+    hex2bin(k_vectors[1].msg_hex, msg, 1);
+
+    for (k = 0; k < 8; k++)
+        ASSERT(eos_ed25519_verify(sig, k_low_order[k], 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
+     * deliberate rather than an omission. Verification computes
+     * [S]B = R + [k]A: a low-order A collapses the equation for any message,
+     * which is the bypass. A low-order R with a genuine A does not, because
+     * k = SHA-512(R || A || M) depends on R, so an attacker choosing R would
+     * have to solve for it.
+     *
+     * Measured rather than argued: all eight low-order encodings as R with
+     * S = 0, against RFC 8032 test 2's real public key, are rejected on the
+     * unfixed verifier too -- 0 of 8, before and after. Pinned here so that
+     * a later "R should be checked as well" change is recognised as scope
+     * creep, and so that removing the key check because "R is the untrusted
+     * half" fails loudly.
+     */
+    uint8_t pk[32], sig[64];
+    size_t r, m;
+
+    hex2bin(k_vectors[1].pubkey_hex, pk, 32);
+
+    for (r = 0; r < sizeof(k_low_order) / sizeof(k_low_order[0]); r++) {
+        for (m = 0; m < sizeof(messages) / sizeof(messages[0]); m++) {
+            memset(sig, 0, sizeof(sig));
+            memcpy(sig, k_low_order[r], 32);
+            ASSERT(eos_ed25519_verify(sig, pk,
+                                      (const uint8_t *)messages[m],
+                                      strlen(messages[m])) != EOS_OK);
+        }
+    }
+}
+
 TEST(test_sha512_known_answers)
 {
     struct { const char *msg; const char *digest; } kat[] = {
@@ -281,10 +399,13 @@ int main(void)
     run_test_ed25519_null_args();
     run_test_ed25519_zero_pubkey_rejected();
     run_test_ed25519_zero_signature_rejected();
+    run_test_ed25519_low_order_forgeries_rejected();
+    run_test_ed25519_low_order_key_with_real_signature_rejected();
+    run_test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery();
     run_test_sha512_known_answers();
     run_test_sha512_streaming_matches_one_shot();
 
-    tests_run = 10;
+    tests_run = 13;
     printf("\n%d/%d tests passed\n", tests_passed, tests_run);
     return (tests_passed == tests_run) ? 0 : 1;
 }