From 42abb8de2bb4dd2c64b50d5c4c602394da059bbc Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Tue, 15 Sep 2026 01:47:20 +0530 Subject: [PATCH 1/7] fix(recovery): refuse an unprovisioned secret, and a write into an unmapped slot Two wire-facing checks in core/recovery.c were weaker than the code around them said. recovery_handle_auth() read the shared secret from OTP and compared the client's response against SHA-256(challenge || secret) with no look at what the secret was. Unprogrammed fuses read back as all zeros or all ones, and both are public: a device whose recovery secret was never provisioned authenticated any client that sent SHA-256(challenge || 00..00). The keystore already refuses an all-zero key for exactly this reason; the recovery path now refuses both patterns before comparing, branch-free so the check does not leak which value the fuses hold, and logs event 0x22. No shipped board port provides otp_read today, so on current boards authentication always failed and this was latent; it is the first board with OTP that would have shipped it. recovery_handle_write() had stopped calling eos_recovery_write_in_range() -- the rule tests/unit/test_recovery.c exercises, which refuses a slot the board leaves unmapped -- and checked offset + len against the slot size inline. #55's replay did that while repairing a duplicate declaration. For a slot whose base is 0 the write then landed at flash address `offset`, which in the test layout is between the boot-control block and its backup. The handler uses the shared rule again, so the wire has one definition. Tests: an all-zero and an all-ones secret are refused and the write that follows is refused too; the same exchange with a provisioned secret still authenticates and writes (the control); a write into an unmapped slot B is refused and flash at the would-be address is untouched. Each new test was run against the unfixed handler: the all-zero secret authenticated (out_buf[33] was ACK) and the unmapped write was accepted (out_buf[34] was ACK). --- CHANGELOG.md | 1 + core/recovery.c | 24 ++++++- tests/unit/test_recovery.c | 132 ++++++++++++++++++++++++++++++++++++- 3 files changed, 154 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ad06fbb..98b3324 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## [Unreleased] ### Security +- **UART recovery refuses an unprovisioned secret, and a write into an unmapped slot.** `recovery_handle_auth()` compared the client's response against `SHA-256(challenge || secret)` for whatever the OTP held at the secret offset -- and unprogrammed fuses read back as all zeros or all ones, both public, so a device whose recovery secret was never provisioned authenticated any client that knew that. Both patterns are now refused before the comparison (logged as event `0x22`), the way the keystore already refuses an all-zero key. `recovery_handle_write()` had stopped calling `eos_recovery_write_in_range()` -- the rule the unit tests exercise -- for an inline check that never looked at the slot base, so a slot the board leaves unmapped (base 0) turned "write at offset" into "write at flash address". The handler uses the shared rule again. No shipped board port provides `otp_read` today, so the first defect was latent (authentication always failed); both are pinned by `tests/unit/test_recovery.c`. - **Image header is now authenticated (header format v2).** `eos_image_verify_signature()` signed `hdr->hash` only — 32 of the header's 156 bytes. Everything else (`image_size`, `load_addr`, `entry_addr`, `flags`, `sig_type`, `image_version`) sat outside the signature, so an attacker holding a legitimately signed image could relocate it, move its entry point, or clear `EOS_IMG_FLAG_HASH_SHA256` to downgrade integrity checking from SHA-256 to forgeable CRC32 — all while keeping the signature valid. The signature now covers `EOS_IMG_SIGNED_LEN` (92) bytes: the whole header except `signature[]` itself. **Existing signed images must be re-signed.** - **`eos_image_parse_header`:** validates `hdr_version`, rejecting 0 and anything newer than this build understands. - **`tools/eos_sign.py`:** `SIG_TYPE_ED25519` was `1` — that is `EOS_SIG_CRC32` in `eos_types.h`, which `eos_image_verify_signature()` rejects outright — and `IMG_FLAG_SIGNED` was `1 << 2`, which is `EOS_IMG_FLAG_DEBUG`. It also never set `EOS_IMG_FLAG_HASH_SHA256`, so the bootloader read the stored SHA-256 as a CRC32. Constants now match `include/eos_types.h`. diff --git a/core/recovery.c b/core/recovery.c index 1e5c816..1d6e686 100644 --- a/core/recovery.c +++ b/core/recovery.c @@ -186,6 +186,23 @@ static int recovery_handle_auth(void) return recovery_send_nack(); } + /* Unprogrammed fuses read back as all zeros or all ones, and both + * are public. A board whose recovery secret was never provisioned + * must not authenticate anyone: the keystore already refuses an + * all-zero key for the same reason. Branch-free so the check does + * not leak which value the fuses hold. */ + uint8_t all_zero = 0, all_ones = 0xFF; + for (size_t i = 0; i < sizeof(shared_secret); i++) { + all_zero |= shared_secret[i]; + all_ones &= shared_secret[i]; + } + if (all_zero == 0 || all_ones == 0xFF) { + auth_fail_count++; + auth_state = RCVR_AUTH_NONE; + eos_boot_log_append(0x22, EOS_SLOT_NONE, auth_fail_count); /* AUTH_UNPROVISIONED */ + return recovery_send_nack(); + } + eos_sha256_update(&ctx, shared_secret, sizeof(shared_secret)); eos_sha256_final(&ctx, expected); @@ -286,8 +303,11 @@ static int recovery_handle_write(eos_slot_t slot, uint32_t offset, uint16_t len) /* offset/len come straight from the wire; without this check a * recovery client can write past the slot boundary into the other - * slot, boot-control blocks, or the boot log. */ - if (slot_size == 0 || (uint64_t)offset + len > (uint64_t)slot_size) + * slot, boot-control blocks, or the boot log. The rule lives in + * eos_recovery_write_in_range() -- the function the unit tests drive -- + * and it also refuses a slot the board leaves unmapped (base 0), which + * would otherwise turn "write at offset" into "write at address". */ + if (eos_recovery_write_in_range(base, slot_size, offset, len) != EOS_OK) return recovery_send_nack(); recovery_send_ack(); diff --git a/tests/unit/test_recovery.c b/tests/unit/test_recovery.c index e99133d..e0ce176 100644 --- a/tests/unit/test_recovery.c +++ b/tests/unit/test_recovery.c @@ -111,11 +111,22 @@ static const uint8_t SIM_CHALLENGE[32] = { 0x18, 0x19, 0x1A, 0x1B, 0x1C, 0x1D, 0x1E, 0x1F, }; +/* What the simulated OTP holds at the recovery-secret offset. Tests point it + * at an unprogrammed pattern to show that such a device authenticates nobody. */ +static const uint8_t *sim_secret = SIM_SHARED_SECRET; +static const uint8_t SIM_SECRET_ALL_ZERO[32] = { 0 }; +static const uint8_t SIM_SECRET_ALL_ONES[32] = { + 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, 0xFF, +}; + static int sim_otp_read(uint32_t offset, void *buf, size_t len) { if (offset != 0x180 || len > sizeof(SIM_SHARED_SECRET)) return EOS_ERR_GENERIC; - memcpy(buf, SIM_SHARED_SECRET, len); + memcpy(buf, sim_secret, len); return EOS_OK; } @@ -236,9 +247,125 @@ static void setup(void) script_pos = 0; out_len = 0; verify_payload_bytes_read = 0; + sim_secret = SIM_SHARED_SECRET; eos_hal_init(&sim_ops); } +/* Script the two-packet authentication exchange, computing the response the + * way a client that knows `secret` would. */ +static void script_auth(const uint8_t *secret) +{ + eos_sha256_ctx_t ctx; + uint8_t auth_response[32]; + eos_sha256_init(&ctx); + eos_sha256_update(&ctx, SIM_CHALLENGE, sizeof(SIM_CHALLENGE)); + eos_sha256_update(&ctx, secret, 32); + eos_sha256_final(&ctx, auth_response); + + uint8_t pkt[8]; + put_pkt(pkt, RCVR_CMD_AUTH, 0, 0, 0); + script_append(pkt, sizeof(pkt)); /* -> server sends challenge */ + put_pkt(pkt, RCVR_CMD_AUTH, 0, 0, 0); + script_append(pkt, sizeof(pkt)); /* -> server awaits response */ + script_append(auth_response, sizeof(auth_response)); +} + +/* A device whose recovery secret was never programmed reads its fuses back + * as all zeros or all ones. Both are public, so a client that knows nothing + * can compute the "right" response. The server must refuse it -- and, having + * refused, must still refuse the write that follows. */ +static void check_unprovisioned_secret_is_refused(const uint8_t *pattern) +{ + setup(); + sim_secret = pattern; + + script_auth(pattern); + + uint8_t pkt[8]; + put_pkt(pkt, RCVR_CMD_WRITE, EOS_SLOT_A, 4, 0); + script_append(pkt, sizeof(pkt)); + uint8_t payload[4] = { 0xDE, 0xAD, 0xBE, 0xEF }; + script_append(payload, sizeof(payload)); + + eos_bootctl_t bctl; + eos_bootctl_init_defaults(&bctl); + if (setjmp(exit_jmp) == 0) { + eos_recovery_enter(&bctl); + } + + /* [0]=ACK, [1..32]=challenge, [33]=auth verdict, [34]=write verdict */ + ASSERT(out_len >= 35); + ASSERT(out_buf[0] == RCVR_ACK); + ASSERT(out_buf[33] == RCVR_NACK); + ASSERT(out_buf[34] == RCVR_NACK); + ASSERT(sim_flash[SIM_SLOT_A_ADDR] == 0xFF); +} + +TEST(test_auth_refuses_an_unprovisioned_secret) +{ + check_unprovisioned_secret_is_refused(SIM_SECRET_ALL_ZERO); + check_unprovisioned_secret_is_refused(SIM_SECRET_ALL_ONES); +} + +/* The control for the test above: the same exchange with a provisioned + * secret authenticates and the write goes through, so the refusal is about + * the secret's value and not about the exchange. */ +TEST(test_auth_accepts_a_provisioned_secret) +{ + script_auth(SIM_SHARED_SECRET); + + uint8_t pkt[8]; + put_pkt(pkt, RCVR_CMD_WRITE, EOS_SLOT_A, 4, 0); + script_append(pkt, sizeof(pkt)); + uint8_t payload[4] = { 0xDE, 0xAD, 0xBE, 0xEF }; + script_append(payload, sizeof(payload)); + + eos_bootctl_t bctl; + eos_bootctl_init_defaults(&bctl); + if (setjmp(exit_jmp) == 0) { + eos_recovery_enter(&bctl); + } + + ASSERT(out_len >= 36); + ASSERT(out_buf[33] == RCVR_ACK); + ASSERT(out_buf[34] == RCVR_ACK); + ASSERT(out_buf[35] == RCVR_ACK); + ASSERT(memcmp(&sim_flash[SIM_SLOT_A_ADDR], payload, sizeof(payload)) == 0); +} + +/* A board that leaves slot B unmapped reports base 0 for it. The write + * handler used to check only offset + len against the slot size, so a + * write "at offset 0x800 of slot B" landed at flash address 0x800 -- in + * this layout, between the boot-control block and its backup. The shared + * range rule refuses an unmapped slot; the handler has to use it. */ +TEST(test_write_refuses_a_slot_the_board_leaves_unmapped) +{ + eos_board_ops_t unmapped_b = sim_ops; + unmapped_b.slot_b_addr = 0; + unmapped_b.slot_b_size = SIM_SLOT_B_SIZE; + eos_hal_init(&unmapped_b); + + script_auth(SIM_SHARED_SECRET); + + uint8_t pkt[8]; + put_pkt(pkt, RCVR_CMD_WRITE, EOS_SLOT_B, 4, 0x800); + script_append(pkt, sizeof(pkt)); + uint8_t payload[4] = { 0xDE, 0xAD, 0xBE, 0xEF }; + script_append(payload, sizeof(payload)); /* only consumed if accepted */ + + eos_bootctl_t bctl; + eos_bootctl_init_defaults(&bctl); + if (setjmp(exit_jmp) == 0) { + eos_recovery_enter(&bctl); + } + + ASSERT(out_len >= 35); + ASSERT(out_buf[33] == RCVR_ACK); /* authenticated */ + ASSERT(out_buf[34] == RCVR_NACK); /* write refused */ + ASSERT(sim_flash[0x800] == 0xFF); + ASSERT(sim_flash[0x801] == 0xFF); +} + TEST(test_write_range_helper_rejects_invalid_bounds) { ASSERT(eos_recovery_write_in_range(SIM_SLOT_A_ADDR, SIM_SLOT_A_SIZE, @@ -377,6 +504,9 @@ int main(void) printf("=== test_recovery ===\n"); run_test_write_range_helper_rejects_invalid_bounds(); run_test_write_rejects_offset_past_slot_end(); + run_test_auth_refuses_an_unprovisioned_secret(); + run_test_auth_accepts_a_provisioned_secret(); + run_test_write_refuses_a_slot_the_board_leaves_unmapped(); run_test_verify_rejects_oversized_image_before_reading_payload(); printf("%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; From 72a4b1fb82f21c3a7697f3093db0480fea0cbf49 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Tue, 15 Sep 2026 02:57:54 +0530 Subject: [PATCH 2/7] fix(recovery): name the authentication events, and let the client decode them core/recovery.c logged its authentication outcomes as bare 0x20, 0x21 and 0x22 with a comment for a name, while include/eos_types.h defines EOS_LOG_* for every other event and tools/uart_recovery.py maps codes to names for its `log` command. That table ended at 0x21, so the event this branch adds -- "this device was never provisioned", the one condition the fix exists to distinguish from a guessed secret -- printed as UNKNOWN(0x22) on the client that exists to read it. EOS_LOG_AUTH_SUCCESS, EOS_LOG_AUTH_FAIL and EOS_LOG_AUTH_UNPROVISIONED now sit beside the other EOS_LOG_* codes and recovery.c uses them by name; the client's BOOT_LOG_EVENT_NAMES gains 0x22. The new guard tests/unit/test_boot_log_event_names.py holds the three lists together: every eos_boot_log_append() in core/, stage0/ and stage1/ passes an EOS_LOG_* name, every code the header defines is in the client's table under the same name, and the client names nothing the header does not define. Against the previous commit it fails on the four bare literals and on the two codes the client named without a definition. The CHANGELOG entry also records the wire change the shared range rule brings with it: a WRITE with len == 0 is refused where the inline check had ACKed it and written nothing. --- CHANGELOG.md | 2 +- core/recovery.c | 8 +- include/eos_types.h | 6 ++ tests/unit/test_boot_log_event_names.py | 109 ++++++++++++++++++++++++ tools/uart_recovery.py | 1 + 5 files changed, 121 insertions(+), 5 deletions(-) create mode 100644 tests/unit/test_boot_log_event_names.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 98b3324..fbf14a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ ## [Unreleased] ### Security -- **UART recovery refuses an unprovisioned secret, and a write into an unmapped slot.** `recovery_handle_auth()` compared the client's response against `SHA-256(challenge || secret)` for whatever the OTP held at the secret offset -- and unprogrammed fuses read back as all zeros or all ones, both public, so a device whose recovery secret was never provisioned authenticated any client that knew that. Both patterns are now refused before the comparison (logged as event `0x22`), the way the keystore already refuses an all-zero key. `recovery_handle_write()` had stopped calling `eos_recovery_write_in_range()` -- the rule the unit tests exercise -- for an inline check that never looked at the slot base, so a slot the board leaves unmapped (base 0) turned "write at offset" into "write at flash address". The handler uses the shared rule again. No shipped board port provides `otp_read` today, so the first defect was latent (authentication always failed); both are pinned by `tests/unit/test_recovery.c`. +- **UART recovery refuses an unprovisioned secret, and a write into an unmapped slot.** `recovery_handle_auth()` compared the client's response against `SHA-256(challenge || secret)` for whatever the OTP held at the secret offset -- and unprogrammed fuses read back as all zeros or all ones, both public, so a device whose recovery secret was never provisioned authenticated any client that knew that. Both patterns are now refused before the comparison (logged as event `0x22`), the way the keystore already refuses an all-zero key. `recovery_handle_write()` had stopped calling `eos_recovery_write_in_range()` -- the rule the unit tests exercise -- for an inline check that never looked at the slot base, so a slot the board leaves unmapped (base 0) turned "write at offset" into "write at flash address". The handler uses the shared rule again -- which also refuses a `WRITE` with `len == 0`, where the inline check had ACKed it and written nothing; a recovery client sees a NACK there now. No shipped board port provides `otp_read` today, so the first defect was latent (authentication always failed); both are pinned by `tests/unit/test_recovery.c`. The authentication outcomes are logged as `EOS_LOG_AUTH_SUCCESS` / `EOS_LOG_AUTH_FAIL` / `EOS_LOG_AUTH_UNPROVISIONED` (`0x20`-`0x22`, now defined in `include/eos_types.h` beside the other `EOS_LOG_*` codes instead of as bare hex), `tools/uart_recovery.py` decodes all three, and `tests/unit/test_boot_log_event_names.py` keeps the firmware, the header and the client's table in step. - **Image header is now authenticated (header format v2).** `eos_image_verify_signature()` signed `hdr->hash` only — 32 of the header's 156 bytes. Everything else (`image_size`, `load_addr`, `entry_addr`, `flags`, `sig_type`, `image_version`) sat outside the signature, so an attacker holding a legitimately signed image could relocate it, move its entry point, or clear `EOS_IMG_FLAG_HASH_SHA256` to downgrade integrity checking from SHA-256 to forgeable CRC32 — all while keeping the signature valid. The signature now covers `EOS_IMG_SIGNED_LEN` (92) bytes: the whole header except `signature[]` itself. **Existing signed images must be re-signed.** - **`eos_image_parse_header`:** validates `hdr_version`, rejecting 0 and anything newer than this build understands. - **`tools/eos_sign.py`:** `SIG_TYPE_ED25519` was `1` — that is `EOS_SIG_CRC32` in `eos_types.h`, which `eos_image_verify_signature()` rejects outright — and `IMG_FLAG_SIGNED` was `1 << 2`, which is `EOS_IMG_FLAG_DEBUG`. It also never set `EOS_IMG_FLAG_HASH_SHA256`, so the bootloader read the stored SHA-256 as a CRC32. Constants now match `include/eos_types.h`. diff --git a/core/recovery.c b/core/recovery.c index 1d6e686..41ca68f 100644 --- a/core/recovery.c +++ b/core/recovery.c @@ -182,7 +182,7 @@ static int recovery_handle_auth(void) /* Fail authentication if OTP secret is unreadable */ auth_fail_count++; auth_state = RCVR_AUTH_NONE; - eos_boot_log_append(0x21, EOS_SLOT_NONE, auth_fail_count); /* AUTH_FAIL */ + eos_boot_log_append(EOS_LOG_AUTH_FAIL, EOS_SLOT_NONE, auth_fail_count); return recovery_send_nack(); } @@ -199,7 +199,7 @@ static int recovery_handle_auth(void) if (all_zero == 0 || all_ones == 0xFF) { auth_fail_count++; auth_state = RCVR_AUTH_NONE; - eos_boot_log_append(0x22, EOS_SLOT_NONE, auth_fail_count); /* AUTH_UNPROVISIONED */ + eos_boot_log_append(EOS_LOG_AUTH_UNPROVISIONED, EOS_SLOT_NONE, auth_fail_count); return recovery_send_nack(); } @@ -216,12 +216,12 @@ static int recovery_handle_auth(void) EOS_SHA256_DIGEST_SIZE) == 0) { auth_state = RCVR_AUTH_AUTHENTICATED; auth_fail_count = 0; - eos_boot_log_append(0x20, EOS_SLOT_NONE, 0); /* AUTH_SUCCESS */ + eos_boot_log_append(EOS_LOG_AUTH_SUCCESS, EOS_SLOT_NONE, 0); return recovery_send_ack(); } else { auth_fail_count++; auth_state = RCVR_AUTH_NONE; - eos_boot_log_append(0x21, EOS_SLOT_NONE, auth_fail_count); /* AUTH_FAIL */ + eos_boot_log_append(EOS_LOG_AUTH_FAIL, EOS_SLOT_NONE, auth_fail_count); return recovery_send_nack(); } } diff --git a/include/eos_types.h b/include/eos_types.h index a36a580..dd7b3f0 100644 --- a/include/eos_types.h +++ b/include/eos_types.h @@ -143,6 +143,12 @@ typedef struct { #define EOS_LOG_FACTORY_RESET 0x0A #define EOS_LOG_WATCHDOG_RESET 0x0B #define EOS_LOG_BOOT_FAIL 0x0C +/* Recovery-session authentication. Emitted by core/recovery.c; decoded by + * tools/uart_recovery.py, whose table tests/unit/test_boot_log_event_names.py + * keeps in step with this list. */ +#define EOS_LOG_AUTH_SUCCESS 0x20 +#define EOS_LOG_AUTH_FAIL 0x21 +#define EOS_LOG_AUTH_UNPROVISIONED 0x22 /* Maximum constants */ #define EOS_MAX_BOOT_ATTEMPTS 3 diff --git a/tests/unit/test_boot_log_event_names.py b/tests/unit/test_boot_log_event_names.py new file mode 100644 index 0000000..d9ee62e --- /dev/null +++ b/tests/unit/test_boot_log_event_names.py @@ -0,0 +1,109 @@ +# SPDX-License-Identifier: MIT +# Copyright (c) 2026 EoS Project + +"""Every boot-log event the firmware emits has a name the recovery client prints. + +core/recovery.c logged its authentication outcomes as bare hex -- 0x20, 0x21 +and then 0x22 -- with a trailing comment for a name, while include/eos_types.h +defined EOS_LOG_* for every other event and tools/uart_recovery.py mapped +codes to names for the `log` command. The table stopped at 0x21, so the one +event that says "this device was never provisioned" printed as UNKNOWN(0x22) +on the client that exists to read it. Nothing tied the three lists together. + +Three rules, all source-level, all cheap: + +1. Every event passed to eos_boot_log_append() in core/, stage0/ and stage1/ + is an EOS_LOG_* name, never a literal. +2. Every EOS_LOG_* code defined in include/eos_types.h is in the client's + BOOT_LOG_EVENT_NAMES, under the same name minus the prefix. +3. The client names no code the header does not define. +""" + +import ast +import re +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[2] +TYPES_H = REPO_ROOT / "include" / "eos_types.h" +CLIENT = REPO_ROOT / "tools" / "uart_recovery.py" +FIRMWARE_DIRS = ("core", "stage0", "stage1") + + +def _strip_c_comments(text): + text = re.sub(r"/\*.*?\*/", " ", text, flags=re.DOTALL) + return re.sub(r"//[^\n]*", " ", text) + + +def header_events(): + """{name: code} for every #define EOS_LOG_ in eos_types.h.""" + text = TYPES_H.read_text(encoding="utf-8") + found = re.findall(r"^\s*#define\s+EOS_LOG_([A-Z0-9_]+)\s+(0x[0-9A-Fa-f]+)\s*$", text, re.M) + events = {name: int(code, 16) for name, code in found if name != "MAGIC"} + assert events, "no EOS_LOG_* event codes found in include/eos_types.h" + return events + + +def client_events(): + """{code: name} from BOOT_LOG_EVENT_NAMES, read from the source without importing + the client (it imports pyserial at module level).""" + tree = ast.parse(CLIENT.read_text(encoding="utf-8")) + for node in ast.walk(tree): + if isinstance(node, ast.Assign) and any( + isinstance(t, ast.Name) and t.id == "BOOT_LOG_EVENT_NAMES" for t in node.targets + ): + table = ast.literal_eval(node.value) + assert table, "BOOT_LOG_EVENT_NAMES is empty" + return table + raise AssertionError("BOOT_LOG_EVENT_NAMES not found in tools/uart_recovery.py") + + +def firmware_append_sites(): + """[(file:line, event expression)] for every eos_boot_log_append() call.""" + sites = [] + for d in FIRMWARE_DIRS: + for path in sorted((REPO_ROOT / d).glob("*.c")): + text = _strip_c_comments(path.read_text(encoding="utf-8")) + for m in re.finditer(r"\beos_boot_log_append\s*\(\s*([^,)]+?)\s*[,)]", text): + expr = m.group(1).strip() + if expr.startswith("uint32_t"): + continue # the definition / extern declaration, not a call + line = text.count("\n", 0, m.start()) + 1 + sites.append((f"{path.relative_to(REPO_ROOT)}:{line}", expr)) + assert sites, "no eos_boot_log_append() call sites found" + return sites + + +def test_firmware_logs_events_by_name_only(): + events = header_events() + literal = [(where, expr) for where, expr in firmware_append_sites() + if not re.fullmatch(r"EOS_LOG_[A-Z0-9_]+", expr)] + assert not literal, "eos_boot_log_append() called with a bare event value: %s" % literal + unknown = [(where, expr) for where, expr in firmware_append_sites() + if expr[len("EOS_LOG_"):] not in events] + assert not unknown, "event names with no #define in include/eos_types.h: %s" % unknown + + +def test_client_names_every_event_the_header_defines(): + events = header_events() + table = client_events() + missing = sorted("EOS_LOG_%s = 0x%02X" % (name, code) + for name, code in events.items() if code not in table) + assert not missing, "tools/uart_recovery.py BOOT_LOG_EVENT_NAMES lacks: %s" % missing + renamed = sorted("0x%02X: header %s, client %s" % (code, name, table[code]) + for name, code in events.items() if table.get(code) not in (None, name)) + assert not renamed, "client and header disagree on a name: %s" % renamed + + +def test_client_names_no_event_the_header_does_not_define(): + codes = set(header_events().values()) + extra = sorted("0x%02X: %s" % (code, name) for code, name in client_events().items() + if code not in codes) + assert not extra, "tools/uart_recovery.py names codes the firmware never emits: %s" % extra + + +def test_the_authentication_events_are_named_and_decodable(): + events = header_events() + assert events["AUTH_SUCCESS"] == 0x20 + assert events["AUTH_FAIL"] == 0x21 + assert events["AUTH_UNPROVISIONED"] == 0x22 + assert client_events()[0x22] == "AUTH_UNPROVISIONED" diff --git a/tools/uart_recovery.py b/tools/uart_recovery.py index 3b27d16..48e23de 100644 --- a/tools/uart_recovery.py +++ b/tools/uart_recovery.py @@ -74,6 +74,7 @@ 0x0C: "BOOT_FAIL", 0x20: "AUTH_SUCCESS", 0x21: "AUTH_FAIL", + 0x22: "AUTH_UNPROVISIONED", } BOOT_LOG_SLOT_NAMES = { From a9c6a2e3ee5e1d33dd80d591b679575026098d4e Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Tue, 15 Sep 2026 11:51:12 +0530 Subject: [PATCH 3/7] test(boot_log): the event-name guard reads a define with a trailing comment header_events() anchored its pattern on end of line, so an EOS_LOG_* define written in this header's own style -- a trailing comment, as EOS_LOG_MAGIC already has -- dropped out of the client cross-check without a trace. Demonstrated by the review with the very next code: EOS_LOG_AUTH_NO_ENTROPY 0x23 with a comment, no client entry, 4 passed. Under-collection is the failure mode a parser cannot report on its own. The match now stops at the value. Two tests pin it: a sample header with C and C++ trailing comments parses to the expected map, and the strict pattern is cross-checked against a looser count of the same lines in the real header, so a style it misses shows up as a mismatch. With the review's probe re-applied, the guard now fails where it should: "BOOT_LOG_EVENT_NAMES lacks: ['EOS_LOG_AUTH_NO_ENTROPY = 0x23']". --- tests/unit/test_boot_log_event_names.py | 42 ++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/tests/unit/test_boot_log_event_names.py b/tests/unit/test_boot_log_event_names.py index d9ee62e..5b488fa 100644 --- a/tests/unit/test_boot_log_event_names.py +++ b/tests/unit/test_boot_log_event_names.py @@ -34,10 +34,20 @@ def _strip_c_comments(text): return re.sub(r"//[^\n]*", " ", text) -def header_events(): - """{name: code} for every #define EOS_LOG_ in eos_types.h.""" - text = TYPES_H.read_text(encoding="utf-8") - found = re.findall(r"^\s*#define\s+EOS_LOG_([A-Z0-9_]+)\s+(0x[0-9A-Fa-f]+)\s*$", text, re.M) +EVENT_DEFINE = re.compile(r"^\s*#define\s+EOS_LOG_([A-Z0-9_]+)\s+(0x[0-9A-Fa-f]+)\b", re.M) + + +def header_events(text=None): + """{name: code} for every #define EOS_LOG_ in eos_types.h. + + The match stops at the value rather than at end of line: this header + writes trailing comments on its defines (EOS_LOG_MAGIC has one), and a + parser that anchors on end of line silently drops any code written that + way -- under-collection, which no downstream rule can notice. + """ + if text is None: + text = TYPES_H.read_text(encoding="utf-8") + found = EVENT_DEFINE.findall(text) events = {name: int(code, 16) for name, code in found if name != "MAGIC"} assert events, "no EOS_LOG_* event codes found in include/eos_types.h" return events @@ -107,3 +117,27 @@ def test_the_authentication_events_are_named_and_decodable(): assert events["AUTH_FAIL"] == 0x21 assert events["AUTH_UNPROVISIONED"] == 0x22 assert client_events()[0x22] == "AUTH_UNPROVISIONED" + + +def test_header_parser_sees_a_define_with_a_trailing_comment(): + """The failure mode of the parser is collecting fewer rows than the file + has, which nothing downstream can detect; pin the one style this header + already uses that an end-of-line anchor would miss.""" + sample = ( + "#define EOS_LOG_BOOT_START 0x01\n" + "#define EOS_LOG_AUTH_NO_ENTROPY 0x23 /* board provides no rng_get */\n" + "#define EOS_LOG_WITH_CPP_COMMENT 0x24 // trailing\n" + "#define EOS_LOG_MAGIC 0x454C4F47 /* \"ELOG\" */\n" + ) + events = header_events(sample) + assert events == {"BOOT_START": 0x01, "AUTH_NO_ENTROPY": 0x23, "WITH_CPP_COMMENT": 0x24} + + +def test_header_parser_collects_every_event_define_in_the_real_header(): + """Cross-check the parser against a looser count of the same lines, so a + future style the strict pattern misses shows up as a mismatch.""" + text = TYPES_H.read_text(encoding="utf-8") + loose = [m.group(1) for m in re.finditer(r"^\s*#define\s+EOS_LOG_([A-Z0-9_]+)\b", text, re.M) + if m.group(1) != "MAGIC"] + assert sorted(header_events()) == sorted(loose) + From 00283e82dd824df099d997c9541917ce5f3a16dc Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Tue, 15 Sep 2026 02:39:42 +0530 Subject: [PATCH 4/7] fix(recovery): issue no challenge on a board without an entropy source recovery_handle_auth() fell back to a linear congruential generator seeded with eos_hal_get_tick_ms() whenever eos_hal_rng_get() failed. The challenge is what stops a captured (challenge, response) pair from being replayed, and with that fallback it was a function of the millisecond at which the AUTH command was handled: a few thousand reachable values on a freshly reset board, shown to the client before it has to answer, retryable after a reset at no cost in failure count. No board port provides rng_get (0 of the 83 ops tables under boards/), so the fallback was the challenge on every board. None provides otp_read either, so no board authenticates anyone today and the defect was latent; a port that adds OTP without an RNG would have gone live with it. The AUTH is now refused outright: NACK, boot-log event EOS_LOG_AUTH_NO_ENTROPY (0x23, defined beside the other EOS_LOG_* codes and decoded by tools/uart_recovery.py), and counted as a failure so the existing backoff and the RCVR_MAX_AUTH_FAILS cap apply. Pinned by test_auth_refuses_when_the_board_has_no_entropy_source, which fails against the old code at the first byte of the reply. --- CHANGELOG.md | 1 + core/recovery.c | 18 +++++++++------ include/eos_types.h | 1 + tests/unit/test_recovery.c | 47 ++++++++++++++++++++++++++++++++++++++ tools/uart_recovery.py | 1 + 5 files changed, 61 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fbf14a9..a1adce5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## [Unreleased] ### Security +- **UART recovery issues no challenge on a board without an entropy source.** When `eos_hal_rng_get()` failed, `recovery_handle_auth()` filled the 32-byte challenge from a linear congruential generator seeded with `eos_hal_get_tick_ms()`. The challenge is the only thing that stops a captured `(challenge, response)` pair from being replayed, and with that fallback it was a function of the millisecond at which the AUTH command was handled -- a few thousand values on a freshly reset board -- which the client sees before it has to answer and can retry after a reset at no cost in failure count. The AUTH is now refused outright (NACK, event `EOS_LOG_AUTH_NO_ENTROPY` = `0x23`, which `tools/uart_recovery.py` decodes; counted like any other failure so the backoff applies). No board port provides `rng_get` (0 of the 83 ops tables under `boards/`), so this was the challenge on every board; none provides `otp_read` either, so no board authenticates anyone today and the defect was latent. Pinned by `test_auth_refuses_when_the_board_has_no_entropy_source` in `tests/unit/test_recovery.c`. - **UART recovery refuses an unprovisioned secret, and a write into an unmapped slot.** `recovery_handle_auth()` compared the client's response against `SHA-256(challenge || secret)` for whatever the OTP held at the secret offset -- and unprogrammed fuses read back as all zeros or all ones, both public, so a device whose recovery secret was never provisioned authenticated any client that knew that. Both patterns are now refused before the comparison (logged as event `0x22`), the way the keystore already refuses an all-zero key. `recovery_handle_write()` had stopped calling `eos_recovery_write_in_range()` -- the rule the unit tests exercise -- for an inline check that never looked at the slot base, so a slot the board leaves unmapped (base 0) turned "write at offset" into "write at flash address". The handler uses the shared rule again -- which also refuses a `WRITE` with `len == 0`, where the inline check had ACKed it and written nothing; a recovery client sees a NACK there now. No shipped board port provides `otp_read` today, so the first defect was latent (authentication always failed); both are pinned by `tests/unit/test_recovery.c`. The authentication outcomes are logged as `EOS_LOG_AUTH_SUCCESS` / `EOS_LOG_AUTH_FAIL` / `EOS_LOG_AUTH_UNPROVISIONED` (`0x20`-`0x22`, now defined in `include/eos_types.h` beside the other `EOS_LOG_*` codes instead of as bare hex), `tools/uart_recovery.py` decodes all three, and `tests/unit/test_boot_log_event_names.py` keeps the firmware, the header and the client's table in step. - **Image header is now authenticated (header format v2).** `eos_image_verify_signature()` signed `hdr->hash` only — 32 of the header's 156 bytes. Everything else (`image_size`, `load_addr`, `entry_addr`, `flags`, `sig_type`, `image_version`) sat outside the signature, so an attacker holding a legitimately signed image could relocate it, move its entry point, or clear `EOS_IMG_FLAG_HASH_SHA256` to downgrade integrity checking from SHA-256 to forgeable CRC32 — all while keeping the signature valid. The signature now covers `EOS_IMG_SIGNED_LEN` (92) bytes: the whole header except `signature[]` itself. **Existing signed images must be re-signed.** - **`eos_image_parse_header`:** validates `hdr_version`, rejecting 0 and anything newer than this build understands. diff --git a/core/recovery.c b/core/recovery.c index 41ca68f..42e3622 100644 --- a/core/recovery.c +++ b/core/recovery.c @@ -137,15 +137,19 @@ static int recovery_handle_auth(void) } if (auth_state == RCVR_AUTH_NONE) { - /* Generate challenge using RNG */ + /* The challenge is the only thing that stops a captured response + * from being replayed, so it has to come from an entropy source. + * A board without one gets no challenge at all: a fallback seeded + * from the millisecond tick lets a client reset the board and + * retry, at no cost, until a challenge it already holds an answer + * for comes back. Refusing here is fail-closed in the same way as + * an unreadable or unprovisioned secret below. */ int rc = eos_hal_rng_get(challenge, RCVR_CHALLENGE_SIZE); if (rc != EOS_OK) { - /* Fallback: use tick-based pseudo-random */ - uint32_t seed = eos_hal_get_tick_ms(); - for (int i = 0; i < RCVR_CHALLENGE_SIZE; i++) { - seed = seed * 1103515245 + 12345; - challenge[i] = (uint8_t)(seed >> 16); - } + auth_fail_count++; + auth_state = RCVR_AUTH_NONE; + eos_boot_log_append(EOS_LOG_AUTH_NO_ENTROPY, EOS_SLOT_NONE, auth_fail_count); + return recovery_send_nack(); } /* Send challenge to client */ diff --git a/include/eos_types.h b/include/eos_types.h index dd7b3f0..4344f7a 100644 --- a/include/eos_types.h +++ b/include/eos_types.h @@ -149,6 +149,7 @@ typedef struct { #define EOS_LOG_AUTH_SUCCESS 0x20 #define EOS_LOG_AUTH_FAIL 0x21 #define EOS_LOG_AUTH_UNPROVISIONED 0x22 +#define EOS_LOG_AUTH_NO_ENTROPY 0x23 /* Maximum constants */ #define EOS_MAX_BOOT_ATTEMPTS 3 diff --git a/tests/unit/test_recovery.c b/tests/unit/test_recovery.c index e0ce176..df7ef1d 100644 --- a/tests/unit/test_recovery.c +++ b/tests/unit/test_recovery.c @@ -449,6 +449,52 @@ TEST(test_write_rejects_offset_past_slot_end) ASSERT(memcmp(&sim_flash[SIM_SLOT_A_ADDR], payload, sizeof(payload)) == 0); } +/* Without an entropy source there is no challenge worth sending. The old + * fallback derived all 32 bytes from the millisecond tick at the moment the + * AUTH command was handled, so a client that had captured one (challenge, + * response) pair could reset the board and retry -- seeing each challenge + * before having to answer it, at no cost in failure count -- until the + * captured one came back. No shipped board port provides rng_get, so this is + * every board in the tree. The server must refuse the AUTH outright, log why, + * and keep refusing the write that follows. */ +TEST(test_auth_refuses_when_the_board_has_no_entropy_source) +{ + eos_board_ops_t no_rng = sim_ops; + no_rng.rng_get = NULL; + eos_hal_init(&no_rng); + eos_boot_log_init(0); + + uint8_t pkt[8]; + put_pkt(pkt, RCVR_CMD_AUTH, 0, 0, 0); + script_append(pkt, sizeof(pkt)); /* -> must be refused, not answered */ + put_pkt(pkt, RCVR_CMD_WRITE, EOS_SLOT_A, 4, 0); + script_append(pkt, sizeof(pkt)); + uint8_t payload[4] = { 0xDE, 0xAD, 0xBE, 0xEF }; + script_append(payload, sizeof(payload)); /* only consumed if accepted */ + + eos_bootctl_t bctl; + eos_bootctl_init_defaults(&bctl); + if (setjmp(exit_jmp) == 0) { + eos_recovery_enter(&bctl); + } + + /* [0]=auth verdict, [1]=write verdict: no 32-byte challenge in between. */ + ASSERT(out_len >= 2); + ASSERT(out_buf[0] == RCVR_NACK); + ASSERT(out_buf[1] == RCVR_NACK); + ASSERT(sim_flash[SIM_SLOT_A_ADDR] == 0xFF); + + /* The refusal is recorded as EOS_LOG_AUTH_NO_ENTROPY, not as a client + * failure, so a field log tells the integrator what is missing. */ + bool logged = false; + for (uint32_t i = 0; i < eos_boot_log_get_head(); i++) { + eos_boot_log_entry_t entry; + ASSERT(eos_boot_log_read(i, &entry) == EOS_OK); + if (entry.event == EOS_LOG_AUTH_NO_ENTROPY) logged = true; + } + ASSERT(logged); +} + static void fill_header(eos_image_header_t *hdr, uint32_t image_size) { memset(hdr, 0, sizeof(*hdr)); @@ -507,6 +553,7 @@ int main(void) run_test_auth_refuses_an_unprovisioned_secret(); run_test_auth_accepts_a_provisioned_secret(); run_test_write_refuses_a_slot_the_board_leaves_unmapped(); + run_test_auth_refuses_when_the_board_has_no_entropy_source(); run_test_verify_rejects_oversized_image_before_reading_payload(); printf("%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; diff --git a/tools/uart_recovery.py b/tools/uart_recovery.py index 48e23de..ff5132a 100644 --- a/tools/uart_recovery.py +++ b/tools/uart_recovery.py @@ -75,6 +75,7 @@ 0x20: "AUTH_SUCCESS", 0x21: "AUTH_FAIL", 0x22: "AUTH_UNPROVISIONED", + 0x23: "AUTH_NO_ENTROPY", } BOOT_LOG_SLOT_NAMES = { From e14456f72d3913f73686d9db3efff9ddf6df3d37 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Tue, 15 Sep 2026 12:00:34 +0530 Subject: [PATCH 5/7] fix(recovery): INFO sent three bytes of stack to whoever asked, and the client printed them as the flash size Finding 1 (P1, pre-existing) and finding 2 (P2) from the review of #131, in one change because they touch the same struct. recovery_handle_info() built its response in a struct that was not packed -- unlike rcvr_packet_t fifteen lines above it -- and never zeroed. On the wire that was 24 bytes: ack, three bytes of padding nothing wrote, then the five geometry fields. RCVR_CMD_INFO needs no authentication, so any client on the UART could ask, repeatedly, and receive three bytes of whatever the previous call had left on the stack. Reproduced with the struct poisoned to 0xA5 and filled exactly as the firmware fills it: sizeof(info)=24 offsetof(flash_size)=4 wire[0..8]: AA A5 A5 A5 00 00 10 00 client decodes flash_size=0x00A5A5A5 slot_a_addr=0x00001000 tools/uart_recovery.py has always parsed the packed 21-byte layout, so it printed the leaked bytes as "Flash size" and every slot address one word off, and the three surplus bytes stayed in the serial buffer to desynchronise the next command. RCVR_CMD_INFO had no test anywhere. The struct is now packed and memset before any field is written, which makes the firmware match what the client always read -- a repair, not a format change -- and means a future field cannot open a new hole. One byte is appended: caps, with RCVR_CAP_RNG and RCVR_CAP_OTP derived from the ops table. #131 makes authenticated recovery unreachable by construction on a board with no entropy source; the event it logs (EOS_LOG_AUTH_NO_ENTROPY) is readable only over RCVR_CMD_LOG, which needs the authentication the missing entropy prevents. What an integrator saw was a bare NACK, indistinguishable from a wrong secret, after 0+1+2+4+8 = 15 s of backoff. The unauthenticated INFO response is where that question is answered, before the first AUTH is sent. It reveals nothing an attacker could not learn by trying. The client reads 22 bytes, decodes caps, and says in words when a board has no authenticated recovery. It refuses a shorter response rather than decoding it -- 21 is the old length, and a firmware still sending 24 would otherwise be parsed one byte wrong forever. Tests: test_recovery.c out_buf poisoned 0xA5; INFO through sim_ops; out_len == 22; every field at the client's offsets; the byte after the response untouched. Second test: ops without rng_get -> caps has RNG clear, OTP set. test_uart_recovery.py a 22-byte response decodes to the printed geometry and leaves the next byte unread; an OTP-only caps byte names the missing entropy; a 21-byte response is refused. Negative controls (file reverted, tests kept, touch + rebuild both ways, recompilation confirmed): core/recovery.c at #131 -> fails at out_len == INFO_WIRE_LEN tools/uart_recovery.py at #131 -> 3 of 7 fail At this head, on #131 at 00283e8 on #127 at a9c6a2e: ctest 31/31, pytest 91. core/recovery.c is CRLF and stays CRLF. --- core/recovery.c | 29 +++++++++++++- tests/unit/test_recovery.c | 66 ++++++++++++++++++++++++++++++++ tests/unit/test_uart_recovery.py | 45 ++++++++++++++++++++++ tools/uart_recovery.py | 15 +++++++- 4 files changed, 151 insertions(+), 4 deletions(-) diff --git a/core/recovery.c b/core/recovery.c index 42e3622..69c76a9 100644 --- a/core/recovery.c +++ b/core/recovery.c @@ -25,6 +25,9 @@ /* Recovery protocol commands */ #define RCVR_CMD_PING 0x01 #define RCVR_CMD_INFO 0x02 +/* Bits of the caps byte in the INFO response. */ +#define RCVR_CAP_RNG 0x01 /* board has an entropy source: AUTH can issue a challenge */ +#define RCVR_CAP_OTP 0x02 /* board has OTP: a shared secret can be provisioned */ #define RCVR_CMD_ERASE 0x03 #define RCVR_CMD_WRITE 0x04 #define RCVR_CMD_VERIFY 0x05 @@ -245,21 +248,43 @@ static int recovery_handle_info(void) if (!ops) return recovery_send_nack(); - struct { + /* Packed, like rcvr_packet_t above it, and zeroed before any field is + * set. Unpacked, this struct was 24 bytes with three bytes of padding + * after ack that nothing wrote, and sizeof(info) sent all 24: an + * unauthenticated caller -- INFO needs no auth -- received three bytes + * of whatever the previous call had left on the stack, and the repo's + * own client, which has always parsed the packed layout, printed them + * as the flash size. The layout on the wire is now what the client + * reads, and memset() means a future field cannot open a new hole. + * + * caps says what the board can do, so an integrator on a board with no + * entropy source learns that here, before authenticating -- which on + * such a board is impossible by construction, and cost 15 s of backoff + * to discover. It reveals nothing an attacker could not learn by trying. + */ + struct rcvr_info { uint8_t ack; uint32_t flash_size; uint32_t slot_a_addr; uint32_t slot_a_size; uint32_t slot_b_addr; uint32_t slot_b_size; - } info; + uint8_t caps; /* RCVR_CAP_* */ + } +#if defined(__GNUC__) || defined(__clang__) + __attribute__((packed)) +#endif + info; + memset(&info, 0, sizeof(info)); info.ack = RCVR_ACK; info.flash_size = ops->flash_size; info.slot_a_addr = ops->slot_a_addr; info.slot_a_size = ops->slot_a_size; info.slot_b_addr = ops->slot_b_addr; info.slot_b_size = ops->slot_b_size; + info.caps = (uint8_t)((ops->rng_get ? RCVR_CAP_RNG : 0) | + (ops->otp_read ? RCVR_CAP_OTP : 0)); return eos_hal_uart_send(&info, sizeof(info)); } diff --git a/tests/unit/test_recovery.c b/tests/unit/test_recovery.c index df7ef1d..cf86f75 100644 --- a/tests/unit/test_recovery.c +++ b/tests/unit/test_recovery.c @@ -545,6 +545,70 @@ TEST(test_verify_rejects_oversized_image_before_reading_payload) ASSERT(verify_payload_bytes_read == 0); } +#define RCVR_CMD_INFO 0x02 +#define RCVR_CAP_RNG 0x01 +#define RCVR_CAP_OTP 0x02 +#define INFO_WIRE_LEN 22 /* ack(1) + five uint32 + caps(1), packed */ + +static uint32_t le32(const uint8_t *p) +{ + return (uint32_t)p[0] | ((uint32_t)p[1] << 8) | ((uint32_t)p[2] << 16) | ((uint32_t)p[3] << 24); +} + +/* Drive RCVR_CMD_INFO -- which needs no authentication -- and decode the + * response exactly as tools/uart_recovery.py does. Before the fix the struct was + * unpacked (24 bytes, three of padding after ack that nothing wrote) and + * sizeof(info) sent all of it, so an unauthenticated caller received three + * bytes of whatever the previous call left on the stack, and the client + * decoded them as the flash size. out_buf is poisoned first so any byte the + * firmware does not write shows as 0xA5 rather than as a lucky zero. */ +TEST(test_info_sends_the_packed_layout_the_client_parses_and_nothing_else) +{ + memset(out_buf, 0xA5, sizeof(out_buf)); + uint8_t pkt[8]; + put_pkt(pkt, RCVR_CMD_INFO, 0, 0, 0); + script_append(pkt, sizeof(pkt)); + + eos_bootctl_t bctl; + eos_bootctl_init_defaults(&bctl); + if (setjmp(exit_jmp) == 0) { + eos_recovery_enter(&bctl); + } + + ASSERT(out_len == INFO_WIRE_LEN); /* not 24: no padding on the wire */ + ASSERT(out_buf[0] == RCVR_ACK); + ASSERT(le32(&out_buf[1]) == SIM_FLASH_SIZE); /* was 0x00A5A5A5 -- the leak */ + ASSERT(le32(&out_buf[5]) == SIM_SLOT_A_ADDR); + ASSERT(le32(&out_buf[9]) == SIM_SLOT_A_SIZE); + ASSERT(le32(&out_buf[13]) == SIM_SLOT_B_ADDR); + ASSERT(le32(&out_buf[17]) == SIM_SLOT_B_SIZE); + ASSERT(out_buf[21] == (RCVR_CAP_RNG | RCVR_CAP_OTP)); /* sim_ops has both */ + ASSERT(out_buf[22] == 0xA5); /* the byte after: untouched */ +} + +/* The capability byte is the answer to "why does AUTH always NACK?" on a board + * with no entropy source, delivered before the 15 s of backoff it used to cost + * to find out. It is derived from the ops table, not configured. */ +TEST(test_info_reports_a_board_without_an_entropy_source) +{ + eos_board_ops_t no_rng = sim_ops; + no_rng.rng_get = NULL; + eos_hal_init(&no_rng); + + uint8_t pkt[8]; + put_pkt(pkt, RCVR_CMD_INFO, 0, 0, 0); + script_append(pkt, sizeof(pkt)); + eos_bootctl_t bctl; + eos_bootctl_init_defaults(&bctl); + if (setjmp(exit_jmp) == 0) { + eos_recovery_enter(&bctl); + } + + ASSERT(out_len == INFO_WIRE_LEN); + ASSERT((out_buf[21] & RCVR_CAP_RNG) == 0); + ASSERT((out_buf[21] & RCVR_CAP_OTP) != 0); /* OTP is still there */ +} + int main(void) { printf("=== test_recovery ===\n"); @@ -555,6 +619,8 @@ int main(void) run_test_write_refuses_a_slot_the_board_leaves_unmapped(); run_test_auth_refuses_when_the_board_has_no_entropy_source(); run_test_verify_rejects_oversized_image_before_reading_payload(); + run_test_info_sends_the_packed_layout_the_client_parses_and_nothing_else(); + run_test_info_reports_a_board_without_an_entropy_source(); printf("%d/%d tests passed\n", tests_passed, tests_run); return (tests_passed == tests_run) ? 0 : 1; } diff --git a/tests/unit/test_uart_recovery.py b/tests/unit/test_uart_recovery.py index 17d4017..8d23230 100644 --- a/tests/unit/test_uart_recovery.py +++ b/tests/unit/test_uart_recovery.py @@ -100,3 +100,48 @@ def test_log_rejects_incomplete_payload(monkeypatch, capsys): assert client.read_boot_log(0, 2) is False assert "Incomplete log response" in capsys.readouterr().out + + +def test_info_decodes_the_packed_22_byte_response(monkeypatch, capsys): + """The firmware sends ack, five little-endian uint32 and a caps byte, packed: + 22 bytes. The client must read exactly that many -- reading fewer leaves + bytes in the serial buffer that desynchronise the next command, which is + what the three padding bytes used to do -- and decode the geometry from the + same offsets test_recovery.c asserts the firmware writes them at.""" + payload = (bytes([0xAA]) + + struct.pack(" bool: print(f"Device responded: {ident} v{version}") return True + # ack(1) + five uint32 (20) + caps(1): the packed layout core/recovery.c sends. + INFO_LEN = 22 + def info(self): self._send_packet(CMD_INFO) - response = self.ser.read(1 + 4 * 5) - if len(response) < 21 or response[0] != ACK: + response = self.ser.read(self.INFO_LEN) + if len(response) < self.INFO_LEN or response[0] != ACK: print("Failed to get device info") return False flash_size, slot_a_addr, slot_a_size, slot_b_addr, slot_b_size = \ struct.unpack(' bool: From 269325946865c7b01ba448d2c1943e9ddfb0a878 Mon Sep 17 00:00:00 2001 From: kartikey1306 Date: Tue, 15 Sep 2026 13:34:02 +0530 Subject: [PATCH 6/7] fix(recovery): the client refuses pre-fix firmware's 24-byte INFO instead of decoding the leak All five findings from the review of e14456f. 1. (High) The client half did not hold against an unpatched device. Firmware before the packed-struct fix sends 24 bytes; read(22) of those succeeds, so `len(response) < INFO_LEN` was false and the response was decoded. Reproduced exactly as the review did, padding poisoned 0xA5: info() returned True and printed "Flash size: 10601K" (0x00A5A5A5), every address one word off, "Entropy: NO -- authenticated recovery is unavailable" invented from a slot-size byte, and two bytes left in the buffer to desynchronise the next command. The defect in this PR's title, still shown by this PR's client, on every device not yet reflashed -- a fix that left its twin. After read(22) the client now probes for surplus bytes with a short timeout of its own (0.25 s; the port's 5 s is for flash work). Two bytes at any baud arrive well inside that; a short read rather than in_waiting because on a real UART they may still be on the wire when read(22) returns. Anything surplus means pre-fix firmware: say so, name the length, drain the buffer, return False. The timeout is restored on every path. 2. (High) The body said "21 is the old length". No firmware ever sent 21 bytes; 21 was only ever what the client read. The 24-byte case is now the test, the 21-byte test is kept as the truncated-read case with its docstring corrected, and the body is corrected. 3. (Medium) __attribute__((packed)) was under #if GNUC/clang only; the struct this one is modelled on also carries the _MSC_VER pragma pair and the copy dropped it. Mirrored. And sizeof(struct rcvr_info) is now _Static_assert'ed at 22, so a toolchain that honours neither turns the returning padding into a compile error. Negative control: with both guards stripped the assert fires ("INFO response must be 22 bytes packed"). 4. (Medium) T-404's planned mitigation -- restrict INFO/LOG to authenticated sessions -- would remove the capability byte this PR relies on being readable before AUTH. That is a design decision, so it is ADR-021: INFO stays unauthenticated, deliberately, because everything it discloses an attacker learns by trying and the integrator needs it before AUTH; LOG stays authenticated, because boot history is the material T-404 is right to protect. T-404 now records caps as disclosed, narrows the mitigation to LOG, and cites the ADR. The ADR index has the row. 5. (Low) docs/architecture.md's command table now gives INFO's 22-byte packed layout field by field, so the wire format has one written definition alongside the struct's assert and the client's parser. Tests: test_uart_recovery.py the reviewer's 24-byte reproduction, byte for byte: info() False, "pre-fix firmware" and "24 bytes" printed, "10601K" and "Entropy:" NOT printed, buffer empty, timeout restored; plus a good-path test that the timeout is restored there too. The 22-byte test no longer plants a stray trailing byte, since a trailing byte is now by design a rejection. Negative controls: tools/uart_recovery.py at e14456f, tests kept -> 1 of 9 fails (the 24-byte case) packing guards stripped from core/recovery.c -> _Static_assert fires At this head: ctest 31/31, test_recovery 9/9, pytest tests/ 93 passed. core/recovery.c is CRLF and stays CRLF (574 -> 585). --- core/recovery.c | 11 +++ ...021-recovery-info-stays-unauthenticated.md | 70 +++++++++++++++++++ docs/adr/README.md | 1 + docs/architecture.md | 2 +- docs/threat_model.md | 2 +- tests/unit/test_uart_recovery.py | 51 +++++++++++--- tools/uart_recovery.py | 30 ++++++++ 7 files changed, 157 insertions(+), 10 deletions(-) create mode 100644 docs/adr/ADR-021-recovery-info-stays-unauthenticated.md diff --git a/core/recovery.c b/core/recovery.c index 69c76a9..8392a8c 100644 --- a/core/recovery.c +++ b/core/recovery.c @@ -262,6 +262,9 @@ static int recovery_handle_info(void) * such a board is impossible by construction, and cost 15 s of backoff * to discover. It reveals nothing an attacker could not learn by trying. */ +#ifdef _MSC_VER +#pragma pack(push, 1) +#endif struct rcvr_info { uint8_t ack; uint32_t flash_size; @@ -275,6 +278,14 @@ static int recovery_handle_info(void) __attribute__((packed)) #endif info; +#ifdef _MSC_VER +#pragma pack(pop) +#endif + /* The layout is the contract with tools/uart_recovery.py. A toolchain + * that honours neither the attribute nor the pragma would put the + * padding -- and the leak -- straight back, with nothing failing; this + * turns that into a compile error instead of a shipped defect. */ + _Static_assert(sizeof(struct rcvr_info) == 22, "INFO response must be 22 bytes packed"); memset(&info, 0, sizeof(info)); info.ack = RCVR_ACK; diff --git a/docs/adr/ADR-021-recovery-info-stays-unauthenticated.md b/docs/adr/ADR-021-recovery-info-stays-unauthenticated.md new file mode 100644 index 0000000..983aa8b --- /dev/null +++ b/docs/adr/ADR-021-recovery-info-stays-unauthenticated.md @@ -0,0 +1,70 @@ +--- +adr: 21 +title: Recovery INFO stays unauthenticated and carries a capability byte; LOG does not +status: Proposed +date: 2026-09-15 +deciders: Architecture Council, eBoot maintainers +source: docs/threat_model.md T-404; the automated review of #131 (finding 2) and #139 (finding 4); .ai/security.md on documented weaker postures +--- + +# ADR-021 — Recovery INFO stays unauthenticated and carries a capability byte; LOG does not + +## Context + +`docs/threat_model.md` T-404 records the recovery `INFO` and `LOG` commands +as an information-disclosure surface, with the planned mitigation "Restrict +INFO/LOG to authenticated sessions (Phase 3)". + +#131 makes authenticated recovery unreachable *by construction* on a board +with no entropy source: `AUTH` refuses to issue a challenge it cannot make +random. The event it logs, `EOS_LOG_AUTH_NO_ENTROPY`, is readable only over +`LOG`, which requires the authentication the missing entropy prevents. What an +integrator on such a board observed was a bare `NACK` — indistinguishable from +a wrong secret, an unreadable OTP, or an unprovisioned one — after +`0+1+2+4+8 = 15 s` of backoff across five attempts. + +#139 answers that by adding one byte, `caps`, to the `INFO` response: +`RCVR_CAP_RNG` and `RCVR_CAP_OTP`, derived from the board's ops table. The +question "why does AUTH always NACK on this board?" is then answered before +the first `AUTH` is sent. That design depends on `INFO` being reachable +*before* authentication, which is exactly what T-404's planned mitigation +would remove. The two cannot both stand. + +## Decision + +1. **`INFO` remains unauthenticated, deliberately.** Its response is flash + geometry (five `uint32`) and the capability byte. Every one of those + values is something an attacker with UART access learns anyway — geometry + from the board's public documentation or from the first `ERASE` that + NACKs out of range, capabilities by attempting `AUTH` once. Disclosing + them costs nothing the attacker did not already have and buys the + integrator a diagnosis that otherwise costs 15 s and a wrong conclusion. + +2. **`LOG` remains authenticated.** Boot history — which slot booted, why the + previous boot failed, recovery entry and exit reasons — is the material + T-404 is right to protect, and nothing in #131 or #139 needs it before + authentication. + +3. **The `INFO` response is a wire contract:** 22 bytes, packed — `ack`, + five little-endian `uint32`, `caps` — defined in `core/recovery.c` with a + `_Static_assert` on its size, documented in `docs/architecture.md`, and + parsed by `tools/uart_recovery.py`, which refuses the 24-byte unpacked + response of pre-fix firmware rather than decoding it. Adding a field means + changing all three and the assert. + +## Consequences + +- T-404's mitigation narrows from "INFO/LOG" to "LOG", and its disclosure + column records `caps`. The `INFO` half is closed by this decision rather + than by Phase 3. +- A future field in `INFO` must pass the same test as `caps`: would an + attacker learn it by trying anyway? If not, it belongs behind `AUTH`, or in + `LOG`. +- The three bytes of uninitialised stack that `INFO` used to send (#140) are + a separate defect — a leak of *unintended* data — and are fixed regardless + of this decision. + +## Note on numbering + +ADR-020 is the highest number in use; this is 021. It stands alone rather +than extending ADR-020, which concerns the install path. diff --git a/docs/adr/README.md b/docs/adr/README.md index bc72656..d889841 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -14,6 +14,7 @@ superseded by a later record that names it. | ADR | Title | Status | |---|---|---| | 020 | [Install path verifies the signature before anti-rollback](ADR-020-install-path-verifies-signature-before-anti-rollback.md) | Proposed | +| 021 | [Recovery INFO stays unauthenticated and carries a capability byte; LOG does not](ADR-021-recovery-info-stays-unauthenticated.md) | Proposed | ## Note on numbering diff --git a/docs/architecture.md b/docs/architecture.md index fcd7efe..e67396f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -96,7 +96,7 @@ Reset_Handler() | Command | Code | Description | |-----------|------|---------------------------------| | PING | 0x01 | Identity check — returns "EOS" | -| INFO | 0x02 | Flash layout and slot addresses | +| INFO | 0x02 | Flash layout, slot addresses and capabilities. Unauthenticated (ADR-021). Response: 22 bytes packed -- `ack`, `flash_size`, `slot_a_addr`, `slot_a_size`, `slot_b_addr`, `slot_b_size` (LE `uint32`), `caps` (bit 0 RNG, bit 1 OTP) | | ERASE | 0x03 | Erase target slot | | WRITE | 0x04 | Write chunk to slot | | VERIFY | 0x05 | Verify slot image integrity | diff --git a/docs/threat_model.md b/docs/threat_model.md index ad4a0bb..0010ac6 100644 --- a/docs/threat_model.md +++ b/docs/threat_model.md @@ -139,7 +139,7 @@ External boundaries: | T-401 | Spoofing | Unauthorized party connects to UART and replaces firmware | Medium | Critical | High | Recovery authentication — challenge-response or shared secret (Phase 3) | Planned | | T-402 | Tampering | Malformed UART packets corrupt flash or boot state | Medium | High | High | Packet validation; length bounds checking; CRC on write data | Partial | | T-403 | Denial of Service | Flood UART with garbage to prevent legitimate recovery | Low | Medium | Low | Command timeout; watchdog feed during recovery; rate limiting | Partial | -| T-404 | Information Disclosure | INFO and LOG commands leak device configuration and boot history | Low | Medium | Low | Restrict INFO/LOG to authenticated sessions (Phase 3) | Planned | +| T-404 | Information Disclosure | INFO discloses flash geometry and, since #139, a capability byte (`caps`: has RNG, has OTP); LOG discloses boot history | Low | Medium | Low | LOG restricted to authenticated sessions (Phase 3). INFO is deliberately unauthenticated per ADR-021: everything it discloses an attacker learns by trying, and the integrator needs it before AUTH to diagnose a board with no entropy source | Partial (INFO decided; LOG planned) | | T-405 | Repudiation | Recovery operations not logged | Medium | Medium | Medium | Boot log records recovery entry, commands executed, exit reason | Planned | | T-406 | Elevation of Privilege | FACTORY command erases all data without authorization | Medium | Critical | High | Require authentication before FACTORY; confirmation sequence | Planned | diff --git a/tests/unit/test_uart_recovery.py b/tests/unit/test_uart_recovery.py index 8d23230..a57f237 100644 --- a/tests/unit/test_uart_recovery.py +++ b/tests/unit/test_uart_recovery.py @@ -15,6 +15,7 @@ def __init__(self, incoming=b""): self.incoming = bytearray(incoming) self.writes = [] self.closed = False + self.timeout = 5.0 # settable, like serial.Serial.timeout def read(self, size): chunk = bytes(self.incoming[:size]) @@ -104,20 +105,20 @@ def test_log_rejects_incomplete_payload(monkeypatch, capsys): def test_info_decodes_the_packed_22_byte_response(monkeypatch, capsys): """The firmware sends ack, five little-endian uint32 and a caps byte, packed: - 22 bytes. The client must read exactly that many -- reading fewer leaves - bytes in the serial buffer that desynchronise the next command, which is - what the three padding bytes used to do -- and decode the geometry from the - same offsets test_recovery.c asserts the firmware writes them at.""" + 22 bytes, and nothing after. The client reads those, probes for surplus + bytes (which would mean pre-fix firmware -- see the 24-byte test), finds + none, and decodes the geometry from the same offsets test_recovery.c + asserts the firmware writes them at.""" payload = (bytes([0xAA]) + struct.pack(" bool: # ack(1) + five uint32 (20) + caps(1): the packed layout core/recovery.c sends. INFO_LEN = 22 + # What firmware before the packed-struct fix sent: ack, three bytes of + # padding, then the five fields -- 24 bytes, never the 21 the client read. + LEGACY_INFO_LEN = 24 + LEGACY_PROBE_TIMEOUT = 0.25 # seconds; generous for two bytes on any baud + def info(self): self._send_packet(CMD_INFO) response = self.ser.read(self.INFO_LEN) if len(response) < self.INFO_LEN or response[0] != ACK: print("Failed to get device info") return False + # A device running pre-fix firmware sends 24 bytes. read(22) of those + # succeeds, and decoding it prints the leaked padding as the flash size + # and invents a capability byte from a slot-size byte -- the exact + # defect this client is meant to have stopped showing. So: probe for + # more, with a short timeout of its own -- two bytes at 115200 baud + # take well under a millisecond, and the port's 5 s TIMEOUT is for + # flash operations, not this. A short read rather than in_waiting, + # because on a real UART those two bytes may still be on the wire + # when read(22) returns and in_waiting would report 0. + saved_timeout = self.ser.timeout + self.ser.timeout = self.LEGACY_PROBE_TIMEOUT + try: + extra = self.ser.read(self.LEGACY_INFO_LEN - self.INFO_LEN) + finally: + self.ser.timeout = saved_timeout + if extra: + print("Device is running pre-fix firmware: INFO responded with " + f"{self.INFO_LEN + len(extra)} bytes (unpacked layout). " + "Its geometry cannot be decoded reliably and its first three " + "bytes after ACK are uninitialised stack. Update the " + "bootloader before using this tool.") + # drain whatever else is queued so the next command starts clean + while self.ser.read(64): + pass + return False flash_size, slot_a_addr, slot_a_size, slot_b_addr, slot_b_size = \ struct.unpack(' Date: Tue, 15 Sep 2026 13:36:39 +0530 Subject: [PATCH 7/7] fix(recovery): build the INFO response as little-endian bytes, not a struct Finding 6 (Low) from the review of e14456f, and it supersedes the finding-3 fix from 2693259 with a stronger one. The five uint32 fields went on the wire in target byte order while tools/uart_recovery.py unpacks ' 00 00 01 08). core/recovery.c is CRLF and stays CRLF. --- core/recovery.c | 88 ++++++++++++++++++++++++------------------------- 1 file changed, 43 insertions(+), 45 deletions(-) diff --git a/core/recovery.c b/core/recovery.c index 8392a8c..fafdc95 100644 --- a/core/recovery.c +++ b/core/recovery.c @@ -28,6 +28,8 @@ /* Bits of the caps byte in the INFO response. */ #define RCVR_CAP_RNG 0x01 /* board has an entropy source: AUTH can issue a challenge */ #define RCVR_CAP_OTP 0x02 /* board has OTP: a shared secret can be provisioned */ +/* INFO response: ack(1) + five little-endian uint32 (20) + caps(1). */ +#define RCVR_INFO_LEN 22 #define RCVR_CMD_ERASE 0x03 #define RCVR_CMD_WRITE 0x04 #define RCVR_CMD_VERIFY 0x05 @@ -242,62 +244,58 @@ static int recovery_handle_ping(void) return eos_hal_uart_send(response, sizeof(response)); } +/* Little-endian store: the recovery protocol's byte order, independent of + * the target's. Returns the next offset. */ +static size_t rcvr_put_le32(uint8_t *buf, size_t at, uint32_t v) +{ + buf[at + 0] = (uint8_t)(v & 0xFF); + buf[at + 1] = (uint8_t)((v >> 8) & 0xFF); + buf[at + 2] = (uint8_t)((v >> 16) & 0xFF); + buf[at + 3] = (uint8_t)((v >> 24) & 0xFF); + return at + 4; +} + static int recovery_handle_info(void) { const eos_board_ops_t *ops = eos_hal_get_ops(); if (!ops) return recovery_send_nack(); - /* Packed, like rcvr_packet_t above it, and zeroed before any field is - * set. Unpacked, this struct was 24 bytes with three bytes of padding - * after ack that nothing wrote, and sizeof(info) sent all 24: an - * unauthenticated caller -- INFO needs no auth -- received three bytes - * of whatever the previous call had left on the stack, and the repo's - * own client, which has always parsed the packed layout, printed them - * as the flash size. The layout on the wire is now what the client - * reads, and memset() means a future field cannot open a new hole. + /* The INFO response is a wire format, so it is built as bytes, not as a + * struct. Before this it was an unpacked struct -- 24 bytes, three of + * padding after ack that nothing wrote -- and sizeof(info) sent all 24: + * an unauthenticated caller (INFO needs no auth) received three bytes of + * whatever the previous call had left on the stack, and the repo's own + * client, which has always parsed the packed layout, printed them as the + * flash size. A byte buffer cannot have padding on any toolchain, needs + * no packing attribute or pragma to stay 22 bytes, and puts the five + * words on the wire little-endian regardless of the target's byte order + * -- which is what tools/uart_recovery.py has always assumed ('flash_size; - info.slot_a_addr = ops->slot_a_addr; - info.slot_a_size = ops->slot_a_size; - info.slot_b_addr = ops->slot_b_addr; - info.slot_b_size = ops->slot_b_size; - info.caps = (uint8_t)((ops->rng_get ? RCVR_CAP_RNG : 0) | - (ops->otp_read ? RCVR_CAP_OTP : 0)); - - return eos_hal_uart_send(&info, sizeof(info)); + uint8_t info[RCVR_INFO_LEN]; + size_t i = 0; + memset(info, 0, sizeof(info)); + info[i++] = RCVR_ACK; + i = rcvr_put_le32(info, i, ops->flash_size); + i = rcvr_put_le32(info, i, ops->slot_a_addr); + i = rcvr_put_le32(info, i, ops->slot_a_size); + i = rcvr_put_le32(info, i, ops->slot_b_addr); + i = rcvr_put_le32(info, i, ops->slot_b_size); + info[i++] = (uint8_t)((ops->rng_get ? RCVR_CAP_RNG : 0) | + (ops->otp_read ? RCVR_CAP_OTP : 0)); + /* Every byte accounted for: the length is the contract, not a sizeof. */ + if (i != RCVR_INFO_LEN) + return recovery_send_nack(); + + return eos_hal_uart_send(info, sizeof(info)); } static int recovery_handle_erase(eos_slot_t slot)