diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4831275f..8ed986c8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,6 +17,12 @@ on: pull_request: branches: [master, develop, reconcile/upstream-sync] +# One run per ref: a new push supersedes the old instead of both burning a +# runner to completion. +concurrency: + group: ci-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + jobs: # ═══════════════════════════════════════════════════════════ # STAGE 1: GATE @@ -62,26 +68,74 @@ jobs: integration: needs: [lint] runs-on: ubuntu-latest - timeout-minutes: 30 + timeout-minutes: 15 - services: - kkemu: - image: kktech/kkemu:latest - ports: - - 11044:11044/udp - - 11045:11045/udp - - 5000:5000 + # NO published emulator image. This job BUILDS one from current firmware. + # + # It used to pull kktech/kkemu:latest -- a floating tag whose image was + # five months and six minor versions stale. That single fact caused every + # symptom we chased: 80 tests gating on requires_firmware("7.15.0") skipped + # silently, and one unskipped test drove a ctime() path that segfaults on + # the old image and does not exist in current firmware. + # + # Publishing a fresher image would only reset that clock. Building from + # source removes the class: the emulator under test is, by construction, + # the firmware the tests were written against. steps: - uses: actions/checkout@v4 with: submodules: recursive + path: python-keepkey + + # python-keepkey is a SUBMODULE of the firmware repo, so the firmware is + # where the emulator lives. alpha is the fork's integration branch. + - name: Checkout firmware + uses: actions/checkout@v4 + with: + repository: BitHighlander/keepkey-firmware + ref: alpha + path: keepkey-firmware + + # NOT `submodules: recursive`. trezor-firmware carries a micropython + # vendor tree whose lib/lwip lives on git.savannah.gnu.org, which serves + # dumb HTTP and cannot do the shallow clone actions/checkout requests -- + # it fails the whole job. The firmware repo's own CI inits exactly these + # paths, non-recursively, for the same reason. + - name: Init the submodules the emulator build needs + working-directory: keepkey-firmware + run: | + git submodule update --init --depth 1 deps/crypto/trezor-firmware + git submodule update --init --depth 1 deps/device-protocol + git submodule update --init --depth 1 deps/googletest + git submodule update --init --depth 1 deps/qrenc/QR-Code-generator + git submodule update --init --depth 1 deps/sca-hardening/SecAESSTM32 + + # Test THIS checkout of python-keepkey, not the one the firmware pins. + - name: Overlay this python-keepkey onto the firmware tree + run: | + rm -rf keepkey-firmware/deps/python-keepkey + cp -a python-keepkey keepkey-firmware/deps/python-keepkey + + - name: Build the emulator + timeout-minutes: 20 + working-directory: keepkey-firmware + run: | + docker build -t kkemu-ci -f scripts/emulator/Dockerfile . + + - name: Start the emulator + run: | + docker run -d --name kkemu \ + -p 11044:11044/udp -p 11045:11045/udp -p 5000:5000 kkemu-ci + sleep 3 + docker logs kkemu | head -5 - uses: actions/setup-python@v5 with: python-version: '3.11' - name: Install dependencies + working-directory: python-keepkey run: | pip install --upgrade pip pip install "protobuf>=3.20,<4" @@ -101,20 +155,67 @@ jobs: sleep 1 done + # "The emulator answered a ping" is not "the emulator is the right + # firmware". CI ran a 7.16-era suite against a 7.10.0 image for five + # months: 80 tests gate on requires_firmware("7.15.0") and silently + # SKIPPED, while one unskipped test drove a code path that segfaults in + # 7.10.0 and is already fixed in 7.15 -- which reads as a product failure + # but is only a stale image. A floating tag cannot tell you that. This + # can, and it fails closed. + - name: Assert the emulator is not older than the suite + timeout-minutes: 2 + env: + KK_TRANSPORT_MAIN: "127.0.0.1:11044" + KK_TRANSPORT_DEBUG: "127.0.0.1:11045" + KK_MIN_FW: "7.15.0" + KK_UDP_TIMEOUT: "20" + working-directory: keepkey-firmware/deps/python-keepkey/tests + run: | + python - <<'PY' + import os, sys + sys.path.insert(0, '..') + import config + from keepkeylib.client import KeepKeyDebuglinkClient + c = KeepKeyDebuglinkClient(config.TRANSPORT(*config.TRANSPORT_ARGS, + **config.TRANSPORT_KWARGS)) + c.set_debuglink(config.DEBUG_TRANSPORT(*config.DEBUG_TRANSPORT_ARGS, + **config.DEBUG_TRANSPORT_KWARGS)) + c.init_device() + f = c.features + got = (f.major_version, f.minor_version, f.patch_version) + floor = tuple(int(x) for x in os.environ['KK_MIN_FW'].split('.')) + print('emulator firmware %d.%d.%d, floor %s' % + (got + (os.environ['KK_MIN_FW'],))) + if got < floor: + sys.exit('FATAL: the emulator image predates the tests that run ' + 'against it. Republish kktech/kkemu from current ' + 'firmware and pin the new digest above.') + PY + + # Step-level timeout, deliberately: a JOB-level timeout ends the job as + # "cancelled", which reads as an infra blip. A step timeout is a FAILURE. - name: Run integration tests + timeout-minutes: 8 env: KK_TRANSPORT_MAIN: "127.0.0.1:11044" KK_TRANSPORT_DEBUG: "127.0.0.1:11045" - PYTHONPATH: "${{ github.workspace }}/keepkeylib:${{ github.workspace }}" + PYTHONPATH: "${{ github.workspace }}/keepkey-firmware/deps/python-keepkey" + # A crashed emulator now raises instead of blocking in recv() forever. + KK_UDP_TIMEOUT: "45" run: | - cd tests + # From the OVERLAID copy, not the standalone checkout: the + # storage-version-gate tests assert against lib/firmware/storage.c, + # which they find by walking UP. Run them as a sibling of the + # firmware and they resolve; run them standalone and they fail + # claiming the sources are missing. + cd keepkey-firmware/deps/python-keepkey/tests pytest -v --junitxml=junit.xml 2>&1 | tee pytest-output.txt echo "${PIPESTATUS[0]}" > status - name: Test summary if: always() run: | - XML="tests/junit.xml" + XML="keepkey-firmware/deps/python-keepkey/tests/junit.xml" echo "## 🔑 KeepKey python-keepkey — Integration Tests" >> "$GITHUB_STEP_SUMMARY" echo "" >> "$GITHUB_STEP_SUMMARY" @@ -161,15 +262,23 @@ jobs: echo "---" >> "$GITHUB_STEP_SUMMARY" echo "*KeepKey python-keepkey CI*" >> "$GITHUB_STEP_SUMMARY" - - name: Upload test results + # NO check_name. With one, this action publishes a SEPARATE check run + # via the Checks API, and its require_tests default of 'false' means an + # absent junit.xml -- which is exactly what a killed pytest leaves behind + # -- reports conclusion:success with zero duration. That green check sat + # on top of a job timing out at 30 minutes for at least six merges. + # annotate_only keeps the inline annotations without minting a check. + - name: Annotate test results uses: mikepenz/action-junit-report@v4 if: always() with: - report_paths: tests/junit.xml - check_name: Integration Tests + report_paths: keepkey-firmware/deps/python-keepkey/tests/junit.xml + annotate_only: true + require_tests: true + fail_on_failure: true - name: Fail on test failure if: always() run: | - STATUS=$(cat tests/status 2>/dev/null || echo "1") + STATUS=$(cat keepkey-firmware/deps/python-keepkey/tests/status 2>/dev/null || echo "1") [ "$STATUS" = "0" ] || exit 1 diff --git a/keepkeylib/transport_udp.py b/keepkeylib/transport_udp.py index 05767de7..1dbdf672 100644 --- a/keepkeylib/transport_udp.py +++ b/keepkeylib/transport_udp.py @@ -2,10 +2,23 @@ '''SocketTransport implements TCP socket interface for Transport.''' +import os import socket from select import select from .transport import Transport +# A dead emulator must surface as an ERROR, not as an infinite wait. +# +# The socket had no timeout, so when the emulator segfaulted mid-suite, +# recv() blocked in a syscall until something outside killed the process -- +# in CI that was a 30-minute job timeout reported as "cancelled", which reads +# as an infrastructure blip rather than the device crash it actually was. It +# hid a real segfault for at least six merges. +# +# Generous by default because a confirm screen legitimately waits on a human; +# override for unattended runs with KK_UDP_TIMEOUT (seconds, 0 disables). +DEFAULT_TIMEOUT = float(os.environ.get('KK_UDP_TIMEOUT', '60')) + class FakeRead(object): # Let's pretend we have a file-like interface def __init__(self, func): @@ -31,6 +44,8 @@ def __init__(self, device, *args, **kwargs): def _open(self): self.socket = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) self.socket.connect(self.device) + if DEFAULT_TIMEOUT > 0: + self.socket.settimeout(DEFAULT_TIMEOUT) def _close(self): self.socket.close() @@ -57,7 +72,19 @@ def _read(self): def _raw_read(self, length): while len(self.buffer) < length: - data = self.socket.recv(64) + try: + data = self.socket.recv(64) + except socket.timeout: + # Name the cause. "timed out" alone sends people looking at the + # test; the device is what stopped answering. + raise IOError( + 'No response from the emulator at %s:%d after %gs -- it is ' + 'not running, has crashed, or is wedged on a confirm screen ' + 'nothing acknowledged. Set KK_UDP_TIMEOUT to change or 0 to ' + 'disable.' % (self.device[0], self.device[1], + DEFAULT_TIMEOUT)) + if not data: + raise IOError('Emulator closed the connection') self.buffer += data[1:] ret = self.buffer[:length] diff --git a/tests/common.py b/tests/common.py index 73dac785..f0b0e65f 100644 --- a/tests/common.py +++ b/tests/common.py @@ -156,6 +156,7 @@ def requires_structured_eip712(self): """ from keepkeylib import messages_ethereum_pb2 as _eth from keepkeylib import messages_pb2 as _proto + from keepkeylib import types_pb2 as _types probe = _eth.EthereumSignTypedData() for n in (0x8000002C, 0x8000003C, 0x80000000, 0, 0): @@ -166,7 +167,7 @@ def requires_structured_eip712(self): resp = self.client.call_raw(probe) if isinstance(resp, _proto.Failure): self.client.init_device() - if resp.code == _proto.Failure_UnexpectedMessage: + if resp.code == _types.Failure_UnexpectedMessage: self.skipTest( "Firmware does not implement structured EIP-712 " "(EthereumSignTypedData is not handled)") diff --git a/tests/test_msg_session_trust_lifetime.py b/tests/test_msg_session_trust_lifetime.py index 7861ec33..76e5caa0 100644 --- a/tests/test_msg_session_trust_lifetime.py +++ b/tests/test_msg_session_trust_lifetime.py @@ -91,6 +91,11 @@ def probe_blob(): )) +# Names `ps -o comm=` reports for the emulator binary. Anything else bound to +# the port is not ours to kill -- see the guard in _emulator_process(). +_EMULATOR_EXE_NAMES = ('kkemu',) + + def _emulator_process(port): """(pid, exe, cwd) of the process BOUND to udp/port, or None. @@ -124,6 +129,20 @@ def _emulator_process(port): exe = subprocess.run(['ps', '-o', 'comm=', '-p', str(pid)], stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True).stdout.strip() + if os.path.basename(exe) not in _EMULATOR_EXE_NAMES: + # Whatever holds this port, it is not the firmware. Whenever the + # emulator runs in a container the bound process is the Docker + # port forwarder -- docker-proxy or dockerd on Linux, + # com.docker.backend on macOS -- in a different pid namespace + # from kkemu. Killing it does not reboot anything: it removes + # the port forward, and every later test in the run then blocks + # forever on a socket that will never answer again. Measured + # here: it took the whole Docker daemon down mid-suite. + # + # Fall through to "not found" so _power_cycle() takes its + # documented skip, which the report renders as WITHHELD rather + # than as a pass. + continue cwd_out = subprocess.run( ['lsof', '-a', '-p', str(pid), '-d', 'cwd', '-Fn'], stdout=subprocess.PIPE, diff --git a/tests/test_storage_version_gate.py b/tests/test_storage_version_gate.py index 866e1dbe..16820394 100644 --- a/tests/test_storage_version_gate.py +++ b/tests/test_storage_version_gate.py @@ -49,6 +49,54 @@ # TestStorageVersionGateSource needs no device at all: it reads the firmware # sources and asserts the gate's own invariants. Those tests run everywhere, # including CI, so this section is never completely dark. +# +# --------------------------------------------------------------------------- +# Why the source tests name no version number +# --------------------------------------------------------------------------- +# +# They used to. test_active_flash_format_is_v20 asserted STORAGE_VERSION == 20 +# and test_burned_versions_are_dispatched_to_the_wipe_path asserted the literal +# string "case StorageVersion_18:", because 7.16 writes V20 and burns 18/19. +# Both are true on the passkeys branch and both are FALSE on the 7.15 line, +# where STORAGE_VERSION is 17 and nothing is burned. python-keepkey is one +# submodule shared by every firmware branch, and CI now builds the emulator +# from whichever branch is under test, so a test pinned to one branch's version +# reports a failure whose only cause is which branch you are on. +# +# A test that reads a source file has to assert properties of what it read. +# What follows is derived, per tree: +# +# STORAGE_VERSION, STORAGE_VERSION_LAST_SHIPPED, include/.../storage.h +# STORAGE_VERSION_BTC_ONLY_BASE +# the version ladder lib/firmware/storage_versions.inc +# which versions are BURNED lib/firmware/storage_versions.inc +# which versions have a reader / hit the wipe path lib/firmware/storage.c +# +# The only version number still written down is +# STORAGE_VERSION_LAST_SHIPPED_FLOOR, and it is a FLOOR, not an equality -- see +# its comment for why that distinction is the whole argument. (The V16 numbers +# in the emulator section are a different thing: they describe the format 7.14.x +# shipped, which is finished history and cannot change. The flash offsets are +# unchanged by the V20 bump -- V20 keeps V17's layout and puts passkey state in +# its reserved area at +501 -- so the migration test reads the same way on both +# lines.) +# +# Decoupling is not the same as weakening. The property docs/StorageVersionGate +# .md exists to protect -- "a bump is a deliberate release act, never an +# accident" -- is enforced harder than before, because it no longer rests on +# somebody also editing a constant in this file. A bare `#define +# STORAGE_VERSION 18` now has to survive: +# +# * the ladder must be contiguous 1..N and END at STORAGE_VERSION, so the +# bump forces an append to storage_versions.inc; +# * every ladder version must be dispatched in storage_fromFlash, so the bump +# forces a case label; +# * the version this firmware WRITES must have a reader, so the bump forces a +# reader behind that label. +# +# Three files have to move together, and every one of them is a file that has +# to move anyway for the firmware to be correct. The old constant was the only +# artifact in the set that did not. from __future__ import print_function @@ -102,9 +150,30 @@ FLAG_AUTHDATA_INITIALIZED = 1 << 18 FLAG_AUTHDATA_ENCRYPTED = 1 << 19 -# include/keepkey/firmware/storage.h +# include/keepkey/firmware/storage.h. Cross-checked against the header by +# test_version_never_drops_below_a_shipped_release -- the emulator tests below +# stamp wallets into this band by hand, so a drift between the two would make +# them exercise a band the firmware does not use. STORAGE_VERSION_BTC_ONLY_BASE = 10000 +# The lowest value STORAGE_VERSION_LAST_SHIPPED may ever hold. 7.15 shipped +# storage V17; that is a fact about the past and cannot become false, so this +# is a RATCHET and not a version pin. Raise it when a later release actually +# ships (the same commit that raises the constant in storage.h); there is no +# branch on which it needs lowering, and lowering it is the edit this exists to +# stop. +# +# The distinction matters. `assertEqual(17, last_shipped)` is wrong the day +# 7.16 ships and wrong on any branch that has already bumped it, so it rots and +# gets "fixed" by whoever the failure inconveniences. `>= 17` is wrong only if +# somebody deletes history. It still catches the edit docs/StorageVersionGate.md +# calls the single highest-severity review item in the file: the static assert +# is STORAGE_VERSION >= STORAGE_VERSION_LAST_SHIPPED, so the way to make a +# LOWERED storage version compile is to lower LAST_SHIPPED to match it, and +# both numbers live in the same header where one commit reaches both. An +# independent witness is the only thing that sees it. +STORAGE_VERSION_LAST_SHIPPED_FLOOR = 17 + MNEMONIC_ALL = " ".join(["all"] * 12) LABEL = "storagegate" PIN = "1234" @@ -116,7 +185,20 @@ # --------------------------------------------------------------------------- def _repo_root(): - """Directory of the firmware checkout this python-keepkey lives under.""" + """Directory of the firmware checkout this python-keepkey lives under. + + KK_FIRMWARE_ROOT wins, so the gate can be pointed at a tree this clone is + not nested inside. That is not a convenience: these tests now derive every + version number from the tree, and the only way to show they hold on BOTH + release lines is to run one checkout of them against two firmware trees. + Unset -- which is how CI runs, from deps/python-keepkey -- the walk up is + unchanged. + """ + env = os.environ.get("KK_FIRMWARE_ROOT") + if env: + assert os.path.isfile(os.path.join(env, "lib", "firmware", "storage.c")), ( + "KK_FIRMWARE_ROOT=%s has no lib/firmware/storage.c" % env) + return env d = _HERE for _ in range(8): if os.path.isfile(os.path.join(d, "lib", "firmware", "storage.c")): @@ -152,6 +234,156 @@ def _define(text, name): return int(m.group(1)) +def _strip_c_comments(text): + """Comments are prose and must never be mistaken for code. + + Both files this module parses argue their case in long comments that name + the very identifiers being searched for -- the burned arm in storage.c says + "there is deliberately NO reader" a few words from where a reader would be + written. Classification runs on the stripped text so a rewording can never + change a verdict. + """ + text = re.sub(r"/\*.*?\*/", " ", text, flags=re.S) + return re.sub(r"//[^\n]*", " ", text) + + +# -- lib/firmware/storage_versions.inc -------------------------------------- + +_LADDER_ENTRY = re.compile( + r"STORAGE_VERSION_(?:ENTRY|LAST)\s*\(\s*(\d+)\s*\)") +_LADDER_LAST = re.compile(r"STORAGE_VERSION_LAST\s*\(\s*(\d+)\s*\)") +_ENTRY_LINE = re.compile(r"^\s*STORAGE_VERSION_ENTRY\s*\(\s*(\d+)\s*\)\s*$") +_BURNED_WORD = re.compile(r"\bBURNED\b") + + +def _ladder(inc): + """Every version in storage_versions.inc, in file order. + + The x-macro definitions at the top of the file take a parameter named X, + not a digit, so they do not match. + """ + return [int(m) for m in _LADDER_ENTRY.findall(_strip_c_comments(inc))] + + +def _ladder_last(inc): + """The single STORAGE_VERSION_LAST(N) entry: the version this build writes.""" + last = _LADDER_LAST.findall(_strip_c_comments(inc)) + assert len(last) == 1, ( + "storage_versions.inc must have exactly one STORAGE_VERSION_LAST entry, " + "found %s" % last) + return int(last[0]) + + +def _burned_declared(inc): + """Versions storage_versions.inc annotates as BURNED. + + THE DECLARATION SITE. A burned version is one that a pre-release build + wrote with a layout that was later abandoned, so devices carrying it exist + and no reader may ever be written for it -- parsing such a blob as the + current format is worse than refusing it, because nothing announces the + misparse. That is a fact about history, not about code, so it cannot be + inferred from the code: it has to be stated somewhere and read from there. + + The convention is a comment containing the word BURNED, immediately above + the entries it applies to: + + STORAGE_VERSION_ENTRY(17) + /* 18 and 19 are BURNED. */ + STORAGE_VERSION_ENTRY(18) + STORAGE_VERSION_ENTRY(19) + STORAGE_VERSION_LAST(20) + + The run ends at the first line that is not a bare STORAGE_VERSION_ENTRY -- + a blank line, another comment, or the STORAGE_VERSION_LAST line, which by + definition is the version being written and so can never be burned. + + Numbers inside the comment text are deliberately NOT scraped: that prose + mentions the commit that reverted the format and the version it reverted + TO, and reading V17 out of it would declare a shipped version burned. + Position is the annotation; the words are for humans. + + An unannotated version that turns out to be dispatched to the wipe path is + a mismatch, not a silent pass -- see + test_burned_versions_agree_between_the_ladder_and_the_dispatch. + """ + burned = set() + lines = inc.splitlines() + i = 0 + while i < len(lines): + if "/*" not in lines[i]: + i += 1 + continue + block = [] + while i < len(lines): + block.append(lines[i]) + if "*/" in lines[i]: + break + i += 1 + i += 1 + if not _BURNED_WORD.search("\n".join(block)): + continue + while i < len(lines): + m = _ENTRY_LINE.match(lines[i]) + if not m: + break + burned.add(int(m.group(1))) + i += 1 + return burned + + +# -- lib/firmware/storage.c -------------------------------------------------- + +_CASE_LABEL = re.compile(r"case\s+StorageVersion_(\w+)\s*:") +_READER_CALL = re.compile(r"\bstorage_read\w*\s*\(") +_WIPE_RETURN = re.compile(r"return\s+SUS_Invalid\b") + + +def _from_flash_arms(c): + """Map every StorageVersion_X label in storage_fromFlash to its arm text. + + Consecutive labels share one arm: `case 2: case 3: ... case 10:` is a + single body reached by nine versions, and each of them must be credited + with what that body does. So labels accumulate until one is followed by + something other than whitespace and comments, and the whole group is + assigned that text. + + Keys are the label suffixes as written -- "17", "BTC_ONLY", "NONE" -- so + the non-numeric arms stay visible to the tests that care about them. + """ + i = c.index("StorageUpdateStatus storage_fromFlash") + body = c[i:c.index("\n}", i)] + assert "case StorageVersion_NONE" in body, ( + "storage_fromFlash body was cut short before the end of its switch; " + "the parse below would under-report every arm") + + labels = list(_CASE_LABEL.finditer(body)) + assert labels, "no case StorageVersion_* labels in storage_fromFlash" + + arms = {} + group = [] + for idx, m in enumerate(labels): + group.append(m.group(1)) + end = labels[idx + 1].start() if idx + 1 < len(labels) else len(body) + own = body[m.end():end] + if _strip_c_comments(own).strip(): + for name in group: + arms[name] = own + group = [] + for name in group: # labels trailing the last statement: no body at all + arms[name] = "" + return arms + + +def _reads(arm): + """Does this arm call a storage_readVxx reader?""" + return bool(_READER_CALL.search(_strip_c_comments(arm))) + + +def _wipes(arm): + """Does this arm return SUS_Invalid -- the reset-and-commit path?""" + return bool(_WIPE_RETURN.search(_strip_c_comments(arm))) + + # --------------------------------------------------------------------------- # Emulator process management # --------------------------------------------------------------------------- @@ -389,7 +621,14 @@ def _capture(client): class TestStorageVersionGateSource(unittest.TestCase): """No device needed. These are the checks that survive a CI runner which - cannot restart an emulator, so the section is never entirely unmeasured.""" + cannot restart an emulator, so the section is never entirely unmeasured. + + Every number these tests compare against is read out of the tree they are + run in, so one copy of this file states the same invariants on the 7.15 + line (STORAGE_VERSION 17, nothing burned) and on 7.16 (20, with 18 and 19 + burned). See the note at the top of the module for why that is a + strengthening rather than a relaxation. + """ def setUp(self): self.h = _read_source("include/keepkey/firmware/storage.h") @@ -398,107 +637,42 @@ def setUp(self): self.version = _define(self.h, "STORAGE_VERSION") self.last_shipped = _define(self.h, "STORAGE_VERSION_LAST_SHIPPED") - def test_active_flash_format_is_v20(self): - """7.16 writes V20. The bump is argued here, which is the point of the - test: it asserts a LITERAL so that raising a header constant cannot - quietly satisfy it. - - The compile-time assert in storage.c compares STORAGE_VERSION against - STORAGE_VERSION_LAST_SHIPPED -- two numbers in the same header, both - editable in one commit -- so raising LAST_SHIPPED to make a build - compile is the edit docs/StorageVersionGate.md calls the highest - severity review item in the file. An independent witness is the only - thing that catches it. - - WHY 20 AND NOT 18. 18 was the clear-sign identity block and 19 the - PIN-KDF migration. Both were ACTIVE, not merely drafted: e109404ee made - 19 live and 6bebde7b2 reverted the format to V17 for 7.15. Any device - that ran an alpha build in that window carries a blob stamped 18 or 19 - whose layout has nothing to do with passkeys, and reading one as CTAP2 - state would misparse it rather than refuse it. 20 is unburned. - - THE READER CHAIN. V17 blobs are read by storage_readV17 and restamped - to STORAGE_VERSION; V20 blobs by storage_readV20. There is deliberately - NO reader for 18 or 19: they remain in the ladder because the enum is - positional and removing an entry renumbers everything after it, but a - blob stamped with either falls through to the default and the device - wipes. That is the documented behaviour for an unrecognised format and - is strictly better than misparsing one. - - ANTI-ROLLBACK. Once a device writes V20, installing 7.15 -- which knows - only up to V17 -- maps the blob to StorageVersion_NONE and storage_init - resets it. The device wipes. That is normal downgrade behaviour and is - stated here so it is a known consequence rather than a field report. - A signed UPGRADE never wipes; only going backwards does. - - RELEASE NOTE. "7.16 changes the on-device storage format to hold - passkey credentials. Upgrading preserves your wallet. Downgrading to - 7.15 or earlier will ERASE it -- back up your recovery phrase before - downgrading." - """ - self.assertEqual( - 20, self.version, - "STORAGE_VERSION is %d, not the V20 format 7.16 introduces. A bump " - "is a deliberate release act (docs/StorageVersionGate.md): confirm " - "the reader chain, the anti-rollback story, and the release notes, " - "then update this test." % self.version) - # LAST_SHIPPED stays at 17 until 7.16 actually SHIPS in a signed - # release. It is the high-water mark of what is IN THE FIELD, not of - # what is in the tree -- and storage.h says two lines above the - # constant that raising it to make a build compile "is the exact edit - # that turns every upgrade in the field into a silent wipe". The - # compile-time assert only needs STORAGE_VERSION >= LAST_SHIPPED, and - # 20 >= 17 holds, so nothing requires the raise. - self.assertEqual( - 17, self.last_shipped, - "STORAGE_VERSION_LAST_SHIPPED is %d. It tracks the last SIGNED " - "release (7.15 = V17) and moves in the release commit that tags " - "7.16, not when a format lands in the tree." % self.last_shipped) + self.ladder = _ladder(self.inc) + self.burned = _burned_declared(self.inc) + self.arms = _from_flash_arms(self.c) - def test_burned_versions_are_dispatched_to_the_wipe_path(self): - """18 and 19 must never be PARSED by 7.16. + # -- helpers ------------------------------------------------------------ - They were real formats in alpha builds before the 7.15 revert, so - devices carrying them exist. A reader for either would parse a - clear-sign identity block or a PIN-KDF blob as passkey state. + def _arm(self, version): + arm = self.arms.get(str(version)) + self.assertIsNotNone( + arm, + "storage_fromFlash has no `case StorageVersion_%d:` -- see " + "test_every_ladder_version_is_dispatched" % version) + return arm - This used to assert the absence of a `case StorageVersion_18:` label, - on the theory that falling to the default is what sends them to the - wipe path. That was wrong twice over: storage_fromFlash has NO default - case -- deliberately, so -Werror=switch names any version we forget -- - so an unlisted version does not fall anywhere, it fails the ARM build. + # -- the ladder --------------------------------------------------------- - So the labels must exist. What must NOT exist is a reader behind them. - Assert the real property: 18 and 19 are dispatched, and what they - dispatch to is SUS_Invalid rather than any storage_readVxx call. - """ - for burned in (18, 19): - label = "case StorageVersion_%d:" % burned - self.assertIn( - label, self.c, - "%s must be listed; storage_fromFlash has no default case, so " - "an unlisted version breaks the -Werror=switch build" % label) - - # The two labels must sit together and return SUS_Invalid before any - # other case begins. Slice from the first burned label to the next - # `case ` that is not one of the burned ones. - i = self.c.index("case StorageVersion_18:") - rest = self.c[i:] - j = len(rest) - for m in re.finditer(r"\n\s*case StorageVersion_(\w+):", rest): - if m.group(1) not in ("18", "19"): - j = m.start() - break - arm = rest[:j] + def test_version_ladder_is_contiguous_and_ends_at_storage_version(self): + """storage_versions.inc may only ever be APPENDED to. - self.assertIn( - "SUS_Invalid", arm, - "the burned versions must return SUS_Invalid (the wipe path); " - "arm was:\n%s" % arm) - self.assertNotIn( - "storage_read", arm, - "a reader behind a burned version would misparse blobs written by " - "pre-revert alpha builds; arm was:\n%s" % arm) + The enum is emitted in .inc order after StorageVersion_NONE = 0, so a + contiguous 1..N list is what makes StorageVersion_N == N. Deleting or + renumbering an entry silently drops a version from version_from_int() + and wipes every device carrying it. + + Ending AT StorageVersion is the half that makes a bare header bump + loud: raise STORAGE_VERSION without appending here and the two numbers + disagree. + """ + self.assertTrue(self.ladder, "no version entries parsed from the ladder") + self.assertEqual(list(range(1, len(self.ladder) + 1)), self.ladder, + "storage_versions.inc is not contiguous from 1") + self.assertEqual( + self.version, _ladder_last(self.inc), + "STORAGE_VERSION is %d but the ladder ends at %d. A version this " + "firmware writes and cannot enumerate is not recognised on the next " + "boot -- it wipes itself." % (self.version, _ladder_last(self.inc))) def test_version_never_drops_below_a_shipped_release(self): """Lowering STORAGE_VERSION wipes every device upgrading FROM a shipped @@ -507,41 +681,247 @@ def test_version_never_drops_below_a_shipped_release(self): stay under the bitcoin-only band, or a multi-chain wallet would be stamped into the band that multi-chain firmware refuses to load.""" self.assertGreaterEqual(self.version, self.last_shipped) - self.assertLess(self.version, STORAGE_VERSION_BTC_ONLY_BASE) - - def test_version_ladder_is_contiguous_and_ends_at_storage_version(self): - """storage_versions.inc may only ever be APPENDED to. - - The enum is emitted in .inc order after StorageVersion_NONE = 0, so a - contiguous 1..N list is what makes StorageVersion_N == N. Deleting or - renumbering an entry silently drops a version from version_from_int() - and wipes every device carrying it. + band = _define(self.h, "STORAGE_VERSION_BTC_ONLY_BASE") + self.assertEqual( + STORAGE_VERSION_BTC_ONLY_BASE, band, + "the header moved the bitcoin-only band to %d; the emulator tests " + "in this file stamp wallets into %d by hand and would be measuring " + "a band the firmware no longer uses" + % (band, STORAGE_VERSION_BTC_ONLY_BASE)) + self.assertLess(self.version, band) + + def test_last_shipped_never_moves_backwards(self): + """STORAGE_VERSION_LAST_SHIPPED is a high-water mark of the FIELD. + + It records the newest format any signed release ever wrote, so it can + only rise, and only in the commit that ships. The compile-time assert + in storage.c is STORAGE_VERSION >= STORAGE_VERSION_LAST_SHIPPED, and + both operands live in the same header -- so the way to make a LOWERED + storage version build is to lower this to match, which is exactly the + edit that turns every upgrade in the field into a silent wipe. + docs/StorageVersionGate.md calls that the highest-severity review item + in the file. + + A floor asserted from outside the header is the independent witness. + It is not a version pin: it stays true when 7.16 raises the constant to + 20, and it is only ever raised, never corrected. """ - entries = [int(m) for m in re.findall( - r"STORAGE_VERSION_(?:ENTRY|LAST)\s*\(\s*(\d+)\s*\)", self.inc)] - self.assertTrue(entries, "no version entries parsed from the ladder") - self.assertEqual(list(range(1, len(entries) + 1)), entries, - "storage_versions.inc is not contiguous from 1") - last = re.findall(r"STORAGE_VERSION_LAST\s*\(\s*(\d+)\s*\)", self.inc) - self.assertEqual([str(self.version)], last) + self.assertGreaterEqual( + self.last_shipped, STORAGE_VERSION_LAST_SHIPPED_FLOOR, + "STORAGE_VERSION_LAST_SHIPPED is %d, below the %d that 7.15 shipped. " + "Either a signed release is being un-remembered to make a lowered " + "STORAGE_VERSION compile, or the ratchet in this file is wrong -- " + "and only one of those two has ever happened." + % (self.last_shipped, STORAGE_VERSION_LAST_SHIPPED_FLOOR)) - def test_every_ladder_version_has_a_reader(self): + # -- the dispatch ------------------------------------------------------- + + def test_every_ladder_version_is_dispatched(self): """Every version in the ladder needs a case in storage_fromFlash(). This is the failure the static asserts do NOT cover. They pin the enum - to its own numbering; they say nothing about the switch. Drop a case - and control falls out of the switch to `return SUS_Invalid` -- which - storage_init() answers with storage_reset(). Every device carrying that - version is wiped on upgrade, and the build stays green. + to its own numbering; they say nothing about the switch. + + The switch has no default case, deliberately, so that -Werror=switch + names any version we forget -- which means on ARM this is also a build + failure. It is asserted anyway because the emulator and the unit tests + are built by other toolchains and other flag sets, and because the + message here says which device gets wiped, where the compiler says + which enumerator is unhandled. + """ + missing = [v for v in self.ladder if str(v) not in self.arms] + self.assertEqual( + [], missing, + "storage_fromFlash has no case for version(s) %s -- a device " + "carrying one is wiped at boot" % missing) + + def test_an_unrecognised_version_reaches_the_wipe_path(self): + """version_from_int() maps anything off the ladder to + StorageVersion_NONE, and that arm must return SUS_Invalid. + + This is the mechanism the downgrade half of the policy rests on: a + device that has run newer firmware carries a stamp older firmware + cannot read, and it must reset rather than load a blob it will + misparse. The emulator test test_unrecognised_version_wipes_on_boot + proves the behaviour end to end; this proves the arm still exists on a + runner with no emulator. """ - body = self.c.split("StorageUpdateStatus storage_fromFlash", 1) - self.assertEqual(2, len(body), "storage_fromFlash not found") - cases = set(int(m) for m in re.findall( - r"case\s+StorageVersion_(\d+)\s*:", body[1])) - missing = sorted(set(range(1, self.version + 1)) - cases) - self.assertEqual([], missing, - "storage_fromFlash has no case for version(s) %s -- a " - "device carrying one is wiped at boot" % missing) + arm = self.arms.get("NONE") + self.assertIsNotNone(arm, "storage_fromFlash has no StorageVersion_NONE case") + self.assertTrue( + _wipes(arm), + "StorageVersion_NONE no longer returns SUS_Invalid. An unknown " + "storage version would be accepted, and an attacker could roll back " + "to an older signed image with a known extraction bug and keep the " + "seed. Arm was:\n%s" % arm) + self.assertFalse( + _reads(arm), + "a reader behind StorageVersion_NONE parses a blob whose format is " + "by definition unknown. Arm was:\n%s" % arm) + + def test_every_dispatched_version_either_reads_or_refuses(self): + """An arm reads a blob or it refuses one. Never both, never neither. + + Neither means control reached a case that falls out of the switch -- + storage_fromFlash ends in `return SUS_Invalid`, so the device wipes, + and nothing in the source says that was meant. + + Both means the classification below cannot say what the arm is for, and + an arm that reads before refusing has already parsed the blob. If a + real reader ever needs an error return, this assertion is where that + design gets argued rather than assumed -- which is the point of the + gate. + """ + for version in self.ladder: + arm = self._arm(version) + reads, wipes = _reads(arm), _wipes(arm) + self.assertNotEqual( + reads, wipes, + "version %d %s. Arm was:\n%s" + % (version, + "both reads a blob and returns SUS_Invalid" if reads else + "neither reads a blob nor returns SUS_Invalid, so it falls " + "out of the switch and wipes without saying so", + arm)) + + def test_every_shipped_version_has_a_reader(self): + """THE upgrade-never-wipes property, for every device in the field. + + An upgrading device arrives carrying the format written by the release + it is leaving. STORAGE_VERSION_LAST_SHIPPED is the newest of those, so + 1..LAST_SHIPPED is the set of formats that exist on real hardware, and + every one of them must be read rather than refused. Lose a reader here + and every wallet carrying that version is erased at boot with no + prompt, while the build stays green. + + This is the test that carries the section on a release line with + nothing burned, and it is the reason a burned version can never be one + that shipped -- see test_no_shipped_version_is_burned. + """ + for version in range(1, self.last_shipped + 1): + arm = self._arm(version) + self.assertTrue( + _reads(arm), + "version %d has SHIPPED (STORAGE_VERSION_LAST_SHIPPED is %d) " + "but storage_fromFlash does not read it. Every device carrying " + "it is wiped on upgrade. Arm was:\n%s" + % (version, self.last_shipped, arm)) + self.assertFalse( + _wipes(arm), + "version %d has SHIPPED but its arm returns SUS_Invalid, which " + "is storage_reset() + storage_commit() at boot. Arm was:\n%s" + % (version, arm)) + + def test_the_version_this_firmware_writes_can_be_read_back(self): + """A device commits STORAGE_VERSION and reboots into the same firmware. + + If the arm for the version it just wrote does not read, storage_init() + resets on the very next boot -- the wallet does not survive a power + cycle of the build that created it. The emulator test + test_reboot_preserves_the_wallet proves this on a running device; here + it also makes a header bump carry a reader with it, because there is no + version so new that the firmware writing it may refuse to read it. + """ + arm = self._arm(self.version) + self.assertTrue( + _reads(arm), + "STORAGE_VERSION is %d and storage_fromFlash does not read version " + "%d. This firmware cannot load the blob it writes. Arm was:\n%s" + % (self.version, self.version, arm)) + self.assertNotIn( + self.version, self.burned, + "storage_versions.inc declares version %d BURNED and storage.h " + "writes it. A burned version is one no reader may exist for." + % self.version) + + # -- burned versions ---------------------------------------------------- + + def test_burned_versions_agree_between_the_ladder_and_the_dispatch(self): + """Two files, one answer. + + storage_versions.inc DECLARES which versions are burned; storage.c + DEMONSTRATES it by dispatching them to SUS_Invalid with no reader. + Neither file can be the only witness: + + * derived from storage.c alone, deleting the reader for a shipped + version would silently reclassify it as burned and the suite would + approve of it; + * declared in the .inc alone, a reader wired in behind a burned label + would parse a blob written by a build whose layout was abandoned, + and the declaration would sit there saying otherwise. + + Requiring the two to match catches both, and matching costs an edit in + two files -- which is what "a deliberate act" means here. On a line + with no burned versions both sides are empty and this test says so. + """ + dispatched = set( + v for v in self.ladder + if not _reads(self._arm(v)) and _wipes(self._arm(v))) + self.assertEqual( + sorted(self.burned), sorted(dispatched), + "storage_versions.inc declares %s BURNED; storage_fromFlash sends " + "%s to the wipe path. Whichever is right, the other is a lie about " + "what happens to a device carrying one of these blobs." + % (sorted(self.burned) or "nothing", sorted(dispatched) or "nothing")) + + def test_burned_versions_are_dispatched_to_the_wipe_path(self): + """A burned version must be listed, must refuse, and must have no reader. + + Burned means: a pre-release build wrote this format, devices carrying + it exist, and the number was then reused for something else -- so the + blob's bytes mean one thing and the stamp claims another. Refusing it + wipes, which is the documented behaviour for a format we do not + recognise and strictly better than misparsing one. + + LISTED, not defaulted. storage_fromFlash has no default case on + purpose, so an unlisted version fails the -Werror=switch build rather + than falling anywhere. + """ + if not self.burned: + self.skipTest( + "no version is declared BURNED in storage_versions.inc on this " + "line -- STORAGE_VERSION is %d and the whole ladder has " + "readers. Nothing to measure here; the upgrade path is carried " + "by test_every_shipped_version_has_a_reader." % self.version) + for version in sorted(self.burned): + self.assertIn( + version, self.ladder, + "version %d is declared BURNED but is not in the ladder. The " + "entry has to stay: the enum is positional, so removing one " + "renumbers every version after it." % version) + arm = self._arm(version) + self.assertTrue( + _wipes(arm), + "burned version %d does not return SUS_Invalid. Arm was:\n%s" + % (version, arm)) + self.assertFalse( + _reads(arm), + "a reader behind burned version %d would parse a blob written " + "by a build whose layout has nothing to do with the current " + "format, and would do it silently. Arm was:\n%s" % (version, arm)) + + def test_no_shipped_version_is_burned(self): + """Burning a version that SHIPPED wipes every device carrying it. + + This is what keeps the burned set from being a loophole. Burnedness is + declared, and a declaration can be written for any number -- so the one + thing it may never cover is a format that reached real hardware. + STORAGE_VERSION_LAST_SHIPPED is where the firmware records how far that + reaches, and STORAGE_VERSION_LAST_SHIPPED_FLOOR keeps that record from + being quietly walked back. + + A version may only be burned if it lives strictly above the last + shipped release: written by an alpha, never by anything signed. + """ + shipped_and_burned = sorted( + v for v in self.burned if v <= self.last_shipped) + self.assertEqual( + [], shipped_and_burned, + "version(s) %s are declared BURNED but are at or below " + "STORAGE_VERSION_LAST_SHIPPED (%d), so signed firmware wrote them " + "and devices in the field carry them. Burning one erases those " + "wallets at boot." + % (shipped_and_burned, self.last_shipped)) # --------------------------------------------------------------------------- @@ -629,8 +1009,18 @@ def test_reboot_preserves_the_wallet(self): fingerprint and the ciphertext all round-tripped together. """ addr, off = self._create_wallet() - self.assertEqual(17, self.emu.read_u32(off, OFF_VERSION), - "this build committed a storage version other than 17") + # The stamp in flash must be the version the header declares. This is + # not a tautology and it is not a version pin either: the emulator was + # built from _ROOT, so the two sides are the WRITER and the DECLARATION, + # and a writer that stamps anything else produces blobs the next boot + # does not recognise. Reading 17 or 20 out of this file instead would + # only record which branch the author was standing on. + declared = _define(_read_source("include/keepkey/firmware/storage.h"), + "STORAGE_VERSION") + self.assertEqual( + declared, self.emu.read_u32(off, OFF_VERSION), + "the firmware committed a storage version other than the %d its " + "header declares" % declared) before = self.emu.image() self.emu.boot()