From 21147a6f3ee2f49b587fca5d66165df4cf1a2558 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Thu, 3 Sep 2026 22:47:41 +0530 Subject: [PATCH] =?UTF-8?q?fix:=20repair=20master=20=E2=80=94=20#86,=20#87?= =?UTF-8?q?=20and=20#57=20merged=20clean=20and=20do=20not=20compile?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit eBoot master (22d8f8b) does not build. Three separate collisions, all from my #86/#87 landing alongside #57. Each merge was conflict-free; none of them was correct. 1. include/eos_image.h — asserts on a field that no longer exists. #87 pinned every header field, reserved[] among them. #57 then split reserved[30] into tlv_len (2) + tlv_hash (28). Both landed: error: no member named 'reserved' in 'eos_image_header_t' (x2) Offset 62 is already pinned by #57's tlv_len assert, so the old reserved[] offset assert is deleted rather than renamed — keeping both would pin one byte range twice. The width guard follows the 30 bytes into their two successors instead, which is what it was there to do. 2. core/ed25519_verify.c — two identical point_is_identity(). #86 and #57 each added one, byte-for-byte the same, plus equivalent subgroup checks under different names. Kept #57's public_key_is_valid_subgroup(), which is the one eos_ed25519_verify() actually calls, and #86's fuller comment, which is the one that explains why both conditions are needed. Dropped #86's now-dead key_has_prime_order(). 3. tests/unit/test_ed25519.c — the merge kept tests and dropped their data. test_ed25519_identity_key_forgery_rejected() was defined twice. messages[] and k_low_order[8][32] were dropped while the test that iterates them survived, so the file did not compile. Restored both verbatim from the #86 branch. Two more, found once it built: - test_ed25519_low_order_R_with_a_valid_key_is_not_a_forgery() was compiled but never called. The merge kept the function and dropped its run_ call, which no build error can catch. - tests_run was hardcoded to 11 while 12 tests ran, so the suite failed on its own arithmetic. It is now incremented by the macro that runs each test, so it cannot go stale again. Verified: header TU compiles (2 errors on master) cmake + ninja clean (3 errors on master) ctest 21/21 passed test_ed25519 13/13 (was 12/11 -> exit 1) low-order forgery probe 0 accepted / 2048 attempts RFC 8032 vectors all still accepted This is the third time in this repo that a clean merge produced code that does not compile. The tests_run case is the one worth noting: a test that is compiled but never called is invisible to the build, to ctest, and to the registration guard, which checks files rather than call sites. --- core/ed25519_verify.c | 56 +++++++++++---------------------------- include/eos_image.h | 16 +++++++---- tests/unit/test_ed25519.c | 49 ++++++++++++++++++++++++---------- 3 files changed, 61 insertions(+), 60 deletions(-) diff --git a/core/ed25519_verify.c b/core/ed25519_verify.c index d36cc7c..f8941a4 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 @@ -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]; diff --git a/include/eos_image.h b/include/eos_image.h index 62744d0..fc8e65a 100644 --- a/include/eos_image.h +++ b/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"); diff --git a/tests/unit/test_ed25519.c b/tests/unit/test_ed25519.c index f78012d..bc3824f 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"); \ @@ -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; }