[rehearsal 1a] EVM clear-signing core (signed-metadata, Insight, EIP-1559/RLP/EIP-712, drainer hardening)#294
Open
BitHighlander wants to merge 25 commits into
Open
[rehearsal 1a] EVM clear-signing core (signed-metadata, Insight, EIP-1559/RLP/EIP-712, drainer hardening)#294BitHighlander wants to merge 25 commits into
BitHighlander wants to merge 25 commits into
Conversation
…ream branches Rehearsal foundation off upstream/develop. Practice-pins: - deps/device-protocol -> keepkey up/release-protocol @ 33521a8 (clearsign 115/116/117 + icon, hive/zcash/ripple/thor proto, bip85 120/121) - deps/python-keepkey -> keepkey reconcile/upstream-sync @ 1674346 (v2 static-schema harness + all 7.15 tests) trezor-firmware stays at upstream/develop's pin; PR2 bumps it to 0ea97b09 (Orchard), PR4 to 56f404e4. Swap these to the real keepkey master SHAs for the actual upstream firmware PR.
chain_id was uint8_t, silently truncating chain IDs above 255. Base (8453), Arbitrum (42161), Avalanche (43114) and others all overflow a byte, causing tokenByChainAddress to match the wrong tokens or return UnknownToken for valid ERC-20 transfers. Widen chain_id to uint32_t in TokenType struct and both lookup functions (tokenByChainAddress, tokenByTicker). (cherry picked from commit 6bc3bb3)
Bug 1 (cancel ignored): confirmName() and confirmValue() cast review() return value to void, allowing signing to continue after user presses cancel. Added USER_CANCELLED error code and propagate cancellation up through the call chain. Bug 2 (strtoll overflow/garbage): strtoll() was called with NULL endptr, silently accepting trailing garbage and wrapping on overflow. Added endptr validation to reject non-numeric input, and reject negative values for uint types. (cherry picked from commit 0646335)
…ocol EVM txs Adds MAYA_ROUTER constant and thor_isMayachainTx() / thor_confirmMayaTx() functions. Maya transactions (targeting the Maya ETH router) now show "Maya data" / "Maya router" on screen instead of "Thorchain data". The depositWithExpiry() ABI is shared between THORChain and Maya; the differentiator is the router contract address. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> (cherry picked from commit 74370c6)
(cherry picked from commit cd4f700)
…error=discarded-qualifiers on ARM) (cherry picked from commit a8b625c)
…bleSanity Pre-existing (NOT 7.x-release related): QTUM/BNB/ZIL/GTO/IOST/CMT/MCO/ODEM are display-only coins[] leftovers whose ERC20 entries were dropped from the generated token table long ago. coins.cpp + the generator (python-keepkey) are byte-identical to develop base 1af2ffe, so this failure predates the RC. Named allowlist keeps the sanity check live for any NEW missing token. Verified: make xunit -> 62 passed, 0 failed, 4 disabled. (cherry picked from commit 1838c56)
…zeros) develop's ethereum.c hashes tx integer fields (nonce, gas, value, EIP-1559 fees) with hash_rlp_field, which does NOT strip leading zeros. A zero/absent EIP-1559 priority fee arrives as a single 0x00 byte and hashes as a literal 0x00 instead of the canonical empty string (0x80). The signature is then over a non-canonical pre-image no standard tool reproduces, so the tx recovers to a random address and is unbroadcastable. Root-caused on-device (fw 7.15.0 RC = this develop line): a zero-priority EIP-1559 tx recovered only to the non-canonical prio=0x00 pre-image, not the device address (keepkey-sdk tests/evm-firmware/eip1559-recover.js). Fix: port hash_rlp_bytes_stripped (already on alpha) and use it for the integer fields (nonce/priority/maxFee/gasPrice/gasLimit/value); addresses (to) keep hash_rlp_field. All-zero -> empty (0x80), matching the length header + canonical encoding. Companion alpha PR drops the has_priority guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit d9967ea)
… bytes ethereum_signing_init builds the signing pre-image in two passes: Stage 1 sums the RLP list length (hash_rlp_list_length) and Stage 2 hashes each field. Commit 4d1299d switched Stage 2 to hash_rlp_bytes_stripped for the integer fields (nonce, gas_price, gas_limit, value, max_fee_per_gas, max_priority_fee_per_gas) but left Stage 1 counting raw .size. When any integer field carries a leading zero byte, Stage 1 over-declares the list-length header while Stage 2 hashes the stripped (shorter) bytes. The keccak pre-image is then malformed, so the produced signature recovers to a garbage address -- the classic "random wrong signer" / dropped-tx symptom. It is data-dependent on the field bytes (a high zero byte in the nonce, gas, value or fee), so it surfaces intermittently across wallets and chains and is unrelated to tx size or passphrase wallets. Add rlp_calculate_length_stripped(), a mirror of hash_rlp_bytes_stripped(), and use it for the six integer fields so the declared length always equals the bytes actually hashed. The `to` (address) and `data` fields keep raw counting, matching their unstripped hashing. (cherry picked from commit e68b685) (cherry picked from commit 5648e53)
ethereum_signing_check accepts a message with has_max_fee_per_gas set but has_max_priority_fee_per_gas unset. In that shape Stage 1 still counts the priority-fee field in the RLP list length (unconditionally, under has_max_fee_per_gas), but Stage 2 skipped hashing it behind an inner has_max_priority_fee_per_gas guard -- so the list header over-declared by one byte and the signature recovered to a wrong address, the same class this branch fixes. max_priority_fee_per_gas is a mandatory EIP-1559 field; when absent it must be encoded as the empty integer (0x80). Drop the inner guard so Stage 2 always hashes it (.size is 0 when unset, which hash_rlp_bytes_stripped emits as 0x80), matching Stage 1. (cherry picked from commit 15397ee) (cherry picked from commit 46ecd08)
The typed prefix (0x02) and access list are emitted from ethereum_tx_type (msg->type), but the fee-field shape and the on-screen fee are chosen from has_max_fee_per_gas. ethereum_signing_check accepted shapes where these disagree, so the device would sign a malformed pre-image (Stage 1 list length vs Stage 2 hashed bytes describe different field lists) -> signature recovers to a wrong address. Three reachable cases: - EIP-1559 (type 2) with absent chain_id: chain_id defaults to 0; Stage 1 counts rlp_calculate_number_length(0)=1 byte but Stage 2 hash_rlp_number(0) hashes nothing. Reject type-2 with chain_id==0. - EIP-1559 with no max_fee_per_gas, or legacy carrying max_fee_per_gas: the emitted typed prefix no longer matches the fee field list. Require type 2 => has_max_fee_per_gas and legacy => !has_max_fee_per_gas. - Fee overflow guard checked only gas_price.size + gas_limit.size; for type-2 gas_price.size is 0 while max_fee_per_gas.size can be 32, so the on-screen fee (fee_per_gas * gas_limit via modular bn_multiply) could wrap and display a wrong value. Bound the fee field actually used for the tx type. Verified against the host (hdwallet-keepkey/src/ethereum.ts): it always pairs setType(EIP_1559) with maxFeePerGas and uses gasPrice for legacy, so these guards reject only malformed shapes, not anything the host sends. (cherry picked from commit 5836ff4) (cherry picked from commit 4cce2fb)
…#255-gate/#260) Port the contract clear-sign handler hardening from alpha to develop: - Gate every contract clear-sign handler on complete calldata + a real call (has_to, full data present) so truncated/partial calldata can't clear-sign (alpha 41b8016 + d550b18, part of fork PR #255 review rounds). - sellToUniswap: validate the dynamic-array offset words so a crafted offset can't display one swap and execute another (alpha 3df0792, HIGH, PR #260). - Uniswap liquidity ops: require recipient == signer, fail closed (alpha 1ff989e, PR #260). - transformERC20: clear-sign at any size — relaxes an over-broad gate so big legit swaps don't fall back to blind-sign (alpha 321328b, PR #260). Files are taken at the alpha tip state (1f147b5), which already reconciles upstream develop; none of them reference Insight/signed-metadata. (cherry picked from commit 8700084)
…port alpha #261, CRITICAL) thor_isThorchainTx matched the deposit selector on ANY to-address, giving any attacker contract the THORChain clear-sign UX and an AdvancedMode blind-sign- gate bypass. Pin the router to the current v4 address (d37bbe57..) and validate the memo's ABI head-pointer offset so the displayed memo is the memo the router actually decodes (alpha 61250a2, fork PR #261). NOTE: THORChain migrates routers periodically; the durable fix is the signed- metadata (Insight) key-pinned approach staged separately. (cherry picked from commit 80a088e)
…n gate (stage #257/#258 → develop) Squash-stage of the Insight EVM clear-signing feature from alpha (fork PRs #257 merge 1f421a8 + #258 CI-greening a3cdca9; commits 8b244b4, ba02b5a, f9fc53e, 53d04ee) onto the eth-hardening stack. - signed_metadata.c/.h: Vault-signed metadata (EthereumTxMetadata msg 115/116, already in the pinned device-protocol) lets the device display verified who/what for contracts firmware can't decode natively. Display gating via contract+selector+chainId match; SIGNATURE binding via deferred full-tx-hash check in send_signature() — metadata.tx_hash must equal the real signed digest, fail closed, before ecdsa_sign_digest (no RLP re-derivation). - AdvancedMode becomes the single blind-sign switch: OFF = sign only natively decoded or Insight-verified transactions, else hard-reject ("Blind signing disabled by policy"); ON = expert mode. Intentional non-backwards-compatible behavior change (was a dismissable warning). - confirm_with_icon + VERIFIED_ICON display plumbing; storage_getRawSeed. - Host-buildable unit suite (signed_metadata.cpp, 44 cases) wired into unittests; enforce logic factored pure for testability. Files taken at alpha-tip state (1f147b5, post review + clang-format-20 + cppcheck rounds); dispatch-table and CMake wiring re-done in develop's context; zero zcash/hive contamination (verified). Emulator-verified on alpha: 28 python-keepkey clear-signing integration tests incl. replay-reject and gate/cancel paths (tests ship in the pinned python-keepkey 452ca986). (cherry picked from commit 4f387d5)
… (phase 1) Phase 1 of Insight clear-signing ships with NO hardcoded "KeepKey says this is safe" key. All METADATA_PUBKEYS slots are zeroed (including the DEBUG_LINK CI slot); every clearsign signer must be loaded at runtime: - New LoadClearsignSigner message (device-protocol 117): host supplies a 33-byte compressed secp256k1 pubkey + printable-ASCII alias for a key slot. Mandatory on-device confirm shows the alias + sha256-prefix key fingerprint; rejecting cancels the load. RAM-only, cleared on reboot. - Every transaction whose metadata was verified by a loaded signer shows a warning screen BEFORE any clearsign page: signer alias + fingerprint, "NOT verified by KeepKey". The method screen for loaded signers drops the "Insight Verified" icon branding — that presentation is reserved for the phase-2 built-in production key, which a loaded signer can never shadow. - tx-hash binding enforcement (signed_metadata_enforce) is unchanged. Unit tests: fixture now loads the CI test key through the runtime path; new coverage for signer validation (prefix/OOB guard, alias sanitization, slot range), replacement invalidation, clear_signers, fingerprint format. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit aee9c95)
Factory reset must not leave runtime trust anchors behind: WipeDevice now calls signed_metadata_clear_signers() (which also drops any metadata the wiped signers verified). Also restores per-test isolation in CI, where the emulator process survives the per-test wipe_device(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 62ec4ab)
…screenshots
- signed_metadata_signer_valid: restrict the clearsign signer alias to
[A-Za-z0-9 _-]. The alias renders inside quotes on the load-confirm and
per-tx warning screens; the old printable-ASCII whitelist let a host inject
a false trust claim (e.g. alias "x' verified by KeepKey. Safe (") that reads
as legitimate text on the exact trust-decision screen. The immutable
"NOT verified by KeepKey" suffix capped the impact, but honest presentation
is the whole point of phase 1. (Found by adversarial security review.)
- scripts/emulator/python-keepkey-tests.sh: FW_VERSION detection used grep -oP,
but the python-keepkey container is Alpine/busybox (no PCRE) — grep errored
and the version silently fell back to 7.14.0, excluding every 7.15.0 section
(Hive, EVM clear-signing) from screenshot capture. Use grep -oE.
Also bumps the python-keepkey pin to the matching test updates.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(cherry picked from commit bf7582a)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 0737cd0)
Clear-signing's job is to show the user WHO/WHAT/WHY, but AMOUNT args rendered
as raw wei and there was no protocol name — the "what" was still unreadable.
Add two attested, fail-closed-validated arg formats:
- ARG_FORMAT_STRING (4): a printable label (e.g. protocol "Aave V3"), sanitized
at parse (printable ASCII, no '%').
- ARG_FORMAT_TOKEN_AMOUNT (5): decimals(1) + symbol_len(1) + symbol(<=10
[A-Za-z0-9]) + amount(1..32 BE). Rendered decimal-scaled with the ticker via
bn_format ("10.5 DAI"); all-0xFF -> "UNLIMITED <symbol>". Layout is validated
in a new arg_value_ok() before storage. METADATA_MAX_ARG_VALUE_LEN grows
32 -> 44 to fit it; legacy formats keep their 32-byte cap.
On device the flagship happy-path now renders the full ordered review of a
real Aave V3 supply(): warning -> Call: supply -> Contract -> protocol: Aave V3
-> asset: DAI -> amount: 10.5 DAI -> onBehalfOf -> tx confirm.
59 signed_metadata unit tests pass (adds STRING + TOKEN_AMOUNT accept/reject
coverage incl. the 44-byte cap not leaking to legacy formats). Bumps the
python-keepkey pin to the matching serializer + report changes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(cherry picked from commit ed03369)
Adds METADATA_VERSION_SCHEMA (0x02): a clear-sign metadata format that carries only the static decode schema (chainId, contract, selector, method, per-arg name + display format [+ static decimals/symbol]) with NO tx_hash and NO argument values. The device decodes the argument values from the exact calldata it is about to sign, so the displayed decode is bound to the signature structurally — no committed digest, no per-transaction signing, no online hot key. The catalog is signed once, offline, and can be served from a host CDN. v1 (per-tx, host-decoded values, tx_hash-bound) is unchanged and still selected by the version byte; v2 is added alongside. Scope: fixed single-word ABI head types (ADDRESS, AMOUNT, TOKEN_AMOUNT) at offset 4 + 32*i — covers approve/transfer/transferFrom and fixed-arg calls. Binding requires the entire calldata to be exactly selector + num_args*32 bytes, wholly present in data_initial_chunk (no hidden/streamed trailing data), else the tx falls to the normal blind-sign path. Dynamic types are out of scope (no v2 schema -> blind sign). - parse_v2_args / decode_v2_args, refactor parse into shared head/trailer + v1/v2 - signed_metadata_matches_tx: v2 branch decodes from the tx calldata - signed_metadata_enforce_schema_decision: structural (no tx_hash) enforce - 12 new unit tests (decode, rejection cases, enforce truth table); 71/71 pass Format: docs at docs/CLEARSIGN-V2-STATIC-SCHEMA.md (keepkey-clearsign). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit b8cbb97)
…nt decode Two review findings on the v2 static-schema path: 1. v2 enforce relied on an implicit caller-ordering invariant (relied_on_metadata is only set after matches_tx()/decode). v2 has no tx_hash fallback, so make it explicit: add metadata_schema_decoded, set only on a successful decode_v2_args() in matches_tx(), and REQUIRE it in signed_metadata_enforce_schema_decision(). A v2 signature can now never be emitted unless the args were provably decoded from this tx's calldata — not merely inferred from call order. 2. decode_v2_args() was not idempotent for TOKEN_AMOUNT: it derived the append offset from value_len, which already included a prior call's 32-byte amount, so a second matches_tx() overflowed the prefix check. Derive the prefix from symlen (value[1]) instead — stable across repeated calls. Adds an idempotency test and a relied-but-not-decoded enforce case (73/73 pass). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 7629932)
…oof) Review finding: metadata_schema_decoded could stay true after a LATER signed_metadata_matches_tx() that failed an early binding (unavailable metadata, wrong contract/selector/chain) before reaching the v2 decode branch — leaving a stale 'decoded' proof from a prior successful match. Since v2 enforce has no tx_hash fallback, that undermined the very independence the flag was added for. Fix: clear metadata_schema_decoded at the TOP of signed_metadata_matches_tx(), before any early return, so it reflects only the current call. Adds a read accessor (signed_metadata_schema_decoded, exported for tests) and a regression test that decodes a v2 blob, then feeds a mismatching tx and asserts the flag is cleared. 74/74 unit tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit ecffaa0)
The firmware compiles nanopb from its OWN include/keepkey/transport/*.options, separate from device-protocol's. Without a max_size the icon bytes field generates a pb_callback_t (forbidden in firmware) and every build fails at lib/transport/kktransport.pb. Mirror the device-protocol max_size:384. (cherry picked from commit ce02949)
…rset proto codegen) The PR0 device-protocol pin (up/release-protocol) is a 7.15 superset that adds ripple Memos + thorchain any-denom fields to protos the firmware codegens wholesale. Their firmware-side nanopb max_size entries are required for the build to succeed even though the ripple-memo / thor-denom *handlers* land in PR2. Pure codegen metadata.
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.
Dress-rehearsal PR 1/5 — reconstitutes the 7.15 EVM clear-signing foundation as one coherent, reviewable diff onto a fresh branch off upstream/develop. 23 cherry-picked commits + PR0 base pins.
Contents
signed_metadata.{h,c}+ 75-test suite; Insight clear-signing; v1 + v2 static-schema on-device calldata decode; runtime-loaded signers (phase-1 warning); LoadClearsignSigner (icon/pubkey/alias).up/release-protocol@33521a8, python-keepkeyreconcile/upstream-sync@1674346(practice tips; swap to master SHAs for the real upstream PR). + ripple/thorchain nanopbmax_sizecodegen metadata for the superset proto.Verified
Local Docker
kkemu+firmware-unitbuild green; 75/75 SignedMetadata/Ethereum unit tests pass.Stack: 1a → 2 (chains) → 3 (robustness) → 1b (per-chain clearsign) → 4 (build variants + CI). Cumulative tip == origin/develop.