diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1cfbad9..ef0704e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -45,6 +45,14 @@ jobs: # dependency of anything in the repository. pip3 install -r requirements.txt pytest-cov + # Installing the dependency fixes today's silent skip; this makes the + # skip impossible here rather than merely unlikely. Without it, a + # dropped requirement or a changed runner image puts the signing suite + # back to collecting 19 tests and running none, with the job still + # green. See tests/unit/test_eos_sign_payload_offset.py. + - name: Signing tests must not silently skip + run: echo "EOS_REQUIRE_SIGNING_TESTS=1" >> "$GITHUB_ENV" + - name: Configure (host) run: | cmake -B build/host -G Ninja \ 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..ce502fd 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -110,6 +110,12 @@ 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) +# --- test_eos_sign_boot_path: the tool's real output through the real parser --- +add_executable(eboot_test_eos_sign_boot_path unit/test_eos_sign_boot_path.c) +target_link_libraries(eboot_test_eos_sign_boot_path PRIVATE eboot_core) +target_include_directories(eboot_test_eos_sign_boot_path PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) +add_test(NAME test_eos_sign_boot_path COMMAND eboot_test_eos_sign_boot_path) + # --- Valgrind test targets --- find_program(VALGRIND valgrind) if(VALGRIND) 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_eos_sign_boot_path.c b/tests/unit/test_eos_sign_boot_path.c new file mode 100644 index 0000000..8307e14 --- /dev/null +++ b/tests/unit/test_eos_sign_boot_path.c @@ -0,0 +1,164 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 EoS Project + +/** + * @file test_eos_sign_boot_path.c + * @brief Run the real boot path over real eos_sign.py output. + * + * tools/eos_sign.py emits [header][TLV area][payload]. It used to stamp + * hdr_size as a fixed 156 -- the struct size alone -- while + * core/image_verify.c computes `payload_addr = addr + hdr->hdr_size`. That + * landed on the TLV block, so the SHA-256 taken from there never matched + * hash[] and every image the tool produced failed verification on-device. + * + * The Python-side test for this checks the tool's own arithmetic, which is + * close to checking that the tool agrees with itself. This one stages the + * tool's actual bytes into simulated flash and calls the bootloader's own + * eos_image_parse_header() and eos_image_verify_integrity(). + * + * The fixture carries both layouts over the same payload and key, so the fix + * and the defect are asserted together: current output must verify, the old + * output must be refused. + */ + +#include "eos_image.h" +#include "eos_hal.h" +#include "eos_types.h" + +#include "../vectors/signed_image_fixture.h" + +#include +#include +#include + +#define SIM_FLASH_SIZE (64 * 1024) +static uint8_t sim_flash[SIM_FLASH_SIZE]; + +static int sim_flash_read(uint32_t addr, void *buf, size_t len) +{ + if (addr + len > SIM_FLASH_SIZE) return EOS_ERR_FLASH; + memcpy(buf, &sim_flash[addr], len); + return EOS_OK; +} +static int sim_flash_write(uint32_t addr, const void *buf, size_t len) +{ + if (addr + len > SIM_FLASH_SIZE) return EOS_ERR_FLASH; + memcpy(&sim_flash[addr], buf, len); + return EOS_OK; +} +static int sim_flash_erase(uint32_t addr, size_t len) +{ + if (addr + len > SIM_FLASH_SIZE) return EOS_ERR_FLASH; + memset(&sim_flash[addr], 0xFF, len); + return EOS_OK; +} +static void sim_noop(void) {} +static void sim_noop_u32(uint32_t v) { (void)v; } +static eos_reset_reason_t sim_reset_reason(void) { return EOS_RESET_POWER_ON; } +static void sim_system_reset(void) {} +static bool sim_recovery_pin(void) { return false; } +static void sim_jump(uint32_t a) { (void)a; } + +static const eos_board_ops_t sim_ops = { + .flash_base = 0, .flash_size = SIM_FLASH_SIZE, + .slot_a_addr = 0x4000, .slot_a_size = 0x8000, + .slot_b_addr = 0xC000, .slot_b_size = 0x8000, + .recovery_addr = 0, .recovery_size = 0, + .bootctl_addr = 0, .bootctl_backup_addr = 0x1000, + .log_addr = 0x2000, .app_vector_offset = 0, + .flash_read = sim_flash_read, + .flash_write = sim_flash_write, + .flash_erase = sim_flash_erase, + .watchdog_init = sim_noop_u32, + .watchdog_feed = sim_noop, + .get_reset_reason = sim_reset_reason, + .system_reset = sim_system_reset, + .recovery_pin_asserted = sim_recovery_pin, + .jump = sim_jump, + .uart_init = NULL, .uart_send = NULL, .uart_recv = NULL, +}; + +static int failures; + +#define CHECK(cond) do { \ + if (!(cond)) { \ + printf(" [FAIL] %s:%d: %s\n", __FILE__, __LINE__, #cond); \ + failures++; \ + } \ +} while (0) + +#define IMAGE_ADDR 0x4000u + +static int stage_and_verify(const unsigned char *img, size_t len, + eos_image_header_t *hdr_out) +{ + memset(sim_flash, 0xFF, sizeof sim_flash); + memcpy(&sim_flash[IMAGE_ADDR], img, len); + eos_hal_init(&sim_ops); + + if (eos_image_parse_header(IMAGE_ADDR, hdr_out) != EOS_OK) + return EOS_ERR_INVALID; + return eos_image_verify_integrity(hdr_out, IMAGE_ADDR); +} + +int main(void) +{ + eos_image_header_t hdr; + int rc; + + printf("=== eos_sign.py output through the real boot path ===\n\n"); + + /* Current output: [header][payload][TLV], so addr + hdr_size is the + * payload and the stored SHA-256 matches what the bootloader hashes. */ + memset(&hdr, 0, sizeof hdr); + rc = stage_and_verify(eos_fixture_image_good, EOS_FIXTURE_GOOD_LEN, &hdr); + CHECK(rc == EOS_OK); + CHECK(hdr.hdr_size == EOS_FIXTURE_GOOD_HDR_SIZE); + CHECK(hdr.image_size == EOS_FIXTURE_PAYLOAD_LEN); + printf(" current layout [hdr][payload][tlv]: verify_integrity -> %d %s\n", + rc, rc == EOS_OK ? "[PASS]" : "[FAIL]"); + + /* Pre-fix output: [header][TLV][payload], so the bootloader hashes the + * TLV block and the first bytes of the payload. It must refuse this. */ + memset(&hdr, 0, sizeof hdr); + rc = stage_and_verify(eos_fixture_image_old, EOS_FIXTURE_OLD_LEN, &hdr); + CHECK(rc != EOS_OK); + printf(" old layout [hdr][tlv][payload]: verify_integrity -> %d %s\n", + rc, rc != EOS_OK ? "[PASS]" : "[FAIL]"); + + /* hdr_size is 156 in both -- that is the point of the fix, and it is why + * the two fixtures have to be distinguished by their bytes rather than by + * a header field. Same payload, same key, different placement. */ + CHECK(EOS_FIXTURE_GOOD_HDR_SIZE == 156); + CHECK(EOS_FIXTURE_OLD_HDR_SIZE == 156); + CHECK(EOS_FIXTURE_GOOD_LEN == EOS_FIXTURE_OLD_LEN); + CHECK(memcmp(eos_fixture_image_good, eos_fixture_image_old, + EOS_FIXTURE_GOOD_LEN) != 0); + + /* The finding this test was extended for: core/rollback.c computes the + * TLV address as image_addr + hdr_size + image_size, i.e. it assumes the + * area follows the payload. Under the old layout that landed past the end + * of the image and the anti-rollback TLV could never be found -- so the + * counter this repository gates downgrades on was unreachable for every + * image the tool produced. verify_integrity alone does not exercise that + * path; this does. */ + memset(&hdr, 0, sizeof hdr); + rc = stage_and_verify(eos_fixture_image_good, EOS_FIXTURE_GOOD_LEN, &hdr); + CHECK(rc == EOS_OK); + { + uint32_t tlv_addr = IMAGE_ADDR + hdr.hdr_size + hdr.image_size; + uint16_t magic = 0; + CHECK(eos_hal_flash_read(tlv_addr, &magic, sizeof magic) == EOS_OK); + printf(" TLV magic at addr + hdr_size + image_size: 0x%04x %s\n", + magic, magic == 0x6907 ? "[PASS]" : "[FAIL]"); + CHECK(magic == 0x6907); + CHECK(hdr.tlv_len == EOS_FIXTURE_TLV_LEN); + } + + if (failures) { + printf("\n[FAIL] %d check(s) failed\n", failures); + return 1; + } + printf("\n[PASS] the bootloader accepts current tool output and refuses the old layout\n"); + return 0; +} diff --git a/tests/unit/test_eos_sign_payload_offset.py b/tests/unit/test_eos_sign_payload_offset.py new file mode 100644 index 0000000..2d97ce9 --- /dev/null +++ b/tests/unit/test_eos_sign_payload_offset.py @@ -0,0 +1,161 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 EoS Project + +"""eos_sign.py must place the payload where the bootloader looks for it. + +core/image_verify.c computes `payload_addr = addr + hdr->hdr_size` and hashes +image_size bytes from there. tools/sign_image.py reads the same range, +`data[hdr_size:hdr_size + image_size]`. hdr_size is therefore the offset from +the image base to the payload, for everything that reads these images. + +eos_sign.py writes [header][TLV area][payload] and used to stamp hdr_size as a +fixed 156 -- the struct size alone -- so that offset landed on the TLV block. +Every image it produced failed integrity verification on-device. +""" + +import hashlib +import struct +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +TOOLS = REPO_ROOT / "tools" + +# A skip is right for a developer without the signing dependency installed and +# wrong for CI, where "collected 19 tests, ran 0" is a green run that checked +# nothing -- the failure .ai/security.md names directly. EOS_REQUIRE_SIGNING_TESTS +# is set in the workflow, so there a missing dependency is a hard error; locally +# the skip still applies. +if os.environ.get("EOS_REQUIRE_SIGNING_TESTS"): + import cryptography # noqa: F401 -- ImportError here must fail the job +else: + pytest.importorskip( + "cryptography", reason="signing tools require 'cryptography'") + +HDR_SIZE_OFFSET = 6 +IMAGE_SIZE_OFFSET = 8 +HASH_OFFSET = 28 +HDR_STRUCT_SIZE = 156 # sizeof(eos_image_header_t) +TLV_LEN_OFFSET = 62 +TLV_HASH_OFFSET = 64 +TLV_HASH_LEN = 28 # EOS_IMG_TLV_HASH_LEN +SIGNED_LEN = 92 # EOS_IMG_SIGNED_LEN +TLV_INFO_MAGIC = 0x6907 + + +@pytest.fixture(scope="module") +def signed(tmp_path_factory): + work = tmp_path_factory.mktemp("eos_sign") + payload = bytes(range(256)) * 8 # 2048 deterministic bytes + (work / "fw.bin").write_bytes(payload) + + subprocess.run( + [sys.executable, str(TOOLS / "eos_sign.py"), "keygen", + "--output", str(work / "kp")], + check=True, capture_output=True, + ) + subprocess.run( + [sys.executable, str(TOOLS / "eos_sign.py"), "sign", + "--key", str(work / "kp_private.pem"), + "--input", str(work / "fw.bin"), + "--output", str(work / "fw.signed")], + check=True, capture_output=True, + ) + return {"image": (work / "fw.signed").read_bytes(), "payload": payload} + + +def test_hdr_size_is_the_offset_to_the_payload(signed): + """The exact computation core/image_verify.c performs.""" + image, payload = signed["image"], signed["payload"] + hdr_size = struct.unpack_from(" 0, "a signed image carries a TLV area" + + # The TLV area follows the payload, which is where core/rollback.c looks. + tlv_offset = hdr_size + img_size + tlv_magic = struct.unpack_from("hdr_size` landed on the +TLV block and every image it produced failed integrity verification on-device. + +An earlier revision of this fix moved hdr_size instead, making it the offset +to the payload. That was the wrong half to move: hdr_size is a wire-format +field with four other readers, two of which reject the new meaning -- +eFirmware/src/efw_image.c checks `hdr_size != 156` exactly, and +core/rollback.c computes the TLV address as hdr_size + image_size. Moving the +bytes rather than the field's meaning leaves both correct by construction, and +it is what master settled on: include/eos_image.h now carries tlv_len and +tlv_hash inside the signed prefix, documented as "the TLV area sits after the +payload". + +So: hdr_size stays 156 always, the TLV area follows the payload, and the +header declares its length and digest. Those two fields are inside +EOS_IMG_SIGNED_LEN, which is what makes a TLV-declared value such as +EOS_TLV_MIN_SEC_VER trustworthy enough to gate anti-rollback on. Usage: python eos_sign.py sign --key private.pem --input firmware.bin --output firmware.signed.bin @@ -19,7 +32,7 @@ python eos_sign.py keygen --output keypair Creates a signed image with the eBoot image header format: - [eos_image_header_t][TLV area][payload] + [eos_image_header_t][payload][TLV area] Supports Ed25519 signatures (default) and SHA-256 integrity hashes. """ @@ -72,9 +85,22 @@ def sha256(data: bytes) -> bytes: return hashlib.sha256(data).digest() +EOS_IMG_STRUCT_SIZE = 156 # sizeof(eos_image_header_t); pinned in eos_image.h +EOS_IMG_TLV_HASH_LEN = 28 # EOS_IMG_TLV_HASH_LEN + + def build_header(payload: bytes, entry_addr: int, load_addr: int, - version: int, sig: bytes, sig_type: int) -> bytes: - """Build the eos_image_header_t structure.""" + version: int, sig: bytes, sig_type: int, + tlv_len: int = 0, tlv_hash: bytes = b'') -> bytes: + """Build the eos_image_header_t structure. + + tlv_len and tlv_hash describe the TLV area that follows the payload. + Both sit inside EOS_IMG_SIGNED_LEN, so the signature covers them and the + trailing area cannot be rewritten without invalidating it. hdr_size is + always EOS_IMG_STRUCT_SIZE -- it is the size of this struct, which is what + core/image_verify.c, core/rollback.c, stage1/jump_app.c and + eFirmware/src/efw_image.c all read it as. + """ payload_hash = sha256(payload) # EOS_IMG_FLAG_HASH_SHA256 is what makes eos_image_verify_integrity() take @@ -85,11 +111,12 @@ def build_header(payload: bytes, entry_addr: int, load_addr: int, # Header: magic(4) + hdr_version(2) + hdr_size(2) + image_size(4) + # load_addr(4) + entry_addr(4) + image_version(4) + flags(4) + - # hash(32) + sig_type(1) + sig_len(1) + reserved(30) + signature(64) - hdr_size = 4 + 2 + 2 + 4 + 4 + 4 + 4 + 4 + 32 + 1 + 1 + 30 + 64 # = 156 - + # hash(32) + sig_type(1) + sig_len(1) + tlv_len(2) + + # tlv_hash(28) + signature(64) + # hdr_size is the size of this struct, always, and the TLV area follows + # the payload rather than preceding it. hdr = struct.pack(' bytes: - """Build TLV area with SHA-256, key hash, and signature entries.""" +def build_tlv(payload_hash: bytes, key_hash: bytes) -> bytes: + """Build the TLV area that follows the payload. + + No signature entry. The signature lives in the header's signature[] field + and covers EOS_IMG_SIGNED_LEN, which includes tlv_hash -- so a TLV area + containing the signature could not be hashed before the signature existed. + Putting the signature in the header and the metadata in the TLV is what + makes that ordering resolvable, and it is the arrangement + include/eos_image.h documents. + """ entries = b'' # SHA-256 hash TLV @@ -124,11 +165,6 @@ def build_tlv(payload_hash: bytes, key_hash: bytes, sig: bytes) -> bytes: entries += struct.pack('= 2 else 0 + if tlv_magic != TLV_INFO_MAGIC: + print(f'FAIL: no TLV magic at offset {tlv_offset}') + sys.exit(1) + if sha256(tlv)[:EOS_IMG_TLV_HASH_LEN] != stored_tlv_hash: + print('FAIL: TLV area does not match tlv_hash in the signed header') + sys.exit(1) # Verify hash computed_hash = sha256(payload) diff --git a/tools/gen_signed_image_fixture.py b/tools/gen_signed_image_fixture.py new file mode 100644 index 0000000..9f58ac8 --- /dev/null +++ b/tools/gen_signed_image_fixture.py @@ -0,0 +1,117 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 EoS Project +"""Emit a signed-image fixture so the C boot path can be run over real tool output. + +tools/eos_sign.py used to emit [header][TLV][payload] while stamping hdr_size +as a fixed 156, so core/image_verify.c computed the payload address as +addr + 156 and landed on the TLV block. Every image the tool produced failed +integrity verification on-device. + +It now emits [header][payload][TLV], which is the layout master documents: +hdr_size stays 156 for every image, and the trailing TLV area is bound to the +signature through the header's tlv_len and tlv_hash. + +Proving that in Python only re-states the tool's own arithmetic. This emits +both layouts as C byte arrays so tests/unit/test_eos_sign_boot_path.c can run +the actual bootloader code over them: the current layout must verify, and the +old one -- TLV between the header and the payload -- must be refused. + +Deterministic -- fixed key seed, fixed payload -- so the fixture is stable and +regenerating it is a no-op unless the tool's output really changed. + + python3 tools/gen_signed_image_fixture.py > tests/vectors/signed_image_fixture.h +""" + +import importlib.util +import pathlib +import struct +import sys + +ROOT = pathlib.Path(__file__).resolve().parents[1] + +spec = importlib.util.spec_from_file_location("eos_sign", ROOT / "tools" / "eos_sign.py") +eos_sign = importlib.util.module_from_spec(spec) +spec.loader.exec_module(eos_sign) + +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey +from cryptography.hazmat.primitives import serialization + +SEED = bytes(range(32)) # fixed, so the fixture is reproducible +PAYLOAD = bytes((i * 7 + 3) & 0xFF for i in range(256)) +ENTRY = 0x08020000 +LOAD = 0x08020000 +VERSION = 0x00010000 + + +def build(tlv_after_payload: bool) -> bytes: + key = Ed25519PrivateKey.from_private_bytes(SEED) + payload_hash = eos_sign.sha256(PAYLOAD) + pub_raw = key.public_key().public_bytes( + serialization.Encoding.Raw, serialization.PublicFormat.Raw) + key_hash = eos_sign.sha256(pub_raw) + + tlv = eos_sign.build_tlv(payload_hash, key_hash) + tlv_hash = eos_sign.sha256(tlv)[:eos_sign.EOS_IMG_TLV_HASH_LEN] + + hdr = bytearray(eos_sign.build_header(PAYLOAD, ENTRY, LOAD, VERSION, b"", + eos_sign.SIG_TYPE_ED25519, + len(tlv), tlv_hash)) + sig = key.sign(bytes(hdr[:eos_sign.SIGNED_LEN])) + hdr[eos_sign.SIGNATURE_OFFSET:eos_sign.SIGNATURE_OFFSET + 64] = sig + + if tlv_after_payload: + return bytes(hdr) + PAYLOAD + tlv + # The old shape: TLV between the header and the payload, so + # addr + hdr_size lands on the TLV block instead of the payload. + return bytes(hdr) + tlv + PAYLOAD + + +def carr(b, indent=" "): + out, line = [], indent + for i, x in enumerate(b): + line += "0x%02x," % x + if (i + 1) % 16 == 0: + out.append(line); line = indent + if line.strip(): + out.append(line) + return "\n".join(out) + + +def main(): + good = build(True) + old = build(False) + pub = Ed25519PrivateKey.from_private_bytes(SEED).public_key().public_bytes( + serialization.Encoding.Raw, serialization.PublicFormat.Raw) + + w = sys.stdout.write + w("/* SPDX-License-Identifier: MIT\n * Copyright (c) 2026 EoS Project\n */\n\n") + w("/* GENERATED FILE -- do not edit by hand.\n") + w(" * tools/gen_signed_image_fixture.py\n *\n") + w(" * Two images over the same payload, the same key and the same header,\n") + w(" * differing only in where the TLV area sits.\n *\n") + w(" * CURRENT: [header][payload][TLV] -- addr + hdr_size is the payload\n") + w(" * OLD: [header][TLV][payload] -- addr + hdr_size is the TLV block\n") + w(" *\n * hdr_size is %d in both: it is the struct size, not a payload offset.\n" % struct.unpack_from('