release: open the 7.14.2 security line from the active release - #418
Merged
Conversation
Cut from 1af2ffe, which is the shipped v7.14.1 plus 12 EVM bugfix commits and the head of develop on both this fork and upstream. Bumps VERSION to 7.14.2 and records the release plan: base selection and why the old release/7.14.2 branch (199e42a, preserved as abandoned/7.14.2-20260515) was not it, the threat model, the fourteen tracked defects, and the evidence standard for the memory-safety claims. No functional change. Fixes land on this branch as separate commits.
7.14.2 adds no features, so it needs nothing outside master on either repo. Both previous pins are strict ancestors of master (ahead=0, behind=1); the single commit each master carries beyond them is an identical CI-only addition (.github/workflows/copilot-review.yml, +19/-0). Repinning is therefore a superset move with no effect on generated code. Verified device-protocol master still defines every message this tree handles: TonSignTx, TronSignTx, SolanaSignTx/SolanaTokenInfo, the EthereumSignTx family, OsmosisMsgSend and CosmosMsgSend, with their .options bounds. deps/crypto/trezor-firmware is deliberately NOT repinned: its canonical branch is 'keepkey', not master, and .gitmodules declares no branch for it.
Guard the four discarded confirm() returns in fsm_msgEthereumSignTypedHash, and add one real confirm() gate immediately before eip712_sign() in e712_types_values, so no EIP-712 signature can be produced without a physical button hold.
Risk: Behaviour changes a legitimate host/user will notice:
1. EthereumSignTypedHash: no visual change in the happy path — the same four screens, each still advanced by the same button hold. Only difference: a host that sends Cancel (or Initialize) during any screen now gets Failure_ActionCancelled instead of a signature. Hosts that never cancel see no change.
2. Ethereum712TypesValues: ONE new OLED screen at the end of the message (eip712typevals != 1) call — title "Sign Typed Data", body "Sign with address 0x<checksummed 40 hex>?" (~62 chars, well under BODY_CHAR_MAX=352 and shorter than the existing "Confirm hash digest: <64 hex>" screen that already ships). One extra button hold per typed-data signature. The domain call (eip712typevals == 1) is untouched — no new screen, it signs nothing.
3. If the user does not confirm that new screen, have_ds is cleared, so a host retrying the message call gets MSG_NO_DS ("EIP-712 domain seperator hash must be calculated first") and must re-send the domain message. Any host that implements retry-after-cancel without re-sending the domain will now see that failure instead of a second prompt. This matches what already happens after a successful signature.
4. Existing pyk tests that drive Ethereum712TypesValues will now see one additional ButtonRequest on the message call; tests using an auto-ack debuglink client are unaffected, tests that count ButtonRequests will need +1.
Residual (NOT closed by this patch, deliberately): the per-field screens inside eip712.c remain (void)review(...), so the user still cannot abort mid-review and the individual field values are still display-only. Closing that requires the confirm-propagation rework (new USER_CANCELLED code, dsConfirm() returning int, 9 checked call sites in parseVals) that 7.15 did on top of confirm_bytes; it is real new machinery and out of scope for 7.14.2. The security defect — signature with zero button presses — is closed by the terminal gate.
Also unchanged by design: no AdvancedMode policy gate on the raw-hash endpoint (that is 7.15's ethereum_typed_hash_policy_allows and would block a currently-working host path).
Closes #403
…n table
Extend the raw-calldata disclosure guard in `ethereum_signing_init` so the `UnknownToken` sentinel (returned on a token-table miss) is treated like `NULL`, routing untabled ERC-20 calls through the warning + `confirm("Confirm Ethereum Data")` gate.
Risk: Behaviour change is confined to `data_total == 68` + standard ERC-20 transfer/approve selector + `tokenByChainAddress()` miss + `data_needs_confirm` (i.e. not a `ethereum_contractHandled` path).
What a user will notice on the OLED for an untabled token:
- One extra screen: "Confirm Ethereum Data" showing the first 24 calldata bytes (`a9059cbb` + zero padding + the leading 8 bytes of the recipient) and "68 bytes", requiring a physical button hold.
- Plus, when the AdvancedMode policy is off, the existing "Warning / Signing of arbitrary ETH contract data..." review screen.
- The preceding "Send Unknown token value to 0x<recipient>" / "Approve withdrawal of up to Unknown token value by 0x..." screen is unchanged, and the fee screen is unchanged (`token != NULL` is still true, so it still says "the tokens").
Hosts sending a tabled token see zero difference. Non-ERC-20 contract calls see zero difference. `ethereum_getStandardERC20Coin` (:107) and `ethereumFormatAmount` (:337) are untouched, so the TRANSFER/coin-lookup paths are unaffected.
The one real friction: a legitimate host that sends a genuinely-new token (not yet in `ethereum_tokens.def`) now costs the user two extra button holds. That is the intended trade — the device was previously signing calldata it never showed.
Explicitly NOT fixed here (flag for a separate issue): the AdvancedMode warning at :770 is still `(void)review(...)`, i.e. informational and not a gate; and neither the ERC-20 confirm screen nor the data screen ever shows the contract address (`msg->to`) for an untabled token. #412 is about reaching the disclosure; both of those are distinct defects with wider blast radius.
Closes #412
Stop rendering the unbindable to_address/amount in fsm_msgTonSignTx; replace both confirms with a single honest blind-sign screen naming the raw_tx byte count, behind an AdvancedMode policy gate. Risk: Behaviour changes a legitimate host will notice: 1. TON signing now requires AdvancedMode. With AdvancedMode off (and per prior hardware findings it is session state — it is OFF after every power cycle), TonSignTx returns Failure_ActionCancelled "Transaction signing disabled by policy" after a "Blocked" review screen. Every existing TON host flow breaks until the user enables it. This is a real, deliberate availability regression; see decision_note. 2. The ButtonRequest count drops from two to one. BASE emitted ButtonRequest_ConfirmOutput (only when has_to_address && has_amount) followed by ButtonRequest_SignTx. Now only ButtonRequest_SignTx is emitted, plus one ButtonRequest_Other on the blocked path. Hosts that drive the ButtonRequest/ButtonAck loop generically (hdwallet, pioneer) are fine; anything that hardcodes "expect exactly two button requests for TON" would desync. 3. OLED text changes: title "Transaction" -> "TON Blind Sign", body "Really sign this TON transaction?" -> "Sign N-byte TON transaction?", and the "Send X TON to <addr>?" screen disappears entirely. Users will read this as a downgrade in information; it is in fact the removal of information the device was never able to verify. With the current proto (raw_tx = 32-byte body cell hash) the normal reading is "Sign 32-byte TON transaction?". 4. ton_formatAmount() (lib/firmware/ton.c:221, declared include/keepkey/firmware/ton.h:58) loses its only caller and becomes dead code. It is a non-static extern function so this produces no warning and no build break; 7.15 left it in place too. I deliberately did not delete it — leaving it costs a few bytes of flash and keeps the diff to a single file. Not a risk: no change to the bytes that get signed, no proto/.options change, no new symbol, no new header. The private key is still memzero'd on every exit path, and the new blocked path returns before fsm_getDerivedNode so no key material is ever derived on it. Closes #404
…ruction Scale SPL TransferChecked amounts only by the decimals inside the signed instruction, show the authenticated mint on its own screen, and drop the host symbol when its claimed decimals disagree or the text is not safe printable ASCII. Risk: Behaviour changes a legitimate host will notice: 1. ONE EXTRA SCREEN per SPL TransferChecked instruction — a \"Token mint\" confirm carrying the full base58 mint, requiring an additional button hold before the amount screen. Plain Transfer (ix 3) is unaffected because has_mint is false there. Multi-instruction token transactions gain one screen per checked transfer (capped at SOL_MAX_INSTRUCTIONS = 8). 2. The unlabelled fallback now reads e.g. \"1.000000 tokens\" instead of \"1000000 tokens\", because it scales by the signed decimals. Strictly more informative, but the string on the OLED changes; any screenshot/OLED golden covering an SPL transfer will need re-baselining. 3. A host that sends token_info whose decimals disagree with the signed instruction silently loses its symbol (shows \"tokens\"). No host with correct metadata is affected. Vault/Pioneer should be spot-checked for hardcoded or stale decimals in the SolanaSignTx builder — if it fills decimals from a token list rather than from the instruction it built, users would see \"tokens\" where they used to see a symbol. 4. A TransferChecked whose data field is 9 bytes now classifies as unknown, making the whole transaction OPAQUE and therefore blocked unless AdvancedMode is on. No conforming host emits that, but a host that hand-rolls the instruction and drops the trailing decimals byte would go from \"signs cleanly\" to \"Enable AdvancedMode to blind-sign\". Not closed by this patch, by design: a host can still name a worthless mint \"USDC\" with matching decimals. BASE has no on-device token table and no signed-metadata verifier, so the symbol is fundamentally unverifiable here. The mitigation is that the authenticated mint is now always disclosed on its own screen, so the symbol is decoration rather than the identity. Full closure is the 7.15 solana_token_info_trusted / known-token work and is explicitly out of scope. ROM/RAM: negligible — one 45-byte stack buffer in an existing case block, one small static predicate; the removed snprintf path roughly offsets it. Closes #406
In fsm_msgCosmosMsgAck's IBC branch, require has_receiver/has_amount and confirm the actual signed receiver (and sender) with confirm_cosmos_address instead of printing the sender as the destination.
Risk: Behaviour change a legitimate host/user will notice: the IBC branch goes from 5 confirm screens to 7. The amount screen no longer names an address at all ("Transfer 1.234 ATOM via IBC?" instead of "Transfer 1.234 ATOM to cosmos1...?"), and two new address screens appear before the source-channel screen: "Confirm sender address" then "Confirm dest. address", each on the cosmos address layout (large address + QR), each requiring a button hold. Two extra holds per IBC transfer.
Deliberate choice worth reviewing: I kept a sender screen. The sender is information the user has today (mislabelled), the firmware never checks it against the device key, and ORPHAN also shows it -- so dropping it would be a silent disclosure regression shipped inside a security fix. If the reviewer prefers strictly two extra-screens-minus-one, delete the "Confirm sender address" block; the receiver block alone still closes #407.
Newly-rejected host messages: an IBC message omitting `receiver` or `amount` now fails fast with Failure_FirmwareError "Message is missing required parameters". Omitting `receiver` already failed at BASE (bech32_decode("") returns false -> Failure_SyntaxError), just later and after a wasted button hold; omitting `amount` previously succeeded and signed a 0-amount transfer. Any host relying on amount-defaults-to-0 breaks -- that is intended.
Compile surface checked at BASE: msg->ibc_transfer.has_receiver / .has_amount exist (all CosmosMsgIBCTransfer fields are `optional` in deps/device-protocol/messages-cosmos.proto:94-103, so nanopb emits has_ for every one); confirm_cosmos_address is declared in include/keepkey/firmware/app_confirm.h:49 and is already called 9 times in this same translation unit; no new header include is needed. No tabs; all lines <= 80 columns and wrapped the way clang-format's greedy bin-packing produces (the six-operand condition splits exactly as ORPHAN's equivalent does).
Not fixed here, flagged for follow-up rather than smuggled in: (1) signtx_tendermint.c :453-457 computes the device's own from_address and then discards it instead of requiring sender == from_address; (2) msg->ibc_transfer.denom is required-present but never used -- the serializer is handed the literal "uatom".
Closes #407
Thread the host-supplied MsgSend denom into the signed amino document (escaped, unbounded-length) instead of hardcoding "uosmo", and require has_denom so nothing unverifiable is ever displayed or signed. Risk: What a legitimate host notices: - A host that omits `denom` on OsmosisMsgSend now gets `Failure_FirmwareError "Message is missing required parameters"` instead of a silent uosmo send. This is the intended behaviour change and the only one that can break an existing integration. Worth checking hdwallet/pioneer emit `denom` on the Osmosis send path before shipping; every other Osmosis message type at BASE (delegate, undelegate, redelegate, IBC) already reads `.denom`, and OsmosisMsgSend.denom is field 3 in device-protocol, so this is almost certainly already populated. - For `denom == "uosmo"` with a plain-digit amount, the hashed bytes are IDENTICAL to BASE. Existing uosmo sends produce the same signature. That is the regression golden below. - Sends with a non-uosmo denom (IBC/factory) now produce a DIFFERENT signature than 7.14.1 did — a correct one. Anyone who somehow had a working IBC flow was getting a uosmo-denominated signature, so nothing legitimate depended on the old bytes. On the OLED: nothing new appears. The same "Send <amount> <denom> to <addr>" body renders; only the denom is now guaranteed to be the one that gets signed. Residual, NOT closed by this patch and pre-existing at BASE: - `confirm_helper()` body rendering has no pagination at BASE (fixed only in 7.15 by paginating inside confirm_helper). `strbuf` is BODY_CHAR_MAX=352 so nothing overflows, but a long IBC denom pushed through `confirm_transaction_output` may still run past what the screen draws. Closing that needs the 7.15 confirm machinery (confirm_bytes / confirm_body_split), which is not importable here. Every other Osmosis message at BASE already displays host denoms through the same path, so this patch does not widen the class. - `float amount = atof(...)` at fsm_msg_osmosis.h:143 loses precision above 2^24 and the display therefore rounds; the SIGNED amount is the exact decimal string, so display and signature can disagree in low digits for very large sends. Pre-existing, shared with delegate/undelegate/redelegate, and out of scope for #408 — flagging it, not touching it. - `osmosis.c:78` hardcodes the FEE denom as uosmo. Correct today (no fee-denom field in the proto), noted so nobody "fixes" it later by mistake. Closes #408
Measure the confirm body before drawing it, and when it will not fit the three on-screen rows, put an explicit "CUT OFF" screen with its own hold in front of it, so a partly-shown body is never shown without saying so. Risk: Behaviour change a user will see on the OLED: any confirm() or review() that goes through layout_standard_notification and whose body needs more than three rows now shows an extra screen titled "CUT OFF" reading "This text is too long for the screen. Only part of it is shown. Hold to view it anyway.", and needs a second button hold before the (still partial) body appears. This is value-dependent, not call-site-dependent - the same format string can be one screen for one amount and two for another, which is the known overflow class. Real BASE paths that will trip it: Bitcoin-family SignMessage and the VerifyMessage review with long host messages, long Cosmos/THORChain memos, long ETH data confirmations. For correct hosts the wire dialogue is unchanged: one ButtonRequest, one ButtonAck, one response. Only the wall-clock time to the response grows. Hosts with a ButtonAck timeout short enough to be tripped by one extra hold would time out, but no such timeout is device-side and the loop already has none. DebugLink and the emulator: each screen consumes one DebugLinkDecision. Any pyk/emulator test that drives a long-body confirm now needs one more decision or it will hang. This is the same cost 7.15's pagination imposes (the thorchain suite there records 5 presses where there used to be 2) - budget for it before running the fw test suite. review*() paths still discard the verdict (review() is hardcoded `return true`), so a host Cancel on the new warning screen ends the warning but review() still returns true and the caller proceeds. That is the pre-existing review() defect, unchanged and untouched here; the warning is still displayed, which is what #410 asks for. The check is a measurement, not a proof: it holds because calc_str_line() and draw_string() implement the same wrap loop today. If someone edits one loop and not the other the predicate under-reports. The new board-unit test does not catch that; it pins the arithmetic, not the coupling. What this does NOT do: it does not disclose the hidden text. A user who holds through the warning still signs something they have only partly seen - informed, but not fully. Full disclosure needs pagination (see decision_note). Adjacent defect noticed and deliberately NOT touched: fsm_msg_coin.h:272 passes (char*)msg->message.bytes to confirm("%s"), and SignMessage.message is `max_size:1024` in include/keepkey/transport/messages.options with no NUL guaranteed - a host sending exactly 1024 bytes makes the "%s" run off the end of the array into the rest of the decoded struct. Separate issue, separate fix. ROM/RAM cost: roughly 120 bytes of code plus ~90 bytes of rodata for the warning string. No new RAM buffers. Closes #410
…source Replace the eight hand-bounded strncat() calls in parseType() with a fail-closed append helper, closing the stack buffer overflow at eip712.c:208 (and the latent 1-byte overflow at :205) and turning silent type-string truncation into a refusal instead of a wrong typehash. Risk: Behaviour change for hosts: an EIP-712 message whose encodeType string exceeds STRBUFSIZE (511) chars now returns UDEF_NAME_ERROR and the device sends Failure_Other "EIP-712 user defined type name too long" instead of producing a signature. No legitimate flow is lost by this: at BASE such a message either truncated (yielding a typehash that no verifying contract or ecrecover-based checker would ever accept — the signature was already useless) or corrupted the stack. Anything that fits today still fits: the helper's boundary (used + added <= STRBUFSIZE) is byte-for-byte the boundary the already-correct strncat sites enforce. For calibration, Seaport's OrderComponents encodeType is ~443 chars, so real-world types do run close to the limit but under it — worth a smoke test against a Seaport/Permit2 payload before tagging. Nothing changes on the OLED. No new screen, no new button press, no new prompt; the type string is never displayed, only hashed. The dsConfirm() review_with_icon() call and every confirm() call site are untouched, so consent semantics are unchanged. Residual, deliberately NOT fixed here (all pre-existing at BASE, all outside #411's strncat/strcat/strcpy scope — file separately rather than widening this patch): - eip712.c:346-348 dsConfirm() sscanf's 40 hex chars out of dsverifyingContract with no length check; a short verifyingContract string reads past its NUL (bounded read within the 2048-byte host JSON buffer, not a write). - eip712.c:152-153 typeType = json_getValue(obTest) is passed straight to encodableType(), which strncmp's it; a JSON value node with no value yields a NULL deref. My helper's !value guard catches it at the append, but encodableType already dereferenced by then. - eip712.c:229/252/282 strncpy(byteStrBuf, ..., 2) can read past the end of a short hex string (read-only, in-buffer). Also note that ORPHAN's 6c27e11 ("reject overflow widths and wrong array sizes") is a genuinely separate defect in encodableType/encodeBytesN and is NOT addressed by this patch. Closes #411
… chunk Add one central guard in `ethereum_contractHandled()` so a contract decoder may only claim an EVM tx when the entire calldata fits in `data_initial_chunk`; oversized calldata falls through to the existing generic raw-data disclosure path. Risk: Behaviour change for legitimate hosts, in full: 1. A THORChain deposit / depositWithExpiry, 0x sellToUniswap, Sablier withdrawFromSalary, Uniswap add/removeLiquidityETH, or Uniswap approve whose calldata exceeds 1024 bytes no longer gets its clear-sign screens. Instead the user sees the Send screen, the "Signing of arbitrary ETH contract data…" AdvancedMode warning, the "Confirm Ethereum Data" hex screen, then the fee screen. Signing is still possible — this is a disclosure downgrade, not a block. Per the sizing above, no known legitimate call for any of those six selectors reaches 1024 bytes, so in practice this path should never fire for an honest host. An unusually long THORChain memo (roughly >640 characters) is the only realistic way to trip it. 2. Nothing changes for 0x transformERC20, by design. 3. makerdao is unaffected — it already applied this exact test internally at makerdao.c:628; the central guard just makes it redundant for that decoder. Residual, stated plainly: transformERC20 still signs trailing calldata that was never displayed. That is a deliberate, ORPHAN-matching exception justified by the pinned proxy address and the displayed input/min-output bounds — not an oversight. If the release wants zero exceptions, drop the hoisted transformERC20 line from the patch and accept that >1KB 0x swaps drop to the raw-data screen. Compile risk: none identified. `data_total` (uint32_t param), `msg->data_initial_chunk.size` (pb_size_t, promotes cleanly for `!=`), and `zx_isZxTransformERC20` are all already used in this exact translation unit at BASE. No new include, no new symbol, no header change. `data_total` was already a used parameter (makerdao_isMakerDAO) so no unused-parameter warning shifts either way. On-OLED: no new screen text, no new screen. Users only ever see the pre-existing generic path. Closes #413
…nd fence blind signing Delete the unbound "Send %s TRX to %s?" screen in fsm_msgTronSignTx and replace it with an honest AdvancedMode-gated blind-sign disclosure, since BASE has no TRON protobuf parser and therefore cannot verify anything about the transaction it signs. Fences TRON signing behind AdvancedMode, consistent with the existing fences for opaque Solana (fsm_msg_solana.h), unknown ETH calldata (ethereum.c) and bare-Ed25519 TON (fsm_msg_ton.h). This stops the device lying about the destination. It does not verify the destination: that needs a raw_data parser and is deliberately out of scope for this release. Risk: Behaviour change for legitimate hosts, and it is the reason this needs a sign-off: TRON signing now requires the AdvancedMode policy to be enabled. AdvancedMode is session state (it is off again after a power cycle), so a KeepKey Vault user sending TRX or TRC-20 will hit `Failure_Other "Enable AdvancedMode to blind-sign"` once per session until they toggle it. Nothing in the vault currently prompts for that, so this is a visible regression on a previously working path unless the host is updated — see decision_note for the ungated variant. On the OLED the "Send 12.5 TRX to TR7NHq..." screen disappears and is replaced by "Blind Sign / Sign unverified 218-byte TRON transaction? Amount and destination unknown." Users will read this as a downgrade; it is not — that destination was never verified, it was echoed back from the host. The flow is still two button holds (blind-sign screen, then the untouched "Transaction / Really sign this TRON transaction?"), same shape as BASE's opaque-Solana path. No new attack surface: no new parsing, no new buffers, no new heap or stack of consequence. Body string is at most 74 chars, well under BODY_CHAR_MAX (352, include/keepkey/board/layout.h:59) and under the ~84-char three-line OLED guarantee, so no truncation and no overflow. No ROM impact worth measuring (one policy call, one confirm, one fewer 32-byte stack buffer). Compile check against BASE: `storage_isPolicyEnabled` is declared at include/keepkey/firmware/storage.h:158 and storage.h is included by fsm.c:59, which is what #includes fsm_msg_tron.h (fsm.c:293). `confirm` is declared at include/keepkey/board/confirm_sm.h:94 with `__attribute__((format(printf, 3, 4)))`, so the `%u` against `(unsigned)msg->raw_data.size` type-checks. `ButtonRequestType_ButtonRequest_SignTx`, `FailureType_Failure_Other`, `FailureType_Failure_ActionCancelled`, `_()`, `memzero`, `layoutHome` are all already used elsewhere in this same file. `tron_formatAmount` stays declared in include/keepkey/firmware/tron.h and defined in tron.c — it is non-static, so dropping its only caller produces no warning. Closes #405
Make the ETH/TRON message-signing and message-verify screens say, in the body text the user actually sees, that the message was truncated and by how much — and fix two ETH-verify rendering bugs found while doing it. Discloses the truncation rather than refusing to sign. Refusing would break every Sign-In-With-Ethereum flow, whose bodies run 200-450 characters. Full review of long bodies needs paginated confirmation, which is 7.15 work. Risk: Behaviour change visible to users on the OLED, only in the truncating case: - ETH/TRON messages longer than 114 printable chars now show 84 chars instead of 114, prefixed with e.g. "TRUNCATED 84/389 bytes: ". Binary messages longer than 57 bytes now show 42 bytes of hex instead of 57, same prefix. Messages that already fit render byte-for-byte as before, because truncBuf is "" and the format is "%s%s". - ETH VerifyMessage: a message whose first 114 bytes are printable but which contains non-printable bytes later now renders on the "Bytes Verified" hex screen instead of the "Message Verified" text screen. That is the point of the change, but it is a visible reclassification a legitimate host could hit with, say, a UTF-8 message containing accented characters. TRON and ETH-sign already behaved this way, so this aligns Verify with the rest of the tree rather than introducing a new rule. - ETH VerifyMessage binary path previously showed size/2 bytes; it now shows up to 57. More hex on screen for short binary messages. No host-visible protocol change: no new failure codes, no new messages, no field changes, no refusal path added. Every confirm() return value that was checked before is still checked; none is newly discarded. Not covered by this patch, and worth stating so nobody reads the issue as closed: the bytes past the preview are still signed unread. This makes the truncation honest, not the message verifiable. ROM/RAM: four snprintf call sites with an extra format arg, plus four 32-byte stack locals. Should be a few hundred bytes of flash at most, but confirm against the CI ROM budget gate before merging — 7.14.2 inherits the tight budget. Closes #409
…onical
Validate the ABI head offset word before clear-signing 0x sellToUniswap and THORChain deposit/depositWithExpiry, and bound every fixed-offset read against the received calldata chunk in all four decoders.
Non-canonical encodings now fail closed rather than being described from hardcoded
offsets.
Risk: Behaviour change visible to users: none for a correct host. Solidity/ethers/web3 and the 0x API all emit canonical ABI head pointers (0x80 for sellToUniswap's tokens[], 0x80/0xa0 for deposit/depositWithExpiry's memo), so every real 0x swap and every real THORChain deposit still clear-signs with exactly the same screens.
What a rejected encoding looks like on the OLED: nothing. The checks run before the first confirm(), so ethereum.c:711-716 aborts immediately and the host gets Failure_ActionCancelled with "Signing cancelled by user". That message is misleading (the user pressed nothing) but it is the pre-existing behaviour for every other decode failure at BASE, e.g. zxswap's numOfTokens not in {2,3}. I did not change it, because doing so means touching ethereum.c and the router — see decision_note.
Residual risk deliberately left open (NOT part of #414, do not scope-creep into 7.14.2):
1. thortx.c still parses the memo as a fixed 64 bytes and ignores the ABI memo-length word. With the offset now pinned the memo starts where the router reads it, but a memo longer than 64 bytes still has its tail (affiliate, min-out) undisplayed. ORPHAN fixes this with length-word parsing + a 256-byte cap + an exact-end check; that is a separate, larger change.
2. thor_isThorchainTx is not pinned to the THORChain router address or chain_id at BASE, so any contract carrying the deposit selector gets the THORChain UX. That is a different issue entirely.
3. zxtransERC20's transformations[] tail remains undisclosed (bounded on-chain by minOutputTokenAmount).
Compile risk assessed as low: every identifier used (`msg->data_initial_chunk.size`, `.bytes`, `memcmp`, `size_t`, `bool`, `adder`, `is_expiry`) exists at BASE; `.size` is `pb_size_t` from PB_BYTES_ARRAY_T(1024) per include/keepkey/transport/messages-ethereum.options. Both comparison shapes I use already appear at BASE and compile. Adjacent-string-literal concatenation happens after escape resolution, so `"...\x00" "\x00...\x80"` is standard-safe. Flash cost is roughly 128 bytes of .rodata plus a few dozen bytes of code — negligible against the 7.14.2 ROM budget.
Closes #414
…arse failure Make thorchain_parseConfirmMemo return a tri-state so a host Cancel can never be mistaken for "memo not parseable" (and vice versa), read the ETH router memo's real ABI length/offset instead of a fixed 64 bytes at a fixed offset, disclose every trailing ':'-separated memo field, fix the memo copy that dropped its last byte and over-read its source, and bind the "Thorchain router" trust label to mainnet. Covers the memo bound and the cancel/parse-failure conflation. The router pin is NOT addressed here: the address migrated v1 -> v3 -> v4, so pinning this tree's literal would send every current THORChain swap to the blind-sign path. Filed separately. Risk: Things a legitimate host/user will notice: 1. Extra button holds. Any memo with fields past the labelled ones now gets one confirm screen per extra field, titled "Additional memo field". A normal ShapeShift swap memo carries ":<affiliate>:<bps>", so a THORChain swap gains 2 screens on all three paths (ETH router, native RUNE, BTC OP_RETURN). That is the point of the fix — those bytes set an affiliate skim and were previously signed invisibly — but it is a visible UX change on the OLED and the labels are generic, not "Affiliate fee N bps to X" as in 7.15. 2. ETH router deposits now hard-fail (Failure_ActionCancelled from ethereum.c, no blind-sign fallback) if the calldata is split across chunks, if the ABI memo offset word is not the canonical 0x80/0xa0, if the memo length word has dirty high bytes or exceeds 256, or if there are bytes after the padded memo. A router deposit is at most ~4+6*32+256 = 452 bytes against a 1024-byte data_initial_chunk, so a correct host is never split; but if some host does send data_length > data_initial_chunk.size for a deposit, it will stop signing. This is deliberate fail-closed behaviour: previously such a tx signed while only the first 64 memo bytes were shown. 3. min_chunk stays 228/260, so a very short memo (<33 bytes, e.g. "ADD:ETH.ETH") still does not clear-sign — exactly as at BASE. I did not import 7.15's relaxation to 164/196. Residual, NOT closed by this patch: - confirm()'s body has no pagination at BASE (no confirm_bytes / confirm_body_split), so a single very long memo field can still truncate on screen. An attacker who can make one field longer than the body budget can still hide its tail. Closing that needs the confirm-pagination work, which is real new machinery. - strtok collapses consecutive ':' separators, so an empty middle field can still shift a later field into an earlier label (e.g. an affiliate name displayed under "Confirm limit"). Every field is now SHOWN, but a field can still be MISLABELLED. The non-destructive empty-preserving splitter that fixes this is a rewrite of the function; not taken. - thor_isThorchainTx is still selector-only: any contract carrying the deposit selector still gets the THORChain clear-sign UI instead of the AdvancedMode blind-sign warning. See decision_note. - The withdraw screen still uses a "%3.2f" float format; untouched, out of scope. Compile notes I checked against BASE: -std=gnu99 -Wall -Wextra -Wshadow -Wsign-compare -Werror. All new comparisons are unsigned-vs-unsigned; both `for (size_t i ...)` loops in thortx.c are in sibling (non-nested) scopes so there is no shadowing or redeclaration; declarations after statements already occur in both files; memcpy/strtok/memzero/confirm are all already used in thorchain.c; msg->has_chain_id / msg->chain_id / msg->has_data_length exist on EthereumSignTx at BASE (makerdao.c already gates on has_chain_id); no unit test references thorchain_parseConfirmMemo (unittests/firmware/thorchain.cpp is not in unittests/firmware/CMakeLists.txt), so the signature change has exactly four call sites. Closes #415
#416 (10-iteration PIN KDF) is out of scope: it needs a flash image first, which is a different threat model from the USB-host defects in this set, and raising the count is a storage-format change rather than a self-contained fix. Also states plainly that the THORChain router pin and TRON destination verification are NOT closed here, so the release notes cannot imply they are. Corrects the build gate: -DKK_BITCOIN_ONLY=ON yields a byte-identical image on this tree (#397), so a 'both variants' checkbox could not fail.
clang-format 20 reflows the confirm() arguments added in the previous commit. Matches what CI's lint-format job enforces; no behaviour change.
secret-scan has been failing on develop itself, including at 1af2ffe, so this is a pre-existing red rather than something this branch introduced. Every finding was decoded before being exempted: - include/keepkey/firmware/u2f/u2f_keys.h holds the FIDO U2F batch attestation key. The standard shares one attestation key across a device batch by design, so it is compiled into every image and extractable from any release binary. It authenticates nothing and guards no funds. - include/keepkey/firmware/u2f/trezordevkey.pem is an orphan: its private scalar does not match U2F_ATT_PRIV_KEY[] and its public key does not match the compiled attestation certificate. It corresponds to nothing that ships. It is exempted rather than deleted because CI scans full history and the file has been present since fe3e5e0 (2019-04-27); deleting it now would not clear the finding. - Everything under deps/ is vendored third-party crypto, flagged by generic-api-key on ed25519-donna constants and AES self-test tables. - Bare 40-character lowercase hex is a git object ID. The security docs record exact submodule pins and gitleaks scores a commit SHA at ~3.8 entropy. Scoped to the git SHA-1 shape; key material here is 32 or 64 bytes and does not match. Verified: full-history scan over 5355 commits reports no leaks, and a planted EC private key under lib/firmware/ is still detected, so the scanner is narrowed and not blinded.
Five confirm() calls across the two Uniswap clear-sign handlers throw away their return value. Because confirm() returns false only on a host-sent Cancel/Initialize tiny-message, discarding it discards the host's cancel: a hostile host can drive the entire dialogue to completion and reach `return true` with zero button presses. zx_confirmApproveLiquidity() is the worst case — it is the 0x095ea7b3 ERC-20 approve path matched against the Uniswap router, both of its confirms are discarded, and it unconditionally returns true, so the ungated path yields a signed token approval. Fix: gate all five confirms with `return false`, matching the confirmFromAccountMatch() call already correctly gated at zxliquidtx.c:167.
Callers checked: Repo-wide grep (excluding .git) for zx_confirmApproveLiquidity / zx_confirmZxLiquidTx / ethereum_contractConfirmed found exactly one call chain and no test or emulator harness callers:
1. lib/firmware/ethereum_contracts.c:73 — `if (zx_isZxLiquidTx(msg)) return zx_confirmZxLiquidTx(data_total, msg);` and :75-76 — `if (zx_isZxApproveLiquid(msg)) return zx_confirmApproveLiquidity(data_total, msg);`. Both are direct tail returns inside ethereum_contractConfirmed(), so a new false propagates verbatim. No change needed there.
2. lib/firmware/ethereum.c:711 (CURRENT working tree, re-read after this release's edits) — the sole caller of ethereum_contractConfirmed():
if (!ethereum_contractConfirmed(data_total, msg, node)) {
fsm_sendFailure(FailureType_Failure_ActionCancelled,
"Signing cancelled by user");
ethereum_signing_abort();
return;
}
Risk: Low. Behaviour changes only along the host-cancel path, which previously produced a signature and now produces Failure_ActionCancelled — the intended direction, and it matches every other clear-sign handler in this tree.
Compile check by inspection: `confirm` is declared bool at include/keepkey/board/confirm_sm.h:105 and both files already include keepkey/board/confirm_sm.h; `ButtonRequestType_ButtonRequest_ConfirmOutput`, `appStr`, `amt`, `tikstr`, `poolstr`, `arStr`, `constr1`, `constr2`, `de
Closes #421
In the working tree at /Users/highlander/WebstormProjects/keepkey-stack/projects/keepkey-firmware-7142, three authenticator mutations call `confirm()` and throw the bool away, then mutate unconditionally: - `wipeAuthData()` — authenticator.c:80-91, confirm at :81, then `storage_wipeAuthData()` + `memzero(authData, ...)` at :86-89. - `addAuthAccount()` — authenticator.c:93-159, confirm at :147, then the slot write + `setAuthData()` at :151-156. - `removeAuthAccount()` — authenticator.c:298-338, confirm at :331, then `memzero(&authData[slot], ...)` + `setAuthData()` at :335-336. (The issue text calls this `deleteAuthAccount`; no such symbol exists in this tree — the function is `removeAuthAccount`.) `confirm()` (lib/board/confirm_sm.c:357-379) returns `confirm_helper()`'s verdict verbatim, so `false` here means the host sent a Cancel/Initialize tiny-message. Discarding it means a hostile host can send `Ping "\x19wipeAuthdata:"`, immediately follow with Cancel, and the device still permanently destroys all ten authenticator secrets while reporting `Success` — the host's own abort is executed as a commit. Same for add (host-chosen TOTP secret is persisted) and remove. Note the screen already blocks: `confirm_helper()` has no timeout, so this is not a "zero-wait" wipe. The defect is purely that the Cancel verdict is dropped, so cancelling and confirming are indistinguishable outcomes. Second half of the issue: `wipeAuthData()`'s body text ends "...If not, unplug Keepkey now." That instruction exists only because Cancel was inert. Once Cancel is honoured the sentence is wrong (it tells the user to yank a device that will now abort cleanly), so the fix removes it. Callers checked: Grepped the entire tree (excluding build/ and .git/) for `wipeAuthData|addAuthAccount|removeAuthAccount|getAuthAccount|generateOTP|AUTH_ERR_TYPE|NUM_AUTHERRS|UNKERR`. Every hit: 1. lib/firmware/fsm_msg_common.h:247 — `errcode = addAuthAccount(...)` inside `fsm_msgPing()`, case INITAUTH. ALREADY consumes the return code; the shared `if (errcode != NOERR)` at :291 turns CANCELED into `fsm_sendFailure(FailureType_Failure_ActionCancelled, "Action cancelled")` + `layoutHome()` + `return`. No change needed. Adding a new enum value does not break it because the switch is over AUTH_MSG_TYPE, not AUTH_ERR_TYPE — there is no `switch (errcode)` anywhere, so no -Wswitch fallout. 2. lib/firmware/fsm_msg_common.h:275 — `errcode = removeAuthAccount(...)`, case REMACC. Same: already consumes the code. No change needed. 3. lib/firmware/fsm_msg_common.h:280 — `wipeAuthData();` with `errcode = NOERR;` h Risk: Low, but three things the applier should know. 1. Behaviour change visible to hosts and to out-of-tree pyk/DebugLink tests: a `Ping` carrying `\x19wipeAuthdata:`, `\x15initializeAuth:` or `\x18removeAccount:` that is followed by a host Cancel now returns `Failure_ActionCancelled` instead of `Success`, and the mutation does not happen. Any external test that asserts Success while cancelling was asserting the bug. No NEW hang is introduced: `confirm_helper()` already blocked with no timeout befor Closes #422
…e resumed Verified against the working tree at /Users/highlander/WebstormProjects/keepkey-stack/projects/keepkey-firmware-7142/lib/firmware/reset.c (unchanged from 1af2ffe in this release — `git diff 1af2ffe -- lib/firmware/reset.c` is empty, so the reported line numbers are accurate). Confirmed layout: `awaiting_entropy` declared at :42, armed at :127 as the last statement of reset_init(), cleared at :146 in reset_entropy(). It is the ONLY gate on the seed-generation path — `fsm_msgEntropyAck` (lib/firmware/fsm_msg_common.h:545) has no CHECK_* macro and calls reset_entropy() unconditionally. reset_init() has FIVE early returns, none of which clear the flag: :55 invalid strength :65 display_random && no_backup :82 no_backup double-confirm not completed :102 Internal Entropy confirm not completed :112 change_pin() failed The exploit is worse than "stale entropy is reused", because reset_init writes the module globals BEFORE it can abort: `strength = _strength;` (:58) and `no_backup = _no_backup;` (:59) execute ahead of every confirm. So: 1. Ceremony A completes reset_init (user consented, no_backup=false) -> awaiting_entropy = true, device sits armed waiting for EntropyAck. 2. Hostile host sends ResetDevice with no_backup=true. Line 59 sets the global no_backup=true, then the double-confirm at :72/:76 is reached. The host sends a Cancel tiny-message, confirm_helper() returns false, and reset_init returns at :82 WITHOUT clearing awaiting_entropy. 3. Host sends EntropyAck. reset_entropy() passes the :131 gate on ceremony A's stale flag, hashes ceremony A's int_entropy with host-chosen ext_entropy, sees the corrupted no_backup==true, and takes the :148 branch: storage_setNoBackup(); storage_setMnemonic(); storage_commit(); fsm_sendSuccess("Device reset"). That is a fully provisioned device with a host-influenced seed, the recovery seed NEVER displayed, and ZERO button presses. The same trick corrupts `strength` (256 -> 128) even on the backup path. No other module clears this state. `git grep -n awaiting_entropy -- lib include` returns only reset.c:42/127/131/146. There is no reset_abort()/reset_cancel() anywhere (`git grep -rn 'reset_abort\|reset_cancel'` is empty), and neither fsm_msgInitialize (fsm_msg_common.h:1-11) nor fsm_msgCancel (:553-561) touches it — both abort recovery_cipher/signing/ethereum/tendermint/eos and call session_clear(), which lives in storage.c and knows nothing about reset.c's statics. reset_entropy() itself has NO equivalent hole: the flag is cleared at :146 before any confirm and before any `goto exit`, so the backup-confirm cancel at :161-166, the MAX_PAGES abort at :198-202 and the per-page confirm_constant_power cancel at :245-249 all run with the flag already down. FIX: one line — `awaiting_entropy = false;` as the first statement of reset_init(), before the strength validation. Callers checked: Traced every consumer of the state and of both functions. No signature and no return type changes, so the caller surface is small and fully enumerated. 1. reset_init — exactly one call site. `git grep -n reset_init -- lib include` -> include/keepkey/firmware/reset.h:36 (decl), lib/firmware/reset.c:46 (def), lib/firmware/fsm_msg_common.h:533 (only call). lib/firmware/fsm_msg_common.h:530-543 `fsm_msgResetDevice`: guarded by CHECK_NOT_INITIALIZED (fsm.c:107-112, which returns before reset_init on an initialized device), then calls reset_init and returns. It is `void`, ignores nothing, and inspects no state afterwards. The added line is invisible to it. Note the CHECK_NOT_INITIALIZED guard does not weaken the fix: storage_isInitialized() (storage.c:1954-1957) tests has_node || has_mnemonic, and an armed-but-unfinished ceremony has committed neither — reset.c only reaches storage_s Risk: Low. One assignment to an existing file-static bool, at the top of a function that already writes that same variable 80 lines later. No new symbol, no new include, no signature change, no header change, no change to any confirm() call or its handling. Compile check by inspection: `awaiting_entropy` is declared at reset.c:42 as `static bool`, i.e. in scope and assignable at reset.c:49 (same translation unit, above the function). `<stdbool.h>` reaches this file via include/keepkey/firmware/reset. Closes #423
…elease #403 fixed a discarded confirm() return value. Sweeping for the same pattern found eight more sites: three in the authenticator, five across the two Uniswap clear-sign handlers. #421-#423 track them and the fixes are on this branch. The worst was zx_confirmApproveLiquidity, the 0x095ea7b3 ERC-20 approve path pinned to the Uniswap router, which discarded both confirmations and returned success unconditionally.
BitHighlander
marked this pull request as ready for review
August 17, 2026 05:21
calc_str_line() returned uint32_t but accumulated into a uint8_t. A confirm body carrying 255 newlines wrapped the count back to 0, so confirm_body_fits() reported that it fitted and the 'Cut Off' warning added earlier in this release was skipped. The 352-byte confirm buffer has room for a benign prefix, 255 newlines and a hidden suffix, so the guard was bypassable by a host that chose its whitespace. Found in review of this branch. The guard was introduced here, so this closes a hole this release opened rather than one it inherited: promoting calc_str_line() to a security boundary without auditing its accumulator. Adds Board.ConfirmBodyFitsLineCountDoesNotWrap, which fails on the previous code at 254/255/256/257/340 newlines and on the prefix+newlines+suffix shape an attacker would actually send.
thor_confirmThorTx() kept a 228/260-byte minimum calldata check while the comment added earlier in this release documents the correct 164/196. The old floor assumed a fixed 64-byte memo and rejects valid short ones: '+:BTC/BTC::t:10' pads to 32 bytes, giving 196 bytes of calldata for deposit(), which was refused and fell through to blind signing. 164/196 is exactly the bound needed to read the memo's ABI length word, which sits at 4 + 4*32 for deposit() and 4 + 5*32 for depositWithExpiry(). Lowering it is safe because the exact-length equality check further down is what actually bounds the memo: memo_off + memo_padded must equal data_initial_chunk.size.
Review flagged that the allowlist carried a global regex for \b[0-9a-f]{40}\b,
which suppressed every 40-character lowercase hex finding anywhere in the tree
rather than only documented commit hashes. Replaced with a docs/ path scope, which
is where the git-SHA false positives actually live.
Full-history scan over 5356 commits reports no leaks, and the negative control still
detects a planted EC private key under lib/firmware/ while allowing a submodule pin
in docs/.
This was referenced Aug 17, 2026
Closed
… jobs Every build and test job declares `needs: [lint-format, static-analysis, check-submodules, secret-scan]`. When one of those gate jobs fails, GitHub marks the whole downstream graph SKIPPED rather than failed -- and a skipped job is not a red check. The run summary shows green ticks on whatever finished, which reads as healthy unless someone opens the job list and notices that the ARM build, the unit tests and both python suites produced nothing. That has happened three times on this release line: gitleaks failing on develop, lint-format timing out inside its apt.llvm.org install (#471), and gitleaks again after an unpinned upstream bump (the commit before this one). Each time the absence of signal was indistinguishable from success. ci-gate needs every required job, runs `if: always()` so it executes even when they skip, and fails unless each reports exactly `success`. failure, cancelled and skipped are all not-success, because for a required job they are. publish-emulator is deliberately excluded: it is workflow_dispatch-only and is legitimately skipped on every push and pull_request. Point branch protection at this job rather than the individual ones. It is the only check whose green means "the entire graph ran and passed". Verified against the two shapes that actually occurred, plus the obvious ones: all success -> PASS one skipped -> FAIL one cancelled -> FAIL one failure -> FAIL python-integration-tests cancelled-> FAIL (today's release/7.14.2) lint-format cancelled + 2 skipped -> FAIL (the #471 run) Consequence, stated up front: release/7.14.2 will now report RED until #466 is fixed, because python-integration-tests is killed at its 30-minute timeout on every run. That is the honest state of the branch and has been all along; this job only stops it being hidden. Closes #471
…-tests-and-pin ci: pin gitleaks, allowlist pre-submodule test history, and add an aggregate gate
…screen away
All four review*() variants called confirm_helper() as (void) and then
`return true`. confirm_helper() returns false for exactly one reason -- this
device has one button and no reject, so false means a host-sent
Cancel/Initialize -- and discarding it meant a host could suppress a screen and
still be told the review happened.
Propagating the result is one line each. What matters is the one caller where
the operation CONTINUED afterwards.
passphrase_sm.c: the "passphrase confirmation" screen is the only place a user
sees the passphrase their keys will be derived from. passphrase_request()
ignored the result and set ret = true, so a host that answered that screen with
a Cancel suppressed the display and still reached
session_cachePassphrase(). The session then derived a different wallet than the
user believed they were opening, with nothing shown on the OLED. All five
callers of passphrase_protect() already test its result, so returning the
review's verdict is enough to close it.
The other ten call sites were audited and none has the same consequence:
- fsm_msg_ton.h:108,182, fsm_msg_solana.h:485, ethereum.c:809 are "Blocked"
screens followed immediately by fsm_sendFailure + abort. Suppressing the
screen hides an explanation for a refusal that happens regardless.
- transaction.c:456 warns about a duplicate transaction and then sets
retval = -1. The transaction is refused either way.
- recovery_cipher.c:604,608,614 report a dry-run result and then send the
matching Success/Failure. Nothing is signed and no state changes.
- authenticator.c:257,264 gate and annotate an OTP display.
They now receive a meaningful value rather than an unconditional true, which is
worth having even where today's consequence is nil.
COVERAGE GAP, STATED DELIBERATELY. There is no automated test for this.
review() drives confirm_screen()'s loop, which blocks on a button press or an
injected USB tiny-message, and no unit test in this tree sets that up -- the
confirm-driver suites are why firmware-unit does not complete unfiltered. The
test belongs in the python-keepkey integration suite, which can send
PassphraseAck followed by Cancel and assert the passphrase is not cached; that
suite is currently pinned to a commit predating this work. Until then this is
verified by reading, by the ARM build, and by the hardware round.
This closes the suppression primitive in #428. It does NOT close #428: the
render measurement still covers only layout_standard_notification, so the
custom-layout address and xpub screens remain outside the measured guarantee.
Verified: ARM cross-compile in the pinned builder image; cppcheck clean under
CI's exact invocation (0 findings); clang-format clean across CI's entire lint
scope.
Refs #428
…d of by timeout
python-integration-tests has never completed on this branch. Two tests deadlock
-- this release added confirmation screens the pinned suite does not
acknowledge, so the firmware waits for a ButtonAck that never arrives while the
test waits for a response that never comes -- and with no per-test bound that is
a 30-minute JOB kill producing no JUnit XML. Phase 2, the documented gate, never
runs, and every file after the stall is unmeasured. Absence of a result is
indistinguishable from a pass.
Bound each test instead of the job. method=signal, not thread: thread kills the
process so one deadlock still costs the rest of the run, while signal raises
inside the blocked test, which fails BY NAME, and the suite continues.
The effect is not subtle. Same emulator, same tests, same firmware:
before 30-minute job kill, no JUnit XML, 2 files known to stall,
everything after them unmeasured
after 22 failed, 362 passed, 47 skipped in 133.59s, 66KB of JUnit XML
The deadlock was hiding 22 failures, not 2. Every one is a consequence of an
intentional change in this release meeting a stale expectation, and none
indicates a firmware defect:
14 AdvancedMode policy gate -- TON, TRON and ETH contract data now require
it (the documented BREAKING change); these tests never enable it
5 chain_id refusal -- #445 working as designed; fixed by python-keepkey #215
2 the deadlocks themselves -- THORChain and EOS extra disclosure screens
1 a CallException still to be triaged individually
60s per test is roughly 30x the slowest healthy file in this suite (multisig,
~2s), so a genuinely slow test is not at risk.
This does not fix the underlying expectation mismatch, which belongs with the
deps/python-keepkey repin. It makes that mismatch legible in two minutes with a
test name attached, rather than as an opaque half-hour timeout -- and it is
worth keeping afterwards regardless: an integration test should never be able to
consume the whole CI budget because one side is waiting for a button
acknowledgement the other forgot to send.
Refs #466
…-cancel fix(confirm): stop review() reporting success when the host took the screen away
…imeout ci(integration): bound every test, so a deadlock fails by name instead of by timeout
…riant The integration suite is red, the quickest route to green is to stop showing a screen, and the commit that does it looks like a test fix. Writing the rule down is the cheapest defence against that, because the pressure arrives disguised as housekeeping and the result would look like progress. Tests adapt to newly required disclosure. Firmware disclosure is never removed to satisfy a stale test. If a test fails because the device now shows more, the test is wrong. The extra THORChain screens disclose affiliate-fee bytes that were previously signed invisibly. Also replaces the known-red section, which was wrong in two ways. It said the integration stall was pre-existing at 4406197 -- a controlled run (same test files, only firmware varying) showed 1af2ffe passes and head hangs, so this release introduced it. And it could not enumerate the failures at all, because the job was killed before producing JUnit XML. With #477 the suite completes in 134s and the list is finite: 22 failed, 362 passed, 47 skipped, every failure a stale pinned expectation and none a firmware defect. Recorded as a table with an owner per row, since that is now the concrete work list for the repin. Adds the two measurement traps that cost real time here: do not run the suite sequentially against one emulator, and treat the PINNED submodule commit as the test source of truth rather than the working tree -- reading master's copy of test__sign_transformERC20 produced a confident and wrong "this breaks nothing". Refs #466 #474 #477
…closure docs(release): make "disclosure never yields to a stale test" an invariant
12 ordered tests derived from what this release actually changed on screen, each read out of the current code rather than from commit messages, and each with a stated failing-photo criterion. Operationally important and not previously recorded: the rc30 artifact is UNSIGNED (ci.yml has no signing step; the release workflow that signs cannot run because release.yml requires TAG_VERSION == CMakeLists VERSION, so v7.14.2-rc30 fails validation). Expect the unofficial-firmware warning and a storage wipe on install. If the device comes up already initialised after flashing, that contradicts the storage-wipe model and is itself a finding. Section 3 records what is unreachable in principle from a physical device, so adjacent evidence is not mistaken for coverage: 1e30736's EIP-712 cancel propagation sits behind a function 424294c hardcodes to false, and confirm_helper()'s source-truncation branch cannot be reached through Ping because Ping.message caps at 256 against BODY_CHAR_MAX 352. Section 5 records where the agent cards disagreed rather than smoothing it. The live one: AdvancedMode persistence is contradicted three ways -- storage.c reads as persisted (bit 12), two commit messages assert session state and build their risk analysis on it, and the rc29 notes say "proven off after power cycle". T8 leg C settles it on hardware; if it persists, two commit messages need correcting before release. Bench setup documents three host traps reproduced live on this machine, not theorised: Vault holding USB interface 0 (LIBUSB_ERROR_ACCESS), the protobuf C++ backend rejecting the vendored _pb2 files, and tests/config.py misfiling this device's single interface into the debug slot so `import config` dies with TypeError. Five of the eight pre-written scripts need the WebUSB shim. Refs #466 #467 #469
draw_string_walk() advanced str_write unconditionally after draw_char_impl(). When the LAST glyph was rejected, have_space went false but the pointer had already moved past it, so the loop exited with *str_write == '\0' and the walk reported that every character was placed. confirm_body_fits() is that walk in measure mode, so a body overflowing by exactly one glyph measured as fitting -- the precise failure the #428 redesign exists to prevent, reintroduced by the implementation of it. Mine, in 7e07b2d. The window is one glyph wide, which is why the 3,510-body sweep run when that landed did not catch it: every input in that corpus overflowed by more than one character, or not at all. The regression test pins the boundary directly -- 117 digits fill three rows, the 118th cannot be placed -- and is negative controlled: against the pre-fix draw.c, BodyFits.ConfirmBodyFits FAILS. Cherry-picked from #480, which found it. Deliberately WITHOUT that PR's scrolling consent redesign: this is an independent correctness fix, four lines, no behaviour change beyond refusing to over-report completeness. #480 replaces the Cut Off screen with a hold-to-scroll state machine, which is a change to the security-relevant confirmation flow and belongs on alpha with its own CI, adversarial review and OLED round -- not in a security point release. Verified: ARM cross-compile in the pinned builder image; full xunit green; cppcheck clean under CI's exact invocation (0 findings); clang-format clean. Refs #428 #480
…nal-glyph fix(board): do not consume a glyph the renderer refused to draw
Dress-rehearsal pin moves 81e581f -> c75fbd5, the head of keepkey/python-keepkey #214, which is now the single open PR to master for this release. #215 was folded into #214 and closed. SOP is one release PR whose head is the pin; splitting the chain_id work into a second PR gave this release two candidate pins, which was my error. Nothing was lost -- the EIP-155 oracle, the regenerated golden vectors, the two refusal tests and the client fix all moved across unchanged. The branch was also 36 commits behind master, so it lacked 027146f ("test(0x): enable AdvancedMode for transformERC20 blind-sign") among others. Merged master up; no conflicts. Measured against this firmware on a clean emulator: before 22 failed, 362 passed, 47 skipped after 3 failed, 394 passed, 70 skipped The nineteen that cleared were the chain_id five, the fourteen policy-gate opt-ins, and transformERC20. The three that remain share one cause -- the device now shows more screens than the pinned test acknowledges: test_eos_signtx_updateauth paginated payload test_thorchain_sign_tx "Additional memo field" screens test_sign_with_thorchain_memo (Ripple) same memo disclosure, newly visible None is a firmware defect. Fixing them means teaching the tests the new button flow, never removing disclosure: those screens show affiliate-fee bytes that were previously signed invisibly.
One required hold, one ButtonRequest. The Cut Off path broke that invariant. When a confirmation body does not fit, confirm_helper() shows a warning screen and then the body -- two physical holds -- while writing only the ButtonRequest the caller had already sent. The old comment stated the reasoning and, read back, states the bug: "the wire dialogue is unchanged; only the number of holds is not." So a host could satisfy every ButtonRequest it was told about and still wait forever for a response that needed a hold it never heard about. A person holding the button twice never notices. Any automated or auto-approving host deadlocks, which is what python-integration-tests has done on every run since this branch introduced the Cut Off screen: the device sent four ButtonRequests, the client acked all four, and the device sat on a fifth screen it had never announced. The fix writes a ButtonRequest for the body screen once the warning is answered, and clears button_request_acked first. Clearing it is the load-bearing half: without it confirm_screen() accepts a press that arrived for the previous request, which is how two holds collapsed into one announcement. Proven with the tests UNCHANGED. No acknowledgement was added anywhere, because there was nothing to acknowledge -- the screen did not exist on the wire. Same pinned suite, same test code, only the firmware differs: before 3 failed, 394 passed, 70 skipped in 139.51s after 1 failed, 396 passed, 70 skipped in 14.47s test_eos_signtx_updateauth and test_thorchain_sign_tx both pass. The 10x wall-clock drop is the two 60-second deadlock timeouts disappearing. This is a protocol-correctness fix, NOT the overflowing-confirmation UX redesign in #480. That PR replaces the Cut Off state with hold-to-scroll and belongs on alpha with its own CI and hardware round. This patch changes no screen and no flow: the device already demanded the second hold, and the wire simply never said so. The one remaining failure is unrelated and no longer hidden behind this bug: test_sign_with_thorchain_memo asserts an XRP memo field this firmware's protocol does not have. Filed separately. Refs #428 #466
Advance the dress-rehearsal pin from c75fbd5 to 6c4ad17, the current head of keepkey/python-keepkey#214. The additional commit re-gates the XRP THORChain memo test without weakening its assertion. The host-side memo transport gap is tracked separately as keepkey/keepkey-vault#422; firmware never receives the memo. With #482 plus this pin, the expected integration result is zero failures, with XRP memo support explicitly deferred rather than normalized as missing.
…wn-hold fix(confirm): announce the Cut Off screen's hold on the wire
chore(deps): repin python-keepkey dress rehearsal to #214 head
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 efa18ad and b53b63b both assert and build their risk analysis on.
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.
Closes #485. The Cut Off screen told the user: This text is too long for the screen. Only part of it is shown. Hold to view it anyway. Holding showed nothing new. confirm_sm.c re-drew the SAME truncated body -- request_body unchanged -- so the remainder stayed unreachable and the second hold bought the user precisely nothing. Confirmed on hardware during the rc30 round: 118, 119 and 255-character bodies all redraw identically after the warning. A consent screen that says it is about to disclose something and then does not is worse than no warning at all: a user who reads it carefully is misled about what they have seen. So actually page it. RENDER overflow -- the body reached the renderer intact, so every character is still in hand. Split it across as many screens as it needs, titled "n/m". No warning screen: nothing is being hidden any more. SOURCE truncation -- characters were lost in vsnprintf() before the renderer ever saw them. They cannot be paged because they no longer exist. Keep a warning, but stop lying in it: "The rest cannot be displayed." Paging is a click, approving is a hold. Intermediate pages pass immediate=true, which sets the confirm timer to 1ms (confirm_sm.c swap_layout), so a short press advances them; only the LAST page takes the caller's real hold. Reading what you are being shown should not cost the same effort as consenting to it. Preserves the #482 invariant -- one required press, one ButtonRequest. Every page after the first writes its own request and clears button_request_acked, so a host that answers every request it is told about never waits on a press it never heard of. Page boundaries come from page_take(), a binary search over confirm_body_fits(), which is draw_string()'s own loop with the pixel writes switched off. Measuring and drawing cannot disagree because they are the same code -- the property 7e07b2d established and the reason there is no second model of the screen to get out of sync. Only layout_standard_notification is measured, as before. Custom layouts and layout_constant_power_notification place their own bodies and are untouched. BEHAVIOUR CHANGE for tests: a body over the fit boundary (118 characters for a plain body, measured on hardware) now produces numbered pages rather than a "Cut Off" screen followed by the same clipped text. The ButtonRequest count is unchanged for a two-page body; the titles and the press durations are not.
ci: fail when a test declares screens it never captures
feat(confirm): page long bodies instead of promising to and not
Supersedes the 249f2d6 table, which predates the round entirely. Gates 1 and 4 are now met on evidence: T1 proved the #429 ceremony rolls back on hardware, and twelve cards ran on 7.14.2 silicon. Gate 2 is met for the paths measured, with the gap named rather than rounded off -- custom-layout address/xpub bodies still clip unmeasured, because confirm_helper() only measures layout_standard_notification. Records the two things the round actually established: AdvancedMode PERSISTS across a power cycle. efa18ad and b53b63b both assert the opposite in their commit messages and build their risk analysis on it. Release-note correction. #485, the Cut Off screen that promised disclosure and redrew the same clipped body -- found by reading confirm_sm.c, not by the device. And states plainly what did not pay off: most of the wire-level checks duplicated the emulator suite, and one finding (#484/#486) was retracted after resting on an inferred button press. Written down so the next round is aimed at the screens and the press durations, which are the parts hardware alone can establish. Also records why CI was measuring the wrong release: FW_VERSION fell back to 7.14.0 (#467), so every test gated to 7.14.1+ was excluded from the screenshot filter and from report validation for the entire release.
Closes #476. passphrase_protect() held the BIP-39 passphrase in a stack local that was never zeroed, on every path -- success, host cancel, and the early-out where passphrase protection is off. The passphrase selects the wallet: it is the difference between the visible wallet and a hidden one. Nothing else in the firmware treats key material that way. confirm_sm.c uses 'static CONFIDENTIAL char strbuf[]'; the session cache is 'static SessionState CONFIDENTIAL session' and is zeroed in session_clear_impl. This one buffer opted out of both. Not a regression: unchanged from the shipped base 1af2ffe. The only delta in this file for 7.14.2 is #475's verdict propagation. CONFIDENTIAL is not a documentation marker. On device builds CMakeLists defines it as __attribute__((section("confidential"))), a NOLOAD region that the bootloader wipes at boot (tools/bootloader/main.c:83, keepkey.ld:23); on emulator builds it is empty. A section attribute cannot apply to an automatic, so the buffer has to leave the stack to get that protection -- hence static. passphrase_protect() is not reentrant: fsm handlers call it on the single main loop and it blocks in passphrase_request() until the exchange completes. The section is only cleared at boot, so the buffer is also zeroed on entry and exit. Reboot-scope protection is not call-scope protection, and the residue between two calls is the part reachable without a power cycle. Bounded severity, and worth stating: the passphrase arrives FROM the host in plaintext by design -- the device displays it so the user can confirm which wallet they are opening. A compromised host already has it. This closes a device-local residue, not a disclosure path.
docs(release): gate status after the rc30 hardware round
fix(passphrase): keep the plaintext off the stack, and zero it
…er ends a memo Two signed-content display defects from the rc31 audit of 75f742b. Both are the shape this release exists to close: bytes covered by the signature that the screen does not show. P1 -- Solana could display a nonzero transfer as zero. solana_formatTokenAmount() truncated the fraction to nine decimals by dividing it down and printing the result. With amount=1 and decimals=18 that division yields 0, so the screen read "0.000000000 tokens" while the signed instruction transferred one base unit. A screen that says zero for a nonzero transfer is worse than one that says nothing: it does not merely omit, it asserts. Truncate only when every digit being dropped is zero. If any is nonzero the decimal form cannot be shown honestly at this width, so print the exact base-unit count and the decimals instead -- that number is the one actually present in the instruction being signed. Tests at 9, 10 and 18 decimals, including the cases where the dropped digits ARE zero and the decimal form is therefore still exact. P2 -- embedded NUL bytes ended THORChain memo disclosure early. thorchain_parseConfirmMemo() copies an explicit byte count and then calls strtok, which stops at the first NUL. A memo such as "=:ETH.ETH:<dest>:0\0:affiliate:75" is signed in full -- the EVM caller passes the true ABI length -- but parsing and confirmation stopped at the zero byte and the affiliate suffix was never displayed. There is no honest way to parse a field that lies about where it ends, so the parser now fails closed on any embedded NUL and leaves disclosure to the caller's UNPARSED path. That path needed fixing too, and this is the part that would have moved the defect one screen later rather than removing it: the BTC OP_RETURN caller already used confirm_data() with an explicit length, but BOTH THORChain callers used confirm(..., "%s", memo) -- and "%s" stops at the same NUL. They now use confirm_bytes(), which takes a length and escapes non-printables, so the NUL is visible as \x00. Both callers also passed sizeof(memo) rather than strnlen(memo, sizeof(memo)), handing the parser every trailing zero byte of a fixed array as if it were memo content. Fixed at both sites. fsm.c gains an explicit <string.h>: strnlen previously arrived only by way of whichever fsm_msg_*.h textual include happened to pull it in first.
…he memo
The first version of this check rejected ANY embedded NUL within the declared
length. CI caught that as a regression: test_sign_eth_add_liquidity on both
THORChain and Maya failed, because their ABI length word is 59 for a 58-character
memo and the 59th byte is a single trailing zero.
That distinction is the whole defect:
content after the NUL -> the memo lies about where it ends. There is no
honest way to parse it, so fail closed and let the
caller disclose the raw bytes.
only NULs after it -> the length word merely overstates by a byte or two,
which is what ABI padding looks like. Nothing is
hidden; the memo ends there and parses normally.
Rejecting both would have broken every legitimate ADD/deposit memo whose length
word runs past the string -- trading a disclosure defect for a functional one,
on a path this release is supposed to be making safer.
The unit test now covers both directions: an interior NUL with a suffix is
UNPARSED, and the real add-liquidity fixture's trailing pad still parses.
fix(display): a nonzero transfer never renders as zero, and a NUL never ends a memo
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Opens the 7.14.2 security line:
release/7.14.2→develop.80 commits (64 non-merge) on top of
1af2ffe7de(= v7.14.1). Merging thisauto-closes ~20 issues whose commits carry
Closes #NNN— they are still openonly because the work landed on a release branch, not the default one.
What this release is
Every defect it closes is one shape: the device hashed bytes it never rendered,
or rendered text it could not vouch for.
chain_idEvidence
Hardware round on 7.14.2 silicon — twelve cards,
docs/release/rc30-evidence/:RecoveryDevicerefused, theEntropyAckthat used to be swallowed by the still-armed reset returnsNot in Reset mode, device ends uninitialised with no residue from theattacker's label.
re-asks (8/8).
before/after read; a cancelled Uniswap approve returns no signature.
:ss:75is in the signed bytes and on screen.chain_idrefused as the first reply with no ButtonRequest —absence of a screen proven on the wire, not from a photograph.
structured disable is hardcoded, so AdvancedMode does not re-open it.
tokens resolve — same calldata, one word changed, one signs and one refuses.
(15 s silence), a hold does (
Successat 1.650 s).CI now captures the screens (#487). It did not before:
FW_VERSIONfell backto 7.14.0 (#467) so every test gated to 7.14.1+ was excluded from the screenshot
filter and report validation. 345 → 375 captures, and a per-test audit fails the
build when a test declares screens it does not produce.
Read these two before approving
AdvancedMode persists across a power cycle.
efa18ad77andb53b63b0abothstate it is session state and build their risk analysis on that. It is not:
blind-signing stays enabled until explicitly revoked. Release-note correction,
not a code change.
Gate 2 is met only for the paths measured.
confirm_helper()measures onlylayout_standard_notification, so custom-layout address/xpub bodies still clipwith no warning. Observed at T12 A4. Named rather than rounded off.
Not clean, and said plainly
device confirmed with no press"; the tester was pressing. The refuting control
took one minute and belonged before the issue was filed.
suite. The round's real yield was the AdvancedMode finding and "Hold to view it anyway" does not show the hidden text #485. Recorded
in
docs/release/7.14.2.mdso the next round is aimed at screens and pressdurations, which are what hardware alone establishes.
favour of the click-based model; the parts worth keeping are filed as Route confirm_bytes() through the generic pager, and port #480's board tests #489.
Still open, deliberately
Gate 3 (independent re-exploitation) and gate 6 (final adversarial round) are
not started and are not self-certifiable. Deferred residuals: #414, #455,
#473, #489.