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..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 @@ -95,6 +112,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.