diff --git a/PROPOSAL.md b/PROPOSAL.md new file mode 100644 index 0000000..b4c9b0c --- /dev/null +++ b/PROPOSAL.md @@ -0,0 +1,260 @@ +# eBoot — Technical Assessment Submission + +**N Vignesh Reddy** · nvigneshreddy26@gmail.com · github.com/vignesh917 +**Project reviewed:** eBoot (`embeddedos-org/eBoot`), commit `13a7a02` +**Finding:** the anti-rollback security counter is read from unauthenticated flash + +--- + +## 1. The issue + +eBoot's anti-rollback gate reads the image's security counter from the +`EOS_TLV_MIN_SEC_VER` entry in the image's TLV area +(`core/rollback.c::eos_rollback_read_image_counter`), and `eos_secure_boot()` +uses that value in step 5b to decide whether an image is new enough to boot. + +The TLV area is not covered by anything the bootloader verifies: + +| Check | Region it covers | +|---|---| +| `eos_image_verify_signature()` | `header[0 .. EOS_IMG_SIGNED_LEN)` = bytes 0–91 | +| `eos_image_verify_integrity()` | `[hdr_size, hdr_size + image_size)` — the payload | +| TLV area | starts at `hdr_size + image_size` — **after** both | + +So the one input that decides whether a downgrade is allowed sits in the only +part of the image nothing signs or hashes. + +An attacker who can write flash — physical access, an exposed debug port, or a +compromised recovery agent, since `recovery_handle_write()` accepts any offset +inside the slot — can take a **genuinely signed old image**, rewrite four bytes +of TLV so its declared counter clears the device floor, and boot it. That is +exactly the downgrade anti-rollback exists to stop. This is not a bypass of +signature checking: the image is authentic, it is just old, and the mechanism +meant to reject it can be told otherwise. + +A second consequence: `eos_secure_boot()` calls `eos_rollback_stage(img_counter)` +with the attacker's value, and `eos_bootctl_confirm()` later commits it to the +monotonic counter. `EOS_ROLLBACK_MAX_STEP` caps a single advance at 16, but a +repeatable +16 per confirmed boot is a path to burning the fuse counter past +every legitimate image — a permanent brick. + +I also found the codebase disagreed with itself about where the TLV area is. +`include/eos_image_tlv.h` documented `[image_header][TLV area][payload]` and +claimed "TLVs are covered by the image signature for tamper protection"; +`core/image_verify.c` and `core/rollback.c` both implement +`[image_header][payload][TLV area]`, and the signature covers no part of it. +`tools/eos_sign.py` follows the header's layout and carries a `KNOWN LIMITATION` +note saying the images it produces do not boot as a result. + +### Demonstration against unmodified `13a7a02` + +A short host program against `libeboot_core` (`poc/tlv_downgrade_poc.c`), with +the device floor at 9 and a signed image declaring counter 3: + +``` +device anti-rollback floor : 9 +read_image_counter (untampered) : rc=0 counter=3 +rollback_verify(3) : -14 (rejected, as it should be) + +after rewriting 4 TLV bytes: + signed header prefix changed? : NO + parse_header : 0 + verify_integrity (SHA-256) : 0 (still PASSES) + read_image_counter : rc=0 counter=9 + rollback_verify(9) : 0 (ACCEPTED -- downgrade succeeded) +``` + +Same binary against the patched tree: the counter reads 0 both before and +after, and `rollback_verify` returns `EOS_ERR_ANTI_ROLLBACK` in both cases. + +--- + +## 2. Proposed solution + +Bind the TLV area to the signed image header, the way mcuboot's *protected* +TLVs are covered by the signature, rather than trusting bytes that sit outside +every check. + +The header has 30 reserved bytes at offsets 62–91, and — usefully — +`EOS_IMG_SIGNED_LEN` is 92, so those bytes are already inside the signed +prefix. I split them into: + +```c +uint16_t tlv_len; /* offset 62, 2 bytes */ +uint8_t tlv_hash[EOS_IMG_TLV_HASH_LEN]; /* offset 64, 28 bytes */ +``` + +`tlv_hash` is SHA-256 of the TLV area truncated to 224 bits. Every other field +keeps the byte offset the signing tools address, the header stays 156 bytes on +the wire, and the static asserts in `eos_image.h` (plus `test_image_abi.c`) pin +that. + +`eos_rollback_read_image_counter()` then: + +- `tlv_len == 0` → the image makes no authenticated claim about a TLV area, so + the area is not read and the counter reports **0**; +- `tlv_len` outside `[sizeof(eos_tlv_info_t), EOS_TLV_MAX_SIZE]`, or the address + range wrapping → `EOS_ERR_INVALID`; +- otherwise, hash `tlv_len` bytes at `hdr_size + image_size` and compare against + `tlv_hash`. Mismatch → `EOS_ERR_INVALID`; match → parse and read the counter + as before. + +--- + +## 3. Technical approach and why these choices + +**Why 224 bits and not a full 32-byte digest.** A full digest plus a length +needs 36 bytes and there are 30. Widening the header would move `signature[]` +off offset 92 and break the on-disk format that `sign_image.py`, `imgpack.py`, +`eos_sign.py` and `test_image_abi.c` all address by absolute offset. Truncated +SHA-256 is standard practice (SHA-256/224 is a FIPS 180-4 variant), and what +matters here is second-preimage resistance on a ~64-byte blob — 224 bits is far +past anything reachable. Trading 32 bits of margin for a format that does not +move felt like the right side of that trade; I have noted the alternative in +§5. + +**Why absence reads as 0 rather than failing.** A counter of 0 can only fail +against the device floor, never clear it. So "no authenticated TLV area" is +already the conservative answer and needs no special case. This is also what +keeps the change backward compatible: every image the project's tools currently +produce has those bytes zeroed, so `tlv_len` reads 0 and behaviour is identical +to today. `tools/sign_image.py` — the one the CI path uses — emits +`[header][payload]` with no TLV area at all, so nothing regresses there. + +**Why a claimed-but-mismatched area fails closed instead of falling back to 0.** +Falling back would let tampering degrade the check silently. If the header +vouches for an area and the bytes disagree, something rewrote flash; that +should be reported, and `eos_secure_boot()` already turns a non-`EOS_OK` return +from this function into a refusal to boot. + +**Why the digest is streamed.** The area is hashed in 64-byte chunks through +`eos_sha256_update()` rather than buffered, so stack use is fixed (~64 B + the +SHA-256 context) instead of scaling with `EOS_TLV_MAX_SIZE`. Stage-1 stacks on +the smaller targets in `boards/` are tight. + +**Why the comparison is constant-time.** It runs over attacker-influenced +bytes, and a bootloader gives an attacker unlimited retries with a logic +analyser. It matches the `secure_compare()` pattern already used in +`keystore.c` and `secure_boot.c`. + +**Documentation.** `eos_image_tlv.h` now states the layout the code actually +implements, and says plainly that the area is covered by neither the signature +nor `hash[]` and must be bound through `tlv_len`/`tlv_hash` to be trusted. + +--- + +## 4. Testing and validation + +**New regression test** — `tests/unit/test_tlv_auth.c`, registered in +`tests/CMakeLists.txt` for both ctest and the valgrind sweep. Seven cases: + +| Test | What it pins | +|---|---| +| `authenticated_tlv_counter_is_read` | a correctly bound area still reads normally | +| `tampered_tlv_counter_is_rejected` | the downgrade now returns `EOS_ERR_INVALID` | +| `tamper_is_invisible_to_signature_and_integrity` | the tampered image still passes SHA-256 and leaves the signed prefix byte-identical — proving the binding is the only thing that can catch it | +| `unbound_tlv_area_is_not_trusted` | an area the header does not vouch for reports 0 | +| `image_without_tlv_area_still_reports_zero` | no behaviour change for `sign_image.py` images | +| `tlv_binding_fields_are_inside_the_signed_prefix` | `tlv_len`/`tlv_hash` stay under `EOS_IMG_SIGNED_LEN` | +| `oversized_tlv_len_is_rejected` | a bogus `tlv_len` is refused before any hashing | + +The third case is the one I would point a reviewer at: it fails only if someone +later moves the TLV area back under an existing check, and it documents why the +new field exists. + +**Results** (Linux x86-64, GCC, `-DEBLDR_BUILD_TESTS=ON`, Debug): + +- `ctest` — **38/38 passed**, including 17 valgrind targets (`--leak-check=full + --error-exitcode=1`), up from 36 before. +- `python run_all_tests.py` — **33/33 passed**. +- `python tests/production_test_suite.py` — 1 failure remaining, `SA-6.9: CI + workflow has sanitizer job`, pre-existing and out of scope here. + +**Not covered by my testing, and I want to be clear about it:** I only built +and ran the native host configuration. I did not cross-compile for any board in +`boards/`, and I have no hardware, so nothing here is validated on a real +target. + +### Two small things fixed on the way + +Both were in the way of running the suite that proves the main change: + +- `tests/production_test_suite.py` hard-coded `/home/ubuntu/eBoot`, so it + crashed with a `FileNotFoundError` for anyone else. Now derived from + `__file__`. Running it revealed two pre-existing failures that the crash had + been hiding — the SPDX one below, and `SA-6.9`. +- `core/sha512.c` was missing its SPDX header (`SA-6.6`). Added. + +--- + +## 5. Limitations and things I would want a maintainer's call on + +1. **The signing tools are not updated to emit the binding.** Nothing regresses + — every current tool writes zeros into those bytes, which now means "no + authenticated TLV area" — but until `sign_image.py` learns to append a TLV + area and fill `tlv_len`/`tlv_hash`, a TLV-declared security counter is + effectively unusable rather than merely untrusted. That is a deliberate + fail-closed default, not a finished feature. I left it out because the + format question in §5.2 should be settled first. + +2. **This is a header-format decision, not just a bug fix.** Consuming the + reserved bytes is cheap and offset-stable, but it spends the header's only + remaining space and settles on a truncated digest. The alternatives — + bumping `hdr_version` to 3 and widening the header, or moving the TLV area + in front of the payload so `hash[]` covers it (which is what + `eos_image_tlv.h` used to claim and `eos_sign.py` still assumes) — are both + defensible and both break existing images. I picked the option that changes + nothing for images already in the field; a maintainer may reasonably prefer + one of the others. + +3. **The `eos_sign.py` / `sign_image.py` split is still unresolved.** + `eos_sign.py` emits `[header][TLV][payload]`, which no part of the + bootloader reads. My change makes the intended layout explicit in the + headers, but does not reconcile the two tools. + +4. **Threat model.** This matters only against an attacker who can write flash. + That is deliberately the anti-rollback threat model — an attacker who cannot + write flash has no way to install the old image in the first place — but it + is worth stating that this is not a remote-network finding. + +5. **The TLV read is still not bounded against slot capacity.** `verify_slot()` + bounds `hdr_size + image_size` against `eos_hal_slot_size()`, but nothing + bounds the TLV area that follows, so hashing `tlv_len` bytes can read up to + 512 bytes past the end of a slot that an image exactly fills. This is not a + regression — `eos_tlv_parse()` already read the same region, bounded the + same way — and every HAL `flash_read` I looked at rejects an out-of-range + address. It is still a loose end, and the natural place to close it is the + slot-capacity check in `slot_manager.c`, alongside the one already there. + +6. **Not addressed, though I noticed them.** `eos_secure_boot()` step 4 reads + the OTP root-of-trust key hash and then does nothing with it (two `TODO`-ish + comments where the TLV keyhash comparison should be). `eos_storage_dump()` + is the only function in `storage.c` that does not null-check `dev`. Both + looked like separate changes rather than things to fold into this one. + +--- + +## Files changed + +``` + core/rollback.c | 69 +++++++++ the fix + include/eos_image.h | 34 ++++- tlv_len / tlv_hash + static asserts + include/eos_image_tlv.h | 16 ++- corrected layout documentation + tests/unit/test_tlv_auth.c | 309 +++++++++ new regression test + tests/CMakeLists.txt | 8 +- register it (ctest + valgrind) + tests/unit/test_image_abi.c | 3 +- ABI pins for the new fields + tests/unit/test_image_verify.c | 3 +- signed-prefix coverage assertions + tests/unit/test_slot_manager.c | 8 +- mock used reserved[0] + tests/production_test_suite.py | 6 +- hard-coded path + core/sha512.c | 3 + missing SPDX header + tools/eos_sign.py | 2 +- comment: name the new fields + tools/imgpack.py | 2 +- same +``` + +Per the screening instructions I have not opened a pull request against +`embeddedos-org/eBoot`. The branch and the proof-of-concept are on my fork and +I am happy to open one if the team would like it. + +I used AI assistance while working through this, and I have read and can walk +through every line of it — including why 224 bits, why absence reads as zero, +and why the mismatch case fails closed rather than degrading. diff --git a/RUN.md b/RUN.md new file mode 100644 index 0000000..471b00c --- /dev/null +++ b/RUN.md @@ -0,0 +1,179 @@ +# eBoot — TLV anti-rollback fix: build, test, and reproduce + +Everything below runs on a plain Linux host (or macOS). No board, no +cross-compiler, no hardware needed — the native build compiles the +platform-agnostic core libraries and the whole unit-test suite. + +The finding and the reasoning are in `PROPOSAL.md`. + +--- + +## 0. Prerequisites + +```bash +# Debian / Ubuntu +sudo apt update +sudo apt install -y build-essential cmake git python3 python3-pip valgrind + +# macOS (Homebrew) +brew install cmake python git +``` + +Python bits used by the script-driven suites: + +```bash +pip3 install -r requirements.txt +pip3 install pytest +``` + +> On newer Debian/Ubuntu, `pip3` may refuse to touch system packages. Either +> use a virtualenv, or append `--break-system-packages` to the two commands. + +`valgrind` is optional. Without it, CMake skips the 17 valgrind targets and +`ctest` runs 21 tests instead of 38; everything else is identical. + +--- + +## 1. Build + +```bash +cd eBoot +cmake -B build -DEBLDR_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug +cmake --build build --parallel +``` + +One `#warning` during the build is expected and intentional — it fires because +no production signing key is configured, so the RFC 8032 test-vector key is +being compiled in as the trust anchor. + +--- + +## 2. Run the tests + +```bash +ctest --test-dir build --output-on-failure +``` + +Expected: **`100% tests passed, 0 tests failed out of 38`** (21 without +valgrind). The new one is `test_tlv_auth`, which you can run on its own: + +```bash +./build/tests/eboot_test_tlv_auth +``` + +``` +TLV authentication (anti-rollback counter) + + test_authenticated_tlv_counter_is_read [PASS] + test_tampered_tlv_counter_is_rejected [PASS] + test_tamper_is_invisible_to_signature_and_integrity [PASS] + test_unbound_tlv_area_is_not_trusted [PASS] + test_image_without_tlv_area_still_reports_zero [PASS] + test_tlv_binding_fields_are_inside_the_signed_prefix [PASS] + test_oversized_tlv_len_is_rejected [PASS] + +7/7 passed +``` + +The script-driven suites: + +```bash +python3 run_all_tests.py # expect 33 passed +python3 tests/production_test_suite.py # expect 1 failure: SA-6.9 (pre-existing) +``` + +`SA-6.9` wants a sanitizer job in the CI workflow. It is unrelated to this +change and deliberately left alone. + +--- + +## 3. Reproduce the original vulnerability + +This is the part worth doing yourself. It builds **upstream, unpatched** eBoot +in a throwaway worktree and runs the proof of concept against it. + +```bash +export EBOOT="$PWD" # run this from the root of this archive +git worktree add /tmp/eboot-upstream 13a7a02 +cd /tmp/eboot-upstream +cmake -B build -DCMAKE_BUILD_TYPE=Debug +cmake --build build --parallel +gcc -I include "$EBOOT/poc/tlv_downgrade_poc.c" -o /tmp/poc \ + build/libeboot_core.a build/libeboot_hal.a -lm +/tmp/poc +``` + +**Before the fix — the downgrade succeeds:** + +``` +device anti-rollback floor : 9 +read_image_counter (untampered) : rc=0 counter=3 +rollback_verify(3) : -14 (rejected, as it should be) + +after rewriting 4 TLV bytes: + signed header prefix changed? : NO + parse_header : 0 + verify_integrity (SHA-256) : 0 (still PASSES) + read_image_counter : rc=0 counter=9 + rollback_verify(9) : 0 (ACCEPTED -- downgrade succeeded) +``` + +Four bytes were rewritten. The SHA-256 payload check still passes and not one +byte of the signed header prefix changed — because the TLV area sits outside +both. + +**After the fix — same binary, patched library:** + +```bash +cd "$EBOOT" +gcc -I include poc/tlv_downgrade_poc.c -o /tmp/poc_after \ + build/libeboot_core.a build/libeboot_hal.a -lm +/tmp/poc_after +``` + +``` +read_image_counter (untampered) : rc=0 counter=0 +rollback_verify(0) : -14 (rejected, as it should be) +... + read_image_counter : rc=0 counter=0 + rollback_verify(0) : -14 (rejected) +``` + +The image declares no authenticated TLV area, so nothing in it can raise the +anti-rollback floor. + +Clean up the worktree when done: + +```bash +git worktree remove --force /tmp/eboot-upstream +``` + +--- + +## 4. Read the change + +```bash +git log --oneline 13a7a02..HEAD +git diff 13a7a02 -- core/rollback.c include/eos_image.h +git show --stat HEAD~1 +``` + +The substance is in three files: + +| File | What changed | +|---|---| +| `include/eos_image.h` | `reserved[30]` → `tlv_len` (2 B) + `tlv_hash` (28 B), both inside the signed prefix, plus static asserts pinning the offsets | +| `core/rollback.c` | hash the declared TLV area and refuse to read a counter from one that does not match | +| `tests/unit/test_tlv_auth.c` | the regression test, 7 cases | + +--- + +## 5. Optional: cross-compile check + +Not required, and not something I validated. If you have the toolchain: + +```bash +cmake -B build-arm -DEBLDR_BOARD=stm32f4 \ + -DCMAKE_TOOLCHAIN_FILE=toolchains/arm-none-eabi.cmake +cmake --build build-arm --parallel +``` diff --git a/core/rollback.c b/core/rollback.c index 35810ef..39405e7 100644 --- a/core/rollback.c +++ b/core/rollback.c @@ -10,12 +10,52 @@ #include "eos_rollback.h" #include "eos_image.h" #include "eos_image_tlv.h" +#include "eos_crypto_boot.h" #include "eos_hal.h" #include static uint32_t g_staged_counter; static bool g_staged_valid; +/** + * Check that the TLV area at @p tlv_addr is the one the signed header vouches + * for, by hashing hdr->tlv_len bytes of it and comparing against hdr->tlv_hash. + * + * Streams the area in small chunks rather than buffering it, so the stack cost + * does not scale with EOS_TLV_MAX_SIZE. + */ +static int tlv_area_matches_header(const eos_image_header_t *hdr, + uint32_t tlv_addr) +{ + eos_sha256_ctx_t sha; + uint8_t chunk[64]; + uint8_t digest[EOS_SHA256_DIGEST_SIZE]; + uint32_t remaining = hdr->tlv_len; + uint32_t addr = tlv_addr; + + eos_sha256_init(&sha); + + while (remaining > 0) { + uint32_t n = (remaining > sizeof(chunk)) ? (uint32_t)sizeof(chunk) + : remaining; + if (eos_hal_flash_read(addr, chunk, n) != EOS_OK) + return EOS_ERR_FLASH; + eos_sha256_update(&sha, chunk, n); + addr += n; + remaining -= n; + } + + eos_sha256_final(&sha, digest); + + /* Constant-time: this compares attacker-influenced bytes, and a bootloader + * offers unlimited retries to anyone with a logic analyser. */ + volatile uint8_t diff = 0; + for (uint32_t i = 0; i < EOS_IMG_TLV_HASH_LEN; i++) + diff |= (uint8_t)(digest[i] ^ hdr->tlv_hash[i]); + + return (diff == 0) ? EOS_OK : EOS_ERR_INVALID; +} + int eos_rollback_read_image_counter(uint32_t image_addr, uint32_t *counter_out) { if (!counter_out) return EOS_ERR_INVALID; @@ -33,6 +73,35 @@ int eos_rollback_read_image_counter(uint32_t image_addr, uint32_t *counter_out) if (tlv_addr + hdr.image_size < tlv_addr) return EOS_ERR_INVALID; tlv_addr += hdr.image_size; + /* Nothing else in the image covers those bytes: the signature stops at + * EOS_IMG_SIGNED_LEN and hash[] stops after image_size payload bytes. An + * attacker able to write flash could otherwise take a genuinely signed old + * image, raise its declared counter, and walk it straight past + * eos_rollback_verify() — the downgrade anti-rollback exists to prevent. + * + * hdr.tlv_len and hdr.tlv_hash are inside the signed prefix, so they are + * the only place a claim about this area can be trusted from. + * + * tlv_len == 0 means the image makes no such claim. The area is then + * unauthenticated and is not read: reporting 0 is the conservative + * reading, since a counter of 0 can only fail against the device floor, + * never raise it. */ + if (hdr.tlv_len == 0) + return EOS_OK; + + if (hdr.tlv_len < sizeof(eos_tlv_info_t) || hdr.tlv_len > EOS_TLV_MAX_SIZE) + return EOS_ERR_INVALID; + + if (tlv_addr + hdr.tlv_len < tlv_addr) + return EOS_ERR_INVALID; + + /* The header claims an authenticated area and the bytes do not match it. + * That is tampering, not absence — fail closed rather than degrading to 0, + * so the condition is reported instead of passing silently. */ + rc = tlv_area_matches_header(&hdr, tlv_addr); + if (rc != EOS_OK) + return rc; + eos_tlv_ctx_t ctx; rc = eos_tlv_parse(&ctx, tlv_addr); if (rc == EOS_ERR_NOT_FOUND) return EOS_OK; diff --git a/core/sha512.c b/core/sha512.c index 5510e47..d9aa57c 100644 --- a/core/sha512.c +++ b/core/sha512.c @@ -1,3 +1,6 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 EoS Project + #include "eos_crypto_boot.h" #include diff --git a/include/eos_image.h b/include/eos_image.h index a2a9d18..c7763bc 100644 --- a/include/eos_image.h +++ b/include/eos_image.h @@ -22,6 +22,22 @@ extern "C" { /* ---------------- Image Header ---------------- */ +/** + * @brief Bytes of the TLV area's SHA-256 digest carried in the header. + * + * The TLV area sits after the payload, so neither the signature (which covers + * the header prefix) nor hash[] (which covers exactly image_size payload bytes) + * reaches it. tlv_len and tlv_hash do, and they are inside the signed prefix — + * that is what makes a TLV-declared value such as EOS_TLV_MIN_SEC_VER + * trustworthy enough to gate anti-rollback on. + * + * The digest is truncated to 224 bits so the pair fits the 30 bytes previously + * reserved between sig_len and signature[], leaving every other field at the + * offset the signing tools already address. Second-preimage resistance at 224 + * bits is far beyond what an attacker rewriting a ~64-byte TLV blob can reach. + */ +#define EOS_IMG_TLV_HASH_LEN 28 + typedef struct { uint32_t magic; /* EOS_IMG_MAGIC */ uint16_t hdr_version; /* Header format version */ @@ -34,7 +50,8 @@ typedef struct { uint8_t hash[EOS_HASH_SIZE]; /* SHA-256 hash of payload */ uint8_t sig_type; /* eos_sig_type_t */ uint8_t sig_len; /* Actual signature length */ - uint8_t reserved[30]; /* Reserved for future use */ + uint16_t tlv_len; /* Bytes of TLV area following the payload; 0 = none */ + uint8_t tlv_hash[EOS_IMG_TLV_HASH_LEN]; /* Truncated SHA-256 of that area */ uint8_t signature[EOS_SIG_MAX_SIZE]; /* Digital signature */ } eos_image_header_t; @@ -51,7 +68,7 @@ typedef struct { * * Everything except the signature field itself: magic, hdr_version, hdr_size, * image_size, load_addr, entry_addr, image_version, flags, hash, sig_type, - * sig_len and reserved. + * sig_len, tlv_len and tlv_hash. * * Signing hash[] alone leaves every other field unauthenticated. An attacker * could keep a legitimately signed image's signature and still change the load @@ -77,8 +94,17 @@ EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, hash) == 28, "hash[] must stay at offset 28"); EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, sig_type) == 60, "sig_type must stay at offset 60"); +EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, tlv_len) == 62, + "tlv_len must occupy the first 2 of the 30 formerly " + "reserved bytes"); +EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, tlv_hash) == 64, + "tlv_hash must occupy the remaining 28 reserved bytes"); EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, signature) == 92, "signature[] must stay at offset 92"); +EOS_IMG_STATIC_ASSERT(offsetof(eos_image_header_t, tlv_hash) + + EOS_IMG_TLV_HASH_LEN == + offsetof(eos_image_header_t, signature), + "the TLV binding must be inside the signed prefix"); /* ---------------- Image Validation API ---------------- */ @@ -93,8 +119,8 @@ int eos_image_parse_header(uint32_t addr, eos_image_header_t *out); /** * @brief Verify image integrity using CRC32 or hash. * @param hdr Parsed image header. - * @param addr Flash address of the image (header base). Payload starts after - * hdr_size and an optional TLV area, matching eos_sign.py. + * @param addr Flash address of the image (header base). The payload starts at + * addr + hdr_size; any TLV area follows it, not precedes it. * @return EOS_OK if integrity check passes, EOS_ERR_CRC on failure. */ int eos_image_verify_integrity(const eos_image_header_t *hdr, uint32_t addr); diff --git a/include/eos_image_tlv.h b/include/eos_image_tlv.h index 2bc858b..cf8bd1e 100644 --- a/include/eos_image_tlv.h +++ b/include/eos_image_tlv.h @@ -6,12 +6,22 @@ * @file eos_image_tlv.h * @brief TLV (Type-Length-Value) metadata for firmware images * - * mcuboot-inspired TLV area appended after the fixed image header. - * TLVs are covered by the image signature for tamper protection. + * mcuboot-inspired TLV area carrying extensible image metadata. * * Layout: - * [image_header][TLV area][payload] + * [image_header][payload][TLV area] * TLV area = [tlv_info][tlv_entry_0][tlv_entry_1]... + * + * The TLV area follows the payload: image_verify.c reads the payload at + * hdr_size and rollback.c looks for the TLV info header at + * hdr_size + image_size. (This block used to document the opposite order, + * which matched neither.) + * + * These bytes are covered by NEITHER the signature (which stops at + * EOS_IMG_SIGNED_LEN) NOR hash[] (which covers exactly image_size payload + * bytes). An image that needs its TLVs trusted -- EOS_TLV_MIN_SEC_VER gates + * anti-rollback -- must bind them through eos_image_header_t::tlv_len and + * ::tlv_hash, which do sit inside the signed prefix. */ #ifndef EOS_IMAGE_TLV_H diff --git a/poc/README.md b/poc/README.md new file mode 100644 index 0000000..5484ab7 --- /dev/null +++ b/poc/README.md @@ -0,0 +1,45 @@ +# Proof of concept — unauthenticated anti-rollback counter + +`tlv_downgrade_poc.c` demonstrates, against **unmodified** eBoot at `13a7a02`, +that the security counter gating `eos_rollback_verify()` can be raised by +rewriting four bytes that neither the image signature nor the payload hash +covers. + +It is a host program, not a test: it links `libeboot_core` and a simulated +flash, lays down a well-formed image declaring counter 3, sets the device +anti-rollback floor to 9, and then rewrites the `EOS_TLV_MIN_SEC_VER` value in +the TLV area that sits after the payload. + +## Run it against upstream + +```bash +git checkout 13a7a02 +cmake -B build -DCMAKE_BUILD_TYPE=Debug && cmake --build build --parallel +gcc -I include poc/tlv_downgrade_poc.c -o /tmp/poc \ + build/libeboot_core.a build/libeboot_hal.a -lm +/tmp/poc +``` + +Expected output before the fix: + +``` +device anti-rollback floor : 9 +read_image_counter (untampered) : rc=0 counter=3 +rollback_verify(3) : -14 (rejected, as it should be) + +after rewriting 4 TLV bytes: + signed header prefix changed? : NO + parse_header : 0 + verify_integrity (SHA-256) : 0 (still PASSES) + read_image_counter : rc=0 counter=9 + rollback_verify(9) : 0 (ACCEPTED -- downgrade succeeded) +``` + +After the fix on this branch the same binary reports `counter=0` in both +cases and `rollback_verify` returns `-14` (`EOS_ERR_ANTI_ROLLBACK`) both +times: the image no longer declares an authenticated TLV area, so nothing in +it can raise the floor. + +The behaviour is pinned as a real regression test in +`tests/unit/test_tlv_auth.c`; this directory exists so the original finding +can be reproduced against upstream without applying the patch. diff --git a/poc/tlv_downgrade_poc.c b/poc/tlv_downgrade_poc.c new file mode 100644 index 0000000..6b72865 --- /dev/null +++ b/poc/tlv_downgrade_poc.c @@ -0,0 +1,77 @@ +/* PoC against unmodified eBoot @ 13a7a02: the anti-rollback counter can be + * raised without touching any byte the signature or payload hash covers. */ +#include "eos_image.h" +#include "eos_image_tlv.h" +#include "eos_rollback.h" +#include "eos_crypto_boot.h" +#include "eos_hal.h" +#include +#include + +#define FLASH_SZ (64*1024) +#define SLOT 0x1000u +#define PAY 256u +static uint8_t f[FLASH_SZ]; +static uint32_t hw_counter = 9; /* device floor: only >= 9 may boot */ + +static int fr(uint32_t a, void *b, size_t l){ if(a+l>FLASH_SZ) return EOS_ERR_FLASH; memcpy(b,&f[a],l); return EOS_OK; } +static int fw_(uint32_t a, const void *b, size_t l){ if(a+l>FLASH_SZ) return EOS_ERR_FLASH; memcpy(&f[a],b,l); return EOS_OK; } +static int fe(uint32_t a, size_t l){ if(a+l>FLASH_SZ) return EOS_ERR_FLASH; memset(&f[a],0xFF,l); return EOS_OK; } +static int mr(uint32_t *v){ *v = hw_counter; return EOS_OK; } +static const eos_board_ops_t ops = { .flash_size=FLASH_SZ, .slot_a_addr=SLOT, .slot_a_size=0x8000, + .flash_read=fr, .flash_write=fw_, .flash_erase=fe, .monotonic_read=mr }; + +int main(void) +{ + memset(f, 0xFF, sizeof f); + eos_hal_init(&ops); + + uint8_t pay[PAY]; + for (unsigned i=0;i diff --git a/tests/production_test_suite.py b/tests/production_test_suite.py index 8082ac8..01e535e 100644 --- a/tests/production_test_suite.py +++ b/tests/production_test_suite.py @@ -147,7 +147,7 @@ def make_image_header( hdr += hash_bytes[:EOS_HASH_SIZE] # 32 hdr += struct.pack('= 83 (actual: {board_count})", board_count >= 83, f"got {board_count}") diff --git a/tests/unit/test_image_abi.c b/tests/unit/test_image_abi.c index 375878b..c9726ad 100644 --- a/tests/unit/test_image_abi.c +++ b/tests/unit/test_image_abi.c @@ -71,7 +71,8 @@ int main(void) CHECK_FIELD(hash, 28, 32); CHECK_FIELD(sig_type, 60, 1); CHECK_FIELD(sig_len, 61, 1); - CHECK_FIELD(reserved, 62, 30); + CHECK_FIELD(tlv_len, 62, 2); + CHECK_FIELD(tlv_hash, 64, 28); CHECK_FIELD(signature, 92, 64); /* No padding anywhere, and none on the end. The struct is written to flash diff --git a/tests/unit/test_image_verify.c b/tests/unit/test_image_verify.c index 9e44af9..c275a4f 100644 --- a/tests/unit/test_image_verify.c +++ b/tests/unit/test_image_verify.c @@ -328,7 +328,8 @@ TEST(test_signed_region_covers_all_metadata) COVERED(hash); COVERED(sig_type); COVERED(sig_len); - COVERED(reserved); + COVERED(tlv_len); + COVERED(tlv_hash); #undef COVERED /* The signature itself is the only thing outside it. */ diff --git a/tests/unit/test_slot_manager.c b/tests/unit/test_slot_manager.c index ec12af6..5bca305 100644 --- a/tests/unit/test_slot_manager.c +++ b/tests/unit/test_slot_manager.c @@ -121,7 +121,7 @@ static int slot_index(uint32_t addr) /* ---- Image verification mocks (override eboot_core's real ones) ---- * * verify_slot() passes the parsed header straight to the integrity and - * signature checks, so the mocks stash the slot index in reserved[0] on + * signature checks, so the mocks stash the slot index in tlv_hash[0] on * parse and read it back to decide which scripted result to return. */ int eos_image_parse_header(uint32_t addr, eos_image_header_t *out) @@ -133,7 +133,7 @@ int eos_image_parse_header(uint32_t addr, eos_image_header_t *out) memset(out, 0, sizeof(*out)); out->magic = EOS_IMG_MAGIC; out->image_version = slot_version[slot]; - out->reserved[0] = (uint8_t)slot; + out->tlv_hash[0] = (uint8_t)slot; return EOS_OK; } @@ -146,8 +146,8 @@ int eos_image_verify_integrity(const eos_image_header_t *hdr, uint32_t addr) int eos_image_verify_signature(const eos_image_header_t *hdr) { - if (!hdr || hdr->reserved[0] > EOS_SLOT_B) return EOS_ERR_INVALID; - return signature_result[hdr->reserved[0]]; + if (!hdr || hdr->tlv_hash[0] > EOS_SLOT_B) return EOS_ERR_INVALID; + return signature_result[hdr->tlv_hash[0]]; } /* ---- Test harness ---- */ diff --git a/tests/unit/test_tlv_auth.c b/tests/unit/test_tlv_auth.c new file mode 100644 index 0000000..28c0e41 --- /dev/null +++ b/tests/unit/test_tlv_auth.c @@ -0,0 +1,309 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 EoS Project +// ISO/IEC 25000 | ISO/IEC/IEEE 15288:2023 + +/** + * @file test_tlv_auth.c + * @brief Regression test: the TLV area feeding anti-rollback must be + * authenticated by the signed image header. + * + * eos_rollback_read_image_counter() takes the image's security counter from + * the EOS_TLV_MIN_SEC_VER entry in the TLV area, which lives at + * image_addr + hdr_size + image_size + * i.e. immediately after the payload. + * + * Nothing used to cover those bytes: + * - eos_image_verify_signature() signs header[0, EOS_IMG_SIGNED_LEN) = [0,92) + * - eos_image_verify_integrity() hashes [hdr_size, hdr_size + image_size) + * + * The TLV area is disjoint from both. An attacker able to write flash could + * therefore take a genuinely signed *old* image, raise its declared counter, + * and walk it past eos_rollback_verify() — defeating the anti-rollback gate + * that exists precisely to stop that downgrade — without disturbing a single + * byte the signature or the payload hash covers. + * + * The fix binds the TLV area to the signed header via hdr.tlv_len and + * hdr.tlv_hash (both inside the signed prefix). This test pins that: + * 1. an authenticated TLV area is still read normally; + * 2. tampering with it is now detected and fails closed; + * 3. an image that declares no authenticated TLV area reports counter 0 + * rather than trusting whatever bytes happen to follow the payload; + * 4. the tampering in (2) really is invisible to signature/integrity + * coverage, so the header binding is the only thing that catches it. + */ + +#include "eos_image.h" +#include "eos_image_tlv.h" +#include "eos_rollback.h" +#include "eos_crypto_boot.h" +#include "eos_hal.h" +#include +#include +#include +#include + +#define SIM_FLASH_SIZE (64 * 1024) +#define SLOT_A_ADDR 0x1000u +#define SLOT_A_SIZE 0x8000u +#define PAYLOAD_SIZE 256u + +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 uint32_t sim_counter = 0; +static int sim_monotonic_read(uint32_t *value) +{ + if (!value) return EOS_ERR_INVALID; + *value = sim_counter; + return EOS_OK; +} + +static const eos_board_ops_t sim_ops = { + .flash_base = 0, + .flash_size = SIM_FLASH_SIZE, + .slot_a_addr = SLOT_A_ADDR, + .slot_a_size = SLOT_A_SIZE, + .flash_read = sim_flash_read, + .flash_write = sim_flash_write, + .flash_erase = sim_flash_erase, + .monotonic_read = sim_monotonic_read, +}; + +static int tests_run = 0; +static int tests_passed = 0; + +#define TEST(name) \ + static void name(void); \ + static void run_##name(void) { \ + memset(sim_flash, 0xFF, sizeof(sim_flash)); \ + sim_counter = 0; \ + eos_hal_init(&sim_ops); \ + printf(" %-58s ", #name); \ + name(); \ + tests_passed++; \ + printf("[PASS]\n"); \ + } \ + static void name(void) + +#define ASSERT(cond) do { \ + if (!(cond)) { \ + printf("[FAIL] %s:%d: %s\n", __FILE__, __LINE__, #cond); \ + exit(1); \ + } \ +} while(0) + +/* ------------------------------------------------------------------ + * Image construction + * ------------------------------------------------------------------ */ + +/* Byte offset of the TLV area, and of the MIN_SEC_VER value inside it. */ +static uint32_t tlv_offset(void) +{ + return SLOT_A_ADDR + sizeof(eos_image_header_t) + PAYLOAD_SIZE; +} + +/* [tlv_info(4)][entry_hdr(4)][uint32 value] */ +#define TLV_AREA_LEN (uint16_t)(sizeof(eos_tlv_info_t) + \ + sizeof(eos_tlv_entry_hdr_t) + sizeof(uint32_t)) +#define TLV_VALUE_OFF (uint32_t)(sizeof(eos_tlv_info_t) + sizeof(eos_tlv_entry_hdr_t)) + +/** + * Lay a well-formed image into the simulated flash. + * + * @param sec_ver value written into the MIN_SEC_VER TLV + * @param emit_tlv write a TLV area after the payload at all + * @param bind_tlv record tlv_len/tlv_hash in the header, i.e. sign it + */ +static void build_image(uint32_t sec_ver, bool emit_tlv, bool bind_tlv) +{ + uint8_t payload[PAYLOAD_SIZE]; + for (uint32_t i = 0; i < PAYLOAD_SIZE; i++) + payload[i] = (uint8_t)(i * 7u + 1u); + + eos_image_header_t hdr; + memset(&hdr, 0, sizeof(hdr)); + hdr.magic = EOS_IMG_MAGIC; + hdr.hdr_version = EOS_IMAGE_HDR_VERSION; + hdr.hdr_size = (uint16_t)sizeof(eos_image_header_t); + hdr.image_size = PAYLOAD_SIZE; + hdr.load_addr = 0; /* skips the entry_addr range check */ + hdr.entry_addr = 0; + hdr.image_version = 0x00010000u; + hdr.flags = EOS_IMG_FLAG_HASH_SHA256; + eos_sha256(payload, PAYLOAD_SIZE, hdr.hash); + hdr.sig_type = EOS_SIG_ED25519; + hdr.sig_len = EOS_SIG_MAX_SIZE; + + uint8_t tlv[TLV_AREA_LEN]; + if (emit_tlv) { + eos_tlv_info_t info = { EOS_TLV_INFO_MAGIC, TLV_AREA_LEN }; + eos_tlv_entry_hdr_t ent = { EOS_TLV_MIN_SEC_VER, sizeof(uint32_t) }; + memcpy(tlv, &info, sizeof(info)); + memcpy(tlv + sizeof(info), &ent, sizeof(ent)); + memcpy(tlv + TLV_VALUE_OFF, &sec_ver, sizeof(sec_ver)); + + if (bind_tlv) { + uint8_t digest[EOS_SHA256_DIGEST_SIZE]; + eos_sha256(tlv, TLV_AREA_LEN, digest); + hdr.tlv_len = TLV_AREA_LEN; + memcpy(hdr.tlv_hash, digest, EOS_IMG_TLV_HASH_LEN); + } + } + + memcpy(&sim_flash[SLOT_A_ADDR], &hdr, sizeof(hdr)); + memcpy(&sim_flash[SLOT_A_ADDR + sizeof(hdr)], payload, PAYLOAD_SIZE); + if (emit_tlv) + memcpy(&sim_flash[tlv_offset()], tlv, TLV_AREA_LEN); +} + +/* Rewrite the MIN_SEC_VER value in place, exactly as an attacker with flash + * write access would: no header byte and no payload byte is touched. */ +static void tamper_sec_ver(uint32_t new_value) +{ + memcpy(&sim_flash[tlv_offset() + TLV_VALUE_OFF], &new_value, sizeof(new_value)); +} + +/* ------------------------------------------------------------------ + * Tests + * ------------------------------------------------------------------ */ + +TEST(test_authenticated_tlv_counter_is_read) +{ + build_image(7, true, true); + + uint32_t counter = 0xDEADBEEF; + ASSERT(eos_rollback_read_image_counter(SLOT_A_ADDR, &counter) == EOS_OK); + ASSERT(counter == 7); +} + +TEST(test_tampered_tlv_counter_is_rejected) +{ + build_image(3, true, true); + + /* Sanity: it reads as signed before the tamper. */ + uint32_t counter = 0; + ASSERT(eos_rollback_read_image_counter(SLOT_A_ADDR, &counter) == EOS_OK); + ASSERT(counter == 3); + + /* The downgrade attack: raise the declared counter past the device floor + * so an old, genuinely signed image sails through eos_rollback_verify(). */ + sim_counter = 9; + ASSERT(eos_rollback_verify(3) == EOS_ERR_ANTI_ROLLBACK); + + tamper_sec_ver(9); + + counter = 0; + int rc = eos_rollback_read_image_counter(SLOT_A_ADDR, &counter); + + /* Fail closed. Before the header binding this returned EOS_OK with + * counter == 9, and eos_rollback_verify(9) then accepted the image. */ + ASSERT(rc == EOS_ERR_INVALID); +} + +TEST(test_tamper_is_invisible_to_signature_and_integrity) +{ + build_image(3, true, true); + + eos_image_header_t hdr; + ASSERT(eos_image_parse_header(SLOT_A_ADDR, &hdr) == EOS_OK); + ASSERT(eos_image_verify_integrity(&hdr, SLOT_A_ADDR) == EOS_OK); + + uint8_t hdr_before[sizeof(eos_image_header_t)]; + memcpy(hdr_before, &sim_flash[SLOT_A_ADDR], sizeof(hdr_before)); + + tamper_sec_ver(9); + + /* The payload hash still matches and every byte of the signed prefix is + * unchanged: neither of the bootloader's two cryptographic checks can see + * this edit. That is why the counter needs its own binding. */ + ASSERT(eos_image_verify_integrity(&hdr, SLOT_A_ADDR) == EOS_OK); + ASSERT(memcmp(hdr_before, &sim_flash[SLOT_A_ADDR], EOS_IMG_SIGNED_LEN) == 0); +} + +TEST(test_unbound_tlv_area_is_not_trusted) +{ + /* A TLV area sitting after the payload that the header does not vouch for + * is attacker-supplied as far as the bootloader can tell. It must not be + * able to raise the counter; 0 is the conservative reading. */ + build_image(9, true, false); + + uint32_t counter = 0xDEADBEEF; + ASSERT(eos_rollback_read_image_counter(SLOT_A_ADDR, &counter) == EOS_OK); + ASSERT(counter == 0); + + sim_counter = 5; + ASSERT(eos_rollback_verify(counter) == EOS_ERR_ANTI_ROLLBACK); +} + +TEST(test_image_without_tlv_area_still_reports_zero) +{ + /* Unchanged behaviour for images from tools/sign_image.py, which emits + * [header][payload] and no TLV area at all. */ + build_image(0, false, false); + + uint32_t counter = 0xDEADBEEF; + ASSERT(eos_rollback_read_image_counter(SLOT_A_ADDR, &counter) == EOS_OK); + ASSERT(counter == 0); +} + +TEST(test_tlv_binding_fields_are_inside_the_signed_prefix) +{ + /* The binding is only worth anything if the signature covers it. */ + ASSERT(offsetof(eos_image_header_t, tlv_len) + + sizeof(((eos_image_header_t *)0)->tlv_len) <= EOS_IMG_SIGNED_LEN); + ASSERT(offsetof(eos_image_header_t, tlv_hash) + + sizeof(((eos_image_header_t *)0)->tlv_hash) <= EOS_IMG_SIGNED_LEN); +} + +TEST(test_oversized_tlv_len_is_rejected) +{ + build_image(7, true, true); + + /* A header claiming a TLV area larger than the parser will ever accept + * must be refused outright, not hashed over an unbounded flash range. */ + eos_image_header_t hdr; + memcpy(&hdr, &sim_flash[SLOT_A_ADDR], sizeof(hdr)); + hdr.tlv_len = (uint16_t)(EOS_TLV_MAX_SIZE + 1); + memcpy(&sim_flash[SLOT_A_ADDR], &hdr, sizeof(hdr)); + + uint32_t counter = 0; + ASSERT(eos_rollback_read_image_counter(SLOT_A_ADDR, &counter) == EOS_ERR_INVALID); +} + +int main(void) +{ + printf("TLV authentication (anti-rollback counter)\n\n"); + + tests_run = 8; + run_test_authenticated_tlv_counter_is_read(); + run_test_tampered_tlv_counter_is_rejected(); + run_test_tamper_is_invisible_to_signature_and_integrity(); + run_test_unbound_tlv_area_is_not_trusted(); + run_test_image_without_tlv_area_still_reports_zero(); + run_test_tlv_binding_fields_are_inside_the_signed_prefix(); + run_test_oversized_tlv_len_is_rejected(); + + tests_run = 7; + printf("\n%d/%d passed\n", tests_passed, tests_run); + return tests_passed == tests_run ? 0 : 1; +} diff --git a/tools/eos_sign.py b/tools/eos_sign.py index 786be9d..59a55f7 100644 --- a/tools/eos_sign.py +++ b/tools/eos_sign.py @@ -102,7 +102,7 @@ def build_header(payload: bytes, entry_addr: int, load_addr: int, # wait for the signature to exist. hdr += struct.pack('B', EOS_SIG_MAX_SIZE if sig_type == SIG_TYPE_ED25519 else (len(sig) if sig else 0)) - hdr += b'\x00' * 30 # reserved + hdr += b'\x00' * 30 # tlv_len(2) + tlv_hash(28); 0 = no authenticated TLV area # Signature field (padded to 64 bytes) sig_padded = (sig or b'').ljust(EOS_SIG_MAX_SIZE, b'\x00') diff --git a/tools/imgpack.py b/tools/imgpack.py index 11affc8..aae6ed0 100644 --- a/tools/imgpack.py +++ b/tools/imgpack.py @@ -71,7 +71,7 @@ def build_header( header += hash_field header += struct.pack('