From 978ac19f792bb4236cb237d6b92027f053bcc665 Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 7 Jul 2026 19:20:19 -0300 Subject: [PATCH 01/25] PR0 base: pin device-protocol + python-keepkey to canonical 7.15 upstream 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. --- .gitmodules | 4 ++-- deps/device-protocol | 2 +- deps/python-keepkey | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.gitmodules b/.gitmodules index 2d6c4446a..dcc3c114d 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,7 +1,7 @@ [submodule "deps/device-protocol"] path = deps/device-protocol url = https://github.com/keepkey/device-protocol.git -branch = master +branch = up/release-protocol [submodule "deps/trezor-firmware"] path = deps/crypto/trezor-firmware url = https://github.com/keepkey/trezor-firmware.git @@ -14,7 +14,7 @@ url = https://github.com/keepkey/code-signing-keys.git [submodule "deps/python-keepkey"] path = deps/python-keepkey url = https://github.com/keepkey/python-keepkey.git -branch = master +branch = reconcile/upstream-sync [submodule "deps/qrenc/QR-Code-generator"] path = deps/qrenc/QR-Code-generator url = https://github.com/keepkey/QR-Code-generator.git diff --git a/deps/device-protocol b/deps/device-protocol index d637b7829..33521a8fd 160000 --- a/deps/device-protocol +++ b/deps/device-protocol @@ -1 +1 @@ -Subproject commit d637b78291a423fd8119df9935a9365be8a7758e +Subproject commit 33521a8fd6f012c8bbca3a8902eea6c1f5aa3389 diff --git a/deps/python-keepkey b/deps/python-keepkey index fabd6c618..1674346be 160000 --- a/deps/python-keepkey +++ b/deps/python-keepkey @@ -1 +1 @@ -Subproject commit fabd6c6189b7f1b3ea7cbd1d372fc13729761178 +Subproject commit 1674346be2b1042b34d2533989a29c92ced93a2b From 212b99bfec0a09a48be80cc7f20b0d125b26de2f Mon Sep 17 00:00:00 2001 From: highlander Date: Thu, 21 May 2026 01:00:38 -0300 Subject: [PATCH 02/25] fix: widen TokenType.chain_id from uint8_t to uint32_t 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 6bc3bb3a432d81541e425a92ddd8d3a31d3a6d5a) --- include/keepkey/firmware/ethereum_tokens.h | 6 +++--- lib/firmware/ethereum_tokens.c | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/include/keepkey/firmware/ethereum_tokens.h b/include/keepkey/firmware/ethereum_tokens.h index 7e26bd164..253c9df01 100644 --- a/include/keepkey/firmware/ethereum_tokens.h +++ b/include/keepkey/firmware/ethereum_tokens.h @@ -39,7 +39,7 @@ enum { typedef struct _TokenType { const char* const address; const char* const ticker; - uint8_t chain_id; + uint32_t chain_id; uint8_t decimals; } TokenType; @@ -51,7 +51,7 @@ extern const TokenType* UnknownToken; const TokenType* tokenIter(int32_t* ctr); -const TokenType* tokenByChainAddress(uint8_t chain_id, const uint8_t* address); +const TokenType* tokenByChainAddress(uint32_t chain_id, const uint8_t* address); /// Tokens don't have unique tickers, so this might not return the one you're /// looking for :/ @@ -64,7 +64,7 @@ const TokenType* tokenByChainAddress(uint8_t chain_id, const uint8_t* address); /// \param[out] token The found token, assuming it was uniquely determinable. /// \returns true iff the token can be uniquely found in the list of known /// tokens. -bool tokenByTicker(uint8_t chain_id, const char* ticker, +bool tokenByTicker(uint32_t chain_id, const char* ticker, const TokenType** token); void coinFromToken(CoinType* coin, const TokenType* token); diff --git a/lib/firmware/ethereum_tokens.c b/lib/firmware/ethereum_tokens.c index 15e5f0678..dd7f57f60 100644 --- a/lib/firmware/ethereum_tokens.c +++ b/lib/firmware/ethereum_tokens.c @@ -42,7 +42,8 @@ const TokenType* tokenIter(int32_t* ctr) { return &(tokens[*ctr - 1]); } -const TokenType* tokenByChainAddress(uint8_t chain_id, const uint8_t* address) { +const TokenType* tokenByChainAddress(uint32_t chain_id, + const uint8_t* address) { if (!address) return 0; for (int i = 0; i < TOKENS_COUNT; i++) { if (chain_id == tokens[i].chain_id && @@ -57,7 +58,7 @@ const TokenType* tokenByChainAddress(uint8_t chain_id, const uint8_t* address) { return UnknownToken; } -bool tokenByTicker(uint8_t chain_id, const char* ticker, +bool tokenByTicker(uint32_t chain_id, const char* ticker, const TokenType** token) { *token = NULL; From 730f1c1d0bc909b01db98011524b6a41aed9dc6a Mon Sep 17 00:00:00 2001 From: highlander Date: Thu, 21 May 2026 00:58:41 -0300 Subject: [PATCH 03/25] fix: EIP-712 cancel propagation and strtoll input validation 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 0646335cdbb0e1e76eae796b6005444d71c88ed5) --- include/keepkey/firmware/eip712.h | 3 ++- lib/firmware/eip712.c | 23 +++++++++++++++++------ 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/include/keepkey/firmware/eip712.h b/include/keepkey/firmware/eip712.h index 686acd6de..9b5ba1a9a 100644 --- a/include/keepkey/firmware/eip712.h +++ b/include/keepkey/firmware/eip712.h @@ -95,8 +95,9 @@ typedef enum { DOMAIN = 1, MESSAGE } dm; #define JSON_TYPE_T_NOVAL 31 #define ADDR_STRING_NULL 32 #define JSON_TYPE_WNOVAL 33 +#define USER_CANCELLED 34 -#define LAST_ERROR JSON_TYPE_WNOVAL +#define LAST_ERROR USER_CANCELLED int encode(const json_t* jsonTypes, const json_t* jsonVals, const char* typeS, uint8_t* hashRet); diff --git a/lib/firmware/eip712.c b/lib/firmware/eip712.c index 1fec75f4b..dba3798d9 100644 --- a/lib/firmware/eip712.c +++ b/lib/firmware/eip712.c @@ -288,16 +288,20 @@ int encodeBytesN(const char* typeT, const char* string, uint8_t* encoded) { int confirmName(const char* name, bool valAvailable) { if (valAvailable) { nameForValue = name; - } else { - (void)review(ButtonRequestType_ButtonRequest_Other, "MESSAGE DATA", - "Press button to continue for\n\"%s\" values", name); + return SUCCESS; + } + if (!review(ButtonRequestType_ButtonRequest_Other, "MESSAGE DATA", + "Press button to continue for\n\"%s\" values", name)) { + return USER_CANCELLED; } return SUCCESS; } int confirmValue(const char* value) { - (void)review(ButtonRequestType_ButtonRequest_Other, "MESSAGE DATA", "%s %s", - nameForValue, value); + if (!review(ButtonRequestType_ButtonRequest_Other, "MESSAGE DATA", "%s %s", + nameForValue, value)) { + return USER_CANCELLED; + } return SUCCESS; } @@ -548,7 +552,14 @@ int parseVals(const json_t* eip712Types, const json_t* jType, } } // all int strings are assumed to be base 10 and fit into 64 bits - long long intVal = strtoll(valStr, NULL, 10); + char* endptr = NULL; + long long intVal = strtoll(valStr, &endptr, 10); + if (endptr == valStr || *endptr != '\0') { + return GENERAL_ERROR; + } + if (0 == strncmp("uint", typeType, 4) && intVal < 0) { + return GENERAL_ERROR; + } // Needs to be big endian, so add to encBytes appropriately encBytes[24] = (intVal >> 56) & 0xff; encBytes[25] = (intVal >> 48) & 0xff; From a618ba71c5020ca27286d790e09b752b66e0917e Mon Sep 17 00:00:00 2001 From: highlander Date: Wed, 25 Mar 2026 00:47:19 -0600 Subject: [PATCH 04/25] fix(maya): show 'Maya data' instead of 'Thorchain data' for Maya Protocol 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 (cherry picked from commit 74370c619e172aaf374d5aa48a63684e2f934c7f) --- .../firmware/ethereum_contracts/thortx.h | 9 ++- lib/firmware/ethereum_contracts.c | 3 + lib/firmware/ethereum_contracts/thortx.c | 68 ++++++++++++------- 3 files changed, 53 insertions(+), 27 deletions(-) diff --git a/include/keepkey/firmware/ethereum_contracts/thortx.h b/include/keepkey/firmware/ethereum_contracts/thortx.h index 8f61c27f3..ddd26ce42 100644 --- a/include/keepkey/firmware/ethereum_contracts/thortx.h +++ b/include/keepkey/firmware/ethereum_contracts/thortx.h @@ -30,12 +30,15 @@ "\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee" \ "\xee\xee" +/* THORChain ETH router (mainnet) */ #define THOR_ROUTER "42a5ed456650a09dc10ebc6361a7480fdd61f27b" +/* Maya Protocol ETH router (mainnet) */ +#define MAYA_ROUTER "d89dce570de35a6f42d3bca7dba50a6d89bfc2a2" + /* deposit(address,address,uint256,string) — legacy selector */ #define THOR_SELECTOR_DEPOSIT "\x1f\xec\xe7\xb4" -/* depositWithExpiry(address,address,uint256,string,uint256) — current selector - */ +/* depositWithExpiry(address,address,uint256,string,uint256) — current selector */ #define THOR_SELECTOR_DEPOSIT_WITH_EXPIRY "\x44\xbc\x93\x7b" typedef struct _EthereumSignTx EthereumSignTx; @@ -43,6 +46,8 @@ typedef struct _EthereumSignTx EthereumSignTx; bool thor_has_deposit_selector(const EthereumSignTx* msg); bool thor_is_expiry_variant(const EthereumSignTx* msg); bool thor_isThorchainTx(const EthereumSignTx* msg); +bool thor_isMayachainTx(const EthereumSignTx* msg); bool thor_confirmThorTx(uint32_t data_total, const EthereumSignTx* msg); +bool thor_confirmMayaTx(uint32_t data_total, const EthereumSignTx* msg); #endif diff --git a/lib/firmware/ethereum_contracts.c b/lib/firmware/ethereum_contracts.c index 87fb61488..76722cc24 100644 --- a/lib/firmware/ethereum_contracts.c +++ b/lib/firmware/ethereum_contracts.c @@ -38,6 +38,7 @@ bool ethereum_contractHandled(uint32_t data_total, const EthereumSignTx* msg, if (zx_isZxLiquidTx(msg)) return true; if (zx_isZxApproveLiquid(msg)) return true; + if (thor_isMayachainTx(msg)) return true; if (thor_isThorchainTx(msg)) return true; if (makerdao_isMakerDAO(data_total, msg)) return true; @@ -62,8 +63,10 @@ bool ethereum_contractConfirmed(uint32_t data_total, const EthereumSignTx* msg, if (zx_isZxApproveLiquid(msg)) return zx_confirmApproveLiquidity(data_total, msg); + if (thor_isMayachainTx(msg)) return thor_confirmMayaTx(data_total, msg); if (thor_isThorchainTx(msg)) return thor_confirmThorTx(data_total, msg); + if (makerdao_isMakerDAO(data_total, msg)) return makerdao_confirmMakerDAO(data_total, msg); diff --git a/lib/firmware/ethereum_contracts/thortx.c b/lib/firmware/ethereum_contracts/thortx.c index 6f48d1ab5..bb24d9d6c 100644 --- a/lib/firmware/ethereum_contracts/thortx.c +++ b/lib/firmware/ethereum_contracts/thortx.c @@ -29,8 +29,7 @@ bool thor_has_deposit_selector(const EthereumSignTx* msg) { if (msg->data_initial_chunk.size < 4) return false; - return (memcmp(msg->data_initial_chunk.bytes, THOR_SELECTOR_DEPOSIT, 4) == - 0 || + return (memcmp(msg->data_initial_chunk.bytes, THOR_SELECTOR_DEPOSIT, 4) == 0 || memcmp(msg->data_initial_chunk.bytes, THOR_SELECTOR_DEPOSIT_WITH_EXPIRY, 4) == 0); } @@ -41,6 +40,22 @@ bool thor_is_expiry_variant(const EthereumSignTx* msg) { THOR_SELECTOR_DEPOSIT_WITH_EXPIRY, 4) == 0; } +/* Format msg->to as lowercase hex string (40 chars + NUL) */ +static void thor_format_to_addr(const EthereumSignTx* msg, char out[41]) { + for (uint32_t i = 0; i < 20; i++) { + snprintf(&out[i * 2], 3, "%02x", msg->to.bytes[i]); + } + out[40] = '\0'; +} + +bool thor_isMayachainTx(const EthereumSignTx* msg) { + if (!msg->has_to || msg->to.size != 20) return false; + if (!thor_has_deposit_selector(msg)) return false; + char toStr[41]; + thor_format_to_addr(msg, toStr); + return strncmp(toStr, MAYA_ROUTER, 40) == 0; +} + bool thor_isThorchainTx(const EthereumSignTx* msg) { if (msg->has_to && msg->to.size == 20 && thor_has_deposit_selector(msg)) { return true; @@ -48,7 +63,10 @@ bool thor_isThorchainTx(const EthereumSignTx* msg) { return false; } -bool thor_confirmThorTx(uint32_t data_total, const EthereumSignTx* msg) { +static bool thor_confirm_deposit_tx(uint32_t data_total, + const EthereumSignTx* msg, + const char* protocol_label, + const char* router_label) { (void)data_total; /* Minimum calldata: selector(4) + vault(32) + asset(32) + amount(32) + @@ -74,32 +92,29 @@ bool thor_confirmThorTx(uint32_t data_total, const EthereumSignTx* msg) { thorchainData = (uint8_t*)(msg->data_initial_chunk.bytes + 4 + (is_expiry ? 6 : 5) * 32); - // Start confirmations - for (ctr = 0; ctr < 20; ctr++) { - snprintf(&confStr[ctr * 2], 3, "%02x", msg->to.bytes[ctr]); - } - if (strncmp(confStr, THOR_ROUTER, sizeof(THOR_ROUTER)) == 0) { + thor_format_to_addr(msg, confStr); + if (strncmp(confStr, THOR_ROUTER, 40) == 0) { conf = "Thorchain router"; + } else if (strncmp(confStr, MAYA_ROUTER, 40) == 0) { + conf = router_label; } else { conf = confStr; } - if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, "Thorchain data", + if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, protocol_label, "Routing through %s", conf)) { return false; } - // just display token address and amount as string for (ctr = 0; ctr < 20; ctr++) { snprintf(&confStr[ctr * 2], 3, "%02x", vaultAddress[ctr]); } - if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, "Thorchain data", + if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, protocol_label, "Using Asgard vault %s", confStr)) { return false; } if (memcmp(contractAssetAddress, ETH_ADDRESS, sizeof(ETH_ADDRESS)) == 0) { - assetAddress = (const uint8_t*) - ETH_NATIVE; // get eth native parameters if asset is not a token + assetAddress = (const uint8_t*)ETH_NATIVE; } else { assetAddress = contractAssetAddress; } @@ -107,30 +122,24 @@ bool thor_confirmThorTx(uint32_t data_total, const EthereumSignTx* msg) { assetToken = tokenByChainAddress(msg->chain_id, assetAddress); if (strncmp(assetToken->ticker, " UNKN", 5) == 0) { - // just display token address and amount as string for (ctr = 0; ctr < 20; ctr++) { snprintf(&confStr[ctr * 2], 3, "%02x", assetAddress[ctr]); } - if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, - "Thorchain data", "from asset %s", confStr)) { + if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, protocol_label, + "from asset %s", confStr)) { return false; } - // We don't know what the exponent should be so just confirm raw unformatted - // number bn_format(&Amount, NULL, " unformatted", 0, 0, false, confStr, sizeof(confStr)); - - if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, - "Thorchain data", "amount %s", confStr)) { + if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, protocol_label, + "amount %s", confStr)) { return false; } - } else { ethereumFormatAmount(&Amount, assetToken, msg->chain_id, confStr, sizeof(confStr)); - - if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, - "Thorchain data", "Confirm sending %s", confStr)) { + if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, protocol_label, + "Confirm sending %s", confStr)) { return false; } } @@ -139,3 +148,12 @@ bool thor_confirmThorTx(uint32_t data_total, const EthereumSignTx* msg) { return true; } + +bool thor_confirmThorTx(uint32_t data_total, const EthereumSignTx* msg) { + return thor_confirm_deposit_tx(data_total, msg, "Thorchain data", + "Thorchain router"); +} + +bool thor_confirmMayaTx(uint32_t data_total, const EthereumSignTx* msg) { + return thor_confirm_deposit_tx(data_total, msg, "Maya data", "Maya router"); +} From b1216c31cf393e0c06cd28bcc95e0e31af0b9688 Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 30 Jun 2026 01:18:45 -0500 Subject: [PATCH 05/25] style: clang-format-20 the staged fix (cherry picked from commit cd4f700a4dd2aa748c06e07d855e8a0bd3022d76) --- include/keepkey/firmware/ethereum_contracts/thortx.h | 3 ++- lib/firmware/ethereum_contracts.c | 1 - lib/firmware/ethereum_contracts/thortx.c | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/include/keepkey/firmware/ethereum_contracts/thortx.h b/include/keepkey/firmware/ethereum_contracts/thortx.h index ddd26ce42..0bb61bac3 100644 --- a/include/keepkey/firmware/ethereum_contracts/thortx.h +++ b/include/keepkey/firmware/ethereum_contracts/thortx.h @@ -38,7 +38,8 @@ /* deposit(address,address,uint256,string) — legacy selector */ #define THOR_SELECTOR_DEPOSIT "\x1f\xec\xe7\xb4" -/* depositWithExpiry(address,address,uint256,string,uint256) — current selector */ +/* depositWithExpiry(address,address,uint256,string,uint256) — current selector + */ #define THOR_SELECTOR_DEPOSIT_WITH_EXPIRY "\x44\xbc\x93\x7b" typedef struct _EthereumSignTx EthereumSignTx; diff --git a/lib/firmware/ethereum_contracts.c b/lib/firmware/ethereum_contracts.c index 76722cc24..fff38d8d4 100644 --- a/lib/firmware/ethereum_contracts.c +++ b/lib/firmware/ethereum_contracts.c @@ -66,7 +66,6 @@ bool ethereum_contractConfirmed(uint32_t data_total, const EthereumSignTx* msg, if (thor_isMayachainTx(msg)) return thor_confirmMayaTx(data_total, msg); if (thor_isThorchainTx(msg)) return thor_confirmThorTx(data_total, msg); - if (makerdao_isMakerDAO(data_total, msg)) return makerdao_confirmMakerDAO(data_total, msg); diff --git a/lib/firmware/ethereum_contracts/thortx.c b/lib/firmware/ethereum_contracts/thortx.c index bb24d9d6c..474a2772a 100644 --- a/lib/firmware/ethereum_contracts/thortx.c +++ b/lib/firmware/ethereum_contracts/thortx.c @@ -29,7 +29,8 @@ bool thor_has_deposit_selector(const EthereumSignTx* msg) { if (msg->data_initial_chunk.size < 4) return false; - return (memcmp(msg->data_initial_chunk.bytes, THOR_SELECTOR_DEPOSIT, 4) == 0 || + return (memcmp(msg->data_initial_chunk.bytes, THOR_SELECTOR_DEPOSIT, 4) == + 0 || memcmp(msg->data_initial_chunk.bytes, THOR_SELECTOR_DEPOSIT_WITH_EXPIRY, 4) == 0); } From a9f15cea032028a91964bc7c2111ea6158cbd760 Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 30 Jun 2026 01:28:14 -0500 Subject: [PATCH 06/25] fix(maya): declare conf as const char* (string literals are const; -Werror=discarded-qualifiers on ARM) (cherry picked from commit a8b625cd5c0fa659f4c929ca6ba070da27ce5b88) --- lib/firmware/ethereum_contracts/thortx.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/firmware/ethereum_contracts/thortx.c b/lib/firmware/ethereum_contracts/thortx.c index 474a2772a..d7edbd685 100644 --- a/lib/firmware/ethereum_contracts/thortx.c +++ b/lib/firmware/ethereum_contracts/thortx.c @@ -77,7 +77,8 @@ static bool thor_confirm_deposit_tx(uint32_t data_total, const size_t min_chunk = is_expiry ? 260 : 228; if (msg->data_initial_chunk.size < min_chunk) return false; - char confStr[41], *conf; + char confStr[41]; + const char* conf; const TokenType* assetToken; uint8_t* thorchainData; const uint8_t* contractAssetAddress; From b487786cb0f43c0c5ee4e34b01f04298a72f43f0 Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 30 Jun 2026 16:23:23 -0500 Subject: [PATCH 07/25] test(coins): allowlist 8 legacy tokens with no live token entry in TableSanity 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 1af2ffe7, 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 1838c56cf52f1aa104f6b48adeb3fcb1e6536c57) --- unittests/firmware/coins.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/unittests/firmware/coins.cpp b/unittests/firmware/coins.cpp index c11a59bd6..5f4a5f631 100644 --- a/unittests/firmware/coins.cpp +++ b/unittests/firmware/coins.cpp @@ -75,6 +75,17 @@ TEST(Coins, TableSanity) { if (!coin.has_contract_address) continue; + // Pre-existing (not 7.x-release related): these legacy coins[] entries are + // display-only leftovers whose ERC20 entries were dropped from the generated + // token table years ago (dead/migrated tokens). Named allowlist so a *new* + // missing token still fails this sanity check. + static const char *const kLegacyNoTokenEntry[] = { + "QTUM", "BNB", "ZIL", "GTO", "IOST", "CMT", "MCO", "ODEM"}; + bool legacy = false; + for (const char *t : kLegacyNoTokenEntry) + if (strcmp(coin.coin_shortcut, t) == 0) { legacy = true; break; } + if (legacy) continue; + const TokenType *token; if (!tokenByTicker(1, coin.coin_shortcut, &token)) { EXPECT_TRUE(false) << "Can't uniquely find " << coin.coin_shortcut; From 6b088037cfd5c17af8f6dbe146fbc398f0d04655 Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 30 Jun 2026 22:27:28 -0500 Subject: [PATCH 08/25] fix(eth): canonicalise zero-valued RLP integer fields (strip leading 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) (cherry picked from commit d9967ea5fc4419787beeb41f4f15542304f7fab5) --- lib/firmware/ethereum.c | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/lib/firmware/ethereum.c b/lib/firmware/ethereum.c index b7c12c1b5..835bf0830 100644 --- a/lib/firmware/ethereum.c +++ b/lib/firmware/ethereum.c @@ -204,6 +204,23 @@ static void hash_rlp_number(uint32_t number) { hash_rlp_field(data + offset, 4 - offset); } +/* Strip leading zero bytes before RLP-encoding an integer field. Per the + * Ethereum yellow paper, integer fields (nonce, gas, value, fees) must have no + * leading zeros, and an all-zero value is the canonical empty string (0x80). + * Without this, a zero-valued field (e.g. a zero/absent EIP-1559 priority fee + * arriving as a single 0x00 byte) hashes as a literal 0x00, producing a + * non-canonical pre-image that recovers to the wrong signer. Addresses are NOT + * integers and must not use this. */ +static void hash_rlp_bytes_stripped(const uint8_t* buf, size_t size) { + size_t offset = 0; + while (offset < size && buf[offset] == 0) offset++; + if (offset == size) { + hash_rlp_field(buf, 0); + } else { + hash_rlp_field(buf + offset, size - offset); + } +} + /* * Calculate the number of bytes needed for an RLP length header. * NOTE: supports up to 16MB of data (how unlikely...) @@ -872,19 +889,20 @@ void ethereum_signing_init(EthereumSignTx* msg, const HDNode* node, hash_rlp_number(chain_id); } - hash_rlp_field(msg->nonce.bytes, msg->nonce.size); + hash_rlp_bytes_stripped(msg->nonce.bytes, msg->nonce.size); if (ethereum_tx_type == ETHEREUM_TX_TYPE_EIP_1559) { - hash_rlp_field(msg->max_priority_fee_per_gas.bytes, - msg->max_priority_fee_per_gas.size); - hash_rlp_field(msg->max_fee_per_gas.bytes, msg->max_fee_per_gas.size); + hash_rlp_bytes_stripped(msg->max_priority_fee_per_gas.bytes, + msg->max_priority_fee_per_gas.size); + hash_rlp_bytes_stripped(msg->max_fee_per_gas.bytes, + msg->max_fee_per_gas.size); } else { - hash_rlp_field(msg->gas_price.bytes, msg->gas_price.size); + hash_rlp_bytes_stripped(msg->gas_price.bytes, msg->gas_price.size); } - hash_rlp_field(msg->gas_limit.bytes, msg->gas_limit.size); - hash_rlp_field(msg->to.bytes, msg->to.size); - hash_rlp_field(msg->value.bytes, msg->value.size); + hash_rlp_bytes_stripped(msg->gas_limit.bytes, msg->gas_limit.size); + hash_rlp_field(msg->to.bytes, msg->to.size); /* address: no strip */ + hash_rlp_bytes_stripped(msg->value.bytes, msg->value.size); hash_rlp_length(data_total, msg->data_initial_chunk.bytes[0]); hash_data(msg->data_initial_chunk.bytes, msg->data_initial_chunk.size); data_left = data_total - msg->data_initial_chunk.size; From 0847c48f401b8d5234aa2a95128710454e3e5d84 Mon Sep 17 00:00:00 2001 From: highlander Date: Mon, 29 Jun 2026 01:49:25 -0500 Subject: [PATCH 09/25] fix(eth): strip leading zeros in RLP list-length calc to match hashed 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 4d1299d9 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 e68b6855b365a8af1f36adfd6f62f4bb78b74c5d) (cherry picked from commit 5648e53be1ca4e4227bfdac59563ecafcf0f357e) --- lib/firmware/ethereum.c | 42 +++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/lib/firmware/ethereum.c b/lib/firmware/ethereum.c index 835bf0830..8d0f3c637 100644 --- a/lib/firmware/ethereum.c +++ b/lib/firmware/ethereum.c @@ -240,6 +240,21 @@ static int rlp_calculate_length(int length, uint8_t firstbyte) { } } +/* Length of an RLP-encoded integer field AFTER stripping leading zero bytes. + * MUST mirror hash_rlp_bytes_stripped(): the Stage-1 list-length header + * (hash_rlp_list_length) and the Stage-2 bytes actually hashed have to agree, + * or the keccak pre-image is malformed and the signature recovers to a garbage + * address (looks like a "random signer" / dropped tx). Any integer field whose + * big-endian form has a leading zero byte hits this. */ +static int rlp_calculate_length_stripped(const uint8_t* buf, size_t size) { + size_t offset = 0; + while (offset < size && buf[offset] == 0) offset++; + if (offset == size) { + return rlp_calculate_length(0, 0); + } + return rlp_calculate_length(size - offset, buf[offset]); +} + static int rlp_calculate_number_length(uint32_t number) { if (number <= 0x7f) { return 1; @@ -825,24 +840,23 @@ void ethereum_signing_init(EthereumSignTx* msg, const HDNode* node, rlp_length += rlp_calculate_number_length(chain_id); } - rlp_length += rlp_calculate_length(msg->nonce.size, msg->nonce.bytes[0]); - if (ethereum_tx_type == ETHEREUM_TX_TYPE_EIP_1559) { - rlp_length += - rlp_calculate_length(msg->max_priority_fee_per_gas.size, - msg->max_priority_fee_per_gas.size - ? msg->max_priority_fee_per_gas.bytes[0] - : 0); - rlp_length += rlp_calculate_length(msg->max_fee_per_gas.size, - msg->max_fee_per_gas.bytes[0]); + rlp_length += rlp_calculate_length_stripped(msg->nonce.bytes, msg->nonce.size); + if (msg->has_max_fee_per_gas) { + rlp_length += rlp_calculate_length_stripped( + msg->max_priority_fee_per_gas.bytes, + msg->max_priority_fee_per_gas.size); + rlp_length += rlp_calculate_length_stripped(msg->max_fee_per_gas.bytes, + msg->max_fee_per_gas.size); } else { - rlp_length += - rlp_calculate_length(msg->gas_price.size, msg->gas_price.bytes[0]); + rlp_length += rlp_calculate_length_stripped(msg->gas_price.bytes, + msg->gas_price.size); } - rlp_length += - rlp_calculate_length(msg->gas_limit.size, msg->gas_limit.bytes[0]); + rlp_length += rlp_calculate_length_stripped(msg->gas_limit.bytes, + msg->gas_limit.size); rlp_length += rlp_calculate_length(msg->to.size, msg->to.bytes[0]); - rlp_length += rlp_calculate_length(msg->value.size, msg->value.bytes[0]); + rlp_length += + rlp_calculate_length_stripped(msg->value.bytes, msg->value.size); rlp_length += rlp_calculate_length(data_total, msg->data_initial_chunk.bytes[0]); From 0f08a94cfb553a0ec301f4110efdce05fd609142 Mon Sep 17 00:00:00 2001 From: highlander Date: Mon, 29 Jun 2026 02:28:47 -0500 Subject: [PATCH 10/25] fix(eth): always hash EIP-1559 priority fee so Stage 1/2 agree 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 15397eeaf2e81871638792f613ec2f2f6e8711a0) (cherry picked from commit 46ecd08fc4ec1aa0b8f46a0ff31f877a0cf28bcd) --- lib/firmware/ethereum.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/firmware/ethereum.c b/lib/firmware/ethereum.c index 8d0f3c637..059a5ccb5 100644 --- a/lib/firmware/ethereum.c +++ b/lib/firmware/ethereum.c @@ -905,7 +905,13 @@ void ethereum_signing_init(EthereumSignTx* msg, const HDNode* node, hash_rlp_bytes_stripped(msg->nonce.bytes, msg->nonce.size); - if (ethereum_tx_type == ETHEREUM_TX_TYPE_EIP_1559) { + if (msg->has_max_fee_per_gas) { + /* max_priority_fee_per_gas is a mandatory EIP-1559 field; when absent it + * encodes as the empty integer (0x80). Stage 1 always counts it + * (unconditionally, above), so Stage 2 must always hash it too -- guarding + * on has_max_priority_fee_per_gas here would under-hash and leave the list + * header over-declared (the same wrong-signer class this commit fixes). + * .size is 0 when unset, which hash_rlp_bytes_stripped emits as 0x80. */ hash_rlp_bytes_stripped(msg->max_priority_fee_per_gas.bytes, msg->max_priority_fee_per_gas.size); hash_rlp_bytes_stripped(msg->max_fee_per_gas.bytes, From becf7d8acb3d93f5cf33af456723c11e473a532f Mon Sep 17 00:00:00 2001 From: highlander Date: Mon, 29 Jun 2026 02:33:45 -0500 Subject: [PATCH 11/25] style: clang-format RLP length helper call sites in ethereum.c (cherry picked from commit 34ee5deb762a2660bd864a4ddcf88870052b1f78) (cherry picked from commit a404625132ec4c12d412029ee5d7453378e1ff3b) --- lib/firmware/ethereum.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/firmware/ethereum.c b/lib/firmware/ethereum.c index 059a5ccb5..0028af5b5 100644 --- a/lib/firmware/ethereum.c +++ b/lib/firmware/ethereum.c @@ -840,11 +840,12 @@ void ethereum_signing_init(EthereumSignTx* msg, const HDNode* node, rlp_length += rlp_calculate_number_length(chain_id); } - rlp_length += rlp_calculate_length_stripped(msg->nonce.bytes, msg->nonce.size); + rlp_length += + rlp_calculate_length_stripped(msg->nonce.bytes, msg->nonce.size); if (msg->has_max_fee_per_gas) { - rlp_length += rlp_calculate_length_stripped( - msg->max_priority_fee_per_gas.bytes, - msg->max_priority_fee_per_gas.size); + rlp_length += + rlp_calculate_length_stripped(msg->max_priority_fee_per_gas.bytes, + msg->max_priority_fee_per_gas.size); rlp_length += rlp_calculate_length_stripped(msg->max_fee_per_gas.bytes, msg->max_fee_per_gas.size); } else { @@ -852,8 +853,8 @@ void ethereum_signing_init(EthereumSignTx* msg, const HDNode* node, msg->gas_price.size); } - rlp_length += rlp_calculate_length_stripped(msg->gas_limit.bytes, - msg->gas_limit.size); + rlp_length += + rlp_calculate_length_stripped(msg->gas_limit.bytes, msg->gas_limit.size); rlp_length += rlp_calculate_length(msg->to.size, msg->to.bytes[0]); rlp_length += rlp_calculate_length_stripped(msg->value.bytes, msg->value.size); From 3e2e0697424c4bc4f683484902443cbd8d4842ec Mon Sep 17 00:00:00 2001 From: highlander Date: Mon, 29 Jun 2026 03:08:30 -0500 Subject: [PATCH 12/25] fix(eth): enforce EIP-1559 tx-type/fee-field consistency before signing 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 5836ff4ce9b511d01ffa25c4a3b1f8098c6ce346) (cherry picked from commit 4cce2fbc657c2dbc8c330152adb4a62310f9c65e) --- lib/firmware/ethereum.c | 42 +++++++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/lib/firmware/ethereum.c b/lib/firmware/ethereum.c index 0028af5b5..71a7da38e 100644 --- a/lib/firmware/ethereum.c +++ b/lib/firmware/ethereum.c @@ -617,8 +617,13 @@ static bool ethereum_signing_check(const EthereumSignTx* msg) { return false; } - if (msg->gas_price.size + msg->gas_limit.size > 30) { - // sanity check that fee doesn't overflow + // Sanity-bound the fee field that this tx type actually uses, so the + // on-screen fee (fee_per_gas * gas_limit) cannot overflow into the modular + // bn_multiply and display a wrong value. EIP-1559 uses max_fee_per_gas; + // legacy uses gas_price (which is 0 for EIP-1559 and vice versa). + size_t fee_per_gas_size = msg->has_max_fee_per_gas ? msg->max_fee_per_gas.size + : msg->gas_price.size; + if (fee_per_gas_size + msg->gas_limit.size > 30) { return false; } @@ -680,17 +685,30 @@ void ethereum_signing_init(EthereumSignTx* msg, const HDNode* node, ethereum_tx_type = ETHEREUM_TX_TYPE_LEGACY; } - if (ethereum_tx_type == ETHEREUM_TX_TYPE_EIP_1559 && chain_id == 0) { - fsm_sendFailure(FailureType_Failure_SyntaxError, - _("EIP-1559 transactions require chain_id")); - ethereum_signing_abort(); - return; - } - - if (ethereum_tx_type == ETHEREUM_TX_TYPE_EIP_1559 && - !msg->has_max_fee_per_gas) { + /* The typed prefix (0x02) and access list are emitted based on + * ethereum_tx_type, while the fee fields are selected by has_max_fee_per_gas. + * If those two disagree, Stage 1 (rlp_length) and Stage 2 (hashed bytes) + * describe different field lists and the signature recovers to a wrong + * address. Enforce a consistent shape up front. */ + if (ethereum_tx_type == ETHEREUM_TX_TYPE_EIP_1559) { + if (chain_id == 0) { + /* chain_id is the mandatory first RLP field of an EIP-1559 tx; absent + * chain_id is counted (1 byte) in Stage 1 but hash_rlp_number(0) hashes + * nothing in Stage 2. */ + fsm_sendFailure(FailureType_Failure_SyntaxError, + _("EIP-1559 transactions require chain_id")); + ethereum_signing_abort(); + return; + } + if (!msg->has_max_fee_per_gas) { + fsm_sendFailure(FailureType_Failure_SyntaxError, + _("EIP-1559 transactions require max_fee_per_gas")); + ethereum_signing_abort(); + return; + } + } else if (msg->has_max_fee_per_gas) { fsm_sendFailure(FailureType_Failure_SyntaxError, - _("EIP-1559 transactions require max_fee_per_gas")); + _("max_fee_per_gas requires an EIP-1559 (type 2) tx")); ethereum_signing_abort(); return; } From 0e767af2c622ea277f78b7a594dc70ca8fbaf4f3 Mon Sep 17 00:00:00 2001 From: highlander Date: Wed, 1 Jul 2026 16:01:04 -0500 Subject: [PATCH 13/25] fix(eth): clear-sign handler gating + 0x drainer hardening (port alpha #255-gate/#260) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 41b8016c + d550b182, 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 3df07921, HIGH, PR #260). - Uniswap liquidity ops: require recipient == signer, fail closed (alpha 1ff989ea, 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 321328be, PR #260). Files are taken at the alpha tip state (1f147b50), which already reconciles upstream develop; none of them reference Insight/signed-metadata. (cherry picked from commit 8700084061065bc429976f42476a33407ac4b916) --- lib/firmware/ethereum_contracts.c | 22 ++++++++++++++++++- lib/firmware/ethereum_contracts/saproxy.c | 3 ++- lib/firmware/ethereum_contracts/zxappliquid.c | 3 ++- lib/firmware/ethereum_contracts/zxliquidtx.c | 21 ++++++++++++------ lib/firmware/ethereum_contracts/zxswap.c | 20 ++++++++++++++++- .../ethereum_contracts/zxtransERC20.c | 4 ++++ 6 files changed, 62 insertions(+), 11 deletions(-) diff --git a/lib/firmware/ethereum_contracts.c b/lib/firmware/ethereum_contracts.c index fff38d8d4..deec3b4eb 100644 --- a/lib/firmware/ethereum_contracts.c +++ b/lib/firmware/ethereum_contracts.c @@ -20,6 +20,7 @@ #include "keepkey/firmware/ethereum_contracts.h" +#include "keepkey/firmware/ethereum.h" // completes EthereumSignTx (msg fields) #include "keepkey/firmware/ethereum_contracts/saproxy.h" #include "keepkey/firmware/ethereum_contracts/thortx.h" #include "keepkey/firmware/ethereum_contracts/zxappliquid.h" @@ -32,8 +33,27 @@ bool ethereum_contractHandled(uint32_t data_total, const EthereumSignTx* msg, const HDNode* node) { (void)node; - if (sa_isWithdrawFromSalary(msg)) return true; + /* Only a CALL to a contract may be clear-signed, never a CREATE + * (to.size == 0 must reach the deploy screen). */ + if (msg->to.size != 20) { + return false; + } + + /* 0x transformERC20 is pinned to the ExchangeProxy and bounded by its + * displayed input/min-output amounts, so it is safe to clear-sign at ANY + * calldata size; its transformations[] tail legitimately exceeds one 1024- + * byte chunk. (It guards its own fixed-offset reads against + * data_initial_chunk.size.) */ if (zx_isZxTransformERC20(msg)) return true; + + /* Every other handler must have the ENTIRE calldata in the first chunk, so + * the fields it parses and displays are the whole transaction and nothing + * unshown streams in afterwards. */ + if (data_total != msg->data_initial_chunk.size) { + return false; + } + + if (sa_isWithdrawFromSalary(msg)) return true; if (zx_isZxSwap(msg)) return true; if (zx_isZxLiquidTx(msg)) return true; if (zx_isZxApproveLiquid(msg)) return true; diff --git a/lib/firmware/ethereum_contracts/saproxy.c b/lib/firmware/ethereum_contracts/saproxy.c index ccab30ba6..c7514995f 100644 --- a/lib/firmware/ethereum_contracts/saproxy.c +++ b/lib/firmware/ethereum_contracts/saproxy.c @@ -45,7 +45,8 @@ bool sa_isWithdrawFromSalary(const EthereumSignTx* msg) { bool sa_confirmWithdrawFromSalary(uint32_t data_total, const EthereumSignTx* msg) { - (void)data_total; + /* reads selector + 2 32-byte words (salaryId, withdrawAmount) */ + if (data_total < 4 + 2 * 32) return false; char confStr[41]; bignum256 salaryId, withdrawAmount; diff --git a/lib/firmware/ethereum_contracts/zxappliquid.c b/lib/firmware/ethereum_contracts/zxappliquid.c index 76d7d3cda..b4e752419 100644 --- a/lib/firmware/ethereum_contracts/zxappliquid.c +++ b/lib/firmware/ethereum_contracts/zxappliquid.c @@ -36,7 +36,8 @@ bool zx_confirmApproveLiquidity(uint32_t data_total, const EthereumSignTx *msg) { - (void)data_total; + /* reads selector + 2 32-byte words (spender, allowance) */ + if (data_total < 4 + 2 * 32) return false; const char *to, *tikstr, *poolstr, *allowance, *amt; unsigned char data[40]; uint8_t digest[SHA3_256_DIGEST_LENGTH] = {0}; diff --git a/lib/firmware/ethereum_contracts/zxliquidtx.c b/lib/firmware/ethereum_contracts/zxliquidtx.c index e70ccd4e3..8f2123830 100644 --- a/lib/firmware/ethereum_contracts/zxliquidtx.c +++ b/lib/firmware/ethereum_contracts/zxliquidtx.c @@ -91,23 +91,29 @@ static bool confirmFromAccountMatch(const EthereumSignTx* msg, if (!hdnode_get_ethereum_pubkeyhash(node, addressBytes)) { memzero(node, sizeof(*node)); + return false; } fromAddress = (const uint8_t*)(msg->data_initial_chunk.bytes + 4 + 5 * 32 - 20); - if (memcmp(fromAddress, addressBytes, 20) == 0) { - fromSrc = "self"; - } else { - fromSrc = "NOT this wallet"; - } + bool isSelf = (memcmp(fromAddress, addressBytes, 20) == 0); + fromSrc = isSelf ? "this wallet" : "NOT this wallet"; for (uint32_t ctr = 0; ctr < 20; ctr++) { snprintf(&addressStr[2 + ctr * 2], 3, "%02x", fromAddress[ctr]); } if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, addremStr, - "Confirming ETH address is %s: %s", fromSrc, addressStr)) { + "Liquidity recipient is %s: %s", fromSrc, addressStr)) { + return false; + } + + /* Fail closed: the liquidity/LP recipient (the `to` parameter) must be the + * signer's own address. Previously a non-self recipient only produced a soft + * "NOT this wallet" warning that the user could click through, letting LP + * tokens / withdrawn ETH be routed to an attacker. */ + if (!isSelf) { return false; } return true; @@ -125,7 +131,8 @@ bool zx_isZxLiquidTx(const EthereumSignTx* msg) { } bool zx_confirmZxLiquidTx(uint32_t data_total, const EthereumSignTx* msg) { - (void)data_total; + /* reads through the deadline word at offset 4 + 6*32 - 8 .. 4 + 6*32 */ + if (data_total < 4 + 6 * 32) return false; const TokenType* token; char constr1[40], constr2[40], tokbuf[32]; const char* arStr = ""; diff --git a/lib/firmware/ethereum_contracts/zxswap.c b/lib/firmware/ethereum_contracts/zxswap.c index 5d033e804..15dccb699 100644 --- a/lib/firmware/ethereum_contracts/zxswap.c +++ b/lib/firmware/ethereum_contracts/zxswap.c @@ -44,7 +44,22 @@ bool zx_isZxSwap(const EthereumSignTx* msg) { } bool zx_confirmZxSwap(uint32_t data_total, const EthereumSignTx* msg) { - (void)data_total; + /* fixed reads run through the fromAddress word at offset 4 + 5*32 + 12 .. +20 + * (== 4 + 6*32); the toAddress word is bounds-checked below once its + * position is known from numOfTokens. */ + if (data_total < 4 + 6 * 32) return false; + + /* The first head word is the ABI offset pointer to the dynamic `tokens[]` + * array. We read numOfTokens / tokens[] at FIXED offsets that are only + * correct when this pointer is canonical (0x80 = 4 head words). If it is + * not, the Solidity decoder follows it elsewhere, so what we display would + * differ from what executes (a drain via display/execution mismatch). Reject + * the non-canonical encoding -> falls through to the blind-sign path. */ + static const uint8_t TOKENS_OFFSET_CANON[32] = {[31] = 0x80}; + if (memcmp(msg->data_initial_chunk.bytes + 4, TOKENS_OFFSET_CANON, 32) != 0) { + return false; + } + const TokenType *from, *to; const uint8_t *fromAddress, *toAddress; char constr1[40], constr2[40]; @@ -71,6 +86,9 @@ bool zx_confirmZxSwap(uint32_t data_total, const EthereumSignTx* msg) { break; } + /* toAddress word ends at offset 4 + (6 + adder + 1) * 32 */ + if (data_total < 4 + (7 + adder) * 32) return false; + fromAddress = (const uint8_t*)(msg->data_initial_chunk.bytes + 4 + 5 * 32 + 12); toAddress = (const uint8_t*)(msg->data_initial_chunk.bytes + 4 + diff --git a/lib/firmware/ethereum_contracts/zxtransERC20.c b/lib/firmware/ethereum_contracts/zxtransERC20.c index 75f64c160..83074443e 100644 --- a/lib/firmware/ethereum_contracts/zxtransERC20.c +++ b/lib/firmware/ethereum_contracts/zxtransERC20.c @@ -45,6 +45,10 @@ bool zx_isZxTransformERC20(const EthereumSignTx* msg) { bool zx_confirmZxTransERC20(uint32_t data_total, const EthereumSignTx* msg) { (void)data_total; + /* Reads selector + 4 static head words (in/out token, in/out amount). This + * handler is allowed at any data_total (large transformations[] tail), so + * guard the read extent against the RECEIVED chunk, not the total length. */ + if (msg->data_initial_chunk.size < 4 + 4 * 32) return false; const TokenType *in, *out; const uint8_t *inAddress, *outAddress; char constr1[40], constr2[40]; From a06e4c02d8eac0ee609b3085f804a2db8071a047 Mon Sep 17 00:00:00 2001 From: highlander Date: Wed, 1 Jul 2026 16:01:52 -0500 Subject: [PATCH 14/25] fix(eth): pin THORChain router to current v4 + validate memo offset (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 61250a22, 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 80a088ec797dfbc41772bd44d9133ae8d5ba25dc) --- .../firmware/ethereum_contracts/thortx.h | 8 +++-- lib/firmware/ethereum_contracts/thortx.c | 33 +++++++++++++++---- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/include/keepkey/firmware/ethereum_contracts/thortx.h b/include/keepkey/firmware/ethereum_contracts/thortx.h index 0bb61bac3..1bdc50aca 100644 --- a/include/keepkey/firmware/ethereum_contracts/thortx.h +++ b/include/keepkey/firmware/ethereum_contracts/thortx.h @@ -30,8 +30,12 @@ "\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee\xee" \ "\xee\xee" -/* THORChain ETH router (mainnet) */ -#define THOR_ROUTER "42a5ed456650a09dc10ebc6361a7480fdd61f27b" +/* THORChain ETH router (mainnet), current v4.1.1. + * NOTE: THORChain migrates this router periodically (v1 42a5ed.. -> v3 + * 3624525.. -> v4 d37bbe..). A hardcoded pin must be updated on each migration; + * the durable path is the signed-metadata clear-sign protocol (host-signed, + * key-pinned) which needs no firmware update per router change. */ +#define THOR_ROUTER "d37bbe5744d730a1d98d8dc97c42f0ca46ad7146" /* Maya Protocol ETH router (mainnet) */ #define MAYA_ROUTER "d89dce570de35a6f42d3bca7dba50a6d89bfc2a2" diff --git a/lib/firmware/ethereum_contracts/thortx.c b/lib/firmware/ethereum_contracts/thortx.c index d7edbd685..433fac2eb 100644 --- a/lib/firmware/ethereum_contracts/thortx.c +++ b/lib/firmware/ethereum_contracts/thortx.c @@ -58,10 +58,15 @@ bool thor_isMayachainTx(const EthereumSignTx* msg) { } bool thor_isThorchainTx(const EthereumSignTx* msg) { - if (msg->has_to && msg->to.size == 20 && thor_has_deposit_selector(msg)) { - return true; - } - return false; + if (!msg->has_to || msg->to.size != 20) return false; + if (!thor_has_deposit_selector(msg)) return false; + /* Pin to the THORChain router. Without this, ANY contract carrying the + * deposit selector would get the THORChain clear-sign UX and bypass the + * AdvancedMode blind-sign gate, letting an attacker contract drain while the + * device shows a benign deposit. Mirrors thor_isMayachainTx. */ + char toStr[41]; + thor_format_to_addr(msg, toStr); + return strncmp(toStr, THOR_ROUTER, 40) == 0; } static bool thor_confirm_deposit_tx(uint32_t data_total, @@ -77,10 +82,24 @@ static bool thor_confirm_deposit_tx(uint32_t data_total, const size_t min_chunk = is_expiry ? 260 : 228; if (msg->data_initial_chunk.size < min_chunk) return false; + /* The memo is a dynamic `string`; its ABI head pointer (word 3, offset + * 4+3*32) must be canonical (0x80 for deposit's 4 head words, 0xa0 for + * depositWithExpiry's 5), else abi.decode on the router reads the memo from a + * different location than we display from the fixed offset below -> the + * executed swap destination can differ from what the user approved. */ + { + static const uint8_t MEMO_OFF_DEPOSIT[32] = {[31] = 0x80}; + static const uint8_t MEMO_OFF_EXPIRY[32] = {[31] = 0xa0}; + const uint8_t* expected = is_expiry ? MEMO_OFF_EXPIRY : MEMO_OFF_DEPOSIT; + if (memcmp(msg->data_initial_chunk.bytes + 4 + 3 * 32, expected, 32) != 0) { + return false; + } + } + char confStr[41]; const char* conf; const TokenType* assetToken; - uint8_t* thorchainData; + const uint8_t* thorchainData; const uint8_t* contractAssetAddress; const uint8_t *vaultAddress, *assetAddress; uint32_t ctr; @@ -91,8 +110,8 @@ static bool thor_confirm_deposit_tx(uint32_t data_total, (const uint8_t*)(msg->data_initial_chunk.bytes + 4 + 32 + 12); bn_from_bytes(msg->data_initial_chunk.bytes + 4 + 2 * 32, 32, &Amount); /* deposit(): memo at 4 + 5*32; depositWithExpiry(): memo at 4 + 6*32 */ - thorchainData = - (uint8_t*)(msg->data_initial_chunk.bytes + 4 + (is_expiry ? 6 : 5) * 32); + thorchainData = (const uint8_t*)(msg->data_initial_chunk.bytes + 4 + + (is_expiry ? 6 : 5) * 32); thor_format_to_addr(msg, confStr); if (strncmp(confStr, THOR_ROUTER, 40) == 0) { From 43129ded004a59d18f0bb5cbbee15b6d91d863fe Mon Sep 17 00:00:00 2001 From: highlander Date: Wed, 1 Jul 2026 16:10:13 -0500 Subject: [PATCH 15/25] =?UTF-8?q?feat(insight):=20signed-metadata=20clear-?= =?UTF-8?q?signing=20+=20AdvancedMode=20blind-sign=20gate=20(stage=20#257/?= =?UTF-8?q?#258=20=E2=86=92=20develop)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Squash-stage of the Insight EVM clear-signing feature from alpha (fork PRs #257 merge 1f421a8f + #258 CI-greening a3cdca98; commits 8b244b42, ba02b5a0, f9fc53ec, 53d04eea) 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 (1f147b50, 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 4f387d5fab5551960d2b62e7141aa12c73395bb6) --- include/keepkey/board/confirm_sm.h | 4 + include/keepkey/board/layout.h | 1 + include/keepkey/firmware/fsm.h | 1 + include/keepkey/firmware/signed_metadata.h | 85 +++ include/keepkey/firmware/storage.h | 5 + lib/board/confirm_sm.c | 23 + lib/board/layout.c | 4 + lib/firmware/CMakeLists.txt | 1 + lib/firmware/ethereum.c | 66 ++- lib/firmware/fsm_msg_ethereum.h | 32 + lib/firmware/messagemap.def | 3 + lib/firmware/signed_metadata.c | 381 ++++++++++++ lib/firmware/storage.c | 4 + unittests/firmware/CMakeLists.txt | 1 + unittests/firmware/signed_metadata.cpp | 646 +++++++++++++++++++++ 15 files changed, 1242 insertions(+), 15 deletions(-) create mode 100644 include/keepkey/firmware/signed_metadata.h create mode 100644 lib/firmware/signed_metadata.c create mode 100644 unittests/firmware/signed_metadata.cpp diff --git a/include/keepkey/board/confirm_sm.h b/include/keepkey/board/confirm_sm.h index fa74c829f..765a90125 100644 --- a/include/keepkey/board/confirm_sm.h +++ b/include/keepkey/board/confirm_sm.h @@ -95,6 +95,10 @@ bool confirm(ButtonRequestType type, const char* request_title, const char* request_body, ...) __attribute__((format(printf, 3, 4))); +bool confirm_with_icon(ButtonRequestType type, IconType iconNum, + const char* request_title, const char* request_body, ...) + __attribute__((format(printf, 4, 5))); + bool confirm_constant_power(ButtonRequestType type, const char* request_title, const char* request_body, ...) __attribute__((format(printf, 3, 4))); diff --git a/include/keepkey/board/layout.h b/include/keepkey/board/layout.h index ed5ab8f33..9ce9c718d 100644 --- a/include/keepkey/board/layout.h +++ b/include/keepkey/board/layout.h @@ -79,6 +79,7 @@ typedef enum { typedef enum { NO_ICON = 0, ETHEREUM_ICON, + VERIFIED_ICON, } IconType; typedef void (*AnimateCallback)(void* data, uint32_t duration, diff --git a/include/keepkey/firmware/fsm.h b/include/keepkey/firmware/fsm.h index 656f0c127..b112181ff 100644 --- a/include/keepkey/firmware/fsm.h +++ b/include/keepkey/firmware/fsm.h @@ -85,6 +85,7 @@ void fsm_msgEthereumSignMessage(EthereumSignMessage* msg); void fsm_msgEthereumVerifyMessage(const EthereumVerifyMessage* msg); void fsm_msgEthereumSignTypedHash(const EthereumSignTypedHash* msg); void fsm_msgEthereum712TypesValues(Ethereum712TypesValues* msg); +void fsm_msgEthereumTxMetadata(const EthereumTxMetadata* msg); void fsm_msgNanoGetAddress(NanoGetAddress* msg); void fsm_msgNanoSignTx(NanoSignTx* msg); diff --git a/include/keepkey/firmware/signed_metadata.h b/include/keepkey/firmware/signed_metadata.h new file mode 100644 index 000000000..4e0ca55c1 --- /dev/null +++ b/include/keepkey/firmware/signed_metadata.h @@ -0,0 +1,85 @@ +#ifndef KEEPKEY_FIRMWARE_SIGNED_METADATA_H +#define KEEPKEY_FIRMWARE_SIGNED_METADATA_H + +#include +#include +#include + +typedef struct _EthereumSignTx EthereumSignTx; + +#define METADATA_MAX_ARGS 8 +#define METADATA_MAX_METHOD_LEN 64 +#define METADATA_MAX_ARG_NAME_LEN 32 +#define METADATA_MAX_ARG_VALUE_LEN 32 +#define METADATA_MAX_KEYS 4 + +typedef enum { + METADATA_OPAQUE = 0, + METADATA_VERIFIED = 1, + METADATA_MALFORMED = 2, +} MetadataClassification; + +typedef enum { + ARG_FORMAT_RAW = 0, + ARG_FORMAT_ADDRESS = 1, + ARG_FORMAT_AMOUNT = 2, + ARG_FORMAT_BYTES = 3, +} ArgFormat; + +typedef struct { + char name[METADATA_MAX_ARG_NAME_LEN + 1]; + ArgFormat format; + uint8_t value[METADATA_MAX_ARG_VALUE_LEN]; + uint16_t value_len; +} MetadataArg; + +typedef struct { + uint8_t version; + uint32_t chain_id; + uint8_t contract_address[20]; + uint8_t selector[4]; + uint8_t tx_hash[32]; + char method_name[METADATA_MAX_METHOD_LEN + 1]; + uint8_t num_args; + MetadataArg args[METADATA_MAX_ARGS]; + MetadataClassification classification; + uint32_t timestamp; + uint8_t key_id; + uint8_t signature[64]; + uint8_t recovery; +} SignedMetadata; + +bool signed_metadata_available(void); +void signed_metadata_clear(void); +MetadataClassification signed_metadata_process(const uint8_t *payload, + size_t payload_len, + uint8_t key_id); +/* Display gate: does this metadata plausibly describe `msg`? Binds contract + * address, selector and chain id so the wrong method is never shown. The + * authoritative full-tx binding is enforced later by signed_metadata_enforce(). + */ +bool signed_metadata_matches_tx(const EthereumSignTx *msg); +bool signed_metadata_confirm(void); + +/* True once a verified confirm has suppressed the raw-data confirmation, i.e. + * the signature is now gated on the metadata matching the final tx hash. */ +bool signed_metadata_relied(void); + +/* Authoritative binding, called after the real Ethereum sighash is finalized + * (in send_signature, the only point it exists). Returns true if signing may + * proceed: either no metadata was relied upon, or the relied-upon metadata's + * committed tx_hash equals `hash`. Fail-closed on any mismatch. */ +bool signed_metadata_enforce(const uint8_t hash[32]); + +/* Pure enforcement decision, exported for unit testing. Given the module flags + * and the metadata's committed tx hash, decides whether signing may proceed for + * the just-finalized `hash`. signed_metadata_enforce() is a thin wrapper that + * feeds the module state into this function. No state, no I/O. */ +bool signed_metadata_enforce_decision(bool relied, bool available, + int classification, + const uint8_t *stored_hash, + const uint8_t *hash); + +const SignedMetadata *signed_metadata_get(void); + +#endif diff --git a/include/keepkey/firmware/storage.h b/include/keepkey/firmware/storage.h index 9fc8c1954..dd904d6b4 100644 --- a/include/keepkey/firmware/storage.h +++ b/include/keepkey/firmware/storage.h @@ -70,6 +70,11 @@ void storage_loadDevice(LoadDevice* msg); /// \return true iff the root node was found. bool storage_getRootNode(const char* curve, bool usePassphrase, HDNode* node); +/// \brief Get the raw 64-byte BIP-39 seed, triggering passphrase if needed. +/// \param usePassphrase[in] Whether to use the passphrase. +/// \returns pointer to 64-byte seed, or NULL on failure. +const uint8_t* storage_getRawSeed(bool usePassphrase); + /// \brief Fetch the node used for U2F signing. /// \returns true iff retrieval was successful. bool storage_getU2FRoot(HDNode* node); diff --git a/lib/board/confirm_sm.c b/lib/board/confirm_sm.c index 0669536ab..54bf6da92 100644 --- a/lib/board/confirm_sm.c +++ b/lib/board/confirm_sm.c @@ -315,6 +315,29 @@ bool confirm(ButtonRequestType type, const char* request_title, return ret; } +bool confirm_with_icon(ButtonRequestType type, IconType iconNum, + const char* request_title, const char* request_body, + ...) { + button_request_acked = false; + + va_list vl; + va_start(vl, request_body); + vsnprintf(strbuf, sizeof(strbuf), request_body, vl); + va_end(vl); + + ButtonRequest resp; + memset(&resp, 0, sizeof(ButtonRequest)); + resp.has_code = true; + resp.code = type; + msg_write(MessageType_MessageType_ButtonRequest, &resp); + + bool ret = + confirm_helper(request_title, strbuf, &layout_standard_notification, + false, iconNum, false); + memzero(strbuf, sizeof(strbuf)); + return ret; +} + bool confirm_constant_power(ButtonRequestType type, const char* request_title, const char* request_body, ...) { button_request_acked = false; diff --git a/lib/board/layout.c b/lib/board/layout.c index 59e9dd9e2..d2b718c36 100644 --- a/lib/board/layout.c +++ b/lib/board/layout.c @@ -327,6 +327,10 @@ void layout_standard_notification(const char* str1, const char* str2, void layout_add_icon(IconType type) { switch (type) { case ETHEREUM_ICON: + /* ponytail: reuse the ETH glyph as the "verified" mark — it's an ETH tx. + * Swap in a dedicated checkmark bitmap if the trust mark needs to differ. + */ + case VERIFIED_ICON: draw_bitmap_mono_rle(canvas, get_ethereum_icon_frame(), false); break; diff --git a/lib/firmware/CMakeLists.txt b/lib/firmware/CMakeLists.txt index f23ebcfb8..9d09781e6 100644 --- a/lib/firmware/CMakeLists.txt +++ b/lib/firmware/CMakeLists.txt @@ -31,6 +31,7 @@ set(sources reset.c ripple.c ripple_base58.c + signed_metadata.c signing.c signtx_tendermint.c solana.c diff --git a/lib/firmware/ethereum.c b/lib/firmware/ethereum.c index 71a7da38e..1cd0025e3 100644 --- a/lib/firmware/ethereum.c +++ b/lib/firmware/ethereum.c @@ -33,6 +33,7 @@ #include "keepkey/firmware/eip712.h" #include "keepkey/firmware/ethereum_contracts.h" #include "keepkey/firmware/ethereum_contracts/makerdao.h" +#include "keepkey/firmware/signed_metadata.h" #include "keepkey/firmware/ethereum_tokens.h" #include "keepkey/firmware/storage.h" #include "keepkey/firmware/thorchain.h" @@ -204,13 +205,10 @@ static void hash_rlp_number(uint32_t number) { hash_rlp_field(data + offset, 4 - offset); } -/* Strip leading zero bytes before RLP-encoding an integer field. Per the - * Ethereum yellow paper, integer fields (nonce, gas, value, fees) must have no - * leading zeros, and an all-zero value is the canonical empty string (0x80). - * Without this, a zero-valued field (e.g. a zero/absent EIP-1559 priority fee - * arriving as a single 0x00 byte) hashes as a literal 0x00, producing a - * non-canonical pre-image that recovers to the wrong signer. Addresses are NOT - * integers and must not use this. */ +/* Strip leading zero bytes before RLP-encoding an integer field. + * Per the Ethereum yellow paper, integer fields (nonce, gas, value, etc.) + * must not have leading zeros. Addresses are NOT integers and must not use + * this function. */ static void hash_rlp_bytes_stripped(const uint8_t* buf, size_t size) { size_t offset = 0; while (offset < size && buf[offset] == 0) offset++; @@ -297,6 +295,19 @@ static void send_signature(void) { } keccak_Final(&keccak_ctx, hash); + + /* Insight clear-signing binding. If a verified metadata blob suppressed the + * raw-data confirmation, the actual signed digest MUST equal the tx hash the + * metadata committed to. This is the first point that digest exists, so the + * check reuses it rather than re-deriving the RLP pre-image. Fail closed — + * never emit a signature the displayed decoded screen did not cover. */ + if (!signed_metadata_enforce(hash)) { + fsm_sendFailure(FailureType_Failure_Other, + "Metadata does not match signed transaction"); + ethereum_signing_abort(); + return; + } + if (ecdsa_sign_digest(&secp256k1, privkey, hash, sig, &v, ethereum_is_canonic) != 0) { fsm_sendFailure(FailureType_Failure_Other, "Signing failed"); @@ -768,6 +779,30 @@ void ethereum_signing_init(EthereumSignTx* msg, const HDNode* node, data_needs_confirm = false; } + // Signed metadata clear signing (backwards compatible). + // Only fires if host sent EthereumTxMetadata before this EthereumSignTx. + if (data_needs_confirm && data_total > 0 && signed_metadata_available()) { + if (signed_metadata_matches_tx(msg)) { + if (signed_metadata_confirm()) { + // Decoded who/what/why approved; raw-data confirm is suppressed. The + // signature is bound to this metadata's tx hash in send_signature(). + needs_confirm = false; + data_needs_confirm = false; + } else { + fsm_sendFailure(FailureType_Failure_ActionCancelled, + "Signing cancelled by user"); + ethereum_signing_abort(); // clears metadata + return; + } + } + } + // Drop metadata now UNLESS we relied on it to suppress the raw-data confirm + // (then it must survive to bind the signature). Prevents stale reuse when the + // contractHandled / ERC-20 paths bypass the metadata check above. + if (!signed_metadata_relied()) { + signed_metadata_clear(); + } + // detect ERC-20 token if (data_total == 68 && ethereum_isStandardERC20Transfer(msg)) { token = tokenByChainAddress(chain_id, msg->to.bytes); @@ -813,15 +848,15 @@ void ethereum_signing_init(EthereumSignTx* msg, const HDNode* node, memset(confirm_body_message, 0, sizeof(confirm_body_message)); if (token == NULL && data_total > 0 && data_needs_confirm) { - // KeepKey custom: warn the user that they're trying to do something - // that is potentially dangerous. People (generally) aren't great at - // parsing raw transaction data, and we can't effectively show them - // what they're about to do in the general case. + // AdvancedMode policy: hard gate for blind-signing arbitrary contract data if (!storage_isPolicyEnabled("AdvancedMode")) { - (void)review( - ButtonRequestType_ButtonRequest_Other, "Warning", - "Signing of arbitrary ETH contract data is recommended only for " - "experienced users. Enable 'AdvancedMode' policy to dismiss."); + (void)review(ButtonRequestType_ButtonRequest_Other, "Blocked", + "Blind signing requires AdvancedMode. " + "Enable in device settings."); + fsm_sendFailure(FailureType_Failure_ActionCancelled, + "Blind signing disabled by policy"); + ethereum_signing_abort(); + return; } layoutEthereumData(msg->data_initial_chunk.bytes, @@ -989,6 +1024,7 @@ void ethereum_signing_txack(EthereumTxAck* tx) { void ethereum_signing_abort(void) { if (ethereum_signing) { memzero(privkey, sizeof(privkey)); + signed_metadata_clear(); layoutHome(); ethereum_signing = false; } diff --git a/lib/firmware/fsm_msg_ethereum.h b/lib/firmware/fsm_msg_ethereum.h index 5c9827164..69031c4a6 100644 --- a/lib/firmware/fsm_msg_ethereum.h +++ b/lib/firmware/fsm_msg_ethereum.h @@ -19,6 +19,38 @@ * along with this library. If not, see . */ +#include "keepkey/firmware/signed_metadata.h" + +void fsm_msgEthereumTxMetadata(const EthereumTxMetadata* msg) { + CHECK_INITIALIZED + CHECK_PIN + + RESP_INIT(EthereumMetadataAck); + + MetadataClassification result = signed_metadata_process( + msg->signed_payload.bytes, msg->signed_payload.size, + msg->has_key_id ? msg->key_id : 0); + + resp->classification = (uint32_t)result; + resp->has_display_summary = true; + + switch (result) { + case METADATA_VERIFIED: + strlcpy(resp->display_summary, "Verified", sizeof(resp->display_summary)); + break; + case METADATA_OPAQUE: + strlcpy(resp->display_summary, "Unverified", + sizeof(resp->display_summary)); + break; + case METADATA_MALFORMED: + default: + strlcpy(resp->display_summary, "Invalid", sizeof(resp->display_summary)); + break; + } + + msg_write(MessageType_MessageType_EthereumMetadataAck, resp); +} + static int process_ethereum_xfer(const CoinType* coin, EthereumSignTx* msg) { if (!ethereum_isStandardERC20Transfer(msg) && msg->data_length != 0) return TXOUT_COMPILE_ERROR; diff --git a/lib/firmware/messagemap.def b/lib/firmware/messagemap.def index ee82cc2bd..719995f7b 100644 --- a/lib/firmware/messagemap.def +++ b/lib/firmware/messagemap.def @@ -178,3 +178,6 @@ DEBUG_OUT(MessageType_MessageType_DebugLinkLog, DebugLinkLog, NO_PROCESS_FUNC) DEBUG_OUT(MessageType_MessageType_DebugLinkFlashDumpResponse, DebugLinkFlashDumpResponse, NO_PROCESS_FUNC) #endif + + MSG_IN(MessageType_MessageType_EthereumTxMetadata, EthereumTxMetadata, fsm_msgEthereumTxMetadata) + MSG_OUT(MessageType_MessageType_EthereumMetadataAck, EthereumMetadataAck, NO_PROCESS_FUNC) diff --git a/lib/firmware/signed_metadata.c b/lib/firmware/signed_metadata.c new file mode 100644 index 000000000..b18974e8c --- /dev/null +++ b/lib/firmware/signed_metadata.c @@ -0,0 +1,381 @@ +#include "keepkey/firmware/signed_metadata.h" + +#include "keepkey/board/confirm_sm.h" +#include "keepkey/board/util.h" +#include "keepkey/firmware/ethereum.h" +#include "trezor/crypto/address.h" +#include "trezor/crypto/bignum.h" +#include "trezor/crypto/ecdsa.h" +#include "trezor/crypto/memzero.h" +#include "trezor/crypto/secp256k1.h" +#include "trezor/crypto/sha2.h" + +#include +#include + +#define _(X) (X) + +static bool metadata_available = false; +static bool relied_on_metadata = false; +static SignedMetadata stored_metadata; + +/* + * Metadata verification public keys. + * Slot 0: active production key + * Slot 1: rotation target + * Slots 2-3: reserved + * + * Keys are derived via KeepKey SignIdentity at keepkey.com/insight. + * Only the public key is stored here — the signing mnemonic is held + * offline and never appears in source code. + * + * To rotate: generate new key with pioneer-insight keygen, + * replace the slot below, ship firmware update. + */ +static const uint8_t METADATA_PUBKEYS[METADATA_MAX_KEYS][33] = { + /* Key 0: production */ + {0x02, 0x18, 0x62, 0x1d, 0x9c, 0x14, 0x47, 0x34, 0x58, 0x71, 0x3b, + 0xd3, 0xe6, 0x72, 0xe5, 0x34, 0x80, 0xaa, 0x70, 0x32, 0xca, 0x9b, + 0x67, 0x35, 0x63, 0x95, 0xe8, 0x87, 0x09, 0xbb, 0x45, 0x22, 0x6a}, + /* Key 1: rotation slot */ + {0x00}, + {0x00}, +#if DEBUG_LINK + /* Key 3: CI test key — only available in emulator/debug builds */ + {0x02, 0xe3, 0xb3, 0x01, 0x5c, 0x47, 0xdd, 0xca, 0xab, 0xe4, 0xf8, + 0xe8, 0x72, 0xf1, 0xed, 0x8f, 0x09, 0xca, 0x14, 0x5a, 0x8d, 0x81, + 0x77, 0x0d, 0x92, 0x21, 0x3d, 0x56, 0xda, 0x31, 0xab, 0x51, 0x07}, +#else + {0x00}, +#endif +}; + +static bool read_u8(const uint8_t **cursor, const uint8_t *end, uint8_t *out) { + if ((size_t)(end - *cursor) < 1) { + return false; + } + + *out = **cursor; + *cursor += 1; + return true; +} + +static bool read_be_u16(const uint8_t **cursor, const uint8_t *end, + uint16_t *out) { + if ((size_t)(end - *cursor) < 2) { + return false; + } + + *out = ((uint16_t)(*cursor)[0] << 8) | (*cursor)[1]; + *cursor += 2; + return true; +} + +static bool read_be_u32(const uint8_t **cursor, const uint8_t *end, + uint32_t *out) { + if ((size_t)(end - *cursor) < 4) { + return false; + } + + *out = ((uint32_t)(*cursor)[0] << 24) | ((uint32_t)(*cursor)[1] << 16) | + ((uint32_t)(*cursor)[2] << 8) | (*cursor)[3]; + *cursor += 4; + return true; +} + +static bool read_bytes(const uint8_t **cursor, const uint8_t *end, uint8_t *out, + size_t size) { + if ((size_t)(end - *cursor) < size) { + return false; + } + + memcpy(out, *cursor, size); + *cursor += size; + return true; +} + +static bool read_string(const uint8_t **cursor, const uint8_t *end, char *out, + size_t max_len) { + uint16_t value_len = 0; + if (!read_be_u16(cursor, end, &value_len) || value_len == 0 || + value_len > max_len || (size_t)(end - *cursor) < value_len) { + return false; + } + + memcpy(out, *cursor, value_len); + out[value_len] = '\0'; + *cursor += value_len; + return true; +} + +static bool read_arg_name(const uint8_t **cursor, const uint8_t *end, char *out, + size_t max_len) { + uint8_t value_len = 0; + if (!read_u8(cursor, end, &value_len) || value_len == 0 || + value_len > max_len || (size_t)(end - *cursor) < value_len) { + return false; + } + + memcpy(out, *cursor, value_len); + out[value_len] = '\0'; + *cursor += value_len; + return true; +} + +static bool parse_metadata_binary(const uint8_t *payload, size_t payload_len, + SignedMetadata *out) { + /* Minimum: version(1) + chain_id(4) + contract(20) + selector(4) + + * tx_hash(32) + method_len(2) + method(1) + num_args(1) + + * classification(1) + timestamp(4) + key_id(1) + sig(64) + recovery(1) + * = 136 bytes */ + if (payload_len < 136) { + return false; + } + + const uint8_t *cursor = payload; + const uint8_t *end = payload + payload_len; + memset(out, 0, sizeof(*out)); + + if (!read_u8(&cursor, end, &out->version) || out->version != 0x01 || + !read_be_u32(&cursor, end, &out->chain_id) || + !read_bytes(&cursor, end, out->contract_address, + sizeof(out->contract_address)) || + !read_bytes(&cursor, end, out->selector, sizeof(out->selector)) || + !read_bytes(&cursor, end, out->tx_hash, sizeof(out->tx_hash)) || + !read_string(&cursor, end, out->method_name, METADATA_MAX_METHOD_LEN) || + !read_u8(&cursor, end, &out->num_args) || + out->num_args > METADATA_MAX_ARGS) { + return false; + } + + for (uint8_t i = 0; i < out->num_args; i++) { + uint8_t format = 0; + uint16_t value_len = 0; + MetadataArg *arg = &out->args[i]; + + if (!read_arg_name(&cursor, end, arg->name, METADATA_MAX_ARG_NAME_LEN) || + !read_u8(&cursor, end, &format) || format > ARG_FORMAT_BYTES || + !read_be_u16(&cursor, end, &value_len) || + value_len > METADATA_MAX_ARG_VALUE_LEN || + !read_bytes(&cursor, end, arg->value, value_len)) { + return false; + } + + arg->format = (ArgFormat)format; + arg->value_len = value_len; + } + + uint8_t classification = 0; + if (!read_u8(&cursor, end, &classification) || classification > 2 || + !read_be_u32(&cursor, end, &out->timestamp) || + !read_u8(&cursor, end, &out->key_id) || + !read_bytes(&cursor, end, out->signature, sizeof(out->signature)) || + !read_u8(&cursor, end, &out->recovery) || cursor != end) { + return false; + } + + out->classification = (MetadataClassification)classification; + return true; +} + +static void bn_from_metadata_bytes(const uint8_t *value, size_t value_len, + bignum256 *out) { + uint8_t padded[32] = {0}; + if (value_len > sizeof(padded)) { + value_len = sizeof(padded); + } + memcpy(padded + (sizeof(padded) - value_len), value, value_len); + bn_read_be(padded, out); + memzero(padded, sizeof(padded)); +} + +bool signed_metadata_available(void) { return metadata_available; } + +void signed_metadata_clear(void) { + memzero(&stored_metadata, sizeof(stored_metadata)); + metadata_available = false; + relied_on_metadata = false; +} + +MetadataClassification signed_metadata_process(const uint8_t *payload, + size_t payload_len, + uint8_t key_id) { + uint8_t digest[32]; + size_t signed_len; + + signed_metadata_clear(); + + if (key_id >= METADATA_MAX_KEYS || METADATA_PUBKEYS[key_id][0] == 0x00 || + !payload || payload_len < 65) { + return METADATA_MALFORMED; + } + + if (!parse_metadata_binary(payload, payload_len, &stored_metadata) || + stored_metadata.key_id != key_id) { + signed_metadata_clear(); + return METADATA_MALFORMED; + } + + signed_len = payload_len - sizeof(stored_metadata.signature) - 1; + sha256_Raw(payload, signed_len, digest); + + if (ecdsa_verify_digest(&secp256k1, METADATA_PUBKEYS[key_id], + stored_metadata.signature, digest) != 0) { + signed_metadata_clear(); + return METADATA_MALFORMED; + } + + metadata_available = true; + return stored_metadata.classification; +} + +bool signed_metadata_matches_tx(const EthereumSignTx *msg) { + if (!metadata_available || !msg || + stored_metadata.classification != METADATA_VERIFIED || + msg->to.size != sizeof(stored_metadata.contract_address) || + msg->data_initial_chunk.size < sizeof(stored_metadata.selector)) { + return false; + } + + /* Contract address binding */ + if (memcmp(stored_metadata.contract_address, msg->to.bytes, + sizeof(stored_metadata.contract_address)) != 0) { + return false; + } + + /* Function selector binding */ + if (memcmp(stored_metadata.selector, msg->data_initial_chunk.bytes, + sizeof(stored_metadata.selector)) != 0) { + return false; + } + + /* Chain ID binding */ + if ((msg->has_chain_id ? msg->chain_id : 0) != stored_metadata.chain_id) { + return false; + } + + /* This only gates what we DISPLAY (so a benign-looking method screen can't + * be shown for the wrong call). The metadata commits to the full tx hash; + * that is enforced against the real signed digest in + * signed_metadata_enforce() because the digest does not exist until + * send_signature() finalizes it. */ + return true; +} + +bool signed_metadata_confirm(void) { + char body[128]; + + if (!metadata_available || + stored_metadata.classification != METADATA_VERIFIED) { + return false; + } + + /* Screen 1: Verified method — use review_with_icon for trust indicator */ + memset(body, 0, sizeof(body)); + snprintf(body, sizeof(body), "Verified call:\n%s", + stored_metadata.method_name); + if (!confirm_with_icon(ButtonRequestType_ButtonRequest_ConfirmOutput, + VERIFIED_ICON, "Insight Verified", "%s", body)) { + return false; + } + + /* Screen 2: Contract address — ALWAYS show full address, never truncate. + * Truncation is a spoofing vector (attacker crafts matching prefix+suffix). + */ + char contract_addr[43] = "0x"; + ethereum_address_checksum(stored_metadata.contract_address, contract_addr + 2, + false, stored_metadata.chain_id); + memset(body, 0, sizeof(body)); + snprintf(body, sizeof(body), "Contract:\n%s", contract_addr); + if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, + stored_metadata.method_name, "%s", body)) { + return false; + } + + /* Screen 3..N: Each decoded argument */ + for (uint8_t i = 0; i < stored_metadata.num_args; i++) { + MetadataArg *arg = &stored_metadata.args[i]; + memset(body, 0, sizeof(body)); + + switch (arg->format) { + case ARG_FORMAT_ADDRESS: { + char addr_full[43] = "0x"; + if (arg->value_len != 20) { + return false; + } + ethereum_address_checksum(arg->value, addr_full + 2, false, + stored_metadata.chain_id); + snprintf(body, sizeof(body), "%s:\n%s", arg->name, addr_full); + break; + } + case ARG_FORMAT_AMOUNT: { + bignum256 amount; + bn_from_metadata_bytes(arg->value, arg->value_len, &amount); + /* Check for MAX_UINT256 (unlimited approval) */ + bool is_max = true; + for (uint16_t j = 0; j < arg->value_len; j++) { + if (arg->value[j] != 0xFF) { + is_max = false; + break; + } + } + if (is_max && arg->value_len == 32) { + snprintf(body, sizeof(body), "%s:\nUNLIMITED", arg->name); + } else { + char formatted[48]; + bn_format(&amount, NULL, " wei", 0, 0, false, formatted, + sizeof(formatted)); + snprintf(body, sizeof(body), "%s:\n%s", arg->name, formatted); + } + break; + } + case ARG_FORMAT_BYTES: + case ARG_FORMAT_RAW: + default: { + char hex[(METADATA_MAX_ARG_VALUE_LEN * 2) + 1]; + size_t display_len = arg->value_len > 16 ? 16 : (size_t)arg->value_len; + data2hex(arg->value, display_len, hex); + snprintf(body, sizeof(body), "%s:\n%s%s", arg->name, hex, + arg->value_len > 16 ? "..." : ""); + break; + } + } + + if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, + stored_metadata.method_name, "%s", body)) { + return false; + } + } + + /* User approved the decoded who/what/why. From here the raw-data confirm is + * suppressed, so the signature MUST be bound to this metadata's tx hash. */ + relied_on_metadata = true; + return true; +} + +bool signed_metadata_relied(void) { return relied_on_metadata; } + +bool signed_metadata_enforce_decision(bool relied, bool available, + int classification, + const uint8_t *stored_hash, + const uint8_t *hash) { + if (!relied) { + return true; /* signature was not gated by metadata */ + } + /* Fail closed: relied on metadata but it's gone, not verified, or the signed + * digest differs from what was displayed → refuse to emit a signature. + * tx_hash is 32 bytes (see SignedMetadata). */ + return hash != NULL && stored_hash != NULL && available && + classification == METADATA_VERIFIED && + memcmp(stored_hash, hash, 32) == 0; +} + +bool signed_metadata_enforce(const uint8_t hash[32]) { + return signed_metadata_enforce_decision( + relied_on_metadata, metadata_available, stored_metadata.classification, + stored_metadata.tx_hash, hash); +} + +const SignedMetadata *signed_metadata_get(void) { + return metadata_available ? &stored_metadata : NULL; +} diff --git a/lib/firmware/storage.c b/lib/firmware/storage.c index c1cd77d12..81ab7d976 100644 --- a/lib/firmware/storage.c +++ b/lib/firmware/storage.c @@ -1866,6 +1866,10 @@ const uint8_t* storage_getSeed(const ConfigFlash* cfg, bool usePassphrase) { return NULL; } +const uint8_t* storage_getRawSeed(bool usePassphrase) { + return storage_getSeed(&shadow_config, usePassphrase); +} + bool storage_getRootNode(const char* curve, bool usePassphrase, HDNode* node) { // if storage has node, decrypt and use it if (shadow_config.storage.pub.has_node && diff --git a/unittests/firmware/CMakeLists.txt b/unittests/firmware/CMakeLists.txt index 647d72571..55a787f30 100644 --- a/unittests/firmware/CMakeLists.txt +++ b/unittests/firmware/CMakeLists.txt @@ -6,6 +6,7 @@ set(sources nano.cpp recovery.cpp ripple.cpp + signed_metadata.cpp storage.cpp usb_rx.cpp u2f.cpp) diff --git a/unittests/firmware/signed_metadata.cpp b/unittests/firmware/signed_metadata.cpp new file mode 100644 index 000000000..a2ccdddfc --- /dev/null +++ b/unittests/firmware/signed_metadata.cpp @@ -0,0 +1,646 @@ +/* + * Unit tests for the EVM clear-signing ("Insight") signed-metadata module. + * + * Requires an emulator/debug host build (KK_EMULATOR=ON, KK_DEBUG_LINK=ON): + * verification slot 3 (the CI test key 02e3b3015c...ab5107) only exists under + * `#if DEBUG_LINK`. All vectors are signed in-process with the matching private + * key (f6d19e15...068a260, whose compressed pubkey IS slot 3) and embed + * key_id=3, so a non-DEBUG build would reject them at the empty-slot guard. + * + * No OLED/button I/O is exercised: signed_metadata_process() and + * signed_metadata_matches_tx() never draw, and signed_metadata_confirm() is + * only called on its no-I/O early-return guards. The relied-path enforce truth + * table is tested through the pure, exported signed_metadata_enforce_decision() + * (see SECTION 2), since relied_on_metadata is only set inside confirm()'s + * interactive tail. + */ + +extern "C" { +#include "messages-ethereum.pb.h" /* full EthereumSignTx definition */ +#include "keepkey/firmware/signed_metadata.h" +#include "trezor/crypto/ecdsa.h" +#include "trezor/crypto/secp256k1.h" +#include "trezor/crypto/sha2.h" +} + +#include "gtest/gtest.h" + +#include +#include +#include +#include +#include + +namespace { + +/* Test signing key. Its compressed pubkey == firmware METADATA_PUBKEYS[3]. */ +const uint8_t TEST_PRIV[32] = { + 0xf6, 0xd1, 0x9e, 0x15, 0xa4, 0x38, 0x5f, 0x03, 0xb7, 0x8b, 0x5a, + 0x1e, 0x16, 0x14, 0xe7, 0xd9, 0xa1, 0x04, 0xd8, 0x1f, 0x73, 0x24, + 0x49, 0x87, 0x56, 0xe5, 0x71, 0x90, 0x40, 0x68, 0xa2, 0x60}; + +/* Expected compressed pubkey for slot 3 (DEBUG_LINK CI key). */ +const uint8_t EXPECTED_SLOT3_PUB[33] = { + 0x02, 0xe3, 0xb3, 0x01, 0x5c, 0x47, 0xdd, 0xca, 0xab, 0xe4, 0xf8, + 0xe8, 0x72, 0xf1, 0xed, 0x8f, 0x09, 0xca, 0x14, 0x5a, 0x8d, 0x81, + 0x77, 0x0d, 0x92, 0x21, 0x3d, 0x56, 0xda, 0x31, 0xab, 0x51, 0x07}; + +const uint8_t TEST_KEY_ID = 3; + +/* Deterministic, opaque test data. Only internal consistency matters. */ +const uint8_t CONTRACT_A[20] = {0xa0, 0xb8, 0x69, 0x91, 0xc6, 0x21, 0x8b, + 0x36, 0xc1, 0xd1, 0x9d, 0x4a, 0x2e, 0x9e, + 0xb0, 0xce, 0x36, 0x06, 0xeb, 0x48}; +const uint8_t CONTRACT_B[20] = {0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, + 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, + 0x11, 0x11, 0x11, 0x11, 0x11, 0x11}; +const uint8_t SEL_TRANSFER[4] = {0xa9, 0x05, 0x9c, 0xbb}; +const uint8_t SEL_APPROVE[4] = {0x09, 0x5e, 0xa7, 0xb3}; +const uint8_t TX_HASH[32] = { + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, + 0x0c, 0x0d, 0x0e, 0x0f, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, + 0x17, 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f, 0x20}; +const uint8_t RECIPIENT[20] = {0x40, 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, + 0x47, 0x48, 0x49, 0x4a, 0x4b, 0x4c, 0x4d, + 0x4e, 0x4f, 0x50, 0x51, 0x52, 0x53}; +const uint8_t AMOUNT32[32] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0x03, 0xe8}; + +/* ---- byte writers ------------------------------------------------------- */ + +void put_u8(std::vector &v, uint8_t x) { v.push_back(x); } +void put_be16(std::vector &v, uint16_t x) { + v.push_back((uint8_t)(x >> 8)); + v.push_back((uint8_t)(x & 0xff)); +} +void put_be32(std::vector &v, uint32_t x) { + v.push_back((uint8_t)(x >> 24)); + v.push_back((uint8_t)(x >> 16)); + v.push_back((uint8_t)(x >> 8)); + v.push_back((uint8_t)(x & 0xff)); +} +void put_bytes(std::vector &v, const uint8_t *b, size_t n) { + v.insert(v.end(), b, b + n); +} + +/* ---- metadata builder --------------------------------------------------- */ + +struct Arg { + std::string name; + uint8_t format; + std::vector value; + int value_len_override; // -1 => use value.size() +}; + +Arg mk_arg(const std::string &name, uint8_t format, const uint8_t *value, + size_t value_len) { + Arg a; + a.name = name; + a.format = format; + a.value.assign(value, value + value_len); + a.value_len_override = -1; + return a; +} + +struct Spec { + uint8_t version; + uint32_t chain_id; + std::vector contract; + std::vector selector; + std::vector tx_hash; + std::string method; + std::vector args; + uint8_t classification; + uint32_t timestamp; + uint8_t key_id; + int method_len_override; // -1 => use method.size() + int num_args_override; // -1 => use args.size() +}; + +/* Canonical VERIFIED metadata: transfer(to:ADDRESS, amount:AMOUNT) on chain 1. */ +Spec base_spec() { + Spec s; + s.version = 0x01; + s.chain_id = 1; + s.contract.assign(CONTRACT_A, CONTRACT_A + 20); + s.selector.assign(SEL_TRANSFER, SEL_TRANSFER + 4); + s.tx_hash.assign(TX_HASH, TX_HASH + 32); + s.method = "transfer"; + s.args.push_back(mk_arg("to", ARG_FORMAT_ADDRESS, RECIPIENT, 20)); + s.args.push_back(mk_arg("amount", ARG_FORMAT_AMOUNT, AMOUNT32, 32)); + s.classification = METADATA_VERIFIED; + s.timestamp = 0; + s.key_id = TEST_KEY_ID; + s.method_len_override = -1; + s.num_args_override = -1; + return s; +} + +/* Serialize the signed region (version .. key_id), exactly matching + * parse_metadata_binary() / serialize_metadata(). */ +std::vector build_body(const Spec &s) { + std::vector b; + put_u8(b, s.version); + put_be32(b, s.chain_id); + put_bytes(b, s.contract.data(), s.contract.size()); + put_bytes(b, s.selector.data(), s.selector.size()); + put_bytes(b, s.tx_hash.data(), s.tx_hash.size()); + + uint16_t mlen = s.method_len_override >= 0 ? (uint16_t)s.method_len_override + : (uint16_t)s.method.size(); + put_be16(b, mlen); + put_bytes(b, (const uint8_t *)s.method.data(), s.method.size()); + + uint8_t na = s.num_args_override >= 0 ? (uint8_t)s.num_args_override + : (uint8_t)s.args.size(); + put_u8(b, na); + for (const Arg &a : s.args) { + put_u8(b, (uint8_t)a.name.size()); + put_bytes(b, (const uint8_t *)a.name.data(), a.name.size()); + put_u8(b, a.format); + uint16_t vl = a.value_len_override >= 0 ? (uint16_t)a.value_len_override + : (uint16_t)a.value.size(); + put_be16(b, vl); + put_bytes(b, a.value.data(), a.value.size()); + } + + put_u8(b, s.classification); + put_be32(b, s.timestamp); + put_u8(b, s.key_id); + return b; +} + +/* sha256(body) -> ecdsa sign with TEST_PRIV -> append sig(64) + recovery(1). + * Mirrors signed_metadata_process(): signed_len = payload_len - 64 - 1. */ +std::vector sign_body(std::vector body) { + uint8_t digest[32]; + sha256_Raw(body.data(), body.size(), digest); + uint8_t sig[64]; + uint8_t pby = 0; + int rc = ecdsa_sign_digest(&secp256k1, TEST_PRIV, digest, sig, &pby, NULL); + EXPECT_EQ(rc, 0); + body.insert(body.end(), sig, sig + 64); + body.push_back((uint8_t)(27 + pby)); + return body; +} + +std::vector base_blob() { return sign_body(build_body(base_spec())); } + +void make_msg(EthereumSignTx *msg, const uint8_t contract[20], + const uint8_t *data, size_t data_len, bool has_chain, + uint32_t chain) { + memset(msg, 0, sizeof(*msg)); + msg->has_to = true; + msg->to.size = 20; + memcpy(msg->to.bytes, contract, 20); + msg->has_data_initial_chunk = true; + msg->data_initial_chunk.size = (pb_size_t)data_len; + memcpy(msg->data_initial_chunk.bytes, data, data_len); + msg->has_chain_id = has_chain; + msg->chain_id = chain; +} + +/* A standard transfer() calldata chunk that matches base_spec(). */ +void make_matching_msg(EthereumSignTx *msg) { + uint8_t data[68]; + memcpy(data, SEL_TRANSFER, 4); + memset(data + 4, 0, sizeof(data) - 4); + make_msg(msg, CONTRACT_A, data, sizeof(data), /*has_chain=*/true, 1); +} + +class SignedMetadataTest : public ::testing::Test { + protected: + void SetUp() override { signed_metadata_clear(); } + void TearDown() override { signed_metadata_clear(); } + + void ExpectMalformed(const std::vector &blob, uint8_t key_id) { + EXPECT_EQ(signed_metadata_process(blob.data(), blob.size(), key_id), + METADATA_MALFORMED); + EXPECT_FALSE(signed_metadata_available()); + EXPECT_EQ(signed_metadata_get(), nullptr); + } +}; + +/* ===================================================================== * + * signed_metadata_process — happy path + DEBUG_LINK slot 3 + * ===================================================================== */ + +TEST_F(SignedMetadataTest, DerivedPubkeyMatchesSlot3) { + uint8_t pub[33]; + ecdsa_get_public_key33(&secp256k1, TEST_PRIV, pub); + EXPECT_EQ(memcmp(pub, EXPECTED_SLOT3_PUB, sizeof(pub)), 0) + << "TEST_PRIV must derive firmware METADATA_PUBKEYS[3]"; +} + +TEST_F(SignedMetadataTest, ValidVerifiedSlot3) { + std::vector blob = base_blob(); + EXPECT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EXPECT_TRUE(signed_metadata_available()); + const SignedMetadata *m = signed_metadata_get(); + ASSERT_NE(m, nullptr); + EXPECT_EQ(m->classification, METADATA_VERIFIED); + EXPECT_EQ(m->chain_id, 1u); + EXPECT_STREQ(m->method_name, "transfer"); + EXPECT_EQ(m->num_args, 2); + EXPECT_EQ(memcmp(m->contract_address, CONTRACT_A, 20), 0); + EXPECT_EQ(memcmp(m->selector, SEL_TRANSFER, 4), 0); + EXPECT_EQ(memcmp(m->tx_hash, TX_HASH, 32), 0); + EXPECT_EQ(m->key_id, TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, ValidOpaqueClassification) { + Spec s = base_spec(); + s.classification = METADATA_OPAQUE; // 0 + std::vector blob = sign_body(build_body(s)); + EXPECT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_OPAQUE); + EXPECT_TRUE(signed_metadata_available()); // available, but not VERIFIED + EXPECT_NE(signed_metadata_get(), nullptr); +} + +TEST_F(SignedMetadataTest, SelfDeclaredMalformedWithValidSignature) { + /* A trusted signer can self-declare MALFORMED(2). Signature verifies, so + * process() returns MALFORMED but leaves the (inert) metadata available. It + * must never be displayed or relied upon. */ + Spec s = base_spec(); + s.classification = METADATA_MALFORMED; // 2 + std::vector blob = sign_body(build_body(s)); + EXPECT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_MALFORMED); + EXPECT_TRUE(signed_metadata_available()); + EXPECT_NE(signed_metadata_get(), nullptr); + + EthereumSignTx msg; + make_matching_msg(&msg); + EXPECT_FALSE(signed_metadata_matches_tx(&msg)); // gated on VERIFIED + EXPECT_FALSE(signed_metadata_confirm()); // gated on VERIFIED +} + +/* ===================================================================== * + * signed_metadata_process — key-slot guards + * ===================================================================== */ + +TEST_F(SignedMetadataTest, KeyIdOutOfRange) { + ExpectMalformed(base_blob(), /*key_id=*/4); // >= METADATA_MAX_KEYS +} + +TEST_F(SignedMetadataTest, EmptyRotationSlot) { + Spec s = base_spec(); + s.key_id = 1; // slot 1 pubkey == {0x00} + ExpectMalformed(sign_body(build_body(s)), /*key_id=*/1); +} + +TEST_F(SignedMetadataTest, NullPayload) { + EXPECT_EQ(signed_metadata_process(nullptr, 200, TEST_KEY_ID), + METADATA_MALFORMED); + EXPECT_FALSE(signed_metadata_available()); + EXPECT_EQ(signed_metadata_get(), nullptr); +} + +TEST_F(SignedMetadataTest, EmbeddedKeyIdMismatch) { + Spec s = base_spec(); + s.key_id = 2; // embedded != protocol key_id (3) + ExpectMalformed(sign_body(build_body(s)), /*key_id=*/3); +} + +TEST_F(SignedMetadataTest, SignatureVerificationFails) { + std::vector blob = base_blob(); + blob[146] ^= 0x01; // flip first signature byte (sig starts after 146B body) + ExpectMalformed(blob, TEST_KEY_ID); +} + +/* ===================================================================== * + * signed_metadata_process — length guards + * ===================================================================== */ + +TEST_F(SignedMetadataTest, PayloadShorterThan65) { + std::vector blob = base_blob(); + blob.resize(64); // process() early guard: payload_len < 65 + ExpectMalformed(blob, TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, PayloadBetween65And135) { + std::vector blob = base_blob(); + blob.resize(100); // passes <65 guard, fails parser <136 minimum + ExpectMalformed(blob, TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, TrailingByteAfterRecovery) { + std::vector blob = base_blob(); + blob.push_back(0x00); // cursor != end + ExpectMalformed(blob, TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, MissingRecoveryByte) { + std::vector blob = base_blob(); + blob.pop_back(); // truncated tail: read recovery fails + ExpectMalformed(blob, TEST_KEY_ID); +} + +/* ===================================================================== * + * parse_metadata_binary — field guards (all re-signed so the PARSE guard, + * not the signature check, is what rejects the blob) + * ===================================================================== */ + +TEST_F(SignedMetadataTest, BadVersion) { + Spec s = base_spec(); + s.version = 0x02; + ExpectMalformed(sign_body(build_body(s)), TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, EmptyMethodName) { + Spec s = base_spec(); + s.method = ""; // 2-byte length prefix == 0 + ExpectMalformed(sign_body(build_body(s)), TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, MethodNameTooLong) { + Spec s = base_spec(); + s.method = std::string(65, 'A'); // > METADATA_MAX_METHOD_LEN (64) + ExpectMalformed(sign_body(build_body(s)), TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, MethodNameLengthOverrun) { + /* Length prefix claims 64 but only "transfer" (8B) is the method; the read + * consumes downstream bytes and parsing misaligns -> MALFORMED. The clean + * read_string short-read guard is unreachable under the >=136 floor (after + * the 63-byte fixed prefix at least 73 bytes always remain), so this pins + * the observable contract rather than a specific internal branch. The + * corrupted signature byte makes rejection deterministic even in the + * vanishingly unlikely event the misaligned parse re-aligns to the end. */ + Spec s = base_spec(); + s.method_len_override = 64; + std::vector blob = sign_body(build_body(s)); + blob[blob.size() - 2] ^= 0xFF; // ensure verify cannot pass + ExpectMalformed(blob, TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, NumArgsTooMany) { + Spec s = base_spec(); + s.num_args_override = 9; // > METADATA_MAX_ARGS (8) + ExpectMalformed(sign_body(build_body(s)), TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, ArgNameEmpty) { + Spec s = base_spec(); + s.args[0] = mk_arg("", ARG_FORMAT_ADDRESS, RECIPIENT, 20); // name_len == 0 + ExpectMalformed(sign_body(build_body(s)), TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, ArgNameTooLong) { + Spec s = base_spec(); + std::string long_name(33, 'x'); // > METADATA_MAX_ARG_NAME_LEN (32) + s.args[0] = mk_arg(long_name, ARG_FORMAT_ADDRESS, RECIPIENT, 20); + ExpectMalformed(sign_body(build_body(s)), TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, ArgFormatOutOfRange) { + Spec s = base_spec(); + s.args[0].format = 4; // > ARG_FORMAT_BYTES (3) + ExpectMalformed(sign_body(build_body(s)), TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, ArgValueTooLong) { + Spec s = base_spec(); + uint8_t big[33] = {0}; + s.args[0] = mk_arg("to", ARG_FORMAT_BYTES, big, 33); // > 32 + ExpectMalformed(sign_body(build_body(s)), TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, ArgValueLengthOverrun) { + /* value_len prefix claims 32 but only 4 value bytes follow; the read eats + * into the fixed tail and parsing misaligns -> MALFORMED. As with the method + * case, the read_bytes short-read guard is dominated by the >=71-byte fixed + * tail, so this asserts the observable MALFORMED outcome. */ + Spec s = base_spec(); + uint8_t four[4] = {0xde, 0xad, 0xbe, 0xef}; + Arg a = mk_arg("amount", ARG_FORMAT_AMOUNT, four, 4); + a.value_len_override = 32; + s.args[1] = a; + std::vector blob = sign_body(build_body(s)); + blob[blob.size() - 2] ^= 0xFF; // ensure verify cannot pass + ExpectMalformed(blob, TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, ClassificationOutOfRange) { + Spec s = base_spec(); + s.classification = 3; // > 2 + ExpectMalformed(sign_body(build_body(s)), TEST_KEY_ID); +} + +/* ===================================================================== * + * signed_metadata_matches_tx — display gate + * ===================================================================== */ + +TEST_F(SignedMetadataTest, MatchesTxAllBindingsMatch) { + std::vector blob = base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EthereumSignTx msg; + make_matching_msg(&msg); + EXPECT_TRUE(signed_metadata_matches_tx(&msg)); +} + +TEST_F(SignedMetadataTest, MatchesTxNotAvailable) { + signed_metadata_clear(); + EthereumSignTx msg; + make_matching_msg(&msg); + EXPECT_FALSE(signed_metadata_matches_tx(&msg)); +} + +TEST_F(SignedMetadataTest, MatchesTxNullMsg) { + std::vector blob = base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EXPECT_FALSE(signed_metadata_matches_tx(nullptr)); +} + +TEST_F(SignedMetadataTest, MatchesTxNotVerifiedClassification) { + Spec s = base_spec(); + s.classification = METADATA_OPAQUE; + std::vector blob = sign_body(build_body(s)); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_OPAQUE); + EthereumSignTx msg; + make_matching_msg(&msg); + EXPECT_FALSE(signed_metadata_matches_tx(&msg)); +} + +TEST_F(SignedMetadataTest, MatchesTxWrongToSize) { + std::vector blob = base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EthereumSignTx msg; + make_matching_msg(&msg); + msg.to.size = 19; // not 20 (e.g. contract-create has 0) + EXPECT_FALSE(signed_metadata_matches_tx(&msg)); +} + +TEST_F(SignedMetadataTest, MatchesTxDataTooShortForSelector) { + std::vector blob = base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EthereumSignTx msg; + make_matching_msg(&msg); + msg.data_initial_chunk.size = 3; // < 4 + EXPECT_FALSE(signed_metadata_matches_tx(&msg)); +} + +TEST_F(SignedMetadataTest, MatchesTxWrongContract) { + std::vector blob = base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + uint8_t data[68]; + memcpy(data, SEL_TRANSFER, 4); + memset(data + 4, 0, sizeof(data) - 4); + EthereumSignTx msg; + make_msg(&msg, CONTRACT_B, data, sizeof(data), true, 1); + EXPECT_FALSE(signed_metadata_matches_tx(&msg)); +} + +TEST_F(SignedMetadataTest, MatchesTxWrongSelector) { + std::vector blob = base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + uint8_t data[68]; + memcpy(data, SEL_APPROVE, 4); // approve, not transfer + memset(data + 4, 0, sizeof(data) - 4); + EthereumSignTx msg; + make_msg(&msg, CONTRACT_A, data, sizeof(data), true, 1); + EXPECT_FALSE(signed_metadata_matches_tx(&msg)); +} + +TEST_F(SignedMetadataTest, MatchesTxWrongChainId) { + std::vector blob = base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + uint8_t data[68]; + memcpy(data, SEL_TRANSFER, 4); + memset(data + 4, 0, sizeof(data) - 4); + + EthereumSignTx wrong_chain; + make_msg(&wrong_chain, CONTRACT_A, data, sizeof(data), true, 137); + EXPECT_FALSE(signed_metadata_matches_tx(&wrong_chain)); + + EthereumSignTx no_chain; + make_msg(&no_chain, CONTRACT_A, data, sizeof(data), false, 0); // treated as 0 + EXPECT_FALSE(signed_metadata_matches_tx(&no_chain)); +} + +/* ===================================================================== * + * signed_metadata_confirm — no-I/O early guards + * ===================================================================== */ + +TEST_F(SignedMetadataTest, ConfirmNotAvailable) { + signed_metadata_clear(); + EXPECT_FALSE(signed_metadata_confirm()); +} + +TEST_F(SignedMetadataTest, ConfirmNotVerified) { + Spec s = base_spec(); + s.classification = METADATA_OPAQUE; + std::vector blob = sign_body(build_body(s)); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_OPAQUE); + EXPECT_FALSE(signed_metadata_confirm()); +} + +/* ===================================================================== * + * signed_metadata_enforce — module-level not-relied path (reachable + * without confirm()'s interactive tail) and clear() reset + * ===================================================================== */ + +TEST_F(SignedMetadataTest, EnforceNotReliedAlwaysAllows) { + std::vector blob = base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + ASSERT_FALSE(signed_metadata_relied()); // process() never sets relied + + uint8_t wrong[32]; + memcpy(wrong, TX_HASH, 32); + wrong[0] ^= 0xFF; + EXPECT_TRUE(signed_metadata_enforce(TX_HASH)); + EXPECT_TRUE(signed_metadata_enforce(wrong)); + EXPECT_TRUE(signed_metadata_enforce(nullptr)); +} + +TEST_F(SignedMetadataTest, ClearResetsAllState) { + std::vector blob = base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + ASSERT_TRUE(signed_metadata_available()); + + signed_metadata_clear(); + EXPECT_FALSE(signed_metadata_available()); + EXPECT_FALSE(signed_metadata_relied()); + EXPECT_EQ(signed_metadata_get(), nullptr); + EXPECT_TRUE(signed_metadata_enforce(TX_HASH)); // not relied +} + +/* ===================================================================== * + * signed_metadata_enforce_decision — pure enforce truth table (SECTION 2). + * Exercises the relied==true cases that confirm()'s OLED/button I/O makes + * unreachable from the module-state API in a unit test. + * ===================================================================== */ + +TEST(SignedMetadataEnforce, NotReliedAlwaysAllow) { + uint8_t h[32] = {0}; + uint8_t hw[32] = {1}; + EXPECT_TRUE(signed_metadata_enforce_decision(false, true, METADATA_VERIFIED, + h, h)); + EXPECT_TRUE(signed_metadata_enforce_decision(false, false, METADATA_OPAQUE, + nullptr, nullptr)); + EXPECT_TRUE(signed_metadata_enforce_decision(false, true, METADATA_VERIFIED, + h, hw)); +} + +TEST(SignedMetadataEnforce, ReliedHashMatches) { + uint8_t h[32]; + memcpy(h, TX_HASH, 32); + EXPECT_TRUE(signed_metadata_enforce_decision(true, true, METADATA_VERIFIED, + h, h)); +} + +TEST(SignedMetadataEnforce, ReliedHashMismatch) { + uint8_t stored[32]; + memcpy(stored, TX_HASH, 32); + uint8_t got[32]; + memcpy(got, TX_HASH, 32); + got[0] ^= 0x01; + EXPECT_FALSE(signed_metadata_enforce_decision(true, true, METADATA_VERIFIED, + stored, got)); +} + +TEST(SignedMetadataEnforce, ReliedHashNull) { + uint8_t stored[32]; + memcpy(stored, TX_HASH, 32); + EXPECT_FALSE(signed_metadata_enforce_decision(true, true, METADATA_VERIFIED, + stored, nullptr)); +} + +TEST(SignedMetadataEnforce, ReliedNotAvailable) { + uint8_t h[32]; + memcpy(h, TX_HASH, 32); + EXPECT_FALSE(signed_metadata_enforce_decision(true, false, METADATA_VERIFIED, + h, h)); +} + +TEST(SignedMetadataEnforce, ReliedNotVerified) { + uint8_t h[32]; + memcpy(h, TX_HASH, 32); + EXPECT_FALSE(signed_metadata_enforce_decision(true, true, METADATA_OPAQUE, + h, h)); + EXPECT_FALSE(signed_metadata_enforce_decision(true, true, METADATA_MALFORMED, + h, h)); +} + +TEST(SignedMetadataEnforce, ReliedStoredHashNull) { + uint8_t h[32]; + memcpy(h, TX_HASH, 32); + EXPECT_FALSE(signed_metadata_enforce_decision(true, true, METADATA_VERIFIED, + nullptr, h)); +} + +} // namespace From 9c72ec7947a059e4cf0ef993caf3073f47be4d66 Mon Sep 17 00:00:00 2001 From: highlander Date: Thu, 2 Jul 2026 02:30:55 -0500 Subject: [PATCH 16/25] feat(clearsign): runtime-loaded signers with mandatory warning screen (phase 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (cherry picked from commit aee9c95d977be35532f0ad77fde0e561edf1c9a4) --- include/keepkey/firmware/fsm.h | 1 + include/keepkey/firmware/signed_metadata.h | 39 ++++ .../transport/messages-ethereum.options | 2 + lib/firmware/fsm_msg_ethereum.h | 33 ++++ lib/firmware/messagemap.def | 1 + lib/firmware/signed_metadata.c | 169 ++++++++++++++---- unittests/firmware/signed_metadata.cpp | 139 ++++++++++++-- 7 files changed, 342 insertions(+), 42 deletions(-) diff --git a/include/keepkey/firmware/fsm.h b/include/keepkey/firmware/fsm.h index b112181ff..4de0813f6 100644 --- a/include/keepkey/firmware/fsm.h +++ b/include/keepkey/firmware/fsm.h @@ -86,6 +86,7 @@ void fsm_msgEthereumVerifyMessage(const EthereumVerifyMessage* msg); void fsm_msgEthereumSignTypedHash(const EthereumSignTypedHash* msg); void fsm_msgEthereum712TypesValues(Ethereum712TypesValues* msg); void fsm_msgEthereumTxMetadata(const EthereumTxMetadata* msg); +void fsm_msgLoadClearsignSigner(const LoadClearsignSigner* msg); void fsm_msgNanoGetAddress(NanoGetAddress* msg); void fsm_msgNanoSignTx(NanoSignTx* msg); diff --git a/include/keepkey/firmware/signed_metadata.h b/include/keepkey/firmware/signed_metadata.h index 4e0ca55c1..6138db39b 100644 --- a/include/keepkey/firmware/signed_metadata.h +++ b/include/keepkey/firmware/signed_metadata.h @@ -12,6 +12,9 @@ typedef struct _EthereumSignTx EthereumSignTx; #define METADATA_MAX_ARG_NAME_LEN 32 #define METADATA_MAX_ARG_VALUE_LEN 32 #define METADATA_MAX_KEYS 4 +#define METADATA_ALIAS_MAX_LEN 31 +/* hex(first 4 bytes of sha256(pubkey)) + NUL */ +#define METADATA_FINGERPRINT_LEN 9 typedef enum { METADATA_OPAQUE = 0, @@ -51,6 +54,42 @@ typedef struct { bool signed_metadata_available(void); void signed_metadata_clear(void); + +/* + * Runtime-loaded clearsign signers (phase 1: the ONLY verification path). + * + * A signer is a compressed secp256k1 pubkey + display alias loaded into a + * key slot at the host's request, gated by a mandatory on-device confirm + * (see fsm_msgLoadClearsignSigner). Loaded signers live in RAM only and are + * gone on reboot. Metadata verified by a loaded signer always shows a + * warning screen naming the alias before any clearsign page — only the + * built-in (phase 2) keys sign warning-free. + */ + +/* Pure validation: slot in range and not occupied by a built-in key, pubkey a + * valid compressed secp256k1 point, alias non-empty printable ASCII within + * METADATA_ALIAS_MAX_LEN. No state, no I/O. */ +bool signed_metadata_signer_valid(uint8_t key_id, const uint8_t *pubkey, + size_t pubkey_len, const char *alias); + +/* Store a signer into a slot. Caller (the FSM handler) MUST have passed + * signed_metadata_signer_valid() and obtained on-device user confirmation + * first — this function is the post-consent write, nothing more. */ +void signed_metadata_store_signer(uint8_t key_id, const uint8_t *pubkey, + const char *alias); + +/* Drop all runtime-loaded signers (and any metadata they verified). */ +void signed_metadata_clear_signers(void); + +/* out = hex of the first 4 bytes of sha256(pubkey[33]), NUL-terminated. + * Shown at load-confirm and on the per-tx warning screen so the user can + * correlate the two. */ +void signed_metadata_pubkey_fingerprint(const uint8_t pubkey[33], + char out[METADATA_FINGERPRINT_LEN]); + +/* True when the currently stored metadata was verified by a runtime-loaded + * signer (=> its confirm flow is warning-first, never "Insight Verified"). */ +bool signed_metadata_from_loaded_signer(void); MetadataClassification signed_metadata_process(const uint8_t *payload, size_t payload_len, uint8_t key_id); diff --git a/include/keepkey/transport/messages-ethereum.options b/include/keepkey/transport/messages-ethereum.options index 65b6a1a2f..eec9eee40 100644 --- a/include/keepkey/transport/messages-ethereum.options +++ b/include/keepkey/transport/messages-ethereum.options @@ -49,4 +49,6 @@ Ethereum712TypesValues.eip712data max_size:2048 EthereumTxMetadata.signed_payload max_size:1024 EthereumMetadataAck.display_summary max_size:32 +LoadClearsignSigner.pubkey max_size:33 +LoadClearsignSigner.alias max_size:32 diff --git a/lib/firmware/fsm_msg_ethereum.h b/lib/firmware/fsm_msg_ethereum.h index 69031c4a6..47e99e3ec 100644 --- a/lib/firmware/fsm_msg_ethereum.h +++ b/lib/firmware/fsm_msg_ethereum.h @@ -51,6 +51,39 @@ void fsm_msgEthereumTxMetadata(const EthereumTxMetadata* msg) { msg_write(MessageType_MessageType_EthereumMetadataAck, resp); } +void fsm_msgLoadClearsignSigner(const LoadClearsignSigner* msg) { + CHECK_INITIALIZED + CHECK_PIN + + CHECK_PARAM(msg->has_key_id && msg->has_pubkey && msg->has_alias, + _("key_id, pubkey and alias required")); + /* Range-check as uint32 BEFORE narrowing: (uint8_t)256 would alias slot 0 */ + CHECK_PARAM(msg->key_id < METADATA_MAX_KEYS, _("key_id out of range")); + CHECK_PARAM(signed_metadata_signer_valid((uint8_t)msg->key_id, + msg->pubkey.bytes, msg->pubkey.size, + msg->alias), + _("Invalid clearsign signer")); + + /* Mandatory on-device consent — the whole trust model hangs on this + * confirm. The same fingerprint reappears on every per-tx warning. */ + char fingerprint[METADATA_FINGERPRINT_LEN]; + signed_metadata_pubkey_fingerprint(msg->pubkey.bytes, fingerprint); + if (!confirm(ButtonRequestType_ButtonRequest_Other, _("Load Clearsigner"), + "Trust signer '%s' (%s) to describe transactions? NOT verified " + "by KeepKey.", + msg->alias, fingerprint)) { + fsm_sendFailure(FailureType_Failure_ActionCancelled, + _("Load clearsign signer cancelled")); + layoutHome(); + return; + } + + signed_metadata_store_signer((uint8_t)msg->key_id, msg->pubkey.bytes, + msg->alias); + fsm_sendSuccess(_("Clearsign signer loaded")); + layoutHome(); +} + static int process_ethereum_xfer(const CoinType* coin, EthereumSignTx* msg) { if (!ethereum_isStandardERC20Transfer(msg) && msg->data_length != 0) return TXOUT_COMPILE_ERROR; diff --git a/lib/firmware/messagemap.def b/lib/firmware/messagemap.def index 719995f7b..42add93e0 100644 --- a/lib/firmware/messagemap.def +++ b/lib/firmware/messagemap.def @@ -181,3 +181,4 @@ MSG_IN(MessageType_MessageType_EthereumTxMetadata, EthereumTxMetadata, fsm_msgEthereumTxMetadata) MSG_OUT(MessageType_MessageType_EthereumMetadataAck, EthereumMetadataAck, NO_PROCESS_FUNC) + MSG_IN(MessageType_MessageType_LoadClearsignSigner, LoadClearsignSigner, fsm_msgLoadClearsignSigner) diff --git a/lib/firmware/signed_metadata.c b/lib/firmware/signed_metadata.c index b18974e8c..20ab28008 100644 --- a/lib/firmware/signed_metadata.c +++ b/lib/firmware/signed_metadata.c @@ -17,39 +17,38 @@ static bool metadata_available = false; static bool relied_on_metadata = false; +static bool metadata_signer_loaded = false; static SignedMetadata stored_metadata; /* - * Metadata verification public keys. - * Slot 0: active production key - * Slot 1: rotation target - * Slots 2-3: reserved + * Built-in metadata verification public keys. * - * Keys are derived via KeepKey SignIdentity at keepkey.com/insight. - * Only the public key is stored here — the signing mnemonic is held - * offline and never appears in source code. + * Phase 1 ships with NO built-in keys: every clearsign signer must be + * loaded at runtime via LoadClearsignSigner (user-confirmed on device, + * RAM-only, gone on reboot), and every transaction it vouches for is + * preceded by a warning screen naming the signer's alias. There is no + * hardcoded "KeepKey says this is safe" path in this phase. * - * To rotate: generate new key with pioneer-insight keygen, - * replace the slot below, ship firmware update. + * Phase 2 (once the offline signing infrastructure is hardened) puts the + * KeepKey production key back in slot 0. A slot holding a built-in key can + * never be shadowed by a loaded signer (see signed_metadata_signer_valid) + * and is the only path that clearsigns without the warning screen. */ static const uint8_t METADATA_PUBKEYS[METADATA_MAX_KEYS][33] = { - /* Key 0: production */ - {0x02, 0x18, 0x62, 0x1d, 0x9c, 0x14, 0x47, 0x34, 0x58, 0x71, 0x3b, - 0xd3, 0xe6, 0x72, 0xe5, 0x34, 0x80, 0xaa, 0x70, 0x32, 0xca, 0x9b, - 0x67, 0x35, 0x63, 0x95, 0xe8, 0x87, 0x09, 0xbb, 0x45, 0x22, 0x6a}, + /* Key 0: production (returns in phase 2) */ + {0x00}, /* Key 1: rotation slot */ {0x00}, {0x00}, -#if DEBUG_LINK - /* Key 3: CI test key — only available in emulator/debug builds */ - {0x02, 0xe3, 0xb3, 0x01, 0x5c, 0x47, 0xdd, 0xca, 0xab, 0xe4, 0xf8, - 0xe8, 0x72, 0xf1, 0xed, 0x8f, 0x09, 0xca, 0x14, 0x5a, 0x8d, 0x81, - 0x77, 0x0d, 0x92, 0x21, 0x3d, 0x56, 0xda, 0x31, 0xab, 0x51, 0x07}, -#else + /* Key 3: was the DEBUG_LINK-only CI test key; CI now loads it through + * LoadClearsignSigner so tests exercise the same warning flow users see. */ {0x00}, -#endif }; +/* Runtime-loaded signers. RAM only — cleared on reboot by construction. */ +static uint8_t loaded_pubkeys[METADATA_MAX_KEYS][33]; +static char loaded_aliases[METADATA_MAX_KEYS][METADATA_ALIAS_MAX_LEN + 1]; + static bool read_u8(const uint8_t **cursor, const uint8_t *end, uint8_t *out) { if ((size_t)(end - *cursor) < 1) { return false; @@ -195,6 +194,85 @@ void signed_metadata_clear(void) { memzero(&stored_metadata, sizeof(stored_metadata)); metadata_available = false; relied_on_metadata = false; + metadata_signer_loaded = false; +} + +void signed_metadata_clear_signers(void) { + memzero(loaded_pubkeys, sizeof(loaded_pubkeys)); + memzero(loaded_aliases, sizeof(loaded_aliases)); + /* Metadata verified by a now-dropped signer must not outlive it. */ + signed_metadata_clear(); +} + +bool signed_metadata_signer_valid(uint8_t key_id, const uint8_t *pubkey, + size_t pubkey_len, const char *alias) { + curve_point point; + size_t alias_len; + + if (key_id >= METADATA_MAX_KEYS || METADATA_PUBKEYS[key_id][0] != 0x00 || + !pubkey || pubkey_len != 33 || !alias) { + return false; + } + + /* Alias is rendered through confirm() format strings on the load screen and + * the per-tx warning — printable ASCII only, no control/spoofing chars. */ + alias_len = strlen(alias); + if (alias_len == 0 || alias_len > METADATA_ALIAS_MAX_LEN) { + return false; + } + for (size_t i = 0; i < alias_len; i++) { + if (alias[i] < 0x20 || alias[i] > 0x7e || alias[i] == '%') { + return false; + } + } + + /* Compressed form only — ecdsa_read_pubkey would read 65 bytes for an + * uncompressed 0x04 prefix, past our 33-byte buffer. Requiring 0x02/0x03 + * also excludes the all-zero "empty slot" sentinel. */ + if (pubkey[0] != 0x02 && pubkey[0] != 0x03) { + return false; + } + return ecdsa_read_pubkey(&secp256k1, pubkey, &point) == 1; +} + +void signed_metadata_store_signer(uint8_t key_id, const uint8_t *pubkey, + const char *alias) { + if (key_id >= METADATA_MAX_KEYS) { + return; + } + memcpy(loaded_pubkeys[key_id], pubkey, sizeof(loaded_pubkeys[key_id])); + strlcpy(loaded_aliases[key_id], alias, sizeof(loaded_aliases[key_id])); + /* Replacing a signer invalidates anything the old one verified. */ + signed_metadata_clear(); +} + +void signed_metadata_pubkey_fingerprint( + const uint8_t pubkey[33], char out[METADATA_FINGERPRINT_LEN]) { + uint8_t digest[32]; + sha256_Raw(pubkey, 33, digest); + data2hex(digest, 4, out); + memzero(digest, sizeof(digest)); +} + +bool signed_metadata_from_loaded_signer(void) { + return metadata_available && metadata_signer_loaded; +} + +/* Resolve the verification key for a slot. Built-in keys win; a runtime- + * loaded signer can only occupy an empty built-in slot. */ +static const uint8_t *metadata_pubkey_for(uint8_t key_id, bool *is_loaded) { + *is_loaded = false; + if (key_id >= METADATA_MAX_KEYS) { + return NULL; + } + if (METADATA_PUBKEYS[key_id][0] != 0x00) { + return METADATA_PUBKEYS[key_id]; + } + if (loaded_pubkeys[key_id][0] != 0x00) { + *is_loaded = true; + return loaded_pubkeys[key_id]; + } + return NULL; } MetadataClassification signed_metadata_process(const uint8_t *payload, @@ -202,11 +280,13 @@ MetadataClassification signed_metadata_process(const uint8_t *payload, uint8_t key_id) { uint8_t digest[32]; size_t signed_len; + bool is_loaded = false; + const uint8_t *pubkey; signed_metadata_clear(); - if (key_id >= METADATA_MAX_KEYS || METADATA_PUBKEYS[key_id][0] == 0x00 || - !payload || payload_len < 65) { + pubkey = metadata_pubkey_for(key_id, &is_loaded); + if (!pubkey || !payload || payload_len < 65) { return METADATA_MALFORMED; } @@ -219,13 +299,14 @@ MetadataClassification signed_metadata_process(const uint8_t *payload, signed_len = payload_len - sizeof(stored_metadata.signature) - 1; sha256_Raw(payload, signed_len, digest); - if (ecdsa_verify_digest(&secp256k1, METADATA_PUBKEYS[key_id], - stored_metadata.signature, digest) != 0) { + if (ecdsa_verify_digest(&secp256k1, pubkey, stored_metadata.signature, + digest) != 0) { signed_metadata_clear(); return METADATA_MALFORMED; } metadata_available = true; + metadata_signer_loaded = is_loaded; return stored_metadata.classification; } @@ -270,13 +351,39 @@ bool signed_metadata_confirm(void) { return false; } - /* Screen 1: Verified method — use review_with_icon for trust indicator */ - memset(body, 0, sizeof(body)); - snprintf(body, sizeof(body), "Verified call:\n%s", - stored_metadata.method_name); - if (!confirm_with_icon(ButtonRequestType_ButtonRequest_ConfirmOutput, - VERIFIED_ICON, "Insight Verified", "%s", body)) { - return false; + if (metadata_signer_loaded) { + /* Warning screen FIRST, before any clearsign page: the decode below is + * vouched for by a signer the user loaded, not by KeepKey. */ + char fingerprint[METADATA_FINGERPRINT_LEN]; + signed_metadata_pubkey_fingerprint(loaded_pubkeys[stored_metadata.key_id], + fingerprint); + memset(body, 0, sizeof(body)); + snprintf(body, sizeof(body), + "Signer '%s' (%s) you loaded describes this tx. NOT verified by " + "KeepKey.", + loaded_aliases[stored_metadata.key_id], fingerprint); + if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, + "Clearsign Warning", "%s", body)) { + return false; + } + + /* Method screen without the "Insight Verified" branding/icon — that + * presentation is reserved for the built-in (phase 2) keys. */ + memset(body, 0, sizeof(body)); + snprintf(body, sizeof(body), "Call:\n%s", stored_metadata.method_name); + if (!confirm(ButtonRequestType_ButtonRequest_ConfirmOutput, "Clearsign", + "%s", body)) { + return false; + } + } else { + /* Screen 1: Verified method — use review_with_icon for trust indicator */ + memset(body, 0, sizeof(body)); + snprintf(body, sizeof(body), "Verified call:\n%s", + stored_metadata.method_name); + if (!confirm_with_icon(ButtonRequestType_ButtonRequest_ConfirmOutput, + VERIFIED_ICON, "Insight Verified", "%s", body)) { + return false; + } } /* Screen 2: Contract address — ALWAYS show full address, never truncate. diff --git a/unittests/firmware/signed_metadata.cpp b/unittests/firmware/signed_metadata.cpp index a2ccdddfc..bfdc7ba08 100644 --- a/unittests/firmware/signed_metadata.cpp +++ b/unittests/firmware/signed_metadata.cpp @@ -1,11 +1,12 @@ /* * Unit tests for the EVM clear-signing ("Insight") signed-metadata module. * - * Requires an emulator/debug host build (KK_EMULATOR=ON, KK_DEBUG_LINK=ON): - * verification slot 3 (the CI test key 02e3b3015c...ab5107) only exists under - * `#if DEBUG_LINK`. All vectors are signed in-process with the matching private - * key (f6d19e15...068a260, whose compressed pubkey IS slot 3) and embed - * key_id=3, so a non-DEBUG build would reject them at the empty-slot guard. + * Phase 1 ships with NO built-in verification keys: METADATA_PUBKEYS is all + * zeros and every signer is loaded at runtime (signed_metadata_store_signer, + * reached in production through the user-confirmed LoadClearsignSigner FSM + * handler). The fixture loads the CI test key (02e3b3015c...ab5107) into + * slot 3 with alias "CI Test"; all vectors are signed in-process with the + * matching private key (f6d19e15...068a260) and embed key_id=3. * * No OLED/button I/O is exercised: signed_metadata_process() and * signed_metadata_matches_tx() never draw, and signed_metadata_confirm() is @@ -39,7 +40,7 @@ const uint8_t TEST_PRIV[32] = { 0x1e, 0x16, 0x14, 0xe7, 0xd9, 0xa1, 0x04, 0xd8, 0x1f, 0x73, 0x24, 0x49, 0x87, 0x56, 0xe5, 0x71, 0x90, 0x40, 0x68, 0xa2, 0x60}; -/* Expected compressed pubkey for slot 3 (DEBUG_LINK CI key). */ +/* Compressed pubkey of TEST_PRIV; loaded into slot 3 by the fixture. */ const uint8_t EXPECTED_SLOT3_PUB[33] = { 0x02, 0xe3, 0xb3, 0x01, 0x5c, 0x47, 0xdd, 0xca, 0xab, 0xe4, 0xf8, 0xe8, 0x72, 0xf1, 0xed, 0x8f, 0x09, 0xca, 0x14, 0x5a, 0x8d, 0x81, @@ -209,10 +210,15 @@ void make_matching_msg(EthereumSignTx *msg) { make_msg(msg, CONTRACT_A, data, sizeof(data), /*has_chain=*/true, 1); } +const char *TEST_ALIAS = "CI Test"; + class SignedMetadataTest : public ::testing::Test { protected: - void SetUp() override { signed_metadata_clear(); } - void TearDown() override { signed_metadata_clear(); } + void SetUp() override { + signed_metadata_clear_signers(); + signed_metadata_store_signer(TEST_KEY_ID, EXPECTED_SLOT3_PUB, TEST_ALIAS); + } + void TearDown() override { signed_metadata_clear_signers(); } void ExpectMalformed(const std::vector &blob, uint8_t key_id) { EXPECT_EQ(signed_metadata_process(blob.data(), blob.size(), key_id), @@ -223,14 +229,14 @@ class SignedMetadataTest : public ::testing::Test { }; /* ===================================================================== * - * signed_metadata_process — happy path + DEBUG_LINK slot 3 + * signed_metadata_process — happy path via a runtime-loaded signer * ===================================================================== */ TEST_F(SignedMetadataTest, DerivedPubkeyMatchesSlot3) { uint8_t pub[33]; ecdsa_get_public_key33(&secp256k1, TEST_PRIV, pub); EXPECT_EQ(memcmp(pub, EXPECTED_SLOT3_PUB, sizeof(pub)), 0) - << "TEST_PRIV must derive firmware METADATA_PUBKEYS[3]"; + << "TEST_PRIV must derive the loaded slot-3 test pubkey"; } TEST_F(SignedMetadataTest, ValidVerifiedSlot3) { @@ -288,7 +294,7 @@ TEST_F(SignedMetadataTest, KeyIdOutOfRange) { TEST_F(SignedMetadataTest, EmptyRotationSlot) { Spec s = base_spec(); - s.key_id = 1; // slot 1 pubkey == {0x00} + s.key_id = 1; // slot 1: no built-in key, nothing loaded ExpectMalformed(sign_body(build_body(s)), /*key_id=*/1); } @@ -579,6 +585,117 @@ TEST_F(SignedMetadataTest, ClearResetsAllState) { EXPECT_TRUE(signed_metadata_enforce(TX_HASH)); // not relied } +/* ===================================================================== * + * Runtime signer loading — the phase-1 trust path + * ===================================================================== */ + +TEST_F(SignedMetadataTest, NoSignerLoadedRejects) { + signed_metadata_clear_signers(); // undo the fixture's load + ExpectMalformed(base_blob(), TEST_KEY_ID); +} + +TEST_F(SignedMetadataTest, FromLoadedSignerTracksMetadata) { + EXPECT_FALSE(signed_metadata_from_loaded_signer()); // nothing processed + std::vector blob = base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EXPECT_TRUE(signed_metadata_from_loaded_signer()); + signed_metadata_clear(); + EXPECT_FALSE(signed_metadata_from_loaded_signer()); +} + +TEST_F(SignedMetadataTest, ClearSignersDropsKeyAndMetadata) { + std::vector blob = base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + signed_metadata_clear_signers(); + EXPECT_FALSE(signed_metadata_available()); + EXPECT_EQ(signed_metadata_get(), nullptr); + ExpectMalformed(blob, TEST_KEY_ID); // the key itself is gone too +} + +TEST_F(SignedMetadataTest, StoreSignerReplacementInvalidatesOldKey) { + std::vector blob = base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + + uint8_t priv2[32]; + memcpy(priv2, TEST_PRIV, sizeof(priv2)); + priv2[31] ^= 0x5a; // a different valid scalar + uint8_t pub2[33]; + ecdsa_get_public_key33(&secp256k1, priv2, pub2); + signed_metadata_store_signer(TEST_KEY_ID, pub2, "Replacement"); + + /* Replacing a signer drops metadata the old one verified... */ + EXPECT_FALSE(signed_metadata_available()); + /* ...and the old key no longer verifies anything. */ + ExpectMalformed(blob, TEST_KEY_ID); +} + +/* ---- signed_metadata_signer_valid (pure) ------------------------------ */ + +TEST(SignedMetadataSignerValid, AcceptsValidCompressedKeyAllSlots) { + for (uint8_t slot = 0; slot < METADATA_MAX_KEYS; slot++) { + EXPECT_TRUE( + signed_metadata_signer_valid(slot, EXPECTED_SLOT3_PUB, 33, "CI Test")) + << "slot " << (int)slot; + } +} + +TEST(SignedMetadataSignerValid, RejectsKeyIdOutOfRange) { + EXPECT_FALSE(signed_metadata_signer_valid(METADATA_MAX_KEYS, + EXPECTED_SLOT3_PUB, 33, "CI Test")); +} + +TEST(SignedMetadataSignerValid, RejectsWrongPubkeyLength) { + EXPECT_FALSE( + signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 32, "CI Test")); + EXPECT_FALSE( + signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 65, "CI Test")); + EXPECT_FALSE(signed_metadata_signer_valid(0, nullptr, 33, "CI Test")); +} + +TEST(SignedMetadataSignerValid, RejectsNonCompressedPrefix) { + /* 0x04 would make ecdsa_read_pubkey read 65 bytes from a 33-byte buffer — + * the prefix guard must reject it before the parser ever runs. */ + uint8_t bad[33]; + memcpy(bad, EXPECTED_SLOT3_PUB, sizeof(bad)); + bad[0] = 0x04; + EXPECT_FALSE(signed_metadata_signer_valid(0, bad, 33, "CI Test")); + bad[0] = 0x00; // the "empty slot" sentinel must never load as a key + EXPECT_FALSE(signed_metadata_signer_valid(0, bad, 33, "CI Test")); +} + +TEST(SignedMetadataSignerValid, RejectsBadAlias) { + EXPECT_FALSE(signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, nullptr)); + EXPECT_FALSE(signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, "")); + std::string too_long(METADATA_ALIAS_MAX_LEN + 1, 'a'); + EXPECT_FALSE( + signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, too_long.c_str())); + std::string max_len(METADATA_ALIAS_MAX_LEN, 'a'); + EXPECT_TRUE( + signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, max_len.c_str())); + /* Rendered through confirm() — control chars and '%' are spoofing vectors */ + EXPECT_FALSE(signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, "a\nb")); + EXPECT_FALSE(signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, "a%sb")); + EXPECT_FALSE( + signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, "a\x7f" "b")); +} + +/* ---- signed_metadata_pubkey_fingerprint -------------------------------- */ + +TEST(SignedMetadataFingerprint, IsSha256Prefix) { + char fp[METADATA_FINGERPRINT_LEN]; + signed_metadata_pubkey_fingerprint(EXPECTED_SLOT3_PUB, fp); + + uint8_t digest[32]; + sha256_Raw(EXPECTED_SLOT3_PUB, 33, digest); + char expected[METADATA_FINGERPRINT_LEN]; + snprintf(expected, sizeof(expected), "%02X%02X%02X%02X", digest[0], digest[1], + digest[2], digest[3]); + EXPECT_STREQ(fp, expected); +} + /* ===================================================================== * * signed_metadata_enforce_decision — pure enforce truth table (SECTION 2). * Exercises the relied==true cases that confirm()'s OLED/button I/O makes From 2d6613f830ab18b9cab96f8b6c9c97f9921a163e Mon Sep 17 00:00:00 2001 From: highlander Date: Thu, 2 Jul 2026 02:56:44 -0500 Subject: [PATCH 17/25] fix(clearsign): drop loaded signers on device wipe 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 (cherry picked from commit 62ec4ab700cf6ea47492ff39bdbcdb8a4606ace4) --- lib/firmware/fsm.c | 1 + lib/firmware/fsm_msg_common.h | 3 +++ 2 files changed, 4 insertions(+) diff --git a/lib/firmware/fsm.c b/lib/firmware/fsm.c index d9d8f4a1f..e37eaef3d 100644 --- a/lib/firmware/fsm.c +++ b/lib/firmware/fsm.c @@ -55,6 +55,7 @@ #include "keepkey/firmware/ripple.h" #include "keepkey/firmware/signing.h" #include "keepkey/firmware/signtx_tendermint.h" +#include "keepkey/firmware/signed_metadata.h" #include "keepkey/firmware/solana.h" #include "keepkey/firmware/storage.h" #include "keepkey/firmware/tendermint.h" diff --git a/lib/firmware/fsm_msg_common.h b/lib/firmware/fsm_msg_common.h index 44173a24c..558f6673f 100644 --- a/lib/firmware/fsm_msg_common.h +++ b/lib/firmware/fsm_msg_common.h @@ -460,6 +460,9 @@ void fsm_msgWipeDevice(WipeDevice* msg) { storage_reset(); storage_resetUuid(); storage_commit(); + /* Factory reset drops runtime trust anchors too: loaded clearsign + * signers (and any metadata they verified) must not survive a wipe. */ + signed_metadata_clear_signers(); fsm_sendSuccess("Device wiped"); layoutHome(); From 83023c63aaf8e44c29955ed9e13aac8e044d3dbc Mon Sep 17 00:00:00 2001 From: highlander Date: Thu, 2 Jul 2026 03:39:42 -0500 Subject: [PATCH 18/25] fix(clearsign): strict alias allowlist; fix FW_VERSION detection for screenshots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 (cherry picked from commit bf7582a74b9fe68350914e92ae2b0a552d5c78f7) --- lib/firmware/signed_metadata.c | 13 ++++++++++--- scripts/emulator/python-keepkey-tests.sh | 9 +++++++-- unittests/firmware/signed_metadata.cpp | 16 +++++++++++++++- 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/lib/firmware/signed_metadata.c b/lib/firmware/signed_metadata.c index 20ab28008..280f8d6d7 100644 --- a/lib/firmware/signed_metadata.c +++ b/lib/firmware/signed_metadata.c @@ -214,14 +214,21 @@ bool signed_metadata_signer_valid(uint8_t key_id, const uint8_t *pubkey, return false; } - /* Alias is rendered through confirm() format strings on the load screen and - * the per-tx warning — printable ASCII only, no control/spoofing chars. */ + /* Alias is rendered INSIDE quotes on the load screen and the per-tx warning + * ("Trust signer '%s' ..."). Restrict to a strict allowlist — letters, + * digits, space, '-' and '_' — so a host-chosen alias cannot break out of + * its quoted region or inject a semantic trust claim (e.g. a quote to close + * the quotes, or "." / "(" to append "verified by KeepKey."). '%' is also + * excluded so it can never reach the format string as a specifier. */ alias_len = strlen(alias); if (alias_len == 0 || alias_len > METADATA_ALIAS_MAX_LEN) { return false; } for (size_t i = 0; i < alias_len; i++) { - if (alias[i] < 0x20 || alias[i] > 0x7e || alias[i] == '%') { + char c = alias[i]; + bool ok = (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || + (c >= '0' && c <= '9') || c == ' ' || c == '-' || c == '_'; + if (!ok) { return false; } } diff --git a/scripts/emulator/python-keepkey-tests.sh b/scripts/emulator/python-keepkey-tests.sh index 28b122fc9..bbbce7519 100755 --- a/scripts/emulator/python-keepkey-tests.sh +++ b/scripts/emulator/python-keepkey-tests.sh @@ -41,9 +41,14 @@ echo "=== End diagnostic ===" # expression for every test with non-empty screenshot expectations. Adding screenshots # to a test in SECTIONS automatically includes it here — no manual filter maintenance. echo "=== Phase 1: Report-driven screenshot capture ===" -# Detect firmware version from CMakeLists if not set in env +# Detect firmware version from CMakeLists if not set in env. +# NOTE: grep -oE (POSIX ERE), NOT -oP — this runs in the Alpine/busybox +# python-keepkey container where grep has no -P (PCRE). With -P grep errored +# and the version silently fell back to 7.14.0, so every 7.15.0 section +# (Hive, EVM clear-signing) was excluded from screenshot capture. if [ -z "$FW_VERSION" ]; then - FW_VERSION=$(sed -n '/^project/,/)/p' /kkemu/CMakeLists.txt | grep -oP '\d+\.\d+\.\d+' || echo "7.14.0") + FW_VERSION=$(sed -n '/^project/,/)/p' /kkemu/CMakeLists.txt | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1) + [ -z "$FW_VERSION" ] && FW_VERSION="7.14.0" echo "Detected FW_VERSION=$FW_VERSION from CMakeLists.txt" fi export FW_VERSION diff --git a/unittests/firmware/signed_metadata.cpp b/unittests/firmware/signed_metadata.cpp index bfdc7ba08..15f2025b3 100644 --- a/unittests/firmware/signed_metadata.cpp +++ b/unittests/firmware/signed_metadata.cpp @@ -675,11 +675,25 @@ TEST(SignedMetadataSignerValid, RejectsBadAlias) { std::string max_len(METADATA_ALIAS_MAX_LEN, 'a'); EXPECT_TRUE( signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, max_len.c_str())); - /* Rendered through confirm() — control chars and '%' are spoofing vectors */ + /* Realistic aliases (letters/digits/space/-/_) are accepted. */ + EXPECT_TRUE(signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, "Pioneer")); + EXPECT_TRUE( + signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, "KeepKey Swap")); + EXPECT_TRUE( + signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, "my-signer_1")); + /* Rendered inside quotes on the trust screen — control chars, '%', and + * semantic-injection punctuation (quote breakout, "." / "(" appending a + * false "verified by KeepKey." claim) are all rejected. */ EXPECT_FALSE(signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, "a\nb")); EXPECT_FALSE(signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, "a%sb")); EXPECT_FALSE( signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, "a\x7f" "b")); + EXPECT_FALSE(signed_metadata_signer_valid( + 0, EXPECTED_SLOT3_PUB, 33, "x' verified by KeepKey. Safe (")); + EXPECT_FALSE( + signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, "safe.KeepKey")); + EXPECT_FALSE( + signed_metadata_signer_valid(0, EXPECTED_SLOT3_PUB, 33, "trust(me)")); } /* ---- signed_metadata_pubkey_fingerprint -------------------------------- */ From f949e7ed73bd4e7a0f2407995ec34e50f9b019a0 Mon Sep 17 00:00:00 2001 From: highlander Date: Thu, 2 Jul 2026 03:56:31 -0500 Subject: [PATCH 19/25] style: clang-format-20 wrap in clearsign load handler + fingerprint sig Co-Authored-By: Claude Fable 5 (cherry picked from commit 0737cd0f53f78751415810af70747f76c900b530) --- lib/firmware/fsm_msg_ethereum.h | 8 ++++---- lib/firmware/signed_metadata.c | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/firmware/fsm_msg_ethereum.h b/lib/firmware/fsm_msg_ethereum.h index 47e99e3ec..2c0ad3d57 100644 --- a/lib/firmware/fsm_msg_ethereum.h +++ b/lib/firmware/fsm_msg_ethereum.h @@ -59,10 +59,10 @@ void fsm_msgLoadClearsignSigner(const LoadClearsignSigner* msg) { _("key_id, pubkey and alias required")); /* Range-check as uint32 BEFORE narrowing: (uint8_t)256 would alias slot 0 */ CHECK_PARAM(msg->key_id < METADATA_MAX_KEYS, _("key_id out of range")); - CHECK_PARAM(signed_metadata_signer_valid((uint8_t)msg->key_id, - msg->pubkey.bytes, msg->pubkey.size, - msg->alias), - _("Invalid clearsign signer")); + CHECK_PARAM( + signed_metadata_signer_valid((uint8_t)msg->key_id, msg->pubkey.bytes, + msg->pubkey.size, msg->alias), + _("Invalid clearsign signer")); /* Mandatory on-device consent — the whole trust model hangs on this * confirm. The same fingerprint reappears on every per-tx warning. */ diff --git a/lib/firmware/signed_metadata.c b/lib/firmware/signed_metadata.c index 280f8d6d7..84d8764d7 100644 --- a/lib/firmware/signed_metadata.c +++ b/lib/firmware/signed_metadata.c @@ -253,8 +253,8 @@ void signed_metadata_store_signer(uint8_t key_id, const uint8_t *pubkey, signed_metadata_clear(); } -void signed_metadata_pubkey_fingerprint( - const uint8_t pubkey[33], char out[METADATA_FINGERPRINT_LEN]) { +void signed_metadata_pubkey_fingerprint(const uint8_t pubkey[33], + char out[METADATA_FINGERPRINT_LEN]) { uint8_t digest[32]; sha256_Raw(pubkey, 33, digest); data2hex(digest, 4, out); From fc9aaf7babd2c796d315341ce573354cbf57f22f Mon Sep 17 00:00:00 2001 From: highlander Date: Thu, 2 Jul 2026 14:43:40 -0500 Subject: [PATCH 20/25] =?UTF-8?q?feat(clearsign):=20human-readable=20WHAT?= =?UTF-8?q?=20=E2=80=94=20STRING=20+=20TOKEN=5FAMOUNT=20arg=20formats?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ". 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 (cherry picked from commit ed03369dd113d41fc5b6a2c74f29bd2eb6f43d31) --- include/keepkey/firmware/signed_metadata.h | 28 +++++- lib/firmware/signed_metadata.c | 90 +++++++++++++++++- unittests/firmware/signed_metadata.cpp | 103 ++++++++++++++++++++- 3 files changed, 213 insertions(+), 8 deletions(-) diff --git a/include/keepkey/firmware/signed_metadata.h b/include/keepkey/firmware/signed_metadata.h index 6138db39b..0a5730256 100644 --- a/include/keepkey/firmware/signed_metadata.h +++ b/include/keepkey/firmware/signed_metadata.h @@ -10,7 +10,10 @@ typedef struct _EthereumSignTx EthereumSignTx; #define METADATA_MAX_ARGS 8 #define METADATA_MAX_METHOD_LEN 64 #define METADATA_MAX_ARG_NAME_LEN 32 -#define METADATA_MAX_ARG_VALUE_LEN 32 +/* Sized for TOKEN_AMOUNT: decimals(1) + symbol_len(1) + symbol(<=10) + + * amount(<=32). Other formats remain capped at 32 by their own guards. */ +#define METADATA_MAX_ARG_VALUE_LEN 44 +#define METADATA_MAX_TOKEN_SYMBOL_LEN 10 #define METADATA_MAX_KEYS 4 #define METADATA_ALIAS_MAX_LEN 31 /* hex(first 4 bytes of sha256(pubkey)) + NUL */ @@ -22,11 +25,26 @@ typedef enum { METADATA_MALFORMED = 2, } MetadataClassification; +/* + * Argument display formats. The goal of clear-signing is that the device + * answers WHO the user is dealing with (validated contract address, protocol + * name), WHAT the transaction does (method + human-readable typed args: + * recipient, "Amount: 1,000 USDC"), and WHY the decode can be trusted + * (signer attestation bound to the exact tx hash). RAW/BYTES hex dumps are + * the fallback, not the product. + */ typedef enum { - ARG_FORMAT_RAW = 0, - ARG_FORMAT_ADDRESS = 1, - ARG_FORMAT_AMOUNT = 2, - ARG_FORMAT_BYTES = 3, + ARG_FORMAT_RAW = 0, /* hex dump (first 16 bytes) */ + ARG_FORMAT_ADDRESS = 1, /* 20 bytes -> full EIP-55 address, never truncated */ + ARG_FORMAT_AMOUNT = 2, /* big-endian uint256 -> raw integer, "wei" */ + ARG_FORMAT_BYTES = 3, /* hex dump (first 16 bytes) */ + /* Attested printable label, e.g. protocol: "Uniswap V2". Same character + * rules as the signer alias minus length (printable subset, no '%'). */ + ARG_FORMAT_STRING = 4, + /* decimals(1) + symbol_len(1) + symbol(<=10, [A-Za-z0-9]) + amount(1..32 + * big-endian). Rendered as a decimal-scaled amount with the symbol, e.g. + * "1000 USDC"; all-0xFF 32-byte amounts render "UNLIMITED ". */ + ARG_FORMAT_TOKEN_AMOUNT = 5, } ArgFormat; typedef struct { diff --git a/lib/firmware/signed_metadata.c b/lib/firmware/signed_metadata.c index 84d8764d7..767096014 100644 --- a/lib/firmware/signed_metadata.c +++ b/lib/firmware/signed_metadata.c @@ -121,6 +121,52 @@ static bool read_arg_name(const uint8_t **cursor, const uint8_t *end, char *out, return true; } +/* Per-format value validation, fail-closed at parse time. STRING and + * TOKEN_AMOUNT carry display semantics, so their byte layout is enforced + * before anything is stored; legacy formats keep their original 32-byte cap + * (METADATA_MAX_ARG_VALUE_LEN grew only to fit TOKEN_AMOUNT). */ +static bool arg_value_ok(uint8_t format, const uint8_t *value, uint16_t len) { + switch (format) { + case ARG_FORMAT_STRING: { + /* Attested printable label ("protocol: Uniswap V2"). Rendered through + * confirm() bodies: printable ASCII only, '%' excluded. */ + if (len == 0 || len > 32) { + return false; + } + for (uint16_t i = 0; i < len; i++) { + if (value[i] < 0x20 || value[i] > 0x7e || value[i] == '%') { + return false; + } + } + return true; + } + case ARG_FORMAT_TOKEN_AMOUNT: { + /* decimals(1) + symbol_len(1) + symbol + amount(1..32 BE) */ + if (len < 4) { + return false; + } + uint8_t decimals = value[0]; + uint8_t symlen = value[1]; + if (decimals > 36 || symlen == 0 || + symlen > METADATA_MAX_TOKEN_SYMBOL_LEN || + (uint16_t)(2 + symlen) >= len || len - 2 - symlen > 32) { + return false; + } + for (uint8_t i = 0; i < symlen; i++) { + char c = (char)value[2 + i]; + bool ok = (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || + (c >= '0' && c <= '9'); + if (!ok) { + return false; + } + } + return true; + } + default: + return len <= 32; + } +} + static bool parse_metadata_binary(const uint8_t *payload, size_t payload_len, SignedMetadata *out) { /* Minimum: version(1) + chain_id(4) + contract(20) + selector(4) + @@ -153,10 +199,11 @@ static bool parse_metadata_binary(const uint8_t *payload, size_t payload_len, MetadataArg *arg = &out->args[i]; if (!read_arg_name(&cursor, end, arg->name, METADATA_MAX_ARG_NAME_LEN) || - !read_u8(&cursor, end, &format) || format > ARG_FORMAT_BYTES || + !read_u8(&cursor, end, &format) || format > ARG_FORMAT_TOKEN_AMOUNT || !read_be_u16(&cursor, end, &value_len) || value_len > METADATA_MAX_ARG_VALUE_LEN || - !read_bytes(&cursor, end, arg->value, value_len)) { + !read_bytes(&cursor, end, arg->value, value_len) || + !arg_value_ok(format, arg->value, value_len)) { return false; } @@ -443,6 +490,45 @@ bool signed_metadata_confirm(void) { } break; } + case ARG_FORMAT_STRING: { + /* Attested printable label, validated at parse (arg_value_ok). */ + char text[33]; + memcpy(text, arg->value, arg->value_len); + text[arg->value_len] = '\0'; + snprintf(body, sizeof(body), "%s:\n%s", arg->name, text); + break; + } + case ARG_FORMAT_TOKEN_AMOUNT: { + /* decimals + symbol + big-endian amount, validated at parse. + * This is the "Amount: 1,000 USDC" the clear-signing plan calls for + * instead of a raw wei integer. */ + uint8_t decimals = arg->value[0]; + uint8_t symlen = arg->value[1]; + char suffix[METADATA_MAX_TOKEN_SYMBOL_LEN + 2]; + suffix[0] = ' '; + memcpy(suffix + 1, arg->value + 2, symlen); + suffix[1 + symlen] = '\0'; + + const uint8_t *amt = arg->value + 2 + symlen; + uint16_t amt_len = arg->value_len - 2 - symlen; + bool is_max = amt_len == 32; + for (uint16_t j = 0; j < amt_len && is_max; j++) { + if (amt[j] != 0xFF) { + is_max = false; + } + } + if (is_max) { + snprintf(body, sizeof(body), "%s:\nUNLIMITED%s", arg->name, suffix); + } else { + bignum256 amount; + bn_from_metadata_bytes(amt, amt_len, &amount); + char formatted[48]; + bn_format(&amount, NULL, suffix, decimals, 0, false, formatted, + sizeof(formatted)); + snprintf(body, sizeof(body), "%s:\n%s", arg->name, formatted); + } + break; + } case ARG_FORMAT_BYTES: case ARG_FORMAT_RAW: default: { diff --git a/unittests/firmware/signed_metadata.cpp b/unittests/firmware/signed_metadata.cpp index 15f2025b3..97e959cd9 100644 --- a/unittests/firmware/signed_metadata.cpp +++ b/unittests/firmware/signed_metadata.cpp @@ -404,10 +404,111 @@ TEST_F(SignedMetadataTest, ArgNameTooLong) { TEST_F(SignedMetadataTest, ArgFormatOutOfRange) { Spec s = base_spec(); - s.args[0].format = 4; // > ARG_FORMAT_BYTES (3) + s.args[0].format = 6; // > ARG_FORMAT_TOKEN_AMOUNT (5) ExpectMalformed(sign_body(build_body(s)), TEST_KEY_ID); } +/* ---- ARG_FORMAT_STRING (attested printable label) ----------------------- */ + +TEST_F(SignedMetadataTest, StringArgAccepted) { + Spec s = base_spec(); + const char *label = "Uniswap V2"; + s.args[0] = mk_arg("protocol", ARG_FORMAT_STRING, (const uint8_t *)label, + strlen(label)); + std::vector blob = sign_body(build_body(s)); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + const SignedMetadata *m = signed_metadata_get(); + ASSERT_NE(m, nullptr); + EXPECT_EQ(m->args[0].format, ARG_FORMAT_STRING); + EXPECT_EQ(memcmp(m->args[0].value, label, strlen(label)), 0); +} + +TEST_F(SignedMetadataTest, StringArgRejectsUnprintableAndPercent) { + const uint8_t nl[] = {'a', '\n', 'b'}; + Spec s = base_spec(); + s.args[0] = mk_arg("protocol", ARG_FORMAT_STRING, nl, sizeof(nl)); + ExpectMalformed(sign_body(build_body(s)), TEST_KEY_ID); + + const uint8_t pct[] = {'a', '%', 's'}; + Spec s2 = base_spec(); + s2.args[0] = mk_arg("protocol", ARG_FORMAT_STRING, pct, sizeof(pct)); + ExpectMalformed(sign_body(build_body(s2)), TEST_KEY_ID); + + Spec s3 = base_spec(); + s3.args[0] = mk_arg("protocol", ARG_FORMAT_STRING, pct, 0); // empty string + ExpectMalformed(sign_body(build_body(s3)), TEST_KEY_ID); +} + +/* ---- ARG_FORMAT_TOKEN_AMOUNT (decimals + symbol + amount) --------------- */ + +std::vector token_amount_value(uint8_t decimals, + const std::string &symbol, + const std::vector &amount) { + std::vector v; + v.push_back(decimals); + v.push_back((uint8_t)symbol.size()); + v.insert(v.end(), symbol.begin(), symbol.end()); + v.insert(v.end(), amount.begin(), amount.end()); + return v; +} + +TEST_F(SignedMetadataTest, TokenAmountAccepted) { + /* 1.00 USDC: 1000000 raw, 6 decimals */ + std::vector amt = {0x0F, 0x42, 0x40}; + std::vector val = token_amount_value(6, "USDC", amt); + Spec s = base_spec(); + s.args[1] = mk_arg("amount", ARG_FORMAT_TOKEN_AMOUNT, val.data(), val.size()); + std::vector blob = sign_body(build_body(s)); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + const SignedMetadata *m = signed_metadata_get(); + ASSERT_NE(m, nullptr); + EXPECT_EQ(m->args[1].format, ARG_FORMAT_TOKEN_AMOUNT); + EXPECT_EQ(m->args[1].value_len, val.size()); +} + +TEST_F(SignedMetadataTest, TokenAmountUnlimited32BytesAccepted) { + /* UNLIMITED approve: 32 x 0xFF + symbol -> value_len 38 (> old 32 cap) */ + std::vector amt(32, 0xFF); + std::vector val = token_amount_value(6, "USDC", amt); + EXPECT_EQ(val.size(), 38u); // 1+1+4+32 + Spec s = base_spec(); + s.args[1] = mk_arg("amount", ARG_FORMAT_TOKEN_AMOUNT, val.data(), val.size()); + std::vector blob = sign_body(build_body(s)); + EXPECT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); +} + +TEST_F(SignedMetadataTest, TokenAmountRejectsBadLayout) { + Spec s = base_spec(); + /* symbol chars outside [A-Za-z0-9] */ + std::vector bad_sym = token_amount_value(6, "US-C", {0x01}); + s.args[1] = mk_arg("amount", ARG_FORMAT_TOKEN_AMOUNT, bad_sym.data(), + bad_sym.size()); + ExpectMalformed(sign_body(build_body(s)), TEST_KEY_ID); + + /* decimals > 36 */ + Spec s2 = base_spec(); + std::vector bad_dec = token_amount_value(37, "USDC", {0x01}); + s2.args[1] = mk_arg("amount", ARG_FORMAT_TOKEN_AMOUNT, bad_dec.data(), + bad_dec.size()); + ExpectMalformed(sign_body(build_body(s2)), TEST_KEY_ID); + + /* symbol_len runs past the value (no amount bytes left) */ + Spec s3 = base_spec(); + std::vector no_amt = {6, 4, 'U', 'S', 'D', 'C'}; + s3.args[1] = mk_arg("amount", ARG_FORMAT_TOKEN_AMOUNT, no_amt.data(), + no_amt.size()); + ExpectMalformed(sign_body(build_body(s3)), TEST_KEY_ID); + + /* legacy formats must NOT accept the larger 44-byte cap */ + Spec s4 = base_spec(); + std::vector big(40, 0xAB); + s4.args[1] = mk_arg("amount", ARG_FORMAT_AMOUNT, big.data(), big.size()); + ExpectMalformed(sign_body(build_body(s4)), TEST_KEY_ID); +} + TEST_F(SignedMetadataTest, ArgValueTooLong) { Spec s = base_spec(); uint8_t big[33] = {0}; From d5da9c6e24e141e65dd1b8c81dbe1fdc3b417909 Mon Sep 17 00:00:00 2001 From: highlander Date: Fri, 3 Jul 2026 00:37:49 -0500 Subject: [PATCH 21/25] feat(clearsign): v2 static-schema blobs + on-device calldata decode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (cherry picked from commit b8cbb97daf920737969fa50fba29a9d117842294) --- include/keepkey/firmware/signed_metadata.h | 27 +++ lib/firmware/signed_metadata.c | 238 +++++++++++++++---- unittests/firmware/signed_metadata.cpp | 253 +++++++++++++++++++++ 3 files changed, 479 insertions(+), 39 deletions(-) diff --git a/include/keepkey/firmware/signed_metadata.h b/include/keepkey/firmware/signed_metadata.h index 0a5730256..72d312ca4 100644 --- a/include/keepkey/firmware/signed_metadata.h +++ b/include/keepkey/firmware/signed_metadata.h @@ -25,6 +25,25 @@ typedef enum { METADATA_MALFORMED = 2, } MetadataClassification; +/* + * Blob format versions (the first payload byte). + * + * LEGACY (v1): per-transaction. The blob carries a committed tx_hash and the + * pre-decoded argument VALUES; the host is trusted for the decode and the + * device only binds it to the signed digest (signed_metadata_enforce). This is + * the format that requires an online, per-tx signer holding the attestation + * key. + * + * SCHEMA (v2): static. The blob carries NO tx_hash and NO values — only how to + * decode the call: (chainId, contract, selector, method, per-arg name + display + * format [+ static decimals/symbol]). The DEVICE decodes the argument values + * from the exact calldata it is about to sign, so the display is bound to the + * signature by construction. No tx_hash, no per-tx signing: the catalog is + * signed ONCE, offline, and can be served from a host CDN (no hot key). + */ +#define METADATA_VERSION_LEGACY 0x01 +#define METADATA_VERSION_SCHEMA 0x02 + /* * Argument display formats. The goal of clear-signing is that the device * answers WHO the user is dealing with (validated contract address, protocol @@ -137,6 +156,14 @@ bool signed_metadata_enforce_decision(bool relied, bool available, const uint8_t *stored_hash, const uint8_t *hash); +/* Pure enforcement decision for v2 (static schema) blobs, exported for unit + * testing. v2 has no committed tx_hash; the binding is structural (args decoded + * from the signed calldata), so signing proceeds when the relied-upon metadata + * is available and VERIFIED — no digest comparison. signed_metadata_enforce() + * dispatches here when the stored blob's version is METADATA_VERSION_SCHEMA. */ +bool signed_metadata_enforce_schema_decision(bool relied, bool available, + int classification); + const SignedMetadata *signed_metadata_get(void); #endif diff --git a/lib/firmware/signed_metadata.c b/lib/firmware/signed_metadata.c index 767096014..52b9d1547 100644 --- a/lib/firmware/signed_metadata.c +++ b/lib/firmware/signed_metadata.c @@ -167,60 +167,195 @@ static bool arg_value_ok(uint8_t format, const uint8_t *value, uint16_t len) { } } -static bool parse_metadata_binary(const uint8_t *payload, size_t payload_len, - SignedMetadata *out) { - /* Minimum: version(1) + chain_id(4) + contract(20) + selector(4) + - * tx_hash(32) + method_len(2) + method(1) + num_args(1) + - * classification(1) + timestamp(4) + key_id(1) + sig(64) + recovery(1) - * = 136 bytes */ - if (payload_len < 136) { - return false; - } - - const uint8_t *cursor = payload; - const uint8_t *end = payload + payload_len; - memset(out, 0, sizeof(*out)); +/* chain_id(4) + contract(20) + selector(4) — shared by both blob versions. */ +static bool parse_common_head(const uint8_t **cursor, const uint8_t *end, + SignedMetadata *out) { + return read_be_u32(cursor, end, &out->chain_id) && + read_bytes(cursor, end, out->contract_address, + sizeof(out->contract_address)) && + read_bytes(cursor, end, out->selector, sizeof(out->selector)); +} - if (!read_u8(&cursor, end, &out->version) || out->version != 0x01 || - !read_be_u32(&cursor, end, &out->chain_id) || - !read_bytes(&cursor, end, out->contract_address, - sizeof(out->contract_address)) || - !read_bytes(&cursor, end, out->selector, sizeof(out->selector)) || - !read_bytes(&cursor, end, out->tx_hash, sizeof(out->tx_hash)) || - !read_string(&cursor, end, out->method_name, METADATA_MAX_METHOD_LEN) || - !read_u8(&cursor, end, &out->num_args) || - out->num_args > METADATA_MAX_ARGS) { +/* classification(1) + timestamp(4) + key_id(1) + sig(64) + recovery(1), then + * the cursor must land exactly on `end` — identical for v1 and v2. */ +static bool parse_trailer(const uint8_t **cursor, const uint8_t *end, + SignedMetadata *out) { + uint8_t classification = 0; + if (!read_u8(cursor, end, &classification) || classification > 2 || + !read_be_u32(cursor, end, &out->timestamp) || + !read_u8(cursor, end, &out->key_id) || + !read_bytes(cursor, end, out->signature, sizeof(out->signature)) || + !read_u8(cursor, end, &out->recovery) || *cursor != end) { return false; } + out->classification = (MetadataClassification)classification; + return true; +} +/* v1 args: name + format + explicit (host-decoded) value. */ +static bool parse_v1_args(const uint8_t **cursor, const uint8_t *end, + SignedMetadata *out) { for (uint8_t i = 0; i < out->num_args; i++) { uint8_t format = 0; uint16_t value_len = 0; MetadataArg *arg = &out->args[i]; - if (!read_arg_name(&cursor, end, arg->name, METADATA_MAX_ARG_NAME_LEN) || - !read_u8(&cursor, end, &format) || format > ARG_FORMAT_TOKEN_AMOUNT || - !read_be_u16(&cursor, end, &value_len) || + if (!read_arg_name(cursor, end, arg->name, METADATA_MAX_ARG_NAME_LEN) || + !read_u8(cursor, end, &format) || format > ARG_FORMAT_TOKEN_AMOUNT || + !read_be_u16(cursor, end, &value_len) || value_len > METADATA_MAX_ARG_VALUE_LEN || - !read_bytes(&cursor, end, arg->value, value_len) || + !read_bytes(cursor, end, arg->value, value_len) || !arg_value_ok(format, arg->value, value_len)) { return false; } - arg->format = (ArgFormat)format; arg->value_len = value_len; } + return true; +} - uint8_t classification = 0; - if (!read_u8(&cursor, end, &classification) || classification > 2 || - !read_be_u32(&cursor, end, &out->timestamp) || - !read_u8(&cursor, end, &out->key_id) || - !read_bytes(&cursor, end, out->signature, sizeof(out->signature)) || - !read_u8(&cursor, end, &out->recovery) || cursor != end) { +/* v2 args: name + display format only (NO value — decoded from calldata later). + * TOKEN_AMOUNT additionally carries its static decimals + symbol, pre-stored as + * the value prefix [decimals, symlen, symbol...] so decode_v2_args() only has + * to append the 32-byte amount word. v2 supports the fixed single-word ABI + * types ADDRESS / AMOUNT / TOKEN_AMOUNT; anything else is out of scope -> blind + * sign. */ +static bool parse_v2_args(const uint8_t **cursor, const uint8_t *end, + SignedMetadata *out) { + for (uint8_t i = 0; i < out->num_args; i++) { + uint8_t format = 0; + MetadataArg *arg = &out->args[i]; + + if (!read_arg_name(cursor, end, arg->name, METADATA_MAX_ARG_NAME_LEN) || + !read_u8(cursor, end, &format)) { + return false; + } + switch (format) { + case ARG_FORMAT_ADDRESS: + case ARG_FORMAT_AMOUNT: + arg->value_len = 0; /* filled from the tx calldata at decode time */ + break; + case ARG_FORMAT_TOKEN_AMOUNT: { + uint8_t decimals = 0, symlen = 0; + if (!read_u8(cursor, end, &decimals) || + !read_u8(cursor, end, &symlen) || decimals > 36 || symlen == 0 || + symlen > METADATA_MAX_TOKEN_SYMBOL_LEN || + (size_t)(end - *cursor) < symlen) { + return false; + } + arg->value[0] = decimals; + arg->value[1] = symlen; + for (uint8_t j = 0; j < symlen; j++) { + char c = (char)(*cursor)[j]; + bool ok = (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || + (c >= '0' && c <= '9'); + if (!ok) { + return false; + } + arg->value[2 + j] = (uint8_t)c; + } + *cursor += symlen; + arg->value_len = (uint16_t)(2 + symlen); + break; + } + default: + return false; + } + arg->format = (ArgFormat)format; + } + return true; +} + +static bool parse_metadata_binary(const uint8_t *payload, size_t payload_len, + SignedMetadata *out) { + const uint8_t *cursor = payload; + const uint8_t *end = payload + payload_len; + memset(out, 0, sizeof(*out)); + + if (!read_u8(&cursor, end, &out->version)) { return false; } - out->classification = (MetadataClassification)classification; + if (out->version == METADATA_VERSION_LEGACY) { + /* Min: version(1)+chain_id(4)+contract(20)+selector(4)+tx_hash(32)+ + * method_len(2)+method(1)+num_args(1)+trailer(71) = 136 */ + if (payload_len < 136 || !parse_common_head(&cursor, end, out) || + !read_bytes(&cursor, end, out->tx_hash, sizeof(out->tx_hash)) || + !read_string(&cursor, end, out->method_name, METADATA_MAX_METHOD_LEN) || + !read_u8(&cursor, end, &out->num_args) || + out->num_args > METADATA_MAX_ARGS || + !parse_v1_args(&cursor, end, out)) { + return false; + } + } else if (out->version == METADATA_VERSION_SCHEMA) { + /* Min (0 args): version(1)+chain_id(4)+contract(20)+selector(4)+ + * method_len(2)+method(1)+num_args(1)+trailer(71) = 104 (no tx_hash) */ + if (payload_len < 104 || !parse_common_head(&cursor, end, out) || + !read_string(&cursor, end, out->method_name, METADATA_MAX_METHOD_LEN) || + !read_u8(&cursor, end, &out->num_args) || + out->num_args > METADATA_MAX_ARGS || + !parse_v2_args(&cursor, end, out)) { + return false; + } + } else { + return false; + } + + return parse_trailer(&cursor, end, out); +} + +/* + * v2 decode: fill each schema arg's value from the transaction calldata. + * + * All v2 args are fixed single 32-byte ABI head words, laid out sequentially + * from offset 4 (right after the selector). We require the ENTIRE calldata to + * be exactly selector + num_args words, wholly present in the initial chunk — + * so the device decodes, displays, AND signs the same bytes with nothing hidden + * in a later chunk or trailing the words. That structural completeness is what + * binds the displayed decode to the signature; v2 has no tx_hash. + */ +static bool decode_v2_args(SignedMetadata *md, const EthereumSignTx *msg) { + uint32_t expected = 4u + 32u * (uint32_t)md->num_args; + uint32_t initsz = msg->data_initial_chunk.size; + uint32_t total = msg->has_data_length ? msg->data_length : initsz; + if (total != expected || initsz != expected) { + return false; + } + + for (uint8_t i = 0; i < md->num_args; i++) { + const uint8_t *word = msg->data_initial_chunk.bytes + 4 + 32u * i; + MetadataArg *arg = &md->args[i]; + + switch (arg->format) { + case ARG_FORMAT_ADDRESS: + /* ABI address is a left-zero-padded 20-byte value; reject dirty high + * bytes rather than silently truncate (they could hide meaning). */ + for (int j = 0; j < 12; j++) { + if (word[j] != 0) { + return false; + } + } + memcpy(arg->value, word + 12, 20); + arg->value_len = 20; + break; + case ARG_FORMAT_AMOUNT: + memcpy(arg->value, word, 32); + arg->value_len = 32; + break; + case ARG_FORMAT_TOKEN_AMOUNT: { + /* value already holds [decimals, symlen, symbol]; append the amount. */ + uint16_t prefix = arg->value_len; + if ((size_t)prefix + 32 > METADATA_MAX_ARG_VALUE_LEN) { + return false; + } + memcpy(arg->value + prefix, word, 32); + arg->value_len = (uint16_t)(prefix + 32); + break; + } + default: + return false; + } + } return true; } @@ -389,11 +524,20 @@ bool signed_metadata_matches_tx(const EthereumSignTx *msg) { return false; } - /* This only gates what we DISPLAY (so a benign-looking method screen can't - * be shown for the wrong call). The metadata commits to the full tx hash; - * that is enforced against the real signed digest in - * signed_metadata_enforce() because the digest does not exist until - * send_signature() finalizes it. */ + if (stored_metadata.version == METADATA_VERSION_SCHEMA) { + /* v2 has no committed values or tx_hash: decode the args straight from the + * calldata this tx will sign. Success here means the schema fully accounts + * for the calldata (decode_v2_args enforces exact length + presence), so + * the display is bound to the signature structurally — nothing is enforced + * later against a digest (there is no tx_hash). A decode failure falls + * through to the normal blind-sign path. */ + return decode_v2_args(&stored_metadata, msg); + } + + /* v1 only gates what we DISPLAY (so a benign-looking method screen can't be + * shown for the wrong call). The metadata commits to the full tx hash; that + * is enforced against the real signed digest in signed_metadata_enforce() + * because the digest does not exist until send_signature() finalizes it. */ return true; } @@ -570,7 +714,23 @@ bool signed_metadata_enforce_decision(bool relied, bool available, memcmp(stored_hash, hash, 32) == 0; } +bool signed_metadata_enforce_schema_decision(bool relied, bool available, + int classification) { + /* v2 (static schema) has no committed tx_hash. Its binding is structural: the + * args were decoded from the exact calldata being signed, and that calldata + * cannot change between decode and sign within one signing operation. So if + * we relied on a verified v2 decode, signing may proceed; there is no digest + * to compare. If we did not rely on it, signing was never gated by metadata. + */ + return !relied || (available && classification == METADATA_VERIFIED); +} + bool signed_metadata_enforce(const uint8_t hash[32]) { + if (metadata_available && + stored_metadata.version == METADATA_VERSION_SCHEMA) { + return signed_metadata_enforce_schema_decision( + relied_on_metadata, metadata_available, stored_metadata.classification); + } return signed_metadata_enforce_decision( relied_on_metadata, metadata_available, stored_metadata.classification, stored_metadata.tx_hash, hash); diff --git a/unittests/firmware/signed_metadata.cpp b/unittests/firmware/signed_metadata.cpp index 97e959cd9..9b64908a3 100644 --- a/unittests/firmware/signed_metadata.cpp +++ b/unittests/firmware/signed_metadata.cpp @@ -875,4 +875,257 @@ TEST(SignedMetadataEnforce, ReliedStoredHashNull) { nullptr, h)); } +/* ===================================================================== * + * SECTION 3 — v2 static-schema blobs + on-device calldata decode + * + * v2 carries NO tx_hash and NO argument values. The blob attests only the + * static schema (chainId, contract, selector, method, per-arg name + display + * format [+ static decimals/symbol]); the device decodes the actual argument + * values from the calldata it is about to sign. These tests drive the full + * parse -> verify -> signed_metadata_matches_tx (which decodes) path and check + * the decoded MetadataArg values, plus the malformed/rejection cases. + * ===================================================================== */ + +struct V2Arg { + std::string name; + uint8_t format; + uint8_t decimals; /* TOKEN_AMOUNT only */ + std::string symbol; /* TOKEN_AMOUNT only */ +}; + +V2Arg v2_addr(const std::string &name) { + return V2Arg{name, ARG_FORMAT_ADDRESS, 0, ""}; +} +V2Arg v2_token(const std::string &name, uint8_t decimals, + const std::string &symbol) { + return V2Arg{name, ARG_FORMAT_TOKEN_AMOUNT, decimals, symbol}; +} + +struct V2Spec { + uint32_t chain_id; + std::vector contract; + std::vector selector; + std::string method; + std::vector args; + uint8_t classification; + uint8_t key_id; + int num_args_override; // -1 => use args.size() +}; + +V2Spec v2_base_spec() { + V2Spec s; + s.chain_id = 1; + s.contract.assign(CONTRACT_A, CONTRACT_A + 20); + s.selector.assign(SEL_TRANSFER, SEL_TRANSFER + 4); + s.method = "transfer"; + s.args.push_back(v2_addr("to")); + s.args.push_back(v2_token("amount", 6, "USDC")); + s.classification = METADATA_VERIFIED; + s.key_id = TEST_KEY_ID; + s.num_args_override = -1; + return s; +} + +std::vector build_v2_body(const V2Spec &s) { + std::vector b; + put_u8(b, METADATA_VERSION_SCHEMA); + put_be32(b, s.chain_id); + put_bytes(b, s.contract.data(), s.contract.size()); + put_bytes(b, s.selector.data(), s.selector.size()); + put_be16(b, (uint16_t)s.method.size()); + put_bytes(b, (const uint8_t *)s.method.data(), s.method.size()); + put_u8(b, s.num_args_override >= 0 ? (uint8_t)s.num_args_override + : (uint8_t)s.args.size()); + for (const V2Arg &a : s.args) { + put_u8(b, (uint8_t)a.name.size()); + put_bytes(b, (const uint8_t *)a.name.data(), a.name.size()); + put_u8(b, a.format); + if (a.format == ARG_FORMAT_TOKEN_AMOUNT) { + put_u8(b, a.decimals); + put_u8(b, (uint8_t)a.symbol.size()); + put_bytes(b, (const uint8_t *)a.symbol.data(), a.symbol.size()); + } + } + put_u8(b, s.classification); + put_be32(b, 0); // timestamp + put_u8(b, s.key_id); + return b; +} + +std::vector v2_base_blob() { + return sign_body(build_v2_body(v2_base_spec())); +} + +/* ABI calldata: selector + one 32-byte head word per arg. */ +void put_addr_word(std::vector &d, const uint8_t addr[20]) { + for (int i = 0; i < 12; i++) d.push_back(0); + d.insert(d.end(), addr, addr + 20); +} + +/* Canonical transfer(to=RECIPIENT, amount=AMOUNT32) calldata (4 + 64 = 68). */ +std::vector v2_transfer_calldata() { + std::vector d(SEL_TRANSFER, SEL_TRANSFER + 4); + put_addr_word(d, RECIPIENT); + d.insert(d.end(), AMOUNT32, AMOUNT32 + 32); + return d; +} + +void make_v2_msg(EthereumSignTx *msg, const uint8_t contract[20], + const std::vector &data, bool has_len, + uint32_t data_length) { + memset(msg, 0, sizeof(*msg)); + msg->has_to = true; + msg->to.size = 20; + memcpy(msg->to.bytes, contract, 20); + msg->has_data_initial_chunk = true; + msg->data_initial_chunk.size = (pb_size_t)data.size(); + memcpy(msg->data_initial_chunk.bytes, data.data(), data.size()); + msg->has_chain_id = true; + msg->chain_id = 1; + msg->has_data_length = has_len; + msg->data_length = has_len ? data_length : 0; +} + +/* Happy path: parse+verify a v2 blob, then matches_tx decodes the args from the + * transfer calldata and populates stored_metadata. */ +TEST_F(SignedMetadataTest, V2SchemaDecodesTransferArgs) { + std::vector blob = v2_base_blob(); + EXPECT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EXPECT_TRUE(signed_metadata_available()); + + const SignedMetadata *md = signed_metadata_get(); + ASSERT_NE(md, nullptr); + EXPECT_EQ(md->version, METADATA_VERSION_SCHEMA); + EXPECT_EQ(md->num_args, 2); + EXPECT_EQ(md->args[0].value_len, 0); // undecoded before matches_tx + + EthereumSignTx msg; + std::vector data = v2_transfer_calldata(); + make_v2_msg(&msg, CONTRACT_A, data, /*has_len=*/true, (uint32_t)data.size()); + EXPECT_TRUE(signed_metadata_matches_tx(&msg)); + + EXPECT_EQ(md->args[0].format, ARG_FORMAT_ADDRESS); + EXPECT_EQ(md->args[0].value_len, 20); + EXPECT_EQ(memcmp(md->args[0].value, RECIPIENT, 20), 0); + + EXPECT_EQ(md->args[1].format, ARG_FORMAT_TOKEN_AMOUNT); + EXPECT_EQ(md->args[1].value_len, 2 + 4 + 32); + EXPECT_EQ(md->args[1].value[0], 6); // decimals + EXPECT_EQ(md->args[1].value[1], 4); // symlen + EXPECT_EQ(memcmp(md->args[1].value + 2, "USDC", 4), 0); + EXPECT_EQ(memcmp(md->args[1].value + 6, AMOUNT32, 32), 0); +} + +/* has_data_length omitted but the initial chunk IS the whole calldata: allowed. */ +TEST_F(SignedMetadataTest, V2AcceptsNoDataLengthWhenChunkComplete) { + std::vector blob = v2_base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EthereumSignTx msg; + std::vector data = v2_transfer_calldata(); + make_v2_msg(&msg, CONTRACT_A, data, /*has_len=*/false, 0); + EXPECT_TRUE(signed_metadata_matches_tx(&msg)); +} + +/* Reject when the tx claims MORE calldata than the schema accounts for. */ +TEST_F(SignedMetadataTest, V2RejectsExtraCalldataLength) { + std::vector blob = v2_base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EthereumSignTx msg; + std::vector data = v2_transfer_calldata(); // 68 bytes + make_v2_msg(&msg, CONTRACT_A, data, /*has_len=*/true, 100); + EXPECT_FALSE(signed_metadata_matches_tx(&msg)); +} + +/* Reject a partial initial chunk (rest would stream later). */ +TEST_F(SignedMetadataTest, V2RejectsPartialInitialChunk) { + std::vector blob = v2_base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EthereumSignTx msg; + std::vector data = v2_transfer_calldata(); + data.resize(40); // selector + partial first word + make_v2_msg(&msg, CONTRACT_A, data, /*has_len=*/true, 68); + EXPECT_FALSE(signed_metadata_matches_tx(&msg)); +} + +/* Reject an ABI address word with non-zero high bytes. */ +TEST_F(SignedMetadataTest, V2RejectsDirtyAddressWord) { + std::vector blob = v2_base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EthereumSignTx msg; + std::vector data = v2_transfer_calldata(); + data[4] = 0x01; // first (should-be-zero) byte of the address word + make_v2_msg(&msg, CONTRACT_A, data, /*has_len=*/true, (uint32_t)data.size()); + EXPECT_FALSE(signed_metadata_matches_tx(&msg)); +} + +/* Wrong selector in calldata -> matches_tx fails before decode. */ +TEST_F(SignedMetadataTest, V2RejectsSelectorMismatch) { + std::vector blob = v2_base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EthereumSignTx msg; + std::vector data = v2_transfer_calldata(); + memcpy(data.data(), SEL_APPROVE, 4); + make_v2_msg(&msg, CONTRACT_A, data, /*has_len=*/true, (uint32_t)data.size()); + EXPECT_FALSE(signed_metadata_matches_tx(&msg)); +} + +/* An unsupported display format (dynamic types out of scope) -> MALFORMED. */ +TEST_F(SignedMetadataTest, V2RejectsUnsupportedFormat) { + V2Spec s = v2_base_spec(); + s.args[1] = V2Arg{"data", ARG_FORMAT_BYTES, 0, ""}; + std::vector blob = sign_body(build_v2_body(s)); + ExpectMalformed(blob, TEST_KEY_ID); +} + +/* Tampered v2 body must fail the signature check. */ +TEST_F(SignedMetadataTest, V2RejectsTamperedBody) { + std::vector blob = v2_base_blob(); + blob[5] ^= 0xFF; // flip a contract-address byte in the signed region + ExpectMalformed(blob, TEST_KEY_ID); +} + +/* Zero-arg v2 schema (selector-only call): valid, decodes nothing. */ +TEST_F(SignedMetadataTest, V2ZeroArgsSelectorOnly) { + V2Spec s = v2_base_spec(); + s.args.clear(); + s.method = "poke"; + std::vector blob = sign_body(build_v2_body(s)); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EthereumSignTx msg; + std::vector data(SEL_TRANSFER, SEL_TRANSFER + 4); + make_v2_msg(&msg, CONTRACT_A, data, /*has_len=*/true, 4); + EXPECT_TRUE(signed_metadata_matches_tx(&msg)); + EXPECT_EQ(signed_metadata_get()->num_args, 0); +} + +/* ---- v2 enforce truth table (pure, no I/O) ------------------------------ */ + +TEST(SignedMetadataEnforceSchema, NotReliedAlwaysAllow) { + EXPECT_TRUE(signed_metadata_enforce_schema_decision(false, true, + METADATA_VERIFIED)); + EXPECT_TRUE(signed_metadata_enforce_schema_decision(false, false, + METADATA_OPAQUE)); +} + +TEST(SignedMetadataEnforceSchema, ReliedVerifiedAllow) { + EXPECT_TRUE(signed_metadata_enforce_schema_decision(true, true, + METADATA_VERIFIED)); +} + +TEST(SignedMetadataEnforceSchema, ReliedButUnavailableOrUnverifiedFails) { + EXPECT_FALSE(signed_metadata_enforce_schema_decision(true, false, + METADATA_VERIFIED)); + EXPECT_FALSE(signed_metadata_enforce_schema_decision(true, true, + METADATA_OPAQUE)); + EXPECT_FALSE(signed_metadata_enforce_schema_decision(true, true, + METADATA_MALFORMED)); +} + } // namespace From 0386211a0ba0d1da07c6428c75e94fe2d4ec30a1 Mon Sep 17 00:00:00 2001 From: highlander Date: Fri, 3 Jul 2026 00:52:31 -0500 Subject: [PATCH 22/25] fix(clearsign): harden v2 enforce with explicit decode flag; idempotent decode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (cherry picked from commit 7629932071880af6c04a721ebd1bd0afc61a0968) --- include/keepkey/firmware/signed_metadata.h | 9 +++-- lib/firmware/signed_metadata.c | 34 ++++++++++++----- unittests/firmware/signed_metadata.cpp | 43 ++++++++++++++++++---- 3 files changed, 67 insertions(+), 19 deletions(-) diff --git a/include/keepkey/firmware/signed_metadata.h b/include/keepkey/firmware/signed_metadata.h index 72d312ca4..2b3705db9 100644 --- a/include/keepkey/firmware/signed_metadata.h +++ b/include/keepkey/firmware/signed_metadata.h @@ -159,10 +159,13 @@ bool signed_metadata_enforce_decision(bool relied, bool available, /* Pure enforcement decision for v2 (static schema) blobs, exported for unit * testing. v2 has no committed tx_hash; the binding is structural (args decoded * from the signed calldata), so signing proceeds when the relied-upon metadata - * is available and VERIFIED — no digest comparison. signed_metadata_enforce() - * dispatches here when the stored blob's version is METADATA_VERSION_SCHEMA. */ + * is available, VERIFIED, and was actually decoded (`decoded`) — no digest + * comparison. `decoded` must be the recorded result of decode_v2_args() for + * this signing operation, not inferred from call order. + * signed_metadata_enforce() dispatches here when the stored blob's version is + * METADATA_VERSION_SCHEMA. */ bool signed_metadata_enforce_schema_decision(bool relied, bool available, - int classification); + bool decoded, int classification); const SignedMetadata *signed_metadata_get(void); diff --git a/lib/firmware/signed_metadata.c b/lib/firmware/signed_metadata.c index 52b9d1547..3eff799a0 100644 --- a/lib/firmware/signed_metadata.c +++ b/lib/firmware/signed_metadata.c @@ -18,6 +18,11 @@ static bool metadata_available = false; static bool relied_on_metadata = false; static bool metadata_signer_loaded = false; +/* v2 only: set true once decode_v2_args() has decoded this metadata's args from + * the tx calldata. The v2 enforce path REQUIRES it — v2 has no committed + * tx_hash, so this is the explicit proof (not an implicit call-order + * assumption) that the displayed values came from the calldata being signed. */ +static bool metadata_schema_decoded = false; static SignedMetadata stored_metadata; /* @@ -343,8 +348,11 @@ static bool decode_v2_args(SignedMetadata *md, const EthereumSignTx *msg) { arg->value_len = 32; break; case ARG_FORMAT_TOKEN_AMOUNT: { - /* value already holds [decimals, symlen, symbol]; append the amount. */ - uint16_t prefix = arg->value_len; + /* value holds [decimals, symlen, symbol] from parse; append the amount. + * Derive the prefix from symlen (value[1]), NOT the current value_len, + * so a repeated decode of the same arg is idempotent (value_len already + * includes a previously-appended amount; value[1] does not change). */ + uint16_t prefix = (uint16_t)(2 + arg->value[1]); if ((size_t)prefix + 32 > METADATA_MAX_ARG_VALUE_LEN) { return false; } @@ -377,6 +385,7 @@ void signed_metadata_clear(void) { metadata_available = false; relied_on_metadata = false; metadata_signer_loaded = false; + metadata_schema_decoded = false; } void signed_metadata_clear_signers(void) { @@ -530,8 +539,11 @@ bool signed_metadata_matches_tx(const EthereumSignTx *msg) { * for the calldata (decode_v2_args enforces exact length + presence), so * the display is bound to the signature structurally — nothing is enforced * later against a digest (there is no tx_hash). A decode failure falls - * through to the normal blind-sign path. */ - return decode_v2_args(&stored_metadata, msg); + * through to the normal blind-sign path. Record the decode explicitly: + * signed_metadata_enforce() requires it for v2, so a signature can never be + * emitted for a v2 blob whose args were not decoded from this tx. */ + metadata_schema_decoded = decode_v2_args(&stored_metadata, msg); + return metadata_schema_decoded; } /* v1 only gates what we DISPLAY (so a benign-looking method screen can't be @@ -715,21 +727,25 @@ bool signed_metadata_enforce_decision(bool relied, bool available, } bool signed_metadata_enforce_schema_decision(bool relied, bool available, - int classification) { + bool decoded, int classification) { /* v2 (static schema) has no committed tx_hash. Its binding is structural: the * args were decoded from the exact calldata being signed, and that calldata * cannot change between decode and sign within one signing operation. So if * we relied on a verified v2 decode, signing may proceed; there is no digest - * to compare. If we did not rely on it, signing was never gated by metadata. - */ - return !relied || (available && classification == METADATA_VERIFIED); + * to compare. `decoded` is the explicit proof that decode_v2_args() ran and + * succeeded for this signing operation — required rather than inferred from + * call order, since v2 has no digest fallback. If we did not rely on the + * metadata, signing was never gated by it. */ + return !relied || + (available && decoded && classification == METADATA_VERIFIED); } bool signed_metadata_enforce(const uint8_t hash[32]) { if (metadata_available && stored_metadata.version == METADATA_VERSION_SCHEMA) { return signed_metadata_enforce_schema_decision( - relied_on_metadata, metadata_available, stored_metadata.classification); + relied_on_metadata, metadata_available, metadata_schema_decoded, + stored_metadata.classification); } return signed_metadata_enforce_decision( relied_on_metadata, metadata_available, stored_metadata.classification, diff --git a/unittests/firmware/signed_metadata.cpp b/unittests/firmware/signed_metadata.cpp index 9b64908a3..3eea30863 100644 --- a/unittests/firmware/signed_metadata.cpp +++ b/unittests/firmware/signed_metadata.cpp @@ -1105,26 +1105,55 @@ TEST_F(SignedMetadataTest, V2ZeroArgsSelectorOnly) { EXPECT_EQ(signed_metadata_get()->num_args, 0); } +/* matches_tx() must be idempotent: a second call decodes to the same values + * (regression for the TOKEN_AMOUNT prefix that used to grow on each call). */ +TEST_F(SignedMetadataTest, V2MatchesTxIsIdempotent) { + std::vector blob = v2_base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EthereumSignTx msg; + std::vector data = v2_transfer_calldata(); + make_v2_msg(&msg, CONTRACT_A, data, /*has_len=*/true, (uint32_t)data.size()); + + EXPECT_TRUE(signed_metadata_matches_tx(&msg)); + const SignedMetadata *md = signed_metadata_get(); + uint16_t len_addr = md->args[0].value_len, len_tok = md->args[1].value_len; + + EXPECT_TRUE(signed_metadata_matches_tx(&msg)); // second call + EXPECT_EQ(md->args[0].value_len, len_addr); + EXPECT_EQ(md->args[1].value_len, len_tok); + EXPECT_EQ(md->args[1].value_len, 2 + 4 + 32); + EXPECT_EQ(memcmp(md->args[0].value, RECIPIENT, 20), 0); + EXPECT_EQ(memcmp(md->args[1].value + 6, AMOUNT32, 32), 0); +} + /* ---- v2 enforce truth table (pure, no I/O) ------------------------------ */ +/* Signature: (relied, available, decoded, classification). */ TEST(SignedMetadataEnforceSchema, NotReliedAlwaysAllow) { - EXPECT_TRUE(signed_metadata_enforce_schema_decision(false, true, + EXPECT_TRUE(signed_metadata_enforce_schema_decision(false, true, true, METADATA_VERIFIED)); - EXPECT_TRUE(signed_metadata_enforce_schema_decision(false, false, + EXPECT_TRUE(signed_metadata_enforce_schema_decision(false, false, false, METADATA_OPAQUE)); } -TEST(SignedMetadataEnforceSchema, ReliedVerifiedAllow) { - EXPECT_TRUE(signed_metadata_enforce_schema_decision(true, true, +TEST(SignedMetadataEnforceSchema, ReliedVerifiedDecodedAllow) { + EXPECT_TRUE(signed_metadata_enforce_schema_decision(true, true, true, METADATA_VERIFIED)); } +TEST(SignedMetadataEnforceSchema, ReliedButNotDecodedFails) { + /* The core hardening: relied + available + VERIFIED but decode never ran. */ + EXPECT_FALSE(signed_metadata_enforce_schema_decision(true, true, false, + METADATA_VERIFIED)); +} + TEST(SignedMetadataEnforceSchema, ReliedButUnavailableOrUnverifiedFails) { - EXPECT_FALSE(signed_metadata_enforce_schema_decision(true, false, + EXPECT_FALSE(signed_metadata_enforce_schema_decision(true, false, true, METADATA_VERIFIED)); - EXPECT_FALSE(signed_metadata_enforce_schema_decision(true, true, + EXPECT_FALSE(signed_metadata_enforce_schema_decision(true, true, true, METADATA_OPAQUE)); - EXPECT_FALSE(signed_metadata_enforce_schema_decision(true, true, + EXPECT_FALSE(signed_metadata_enforce_schema_decision(true, true, true, METADATA_MALFORMED)); } From 55d028ccfb975795354a8018371e71e3080b7b7f Mon Sep 17 00:00:00 2001 From: highlander Date: Fri, 3 Jul 2026 14:00:41 -0500 Subject: [PATCH 23/25] fix(clearsign): reset v2 decode flag per matches_tx call (no stale proof) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (cherry picked from commit ecffaa0a40ff77c42022553326e8574b13566b4b) --- include/keepkey/firmware/signed_metadata.h | 7 +++++++ lib/firmware/signed_metadata.c | 9 +++++++++ unittests/firmware/signed_metadata.cpp | 23 ++++++++++++++++++++++ 3 files changed, 39 insertions(+) diff --git a/include/keepkey/firmware/signed_metadata.h b/include/keepkey/firmware/signed_metadata.h index 2b3705db9..b2cf2800c 100644 --- a/include/keepkey/firmware/signed_metadata.h +++ b/include/keepkey/firmware/signed_metadata.h @@ -90,6 +90,13 @@ typedef struct { } SignedMetadata; bool signed_metadata_available(void); + +/* True when the stored v2 (schema) metadata was decoded from the current tx's + * calldata by the most recent signed_metadata_matches_tx() call. Reset at the + * top of every matches_tx() so it reflects only that call (never a stale prior + * match). The v2 enforce path requires it; exported for unit testing. */ +bool signed_metadata_schema_decoded(void); + void signed_metadata_clear(void); /* diff --git a/lib/firmware/signed_metadata.c b/lib/firmware/signed_metadata.c index 3eff799a0..d81a9efcc 100644 --- a/lib/firmware/signed_metadata.c +++ b/lib/firmware/signed_metadata.c @@ -380,6 +380,8 @@ static void bn_from_metadata_bytes(const uint8_t *value, size_t value_len, bool signed_metadata_available(void) { return metadata_available; } +bool signed_metadata_schema_decoded(void) { return metadata_schema_decoded; } + void signed_metadata_clear(void) { memzero(&stored_metadata, sizeof(stored_metadata)); metadata_available = false; @@ -509,6 +511,13 @@ MetadataClassification signed_metadata_process(const uint8_t *payload, } bool signed_metadata_matches_tx(const EthereumSignTx *msg) { + /* Reset the v2 decode proof up front: it must reflect ONLY the current call. + * Any early return below (unavailable, wrong contract/selector/chain) leaves + * it false, so a stale `true` from a prior successful match can never let + * signed_metadata_enforce() pass for a v2 blob that did not decode this tx. + */ + metadata_schema_decoded = false; + if (!metadata_available || !msg || stored_metadata.classification != METADATA_VERIFIED || msg->to.size != sizeof(stored_metadata.contract_address) || diff --git a/unittests/firmware/signed_metadata.cpp b/unittests/firmware/signed_metadata.cpp index 3eea30863..2fd904b68 100644 --- a/unittests/firmware/signed_metadata.cpp +++ b/unittests/firmware/signed_metadata.cpp @@ -1127,6 +1127,29 @@ TEST_F(SignedMetadataTest, V2MatchesTxIsIdempotent) { EXPECT_EQ(memcmp(md->args[1].value + 6, AMOUNT32, 32), 0); } +/* The v2 decode flag must reflect ONLY the latest matches_tx() call: a + * successful decode followed by a mismatching tx must leave it false, so a + * stale "decoded" proof can never survive into enforce. */ +TEST_F(SignedMetadataTest, V2SchemaDecodedFlagNotStaleAfterMismatch) { + std::vector blob = v2_base_blob(); + ASSERT_EQ(signed_metadata_process(blob.data(), blob.size(), TEST_KEY_ID), + METADATA_VERIFIED); + EXPECT_FALSE(signed_metadata_schema_decoded()); // not decoded yet + + EthereumSignTx ok; + std::vector data = v2_transfer_calldata(); + make_v2_msg(&ok, CONTRACT_A, data, /*has_len=*/true, (uint32_t)data.size()); + ASSERT_TRUE(signed_metadata_matches_tx(&ok)); + EXPECT_TRUE(signed_metadata_schema_decoded()); // decoded this tx + + /* Now a tx that fails an EARLY binding (wrong contract) — before the decode + * branch. The flag must be cleared, not left over from the match above. */ + EthereumSignTx bad; + make_v2_msg(&bad, CONTRACT_B, data, /*has_len=*/true, (uint32_t)data.size()); + EXPECT_FALSE(signed_metadata_matches_tx(&bad)); + EXPECT_FALSE(signed_metadata_schema_decoded()); +} + /* ---- v2 enforce truth table (pure, no I/O) ------------------------------ */ /* Signature: (relied, available, decoded, classification). */ From 5c89df26f3d5bd24bf4e163b6916d35cdd5757b4 Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 7 Jul 2026 16:27:55 -0300 Subject: [PATCH 24/25] fix(proto): add firmware-side max_size for LoadClearsignSigner.icon 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 ce0294955940e07b202b748a57d124aaad9f0a38) --- include/keepkey/transport/messages-ethereum.options | 1 + 1 file changed, 1 insertion(+) diff --git a/include/keepkey/transport/messages-ethereum.options b/include/keepkey/transport/messages-ethereum.options index eec9eee40..e4a769b7d 100644 --- a/include/keepkey/transport/messages-ethereum.options +++ b/include/keepkey/transport/messages-ethereum.options @@ -51,4 +51,5 @@ EthereumTxMetadata.signed_payload max_size:1024 EthereumMetadataAck.display_summary max_size:32 LoadClearsignSigner.pubkey max_size:33 LoadClearsignSigner.alias max_size:32 +LoadClearsignSigner.icon max_size:384 From b0c279d18c378dc9ecb11fa093b1d1f20f16e59a Mon Sep 17 00:00:00 2001 From: highlander Date: Tue, 7 Jul 2026 19:31:30 -0300 Subject: [PATCH 25/25] chore(proto): nanopb max_size for ripple.memo + thorchain.denom (superset 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. --- include/keepkey/transport/messages-ripple.options | 2 ++ include/keepkey/transport/messages-thorchain.options | 1 + 2 files changed, 3 insertions(+) diff --git a/include/keepkey/transport/messages-ripple.options b/include/keepkey/transport/messages-ripple.options index b3f2e1987..219014bf6 100644 --- a/include/keepkey/transport/messages-ripple.options +++ b/include/keepkey/transport/messages-ripple.options @@ -6,5 +6,7 @@ RippleSignTx.address_n max_count:8 RipplePayment.destination max_size:36 +RippleSignTx.memo max_size:200 + RippleSignedTx.signature max_size:75 RippleSignedTx.serialized_tx max_size:1024 diff --git a/include/keepkey/transport/messages-thorchain.options b/include/keepkey/transport/messages-thorchain.options index 14cb39b18..c17a9368c 100644 --- a/include/keepkey/transport/messages-thorchain.options +++ b/include/keepkey/transport/messages-thorchain.options @@ -8,6 +8,7 @@ ThorchainSignTx.memo max_size:256 ThorchainMsgSend.from_address max_size:46 ThorchainMsgSend.to_address max_size:46 +ThorchainMsgSend.denom max_size:69 ThorchainMsgDeposit.asset max_size:20 ThorchainMsgDeposit.memo max_size:256