diff --git a/core/ed25519_verify.c b/core/ed25519_verify.c index d36cc7c..34aca77 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 @@ -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]; diff --git a/include/eos_image.h b/include/eos_image.h index 62744d0..24cbb16 100644 --- a/include/eos_image.h +++ b/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"); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b394f24..4f61cad 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -4,27 +4,36 @@ # --- test_bootctl: Boot control block --- add_executable(eboot_test_bootctl unit/test_bootctl.c) target_link_libraries(eboot_test_bootctl PRIVATE eboot_core) +# Every unit suite registered below appends its ctest name here, so the +# Valgrind block at the bottom derives its list instead of repeating it. +set(EBLDR_UNIT_TESTS "") + add_test(NAME test_bootctl COMMAND eboot_test_bootctl) +list(APPEND EBLDR_UNIT_TESTS test_bootctl) # --- test_crypto: SHA-256 against known vectors --- add_executable(eboot_test_crypto unit/test_crypto.c) target_link_libraries(eboot_test_crypto PRIVATE eboot_core) add_test(NAME test_crypto COMMAND eboot_test_crypto) +list(APPEND EBLDR_UNIT_TESTS test_crypto) # --- test_image_verify: Image header parse bounds --- add_executable(eboot_test_image_verify unit/test_image_verify.c) target_link_libraries(eboot_test_image_verify PRIVATE eboot_core) add_test(NAME test_image_verify COMMAND eboot_test_image_verify) +list(APPEND EBLDR_UNIT_TESTS test_image_verify) # --- test_image_abi: pins the .efw image header wire format against eFirmware --- add_executable(eboot_test_image_abi unit/test_image_abi.c) target_link_libraries(eboot_test_image_abi PRIVATE eboot_core) add_test(NAME test_image_abi COMMAND eboot_test_image_abi) +list(APPEND EBLDR_UNIT_TESTS test_image_abi) # --- test_secure_boot: Secure boot policy gates --- add_executable(eboot_test_secure_boot unit/test_secure_boot.c) target_link_libraries(eboot_test_secure_boot PRIVATE eboot_core) add_test(NAME test_secure_boot COMMAND eboot_test_secure_boot) +list(APPEND EBLDR_UNIT_TESTS test_secure_boot) # --- test_recovery: UART recovery write range --- # eboot_stage1 links eboot_core PUBLIC, so naming eboot_core here too put it on @@ -34,92 +43,112 @@ add_test(NAME test_secure_boot COMMAND eboot_test_secure_boot) add_executable(eboot_test_recovery unit/test_recovery.c) target_link_libraries(eboot_test_recovery PRIVATE eboot_stage1) add_test(NAME test_recovery COMMAND eboot_test_recovery) +list(APPEND EBLDR_UNIT_TESTS test_recovery) # --- test_fw_transport: UART raw/XMODEM/YMODEM firmware transport --- add_executable(eboot_test_fw_transport unit/test_fw_transport.c) target_link_libraries(eboot_test_fw_transport PRIVATE eboot_core) add_test(NAME test_fw_transport COMMAND eboot_test_fw_transport) +list(APPEND EBLDR_UNIT_TESTS test_fw_transport) # --- test_slot_size_bounds: verify_slot() must reject image_size > slot capacity --- add_executable(eboot_test_slot_size_bounds unit/test_slot_size_bounds.c) target_link_libraries(eboot_test_slot_size_bounds PRIVATE eboot_core) add_test(NAME test_slot_size_bounds COMMAND eboot_test_slot_size_bounds) +list(APPEND EBLDR_UNIT_TESTS test_slot_size_bounds) # --- test_device_table: UEFI-style device table --- add_executable(eboot_test_device_table unit/test_device_table.c) target_link_libraries(eboot_test_device_table PRIVATE eboot_core) add_test(NAME test_device_table COMMAND eboot_test_device_table) +list(APPEND EBLDR_UNIT_TESTS test_device_table) # --- test_runtime_svc: Runtime variable store --- add_executable(eboot_test_runtime_svc unit/test_runtime_svc.c) target_link_libraries(eboot_test_runtime_svc PRIVATE eboot_core) add_test(NAME test_runtime_svc COMMAND eboot_test_runtime_svc) +list(APPEND EBLDR_UNIT_TESTS test_runtime_svc) # --- test_board_config: Declarative hardware config --- add_executable(eboot_test_board_config unit/test_board_config.c) target_link_libraries(eboot_test_board_config PRIVATE eboot_core) add_test(NAME test_board_config COMMAND eboot_test_board_config) +list(APPEND EBLDR_UNIT_TESTS test_board_config) # --- test_multicore: Multicore boot management --- add_executable(eboot_test_multicore unit/test_multicore.c) target_link_libraries(eboot_test_multicore PRIVATE eboot_core) add_test(NAME test_multicore COMMAND eboot_test_multicore) +list(APPEND EBLDR_UNIT_TESTS test_multicore) # --- test_board_registry: Runtime board selection --- add_executable(eboot_test_board_registry unit/test_board_registry.c) target_link_libraries(eboot_test_board_registry PRIVATE eboot_core) add_test(NAME test_board_registry COMMAND eboot_test_board_registry) +list(APPEND EBLDR_UNIT_TESTS test_board_registry) # --- test_slot_manager: Firmware slot management --- add_executable(eboot_test_slot_manager unit/test_slot_manager.c) target_link_libraries(eboot_test_slot_manager PRIVATE eboot_core) add_test(NAME test_slot_manager COMMAND eboot_test_slot_manager) +list(APPEND EBLDR_UNIT_TESTS test_slot_manager) # --- test_boot_log: Boot log subsystem --- add_executable(eboot_test_boot_log unit/test_boot_log.c) target_link_libraries(eboot_test_boot_log PRIVATE eboot_core) add_test(NAME test_boot_log COMMAND eboot_test_boot_log) +list(APPEND EBLDR_UNIT_TESTS test_boot_log) # --- test_ed25519: Ed25519 signature verification --- add_executable(eboot_test_ed25519 unit/test_ed25519.c) target_link_libraries(eboot_test_ed25519 PRIVATE eboot_core) add_test(NAME test_ed25519 COMMAND eboot_test_ed25519) +list(APPEND EBLDR_UNIT_TESTS test_ed25519) # --- test_keystore: Key management --- add_executable(eboot_test_keystore unit/test_keystore.c) target_link_libraries(eboot_test_keystore PRIVATE eboot_core) add_test(NAME test_keystore COMMAND eboot_test_keystore) +list(APPEND EBLDR_UNIT_TESTS test_keystore) # --- test_rollback: Anti-rollback security counter --- add_executable(eboot_test_rollback unit/test_rollback.c) target_link_libraries(eboot_test_rollback PRIVATE eboot_core) add_test(NAME test_rollback COMMAND eboot_test_rollback) +list(APPEND EBLDR_UNIT_TESTS test_rollback) # --- test_tlv_auth: TLV area must be bound to the signed header --- add_executable(eboot_test_tlv_auth unit/test_tlv_auth.c) target_link_libraries(eboot_test_tlv_auth PRIVATE eboot_core) add_test(NAME test_tlv_auth COMMAND eboot_test_tlv_auth) +list(APPEND EBLDR_UNIT_TESTS test_tlv_auth) # --- test_storage: Unified storage abstraction --- add_executable(eboot_test_storage unit/test_storage.c) target_link_libraries(eboot_test_storage PRIVATE eboot_core) add_test(NAME test_storage COMMAND eboot_test_storage) +list(APPEND EBLDR_UNIT_TESTS test_storage) # --- test_ecc: ECC memory range validation --- 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) +list(APPEND EBLDR_UNIT_TESTS test_ecc) # --- Valgrind test targets --- +# +# The list is derived from the suites registered above, not written out again. +# Hand-maintained, it drifted: it had 17 of the 21 registered tests, missing +# test_ecc, test_rollback, test_secure_boot and test_storage -- and nothing +# failed when a name was forgotten, because a missing entry is simply a test +# that never gets a memory-safety run. +# +# EBLDR_UNIT_TESTS is appended by each add_test() above, so a suite added +# without touching this block still gets a Valgrind target. find_program(VALGRIND valgrind) if(VALGRIND) set(VALGRIND_OPTS --leak-check=full --error-exitcode=1 --quiet) - foreach(TEST_NAME test_bootctl test_crypto test_ed25519 test_keystore - test_device_table test_runtime_svc test_board_config - test_multicore test_board_registry test_slot_manager - test_boot_log test_image_verify test_image_abi - test_recovery test_slot_size_bounds test_fw_transport - test_tlv_auth) + foreach(TEST_NAME ${EBLDR_UNIT_TESTS}) add_test( NAME valgrind_${TEST_NAME} COMMAND ${VALGRIND} ${VALGRIND_OPTS} $ diff --git a/tests/unit/test_board_config.c b/tests/unit/test_board_config.c index 3096e03..64cf84e 100644 --- a/tests/unit/test_board_config.c +++ b/tests/unit/test_board_config.c @@ -20,6 +20,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"); \ @@ -188,7 +189,6 @@ int main(void) run_test_total_ram(); run_test_total_flash(); - tests_run = 9; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_board_registry.c b/tests/unit/test_board_registry.c index 991b28b..a70ffe2 100644 --- a/tests/unit/test_board_registry.c +++ b/tests/unit/test_board_registry.c @@ -21,6 +21,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"); \ @@ -134,7 +135,6 @@ int main(void) run_test_get_by_index(); run_test_register_null(); - tests_run = 8; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_boot_log.c b/tests/unit/test_boot_log.c index 5770780..0bde90f 100644 --- a/tests/unit/test_boot_log.c +++ b/tests/unit/test_boot_log.c @@ -300,6 +300,6 @@ int main(void) RUN(test_clear_reports_erase_failure); RUN(test_append_does_not_advance_head_when_write_fails); RUN(test_entry_layout_is_stable); - printf("\n%d/11 tests passed\n", tests_passed); + printf("\n%d/%d tests passed\n", tests_passed); return 0; } diff --git a/tests/unit/test_bootctl.c b/tests/unit/test_bootctl.c index 7c56d7f..f561b09 100644 --- a/tests/unit/test_bootctl.c +++ b/tests/unit/test_bootctl.c @@ -109,6 +109,7 @@ static int tests_passed = 0; static void run_##name(void) { \ setup(); \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -292,7 +293,6 @@ int main(void) run_test_version_encoding(); run_test_validate_null(); - tests_run = 12; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } \ No newline at end of file diff --git a/tests/unit/test_crypto.c b/tests/unit/test_crypto.c index 279bcd1..59d1ff5 100644 --- a/tests/unit/test_crypto.c +++ b/tests/unit/test_crypto.c @@ -20,6 +20,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"); \ @@ -118,7 +119,6 @@ int main(void) run_test_sha256_incremental(); run_test_crypto_null_args(); - tests_run = 5; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_device_table.c b/tests/unit/test_device_table.c index 01a39cd..6daf412 100644 --- a/tests/unit/test_device_table.c +++ b/tests/unit/test_device_table.c @@ -20,6 +20,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"); \ @@ -145,7 +146,6 @@ int main(void) run_test_corrupt_crc_fails(); run_test_oversized_counts_fail(); - tests_run = 8; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_ed25519.c b/tests/unit/test_ed25519.c index f78012d..9ea837c 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"); \ @@ -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; } diff --git a/tests/unit/test_fw_transport.c b/tests/unit/test_fw_transport.c index 61a43ac..d22aadb 100644 --- a/tests/unit/test_fw_transport.c +++ b/tests/unit/test_fw_transport.c @@ -163,6 +163,7 @@ static int tests_passed = 0; static void run_##name(void) { \ setup(); \ printf(" %-56s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[OK]\n"); \ @@ -555,7 +556,6 @@ int main(void) run_test_raw_oversized_length_is_rejected(); run_test_raw_zero_length_is_rejected(); - tests_run = 12; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_image_verify.c b/tests/unit/test_image_verify.c index c275a4f..f714dd7 100644 --- a/tests/unit/test_image_verify.c +++ b/tests/unit/test_image_verify.c @@ -95,6 +95,7 @@ static int tests_passed = 0; sim_tick = 0; \ eos_hal_init(&sim_ops); \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -481,7 +482,6 @@ int main(void) run_test_unsigned_signature_types_are_rejected(); run_test_header_version_is_validated(); run_test_tlv_unreadable_entry_fails_closed(); - tests_run = 17; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_keystore.c b/tests/unit/test_keystore.c index 7236bef..1e1960d 100644 --- a/tests/unit/test_keystore.c +++ b/tests/unit/test_keystore.c @@ -82,6 +82,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"); \ @@ -276,7 +277,6 @@ int main(void) run_test_revocation_is_persisted_without_clobbering_other_slots(); run_test_revoke_reports_a_failed_persist(); - tests_run = 9; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_multicore.c b/tests/unit/test_multicore.c index 0355860..adeb405 100644 --- a/tests/unit/test_multicore.c +++ b/tests/unit/test_multicore.c @@ -21,6 +21,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,7 +212,6 @@ int main(void) run_test_invalid_core_id(); run_test_ipi_mailbox_fallback(); - tests_run = 10; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_rollback.c b/tests/unit/test_rollback.c index 328eb86..88304a5 100644 --- a/tests/unit/test_rollback.c +++ b/tests/unit/test_rollback.c @@ -22,6 +22,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"); \ @@ -233,7 +234,6 @@ int main(void) run_test_commit_unsupported_without_hardware(); run_test_clear_staged_prevents_commit(); run_test_downgrade_blocked_after_confirmed_update(); - tests_run = 14; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_runtime_svc.c b/tests/unit/test_runtime_svc.c index 2e59c6b..30ca23b 100644 --- a/tests/unit/test_runtime_svc.c +++ b/tests/unit/test_runtime_svc.c @@ -21,6 +21,7 @@ static int tests_passed = 0; static void run_##name(void) { \ eos_rtsvc_init(); \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -140,7 +141,6 @@ int main(void) run_test_next_boot_slot(); run_test_time(); - tests_run = 9; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_secure_boot.c b/tests/unit/test_secure_boot.c index 7358178..5f9160a 100644 --- a/tests/unit/test_secure_boot.c +++ b/tests/unit/test_secure_boot.c @@ -84,6 +84,7 @@ static const eos_board_ops_t sim_ops = { .deinit_peripherals = sim_noop, }; +static int tests_run = 0; static int tests_passed = 0; #define TEST(name) \ @@ -93,6 +94,7 @@ static int tests_passed = 0; sim_tick = 0; \ eos_hal_init(&sim_ops); \ printf(" %-50s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -203,6 +205,10 @@ int main(void) run_test_encrypted_image_rejected_while_decrypt_unimplemented(); run_test_plaintext_image_boots_when_encryption_not_required(); run_test_decrypt_failure_is_attested(); - printf("%d passed\n", tests_passed); - return 0; + /* Compare, and let the exit code carry it. `return 0` meant a suite that + * ran nothing at all still reported success -- the ASSERT macro exits on + * failure, so the only thing this return could ever have signalled is + * exactly the case it ignored. */ + printf("\n%d/%d passed\n", tests_passed, tests_run); + return tests_passed == tests_run ? 0 : 1; } diff --git a/tests/unit/test_slot_manager.c b/tests/unit/test_slot_manager.c index 5bca305..ebb70d1 100644 --- a/tests/unit/test_slot_manager.c +++ b/tests/unit/test_slot_manager.c @@ -171,6 +171,7 @@ static int tests_passed = 0; erased_size = 0; \ eos_hal_init(&sim_ops); \ printf(" %-55s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -338,7 +339,6 @@ int main(void) run_test_boot_attempts_drive_rollback(); run_test_boot_attempts_reject_invalid_slot(); run_test_erase_resets_boot_attempts(); - tests_run = 9; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_slot_size_bounds.c b/tests/unit/test_slot_size_bounds.c index c8acc8d..9efe5ad 100644 --- a/tests/unit/test_slot_size_bounds.c +++ b/tests/unit/test_slot_size_bounds.c @@ -119,6 +119,7 @@ static int tests_passed = 0; payload_bytes_read = 0; \ eos_hal_init(&sim_ops); \ printf(" %-55s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -190,7 +191,6 @@ int main(void) printf("=== eBootloader: Slot-Size Bounds Regression Tests ===\n\n"); run_test_oversized_image_rejected_before_reading_payload(); run_test_in_bounds_image_is_not_over_rejected(); - tests_run = 2; printf("\n%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_suite_bookkeeping.py b/tests/unit/test_suite_bookkeeping.py new file mode 100644 index 0000000..afa59bb --- /dev/null +++ b/tests/unit/test_suite_bookkeeping.py @@ -0,0 +1,112 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 EoS Project +"""Two guards against counts and lists that are maintained by hand. + +Both patterns below were found in review, one after another, and both have the +same shape: a number or a list that describes what the suite does, written out +separately from the thing it describes, with nothing checking the two agree. + + 1. `main()` assigned `tests_run = ` while the `TEST()` macro + incremented only `tests_passed`. A test defined but never wired into + `main()` was then skipped with a zero exit -- the suite reported + "N/N passed" for an N that was a claim, not a count. + + Not hypothetical: `tests/unit/test_ed25519.c` carried a hardcoded 11 that + masked one test called twice and two never called at all, and + `tests/unit/test_tlv_auth.c` assigned `tests_run` twice in one function + (8, then 7) with the stale value surviving only because the later + assignment won. + + 2. The Valgrind `foreach` named its suites again by hand, and had drifted to + 17 of 21 -- test_ecc, test_rollback, test_secure_boot and test_storage + got no memory-safety run, and nothing failed when a name was forgotten. + +These tests are cheap and need no C toolchain. +""" + +import re +from pathlib import Path + +REPO = Path(__file__).resolve().parents[2] +UNIT = REPO / "tests" / "unit" +CMAKE = REPO / "tests" / "CMakeLists.txt" + +#: Suites with no TEST() macro -- they count differently or not at all. A name +#: may sit here only with a reason about the suite itself. +NO_TEST_MACRO = { + "test_boot_log.c": "prints its own summary and has no TEST() macro", + "test_ecc.c": "single-scenario suite; no per-test harness", + "test_image_abi.c": "compile-time _Static_asserts; nothing runs per test", +} + + +def _suites(): + found = sorted(UNIT.glob("test_*.c")) + assert found, f"no unit suites under {UNIT}" + return found + + +def _macro_block(source): + m = re.search(r"#define TEST\(name\)(?:[^\n]*\\\n)*[^\n]*\n", source) + return m.group(0) if m else None + + +def test_no_suite_hardcodes_its_own_total(): + """`tests_run = ` is a claim about the suite, not a measurement.""" + offenders = [] + for path in _suites(): + source = path.read_text(encoding="utf-8") + for literal in re.findall(r"tests_run\s*=\s*([1-9]\d*)\s*;", source): + offenders.append(f"{path.name}: tests_run = {literal}") + # `printf("%d/12 tests passed")` is the same claim in another place. + for literal in re.findall(r"%d\s*/\s*(\d+)\s*(?:tests )?passed", source): + offenders.append(f"{path.name}: literal total {literal} in the summary") + assert not offenders, ( + "these suites state a total instead of counting one; a test that is " + "defined but never called is then invisible:\n " + "\n ".join(offenders) + ) + + +def test_every_test_macro_counts_the_test_it_runs(): + offenders = [] + for path in _suites(): + if path.name in NO_TEST_MACRO: + continue + block = _macro_block(path.read_text(encoding="utf-8")) + if block is None: + offenders.append(f"{path.name}: has no TEST() macro and is not in NO_TEST_MACRO") + elif "tests_run++" not in block: + offenders.append(f"{path.name}: TEST() does not increment tests_run") + assert not offenders, "\n ".join([""] + offenders) + + +def test_every_defined_test_is_actually_called(): + """A TEST() nobody calls is a test that silently does not run.""" + offenders = [] + for path in _suites(): + source = path.read_text(encoding="utf-8") + defined = set(re.findall(r"^TEST\((\w+)\)", source, re.M)) + if not defined: + continue + called = set(re.findall(r"run_(\w+)\s*\(\s*\)\s*;", source)) + for name in sorted(defined - called): + offenders.append(f"{path.name}: {name}() is defined but never called") + assert not offenders, "\n ".join([""] + offenders) + + +def test_the_valgrind_list_is_derived_not_repeated(): + """Every registered suite gets a Valgrind run, by construction.""" + cmake = CMAKE.read_text(encoding="utf-8") + + registered = re.findall(r"add_test\(NAME (\w+) COMMAND", cmake) + appended = re.findall(r"list\(APPEND EBLDR_UNIT_TESTS (\w+)\)", cmake) + + assert "foreach(TEST_NAME ${EBLDR_UNIT_TESTS})" in cmake, ( + "the Valgrind foreach must iterate the accumulated list rather than " + "name its suites again; a hand-written list drifts and nothing fails" + ) + missing = sorted(set(registered) - set(appended)) + assert not missing, ( + f"these suites are registered with ctest but never appended to " + f"EBLDR_UNIT_TESTS, so they get no Valgrind run: {missing}" + ) diff --git a/tests/unit/test_tlv_auth.c b/tests/unit/test_tlv_auth.c index 28c0e41..de46f5f 100644 --- a/tests/unit/test_tlv_auth.c +++ b/tests/unit/test_tlv_auth.c @@ -99,6 +99,7 @@ static int tests_passed = 0; sim_counter = 0; \ eos_hal_init(&sim_ops); \ printf(" %-58s ", #name); \ + tests_run++; \ name(); \ tests_passed++; \ printf("[PASS]\n"); \ @@ -294,7 +295,6 @@ int main(void) { printf("TLV authentication (anti-rollback counter)\n\n"); - tests_run = 8; run_test_authenticated_tlv_counter_is_read(); run_test_tampered_tlv_counter_is_rejected(); run_test_tamper_is_invisible_to_signature_and_integrity(); @@ -303,7 +303,6 @@ int main(void) run_test_tlv_binding_fields_are_inside_the_signed_prefix(); run_test_oversized_tlv_len_is_rejected(); - tests_run = 7; printf("\n%d/%d passed\n", tests_passed, tests_run); return tests_passed == tests_run ? 0 : 1; }