fix: repair master — #86, #87 and #57 merged clean and do not compile

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
@@ -307,48 +317,12 @@ static int point_is_identity(gf p[4])
  * 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.
+ * Formulation from eBoot#57 by @muhammadburhandevv-hub. #86 landed an
+ * equivalent key_has_prime_order() alongside it; the two merged cleanly
+ * into one file with two identical point_is_identity() definitions, which
+ * did not compile. This keeps #57's function, which is the one the
+ * verifier calls, and #86's reasoning, which is the fuller of the two.
  */
-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];

include/eos_image.h

@@ -132,15 +132,21 @@ 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");
+/* reserved[30] became tlv_len (2) + tlv_hash (28). Offset 62 is pinned by the
+ * tlv_len assert above, so the old reserved[] offset assert is gone rather
+ * than renamed -- keeping both would pin one byte range twice. */
 
 /* 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 tlv_len/tlv_hash pair absorbs
+ * exactly that, which is why their combined width is pinned too. */
 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) +
+                      sizeof(((eos_image_header_t *)0)->tlv_hash) == 30,
+                      "tlv_len and tlv_hash together fill the 30 bytes "
+                      "reserved[] used to occupy");
+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/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"); \
@@ -260,21 +261,42 @@ 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) ---- */
 
+/* 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_R_with_a_valid_key_is_not_a_forgery)
 {
     /* The subgroup check guards the public key, not R, and that is
@@ -367,13 +389,12 @@ 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_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;
 }