From df64c8177ffec5d11146758b7dc874b08d762367 Mon Sep 17 00:00:00 2001 From: highlander Date: Wed, 19 Aug 2026 23:17:15 -0500 Subject: [PATCH 1/2] ci: fail when a test declares screens it never captures The screenshot gate was `total PNG count > 0`. On the rc30 artifact that gate passed with 345 PNGs while EVERY suite 7.14.2 changed captured zero: the rendering evidence for a release whose whole subject is what reaches the screen did not exist, and nothing in CI said so. msg_ethereum_erc20_0x_signtx ABSENT #444/#468/#472 msg_display_disclosure ABSENT the byte-exact pager msg_binance_sign_tx ABSENT #430 test_verify_typed_data ABSENT EIP-712 The 33 suites that ARE captured are the stable ones nobody touched. This is the same silent-absence class as the Stage-1 skip that ci-gate closed in #474 and the deadlock that #477 made legible: the check reported on the wrong thing, so its green meant less than it appeared to. Adds a per-test audit after the count gate. Any SECTIONS entry that DECLARED screens must have captured some; skipped tests are excluded because a version-gated test cannot draw. Run against the rc30 artifact it names exactly the eight tests that declared screens and captured none. Repins python-keepkey to 1a6e785, which carries the SECTIONS entries, the --screenshot-audit implementation, and the fix for test_msg_display_disclosure answering ButtonRequests through its own callback -- which bypassed the capture hook, so the one suite written to police what the screen shows was the one suite whose screens nobody could look at. Also lands the rc30 hardware evidence (T1-T12), including the retraction of the #484 false finding and the T8 leg C result: AdvancedMode PERSISTS across a power cycle, contradicting what efa18ad77 and b53b63b0a both assert and build their risk analysis on. --- deps/python-keepkey | 2 +- .../rc30-evidence/T1-setup-ceremony.md | 63 ++++++++++++++++ .../rc30-evidence/T10-T11-0x-clearsign.md | 74 ++++++++++++++++++ .../rc30-evidence/T12-altcoin-binding.md | 70 +++++++++++++++++ .../rc30-evidence/T2-passphrase-cancel.md | 45 +++++++++++ .../T3-uniswap-authenticator-cancel.md | 68 +++++++++++++++++ .../rc30-evidence/T4-thorchain-memo.md | 51 +++++++++++++ .../rc30-evidence/T5-cutoff-findings.md | 75 +++++++++++++++++++ .../rc30-evidence/T6-T7-pager-chainid.md | 53 +++++++++++++ .../rc30-evidence/T8-advancedmode-gates.md | 69 +++++++++++++++++ docs/release/rc30-evidence/T9-eip712.md | 58 ++++++++++++++ scripts/emulator/python-keepkey-tests.sh | 16 ++++ 12 files changed, 643 insertions(+), 1 deletion(-) create mode 100644 docs/release/rc30-evidence/T1-setup-ceremony.md create mode 100644 docs/release/rc30-evidence/T10-T11-0x-clearsign.md create mode 100644 docs/release/rc30-evidence/T12-altcoin-binding.md create mode 100644 docs/release/rc30-evidence/T2-passphrase-cancel.md create mode 100644 docs/release/rc30-evidence/T3-uniswap-authenticator-cancel.md create mode 100644 docs/release/rc30-evidence/T4-thorchain-memo.md create mode 100644 docs/release/rc30-evidence/T5-cutoff-findings.md create mode 100644 docs/release/rc30-evidence/T6-T7-pager-chainid.md create mode 100644 docs/release/rc30-evidence/T8-advancedmode-gates.md create mode 100644 docs/release/rc30-evidence/T9-eip712.md diff --git a/deps/python-keepkey b/deps/python-keepkey index 6c4ad17fc..1a6e78547 160000 --- a/deps/python-keepkey +++ b/deps/python-keepkey @@ -1 +1 @@ -Subproject commit 6c4ad17fced26abd7959aadb987d6286b13ab620 +Subproject commit 1a6e78547244bd128df8528b2600a11ec0a95049 diff --git a/docs/release/rc30-evidence/T1-setup-ceremony.md b/docs/release/rc30-evidence/T1-setup-ceremony.md new file mode 100644 index 000000000..efc413ba0 --- /dev/null +++ b/docs/release/rc30-evidence/T1-setup-ceremony.md @@ -0,0 +1,63 @@ +# T1 — setup ceremony staged as a transaction (#429) + +Device: 7.14.2, fw_hash fd7b3901…, variant KeepKey, device_id 39353036114736342A004600. +Vehicle: `ResetDevice` with an injected `RecoveryDevice`, per plan §2 T1. + +## VERDICT: PASS + +### Phase A — injection refused, nothing committed + +| step | response | +|---|---| +| inject `RecoveryDevice` (label `pwned`, pin off, passphrase off) | `Failure code=1 'Device is in the middle of setup. Send Initialize or Cancel first.'` | +| `Cancel` | `Failure code=4 'Aborted'` | +| `EntropyAck` — **the exploit step** | `Failure code=1 'Not in Reset mode'` | +| end state | `initialized=False pin_protection=False passphrase_protection=False label=''` | + +Tester confirmed: photos 3 and 4 were the plain home logo, no `RECOVERY` cipher +screen at any point, and **no hold was demanded anywhere after the two PIN +screens**. + +This is the core of #429. On base `1af2ffe7de` the reset was still armed at the +`EntropyAck`, so it was consumed and committed a seed with the user's PIN and +passphrase silently stripped. `CHECK_NO_CEREMONY` (`fsm.c:118-125`) now refuses +before `recovery_cipher_init()` can write anything, and no screen is drawn — the +refusal is invisible to the user because nothing was ever staged. + +Note the label: `''`, not `pwned`. The injected settings left no residue. + +### Phase B — ceremony completes with the user's own settings + +| step | response | +|---|---| +| inject `RecoveryDevice` again | `Failure code=1` | +| `EntropyAck`, ceremony proceeds | seed backup screens, holds taken | +| end state | `initialized=True pin_protection=True passphrase_protection=True label='ceremony-B'` | + +## Photo 11 — resolved, was NOT a failure + +During the run that set the PIN, `Ping(pin_protection=True)` returned `Success` +with **no** `PinMatrixRequest`. The plan lists that as a hard fail. + +It is not, in this context. `Pin Caching` is an enabled policy and the PIN had +just been entered during the ceremony, so the session legitimately held it. On a +fresh session after a replug: + + initialized=True pin_protection=True passphrase_protection=True label='ceremony-B' + -> PinMatrixRequest + +**The plan's criterion needs qualifying:** `Ping(pin_protection=True)` returning +`Success` with no matrix is a hard fail only on a session that has not already +authenticated. Re-testing it inside the setup session tests the cache, not the gate. + +## Method note + +The photo-11 check in `gh429_setup_ceremony.py` printed PASS when the matrix never +appeared, because the assertion lived inside the `if PinMatrixRequest:` branch — +a check that cannot fail is not a check. Same shape as the T5 error. Verified +separately in `t1_pin_gate_check.py`, which asserts on the branch actually taken. + +## Device left as + +Initialised, random seed, PIN `789456`, passphrase ON, label `ceremony-B`. +T2 wipes this and loads `mnemonic12`. diff --git a/docs/release/rc30-evidence/T10-T11-0x-clearsign.md b/docs/release/rc30-evidence/T10-T11-0x-clearsign.md new file mode 100644 index 000000000..d41dd9215 --- /dev/null +++ b/docs/release/rc30-evidence/T10-T11-0x-clearsign.md @@ -0,0 +1,74 @@ +# T10 — transformERC20 bound to complete calldata (#444, #468) +# T11 — sellToUniswap clear-signs only when BOTH tokens resolve (#472) + +Device: 7.14.2, all-all-all seed, no PIN, no passphrase. + +## VERDICT: PASS (5/5) + +| run | payload | AdvancedMode | result | +|---|---|---|---| +| T10 A | transformERC20, 1480 B | OFF | **REFUSED** `Arbitrary contract data signing disabled by policy` | +| T10 B | transformERC20, 1480 B | ON | SIGNED v=38 | +| T10 C | transformERC20, 1024 B | OFF | **SIGNED v=37** (clear-signed) | +| T11 A | sellToUniswap, both tokens resolve | OFF | **SIGNED v=38** (clear-signed) | +| T11 B | sellToUniswap, `tokens[0]` = PEPE | OFF | **REFUSED** | + +## T10 — the wire proves RUN C without a photo + +RUN A establishes that with AdvancedMode OFF the blind path is refused outright. +So the ONLY way RUN C could sign is the 0x decoder claiming it. **The gate binds +clear-signing to complete calldata without killing it** — #468 did not trade one +defect for a usability cliff. + +Before `d3be389af`, `ethereum_contractHandled()` had +`if (zx_isZxTransformERC20(msg)) return true;` ABOVE the chunk-completeness +check, so the 1480-byte payload was claimed by the 0x decoder: the device showed +`TRANSFORM ERC20 / Input 53086.65334 USDT / Output 53029.30814 USDC`, then +`TRANSACTION`, and the 456 bytes past the initial chunk streamed in, were hashed, +and were never rendered. AdvancedMode was never consulted — a DEFAULT device +signed it. + +RUN C caveat: synthetic payload (fixture truncated to 1024 B, transformations[] +tail cut). It is also the only sub-1024-byte transformERC20 exercise in the plan, +filling the gap T11's card declared "a separate card". + +## T11 — one word apart + +Legs A and B are the SAME calldata on the SAME chain with ONE 32-byte word +changed. A clear-signs; B refuses. + +Before `dea1cd7e6`, `zx_isZxSwap()` claimed any `d9627aa4` call to the 0x proxy +on an allowlisted chain WITHOUT LOOKING AT THE TOKEN WORDS. +`zx_confirmZxSwap()` then called `ethereumFormatAmount()`, which emits the +literal `Unknown token value` on a lookup miss — so an unlisted sell token +produced exactly one screen, `UNISWAP / Sell Unknown token value / Buy at least +0.000389574704633884 ETH`, **and the device signed**. A screen naming a DEX, +naming no amount, hiding 296 bytes of calldata. + +The refusal lives in the PREDICATE, not the confirm: a false from +`ethereum_contractConfirmed()` would be read as a user cancel. + +### Evidence for the #455 diagnosis + +Leg B runs on **chain 1**, where no `uint8_t` chain-id truncation occurs, and +still refuses. That is direct evidence the #455 diagnosis is right: the failure +mode is an **unresolved token lookup**, not chain-id truncation. + +### Does NOT prove #414 + +Leg A still hides the `869584cd` affiliate tail. #414 remains open. + +## Photo checks with the tester + +- **B3 is load-bearing:** `CONFIRM ETHEREUM DATA` must read `1480 bytes` — + not `456`, not `1024`. First row exactly `415565b0000000000000000000000000`. +- C2 must be `TRANSFORM ERC20` with both token lines and NO `SEND` / + `CONFIRM ETHEREUM DATA`. +- T11 photo 1: `Sell 1 USDC` and `0.000389574704633884 ETH` — note the DOUBLE + SPACE, the sentinel ticker is literally `" ETH"`. `Unknown token value` + must appear nowhere. +- T11 leg B: no `UNISWAP` screen at any point. + +## Device left as + +all-all-all, no PIN, no passphrase, AdvancedMode OFF. diff --git a/docs/release/rc30-evidence/T12-altcoin-binding.md b/docs/release/rc30-evidence/T12-altcoin-binding.md new file mode 100644 index 000000000..26d367eb6 --- /dev/null +++ b/docs/release/rc30-evidence/T12-altcoin-binding.md @@ -0,0 +1,70 @@ +# T12 — altcoin amount and address binding (#408, #438, #407, #406, #430) + +Device: 7.14.2, all-all-all seed, no PIN, no passphrase, AdvancedMode OFF. + +## VERDICT: 7 of 8 required steps PASS. Step D (Solana SPL) NOT RUN. + +| step | payload | result | +|---|---|---| +| A1 | 1234567 uosmo | PASS signed | +| A2 | **1 uosmo** — the `base_to_precision` digit-drop test | PASS signed | +| A3 | 123456789012345678 uion (non-uosmo denom) | PASS signed | +| A4 | 68-char `ibc/2739…5EB2` denom | signed (informational, see below) | +| A5 | incomplete MsgSend | PASS `Failure 'Message is missing required parameters'`, no screen | +| B | LP add, 18 fractional digits | PASS signed | +| C | **Cosmos IBC transfer** | PASS signed | +| D | Solana SPL | **NOT RUN** | +| E | **31-char Binance denom (#430)** | PASS signed, no reboot | + +## What each step defends + +**A2** — `strlcpy(dst,src,n)` copied `n-1` digits, dropping the last, and wrote +`dest[dest_len]`, one past the caller's buffer. The screen must read +`0.000001 OSMO`; five zeros means the digit-drop is live. + +**A3** — before, `float amount = atof(...)` then `"%.6f %s"`: non-`uosmo` denoms +were never divided but still got a `.000000` tail, float32 lost everything past +~8 significant digits, **and `osmosis.c` hardcoded `"denom":"uosmo"` into the +SIGNED amino doc regardless of what was displayed.** + +**C** — before, ONE screen `Transfer %s to %s?` fed with +`msg->ibc_transfer.sender`: the sender printed in the destination slot, and the +signed receiver was never displayed at all. Now three screens with distinct +bodies. + +**E** — `char denom_str[14]` with `snprintf(denom_str, strlen(denom)+2, " %s", +denom)`: a 31-char denom is a 33-byte write into 14 bytes, a 19-byte stack +overflow. Device signed and did not reboot. + +## A4 — informational, file against #428 not #408 + +`confirm_transaction_output` goes through `layout_notification_no_title_bold`, +which `confirm_helper()` does NOT measure (it only measures +`layout_standard_notification`), so a 129-char body clips with no `CUT OFF`. +**The denom displayed IS the one being hashed**, so this is a rendering gap, not +a binding defect. Record the exact last character shown against #428. + +## Step D not run + +Step D hand-builds a Solana message (header bytes, account index table, +instruction data). The plan flags it as fragile: if the layout is off the device +errors before drawing anything — a harness failure, not a firmware finding. +Not attempted rather than recorded as a false result. + +**Whole-test pass requires A1, A2, A3, A5, B, C, D, E — so T12 is INCOMPLETE +until D runs.** Seven of the eight required steps pass. + +## Harness bugs (proto field types, all silent until they throw) + +- `CosmosMsgIBCTransfer.revision_height` / `.revision_number` are **strings** +- `CosmosMsgIBCTransfer.amount` is **uint64**, not a string +- `BinanceTransferMsg.BinanceInputOutput.address` is a **bech32 string**, not 20 + raw bytes. Passing bytes yields `'Failed to include transfer message in + transaction'` — which looks like a firmware refusal but is not one. + +## Photo checks with the tester + +A2 `0.000001 OSMO` (six digits, last a 1) · A3 all 18 digits with no decimal +point and no `.000000` tail · B `1.234567890123456789 GAMM-1 shares?` (eighteen +fractional digits) · C screen 3 `Confirm dest. address` showing +`osmo18vhdczj…` and NOT `cosmos15cenya…` · E all 31 denom chars legible. diff --git a/docs/release/rc30-evidence/T2-passphrase-cancel.md b/docs/release/rc30-evidence/T2-passphrase-cancel.md new file mode 100644 index 000000000..8925606ac --- /dev/null +++ b/docs/release/rc30-evidence/T2-passphrase-cancel.md @@ -0,0 +1,45 @@ +# T2 — a cancelled passphrase confirmation must not cache (#428a) + +Device: 7.14.2, fw_hash fd7b3901…, mnemonic12, passphrase ON, no PIN, label `test`. + +## VERDICT: PASS (8/8) + +| check | result | +|---|---| +| passphrase requested | PASS | +| confirmation screen raised for `topsecret` | PASS | +| host Cancel produced `Failure code=4 'Ping cancelled'` | PASS | +| **re-asked after cancel (the #428a fix)** | **PASS** | +| confirmation screen raised for `secondtry` | PASS | +| second ping succeeded | PASS | +| control: cached passphrase not re-requested | PASS | + +Before `b286dc009`, `passphrase_request()` discarded `review()`'s verdict and did +`ret = true`, so `session_cachePassphrase()` cached a passphrase whose +confirmation screen the host had just suppressed. The next passphrase-protected +request drew nothing and derived keys from it — the user was in a different +wallet than the one they believed they had opened. `passphrase_sm.c:158-161` now +propagates the verdict. + +The step-5 control matters: after a *successful* confirmation the passphrase IS +cached and is not re-requested. The fix cancels correctly without disabling +caching. + +## First run was INVALID — procedure, not firmware + +The first attempt reported 4 failures. It had a 3-second "photograph now" pause +between the ButtonAck and the Cancel; the tester held the button during it, the +confirmation completed (`Success`, message `cancel-probe`), and `topsecret` was +then cached entirely legitimately. All four failures followed from that one press. + +**Rule reinforced (see T5):** never leave a human window inside a test whose +premise is that no press occurs. The Cancel is now sent with no pause at all, so +there is no window to press in. Photo 2 is captured in a separate deliberate pass. + +`Initialize` calls `session_clear(false)` (`fsm_msg_common.h:9`) — drops a cached +passphrase, keeps the PIN. The script uses it to reset state between probes +instead of requiring a replug. + +## Device left as + +`mnemonic12`, passphrase ON, no PIN, label `test`. diff --git a/docs/release/rc30-evidence/T3-uniswap-authenticator-cancel.md b/docs/release/rc30-evidence/T3-uniswap-authenticator-cancel.md new file mode 100644 index 000000000..4cababb91 --- /dev/null +++ b/docs/release/rc30-evidence/T3-uniswap-authenticator-cancel.md @@ -0,0 +1,68 @@ +# T3 — Uniswap clear-sign and authenticator wipe honour a cancel (#421, #422) + +Device: 7.14.2, fw_hash fd7b3901…, mnemonic12, no PIN, no passphrase. + +## VERDICT: PASS (8/8 wire checks; photo checks with the tester) + +### Leg A — authenticator (#421) + +| check | result | +|---|---| +| authdata reset | PASS | +| account added | PASS | +| **account readable BEFORE cancel** | PASS `'keepkey.com:alice'` | +| wipe cancel produced Failure | PASS `Action cancelled` | +| **account survived the cancelled wipe** | PASS `'keepkey.com:alice'` | + +The before/after pair is the evidence: the account is provably present, the wipe +is cancelled, and it is still readable. On the old firmware the host's own abort +executed as a commit and destroyed every authenticator secret. + +Text change `5bccac024` verified in source: the wipe body is now only +`"Do you want to PERMANENTLY delete all authenticator accounts?"`. The sentence +`"If not, unplug Keepkey now."` is absent from `lib/` repo-wide — it existed only +while Cancel was inert. + +### Leg B — Uniswap approve (#422) + +| check | result | +|---|---| +| uniswap raised a confirm screen | PASS | +| uniswap cancel produced Failure | PASS `Signing cancelled by user` | +| **no signature returned** | PASS | + +`fd99fa75a` changes no text at all — every `confirm()` title and format string is +byte-identical. The diff only wraps them in `if (!confirm(...)) return false;`. +Before it, the handler walked forward to `return true` and signed a MAX_ALLOWANCE +approval to the Uniswap V2 router with zero button presses. + +Calldata is verbatim from `test_msg_ethereum_erc20_uniswap_liquidity.py:: +test_sign_uni_approve_liquidity_ETH` — `approve(0x7a250d56…, 0xffff…ff)` to the +FOX pool `0x470e8de2ebaef52014a47cb5e6af86884947f08c`. + +## First attempt was INVALID — precondition, not firmware + +Leg A initially failed with `Failure code=4 'passphrase incorrect for authdata'`, +not `Account not found`. Authdata is encrypted under a key derived from the seed; +T3's step 0 reloads the seed, so authdata left from the previous wallet +(`ceremony-B`) could not be decrypted and `initializeAuth` never ran. + +**Correct ordering, now in `t3_legA_authenticator.py`:** complete a wipe first +(hold it — `storage_wipeAuthData()` resets the encryption flag), then add the +account, then run the cancel test. Also added the A1b read-back so the test +proves the account exists before claiming it survived. + +Note the failure mode was loud, not silent — the prefix bytes were correct. +Verified against `fsm_msg_common.h:205-225`: +`\x15 initializeAuth:` `\x16 generateOTPFrom:` `\x17 getAccount:` +`\x18 removeAccount:` `\x19 wipeAuthdata:` + +## Coverage caveat — state it in the release record + +This exercises **2 of the 5** `confirm()` calls gated by `fd99fa75a`, and +**1 of the 3** authenticator mutations gated by `5bccac024`. The remaining paths +are argued-by-symmetry, not measured. See plan §4. + +## Device left as + +mnemonic12, no PIN, no passphrase, authenticator holds `keepkey.com:alice`. diff --git a/docs/release/rc30-evidence/T4-thorchain-memo.md b/docs/release/rc30-evidence/T4-thorchain-memo.md new file mode 100644 index 000000000..6a7c03359 --- /dev/null +++ b/docs/release/rc30-evidence/T4-thorchain-memo.md @@ -0,0 +1,51 @@ +# T4 — THORChain trailing memo fields disclosed (#415) + +Device: 7.14.2, variant KeepKey, mnemonic12, no PIN, no passphrase (state left by T3). + +## VERDICT: wire checks PASS; photo checks with the tester + +| check | result | +|---|---| +| A: signed bytes contain the affiliate tail `:ss:75` | PASS | +| B: matches the 7.14.1 fixture byte for byte | PASS | + + A hex tail: ...3a3432303a73733a3735 00000000 = ":420:ss:75" + B hex: identical to the pinned 7.14.1 fixture + +**Test A** proves the device signed exactly the bytes it displayed. Before +`354315c65`, everything after the limit — affiliate name, affiliate fee in bps, +aggregator routing — was hashed and signed with nothing on screen. A 75 bps skim +the user never saw. + +**Test B** proves the new `strtok` loop does not mutate what gets hashed: a +legacy memo signs identically to 7.14.1 and gains no spurious screens. + +## Card rewritten, as the plan required + +The original card subclassed `common.KeepKeyTest`, which demands a DEBUG_LINK +build — not the release artifact, so its evidence would be weaker for a release +gate. That requirement was an artifact of reusing the harness: these memo screens +are entirely host-driven and the pinned serialized tx depends only on the seed. +Rewritten as a plain `KeepKeyClient` script, holds taken physically, no +`set_buttonwait`. Prev tx served from `tests/txcache/` — no network. + +## Product-layer note (the reason this test matters twice) + +T4 verifies the *firmware* discloses trailing memo fields. It does NOT verify any +product path ever sends them. That distinction is not hypothetical: the XRP +THORChain memo bug was firmware-innocent and lost host-side in hdwallet +(`keepkey-vault#422`), and a firmware-only test would have shown a clean pass +forever. + +Vault's `/utxo/sign-transaction` schema types `outputs` as `z.array(z.any())` +(`schemas.ts:124`), so an `opReturnData` field passes through unstripped — the +structural flaw that bit XRP (`.strip()` eating tx fields) is absent here. That +is a schema reading, NOT a hardware measurement; a product-path probe is tracked +separately. + +**Plan §3 should gain a sibling section: "verified in firmware but not proven +reachable in the product."** T4 belongs in it today. + +## Device left as + +Unchanged — mnemonic12, no PIN, no passphrase. diff --git a/docs/release/rc30-evidence/T5-cutoff-findings.md b/docs/release/rc30-evidence/T5-cutoff-findings.md new file mode 100644 index 000000000..d7be6a73f --- /dev/null +++ b/docs/release/rc30-evidence/T5-cutoff-findings.md @@ -0,0 +1,75 @@ +# T5 — Cut Off pre-screen (#428/#481/#482), hardware round + +Device: 7.14.2, variant KeepKey, device_id 39353036114736342A004600, AdvancedMode=False. +Vehicle: `Ping` with `button_protection` — `fsm_msgPing` -> `confirm(..., "Ping", "%s", msg->message)`. + +## VERDICT: PASS + +| body | ButtonRequests | result | +|---|---|---| +| 100 ch | 1 | one screen, no warning | +| 117 ch | 1 | three full rows, no warning | +| 118 ch | 2 | CUT OFF — boundary is 118 | +| 119 ch | 2 | CUT OFF | +| 255 ch | 2 | CUT OFF | + +- **#481 confirmed on hardware.** Boundary is 118, not the 119 the plan predicted. + A clipped final glyph no longer reports as fitting (`draw.c:213-219`). +- **#482 confirmed on hardware.** The Cut Off screen emits its own ButtonRequest + (`code=1`, ButtonRequest_Other), so an auto-approving host cannot deadlock. +- No false positives at 100 or 117. +- Host `Cancel` aborts correctly. + +The plan's stated expectation that "only one ButtonRequest goes on the wire either +way" is stale — it predates #482. The count is now 1 for a fitting body and 2 for +a truncated one, and that count IS the #482 evidence. + +## FINDING — "Hold to view it anyway" discloses nothing (#485) + +`confirm_sm.c:441` re-draws the SAME truncated body after the warning: + + return confirm_screen(request_title, request_body, ...); + +`request_body` is unchanged and the generic `confirm()` path has no pager. The +byte-exact pager (`confirm_bytes()`, n/m counters) covers only the three +SignMessage handlers. The hidden remainder stays hidden; the second hold shows +the user nothing new. + +This is a source-level fact, independent of any hardware measurement. + +## RETRACTED — the "release bounce" defect (#484, PR #486, both closed) + +An earlier version of this document reported that a confirm screen accepted the +release bounce of the previous hold as consent, based on screens completing at +~1.6s with the tester "not touching the button". The tester was pressing it. No +such defect exists. + +**The control that refutes it:** a SINGLE confirm screen (117 chars, one +ButtonRequest), acked and left alone, completed at 5.076s. With no preceding +screen there is no transition and no bounce to inherit. + +**Method rules adopted for the remaining tests, T1-T4 and T6-T12:** + +1. **Never infer "no press" from a timing.** Absence of physical input is not + observable from the host. Do not design a pass criterion that depends on it. +2. **Prefer wire-level counts.** The ButtonRequest count is exact, host-visible, + and independent of the tester. Everything T5 genuinely proved came from it. +3. **Use host `Cancel` for abort tests**, never "the tester declines to press" — + Cancel is deterministic and the device honours it. +4. **A fix that does not move its target metric is refuted, not inconclusive.** + The #484 fix moved 1.602s to 1.662s and I read that as noise. +5. **Check the mechanism against the numbers.** CONFIRM_TIMEOUT_MS is 1200ms, so + confirming needs a press held over a second; a contact bounce is microseconds. + That contradiction was visible in the first measurement. + +## Trap (real, keep) + +An aborted run leaves a ButtonRequest queued that survives into the next session, +and the next run answers it silently — observed as BR1 returning code=4 instead of +the Ping's own code=23. `hwpreflight.idle_or_die()` now guards every script, and +the first-code assertion catches the rest. + +## Reproduce + + cd deps/python-keepkey/tests + PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python python3 cutoff_428.py diff --git a/docs/release/rc30-evidence/T6-T7-pager-chainid.md b/docs/release/rc30-evidence/T6-T7-pager-chainid.md new file mode 100644 index 000000000..876df73dd --- /dev/null +++ b/docs/release/rc30-evidence/T6-T7-pager-chainid.md @@ -0,0 +1,53 @@ +# T6 — exact-byte pager (#428/#410/#432) · T7 — omitted chain_id (#445) + +Device: 7.14.2, variant KeepKey, mnemonic12, no PIN, no passphrase, AdvancedMode=False. + +## T6 — VERDICT: wire checks PASS (4/4), photo checks with the tester + +| check | result | +|---|---| +| AdvancedMode is False (the #432 gates are gone) | PASS | +| BTC SignMessage with embedded NUL signed | PASS | +| ETH personal_sign with 44 spaces padding signed | PASS | +| TRON SignMessage with embedded NUL signed | PASS | + +No `Failure` on any step while AdvancedMode is False — the #432 AdvancedMode +gates on ETH and TRON SignMessage are gone, deliberately, in favour of full +disclosure. + +**A device that REFUSES to sign is not a pass here.** Refusal satisfies the +weaker property; this card asserts the shipped behaviour, which is that the +bytes are paged and shown. All three signed. + +Photo checks remain the substance of T6: three numbered ETH pages (1/3, 2/3, +3/3), `0xATTACKER` visible in full on 3/3, a literal `\x00` mid-body on the BTC +and TRON screens, and NOT ONE real blank gap — every space must render as the +four glyphs `\x20`. + +## T7 — VERDICT: PASS (4/4) + +| check | result | +|---|---| +| chain_id=1 signed with EIP-155 v | PASS `signature_v=38` | +| **omitted chain_id refused BEFORE any screen** | PASS `Failure code=3 'Chain Id out of bounds'` | +| **no ButtonRequest was emitted** | PASS | +| chain_id=3 signed with EIP-155 v | PASS `signature_v=41` | + +The refusal was the FIRST reply, read with `call_raw`. Because `confirm()` +writes its `ButtonRequest` before it draws, this proves the absence of a screen +ON THE WIRE — which a photo of an unchanged home screen cannot do alone. + +Before `885b485c4`, omission fell through to `chain_id = 0`, execution reached +both `confirm()` calls, no screen named a network, and a hold produced +`signature_v = v + 27`: a pre-EIP-155 signature replayable on every EVM chain. + +### Observation for the record, NOT a defect + +Chain 3 is accepted and correctly signed (`v=41`) but matches no case in the cid +switch, so photos D/E name no network and show bare `0.1` / `0.00042` with no +ticker. **#445 closes the signing hole only.** The display ambiguity is the same +one flagged for Base/Arbitrum/Avalanche under #455. + +## Device left as + +Unchanged — mnemonic12, no PIN, no passphrase. diff --git a/docs/release/rc30-evidence/T8-advancedmode-gates.md b/docs/release/rc30-evidence/T8-advancedmode-gates.md new file mode 100644 index 000000000..d1f13bb77 --- /dev/null +++ b/docs/release/rc30-evidence/T8-advancedmode-gates.md @@ -0,0 +1,69 @@ +# T8 — the four AdvancedMode refusal paths (#404, #405, #433, typed-hash gate) + +Device: 7.14.2, variant KeepKey, mnemonic12, no PIN, no passphrase. + +## VERDICT: legs A+B PASS (9/9). Leg C is diagnostic and produced a FINDING. + +### Leg A — AdvancedMode OFF: four refusals, two silent by design + +| gate | commit | result | screen? | +|---|---|---|---| +| TON SignTx | `efa18ad77` | `Failure 'Transaction signing disabled by policy'` | BLOCKED | +| ETH arbitrary call data | `30fbf9473` | `Failure 'Arbitrary contract data signing disabled by policy'` | BLOCKED | +| TRON SignTx | `b53b63b0a` | `Failure 'Enable AdvancedMode to blind-sign'` | **silent** | +| EthereumSignTypedHash | `424294ccc` | `Failure 'Enable AdvancedMode to blind-sign typed hashes'` | **silent** | +| negative control: `personal_sign` | — | **signed** | SIGN ETHEREUM MESSAGE | + +The asymmetry is the point and it held exactly. #433 is the one with a real +signing regression behind it: before, the code called `(void)review(…, "Warning", +…)` and DISCARDED the return, falling straight through to the data screen, the +fee screen, and a signature. + +The negative control matters as much as the refusals: a `BLOCKED` there would +mean `424294ccc`'s removal of the #432 SignMessage gates did not land, and every +Sign-In-With-Ethereum flow would be broken by default. + +ETH step passed `chain_id=1` explicitly. The suite's own +`test_ethereum_blind_sign_blocked` no longer exercises this gate — it omits +`chain_id`, so T7's guard rejects it first. + +### Leg B — AdvancedMode ON: all four sign + +TON, ETH arbitrary data, TRON, and typed hash all signed. 4/4. + +## FINDING — AdvancedMode PERSISTS across a power cycle + + AdvancedMode after physical replug: True + +Two independent lines agree: +- **Code:** `storage_commit` at `fsm_msg_common.h:738`; bit 12 at + `storage.c:803/920`, read back at `:862/:990`. That is flash, not session. +- **Measurement:** still `True` after unplug/replug. + +The outlier is the commit messages. **Both `efa18ad77` and `b53b63b0a` state +"AdvancedMode is session state (it is off again after a power cycle)" and build +their risk analysis on it.** That reasoning is wrong on this build: once a user +enables AdvancedMode, blind-signing stays enabled until they explicitly disable +it. The blast radius is "until revoked", not "until unplugged". + +**Release-note correction, not a test failure.** + +⚠️ A note from the rc29 round (7.15 line) claims AdvancedMode was proven to be +session state, off after a power cycle. Either the behaviour differs between +branches or one measurement is wrong. Today's result is for 7.14.2 and is backed +by the code; **the 7.15 claim needs re-checking on its own branch before it is +relied on again.** + +## Harness bugs found (would silently break other tests) + +1. **`client.tron_sign_tx()` is broken in the pinned python-keepkey.** It passes + `raw_tx=`, but `TronSignTx`'s field is `raw_data`. The call dies in protobuf + before anything reaches the device — a test using it never tests anything. + Build `TronSignTx(address_n=…, raw_data=…)` directly. +2. **TON requires a fully-hardened path** (ed25519). `m/44'/607'/0'/0/0` fails + with `Failed to derive private key`; `m/44'/607'/0'/0'/0'` works. Leg A's TON + refusal was unaffected because the policy check runs BEFORE derivation. + +## Device left as + +mnemonic12, no PIN, no passphrase, **AdvancedMode OFF** (verified, not assumed). diff --git a/docs/release/rc30-evidence/T9-eip712.md b/docs/release/rc30-evidence/T9-eip712.md new file mode 100644 index 000000000..0ab3ff5ba --- /dev/null +++ b/docs/release/rc30-evidence/T9-eip712.md @@ -0,0 +1,58 @@ +# T9 — EIP-712: both endpoints closed by default (#403, #411, #436, #439, #440) + +Device: 7.14.2, **all-all-all** seed (reseeded here), no PIN, no passphrase. + +## VERDICT: PASS (10/10) + +| check | result | +|---|---| +| AdvancedMode OFF at start | PASS | +| structured EIP-712 refused | PASS **0.021s** `'Structured EIP-712 disabled pending canonical display hardening'` | +| step 1 returned fast (no screen drawn) | PASS 0.021s | +| typed hash refused, AdvancedMode OFF | PASS **0.021s** `'Enable AdvancedMode to blind-sign typed hashes'` | +| step 2 returned fast (no screen drawn) | PASS 0.021s | +| AdvancedMode enabled (control frame) | PASS | +| typed hash signs with AdvancedMode ON | PASS sig `2c2d8c7c1fac…` | +| address matches all-all-all fixture | PASS `0x73d0385F4d8E00C5e6504C6030F47BF6212736A8` | +| **structured STILL refused with AdvancedMode ON** | PASS | +| AdvancedMode restored to OFF | PASS | + +## Why the timing is the evidence + +Both closed endpoints returned in **21 ms**. A drawn screen blocks on the +button, so a sub-second return proves no screen existed — a stronger claim than +a photograph of an unchanged home screen, which cannot distinguish "no screen" +from "device asleep" or "camera on the wrong unit". Photo 3 (`ENABLE POLICY`) +is the control frame that makes the blank photos meaningful. + +## The containment property (step 6) + +With AdvancedMode **ON**, structured EIP-712 is still refused. The disable is +hardcoded — `ethereum_structured_eip712_enabled()` returns `false` at +`ethereum.c:59` — not a policy. A user who enables blind-signing does NOT +re-open the withdrawn parser. + +Before the change, that parser drew a screen per field plus a domain screen +**whose title was the attacker-supplied domain name**. + +The disable is in `424294ccc`, not `1e30736ed`: +`git log -S ethereum_structured_eip712_enabled 1af2ffe7de..HEAD` returns only +`424294ccc`. + +## Photo checks with the tester + +Photo 4 must be `EIP-712 BLIND SIGN` / "Cannot verify these hashes. Trust the +host?" and must come FIRST, ahead of `VERIFY ADDRESS`. Each digest body must +show all 64 hex characters un-truncated with no `CUT OFF` in front of it. + +Overlaps T8's leg B4 — same path, same fixture. This is primary; B4 corroborates. + +## Harness note + +`load_device_by_mnemonic()` takes `language` as a REQUIRED positional argument. +Omitting it raises TypeError after the wipe has already run, leaving the device +blank mid-script. + +## Device left as + +all-all-all, no PIN, no passphrase, AdvancedMode OFF (verified). diff --git a/scripts/emulator/python-keepkey-tests.sh b/scripts/emulator/python-keepkey-tests.sh index b716fefbc..18b27a7b3 100755 --- a/scripts/emulator/python-keepkey-tests.sh +++ b/scripts/emulator/python-keepkey-tests.sh @@ -95,6 +95,22 @@ if [ "$SCREENSHOT_COUNT" -eq 0 ]; then exit 1 fi +# A total count > 0 cannot distinguish "captured everything" from "captured +# something". On the 7.14.2 rc30 artifact this gate passed with 345 PNGs while +# EVERY suite the release changed captured zero -- the rendering evidence for a +# release about what reaches the screen did not exist, and nothing said so. +# Audit per test: any SECTIONS entry that DECLARED screens must have captured +# some. Skipped tests are excluded; a version-gated test cannot draw. +echo "=== Screenshot audit (per-test) ===" +python3 ../scripts/generate-test-report.py \ + --screenshot-audit /kkemu/test-reports/screenshots \ + --audit-junit /kkemu/test-reports/python-keepkey/junit-screenshots.xml \ + --fw-version=$FW_VERSION || { + echo "FATAL: tests declared screens they did not capture (see list above)." + echo "1" > /kkemu/test-reports/python-keepkey/status + exit 1 +} + # Phase 2: Full test suite — SECTIONS is the source of truth. # pytest may exit non-zero (some tests fail before gating kicks in), # so we capture the JUnit XML regardless, then validate against SECTIONS. From 75dccea3564210ae44184ece33300cab6b90f17e Mon Sep 17 00:00:00 2001 From: highlander Date: Wed, 19 Aug 2026 23:29:53 -0500 Subject: [PATCH 2/2] ci: stop guessing FW_VERSION, and stop guessing it wrong The emulator test script detected the firmware version with: FW_VERSION=$(sed -n '/^project/,/)/p' CMakeLists.txt \ | grep -oP '\d+\.\d+\.\d+' || echo "7.14.0") `grep -oP` is a GNU extension. This container's grep is BusyBox, which has no -P, so that command ALWAYS failed and the fallback silently supplied 7.14.0 -- on the 7.14.2 release branch. CI has been reporting "Detected FW_VERSION=7.14.0 from CMakeLists.txt" while CMakeLists says 7.14.2. Everything downstream keys off that number. SECTIONS entries are selected by ver_ge(fw_version, min_fw), so at 7.14.0 every test gated to 7.14.1 or later was excluded from BOTH the screenshot filter and validate_junit. That is the root cause of the gap #487 set out to fix: the suites this release changed captured no screens because the report system did not believe those tests existed yet. It also means the per-test screenshot audit added in #487 passed vacuously -- with no 7.14.2 sections active, nothing declared screens, so nothing could be missing. The audit was correct; it was being asked about the wrong release. Two changes: - read VERSION with sed alone, portable to BusyBox - FAIL if it cannot be read, rather than defaulting. A wrong version here is invisible and silently narrows what CI checks -- exactly the failure mode ci-gate (#474) and the screenshot audit were added to prevent. Same class as those two: the check ran, went green, and was measuring something other than what its name implied. --- scripts/emulator/python-keepkey-tests.sh | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/scripts/emulator/python-keepkey-tests.sh b/scripts/emulator/python-keepkey-tests.sh index 18b27a7b3..4fe649457 100755 --- a/scripts/emulator/python-keepkey-tests.sh +++ b/scripts/emulator/python-keepkey-tests.sh @@ -61,7 +61,24 @@ echo "=== End diagnostic ===" echo "=== Phase 1: Report-driven screenshot capture ===" # Detect firmware version from CMakeLists if not set in env if [ -z "$FW_VERSION" ]; then - FW_VERSION=$(sed -n '/^project/,/)/p' /kkemu/CMakeLists.txt | grep -oP '\d+\.\d+\.\d+' || echo "7.14.0") + # grep -oP is a GNU extension. This container's grep is BusyBox, which has + # no -P, so the old command ALWAYS failed and `|| echo "7.14.0"` silently + # supplied a wrong version. Everything downstream keys off this: SECTIONS + # entries are filtered by ver_ge(fw_version, min_fw), so on the 7.14.2 + # release branch every test gated to 7.14.1 or later was excluded from the + # screenshot filter AND from report validation. That is why the suites this + # release changed captured no screens. + # + # Use sed only, and FAIL rather than defaulting: a wrong version here is + # invisible and silently narrows what CI checks. + FW_VERSION=$(sed -n 's/^[[:space:]]*VERSION[[:space:]]\{1,\}\([0-9]\{1,\}\.[0-9]\{1,\}\.[0-9]\{1,\}\).*/\1/p' /kkemu/CMakeLists.txt | head -1) + if [ -z "$FW_VERSION" ]; then + echo "FATAL: could not read VERSION from /kkemu/CMakeLists.txt." + echo "Refusing to guess -- a wrong FW_VERSION silently narrows the" + echo "screenshot filter and the SECTIONS validation." + echo "1" > /kkemu/test-reports/python-keepkey/status + exit 1 + fi echo "Detected FW_VERSION=$FW_VERSION from CMakeLists.txt" fi export FW_VERSION