Skip to content

Fix OOB flash write and OOB stack read in recovery firmware-update path - #33

Open
simran1002 wants to merge 1 commit into
embeddedos-org:masterfrom
simran1002:fix/recovery-oob-write-and-ymodem-oob-read
Open

Fix OOB flash write and OOB stack read in recovery firmware-update path#33
simran1002 wants to merge 1 commit into
embeddedos-org:masterfrom
simran1002:fix/recovery-oob-write-and-ymodem-oob-read

Conversation

@simran1002

Copy link
Copy Markdown

Fix: two out-of-bounds bugs in the recovery-mode firmware-update path

The issue

Two memory-safety bugs in the code that handles field firmware updates over
UART — the most attacker-facing surface in a bootloader, since it's reachable
by anyone who can talk to the recovery port, and it runs before any application
code:

1. Out-of-bounds flash write (core/recovery.c, recovery_handle_write())

The WRITE command's offset field comes straight off the wire and was
checked only against the write's own chunk size (len > sizeof(buf)) — never
against the target slot's actual size. eos_hal_flash_write() is a thin
pass-through to raw flash writes with no bounds checking of its own. Result:
an authenticated recovery client (the command is auth-gated, but nothing more)
can write past a slot's boundary into the other slot, the boot-control
blocks, or the boot log — corrupting boot state or defeating A/B rollback
protection. This is a live path: eos_recovery_enter() is called directly from
stage1/main.c whenever no valid image is found.

2. Out-of-bounds stack read (core/fw_transport_uart.c, ymodem_receive())

For YMODEM "block 0" (the filename/size header), only block_size + 2 bytes
(130 of 1026) of the block stack buffer are actually filled by
eos_hal_uart_recv(). Parsing then did strlen(name) over that buffer — if
the sender never sends a NUL within the received region, strlen walks off
into ~896 bytes of uninitialized stack looking for a zero byte. The
size-parsing loop right after it had the same problem. A malicious or just
buggy YMODEM sender can crash or hang the bootloader mid-update.

The fix

  • recovery_handle_write(): validate (uint64_t)offset + len <= eos_hal_slot_size(slot) before touching flash, NACK otherwise. Matches the
    bound-check the project's own docs/security_review_checklist.md already
    calls out ("Flash read/write operations validate address ranges against
    slot boundaries") but wasn't actually implemented here.
  • ymodem_receive(): replace strlen() with strnlen(name, block_size), and
    bound the size-digit scan to the actually-received region
    (block + block_size) instead of scanning until a non-digit byte turns up
    wherever it turns up.

Testing

Added tests/unit/test_recovery.c — an end-to-end test that drives the real
eos_recovery_enter() state machine through a simulated UART/flash/OTP, not a
mock of the handler function:

  1. Authenticates via the actual challenge-response protocol (SHA-256 over a
    deterministic simulated RNG challenge + a simulated OTP shared secret,
    computed with the same primitives the bootloader uses).
  2. Sends a WRITE with offset exactly at the slot boundary — asserts it's
    NACKed and flash is untouched (fails before the fix: it would ACK and
    attempt the out-of-bounds write).
  3. Sends a normal in-bounds WRITE immediately after — asserts it still
    succeeds, proving the fix doesn't affect legitimate writes.

Registered in tests/CMakeLists.txt. Full suite (Docker/Ubuntu 22.04, no
local toolchain available): ctest --test-dir build12/12 passed
(11 pre-existing + 1 new).

The strnlen/scan-bound fix for fw_transport_uart.c doesn't yet have a
dedicated test — see limitations below.

Limitations / considerations

  • Only the host/native build was tested (via Docker); the cross-compiled
    ARM/RISC-V board targets under boards/ were not built or tested.
  • No regression test was added for the YMODEM strnlen fix specifically —
    it would need a UART transport test harness that doesn't currently exist
    for fw_transport_uart.c. Worth a follow-up.
  • Not fixed here (out of scope): tests/fuzz/fuzz_recovery_protocol.c
    references a function, eos_recovery_parse_packet, that doesn't exist
    anywhere in core/recovery.c — that fuzz harness appears to be dead code
    that has never actually compiled/linked. Flagging it since it's exactly the
    kind of harness that should be exercising this code path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant