diff --git a/core/ed25519_verify.c b/core/ed25519_verify.c index 022b679..5040122 100644 --- a/core/ed25519_verify.c +++ b/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]; diff --git a/tests/unit/test_ed25519.c b/tests/unit/test_ed25519.c index f9933fe..e2722a4 100644 --- a/tests/unit/test_ed25519.c +++ b/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; }