diff --git a/docs/release/7.14.2.md b/docs/release/7.14.2.md index b9f1f5383..8e335eb57 100644 --- a/docs/release/7.14.2.md +++ b/docs/release/7.14.2.md @@ -165,6 +165,51 @@ landed, so the table below is the current state, not what #440 originally set. | ETH arbitrary contract data | AdvancedMode | no pager exists for streamed calldata; see #400 and #428 | | TRON `SignTx` | AdvancedMode | `raw_data` still cannot be parsed on this line (#405) | | EVM `SignTx` with **no `chain_id`** | **REFUSED** | produced a pre-EIP-155 signature replayable on every EVM chain, and the confirm screen named no network (#445) | +| 0x `transformERC20` over 1024 bytes | **REFUSED** by default, AdvancedMode to sign | was clear-signed with an undisclosed `transformations[]` tail; now falls through to the raw-hex path (#444) | + +**On the 0x change — this affects real swaps, not an edge case.** +`EthereumSignTx.data_initial_chunk` holds 1024 bytes and 0x aggregates across liquidity +sources, so production routes exceed it. Measured, not estimated: the recorded mainnet +transaction in `deps/python-keepkey/tests/test_msg_ethereum_erc20_0x_signtx.py` +(etherscan `0xcf94f79d…`) is **1480 bytes** of calldata — its second transformation +(`PayTakerTransformer`) starts at byte 1060, already past the boundary. Dropping that +transformation would leave the funds in the Exchange Proxy, so no host emits a shorter +form. + +Those swaps previously showed a clear-signed "Transform ERC20" screen while the tail +streamed in unshown. They now reach the generic contract-data path, and **on a +default-configuration device that path is a refusal**: `AdvancedMode` is off by default +(`include/keepkey/firmware/policy.h:34`) and `lib/firmware/ethereum.c:808` answers with +`fsm_sendFailure(ActionCancelled, "Arbitrary contract data signing disabled by +policy")`. Enabling AdvancedMode signs it and shows the raw-hex disclosure. That is the +policy — never block someone from their own funds, but never present bytes the device +did not interpret as though it had — but the release notes must say plainly that the +default answer for a 0x swap is now "no, turn on AdvancedMode", not "yes, with a worse +screen". + +**This closes #444, which was scoped to the streamed tail.** It does not close the +adjacent in-buffer weakness, and that is deliberate: the gate proves the calldata was +*received*, not *displayed*. `transformERC20`'s ABI head is 164 bytes and the decoder +shows four values, all inside it, so a payload that fits one chunk still clear-signs +with up to 860 bytes of `transformations[]` never shown. That primitive is unchanged by +this commit — the code it lives in is untouched — and it is tracked by **#414**, which +names `zxtransERC20.c` and describes decoders reading confirmation text from hardcoded +ABI offsets without consulting the offset word. + +**The bounded-outcome argument does not hold on every allowlisted chain.** The four +displayed values are static head words and are read correctly. The argument for the old +exemption was that the input amount and minimum output amount bound the loss. Two things +undercut it. First, that bound is enforced by the 0x Exchange Proxy, which is +upgradeable, so it is not a guarantee this firmware can make. Second and more concretely: +`TokenType.chain_id` and `tokenByChainAddress()` are `uint8_t` +(`include/keepkey/firmware/ethereum_tokens.h:42,54`), so of the six chains in +`zx_isExchangeProxyChain()` three truncate — Base `8453 -> 5`, Arbitrum `42161 -> 177`, +Avalanche `43114 -> 106`. The lookup misses, returns `UnknownToken`, and +`ethereumFormatAmount()` short-circuits to the literal `"Unknown token value"` +(`lib/firmware/ethereum.c:347`) for **both** operands. On those chains the screen reads +"Input Unknown token value / Output Unknown token value" and displays no bound at all. +Tracked as **#455**; the immediate remedy is to drop those three chains from the +allowlist so they take the raw-hex path. **AdvancedMode is session state, not a setting** — it resets on power cycle, proven on hardware during the 7.15 work. Anything gated by it must be re-enabled on every diff --git a/lib/firmware/ethereum_contracts.c b/lib/firmware/ethereum_contracts.c index 839f687ee..02cdda856 100644 --- a/lib/firmware/ethereum_contracts.c +++ b/lib/firmware/ethereum_contracts.c @@ -53,19 +53,21 @@ bool ethereum_contractHandled(uint32_t data_total, const EthereumSignTx* msg, const HDNode* node) { (void)node; + /* Every handler parses and displays fixed offsets inside the initial chunk + * only. If the calldata does not fit in that chunk, the remainder streams + * in via EthereumTxAck and is hashed into the signature without ever being + * shown, so refuse to claim the tx and fall through to the generic raw-data + * disclosure path. This gate runs BEFORE any decoder, including 0x + * transformERC20, so a transformERC20 whose transformations[] tail exceeds + * one 1024-byte chunk is NOT clear-signed blind: it falls through to raw + * disclosure (AdvancedMode-gated). */ + if (data_total != msg->data_initial_chunk.size) return false; + /* 0x transformERC20 is pinned to the ExchangeProxy address and its outcome * is bounded by the input amount and minimum output amount shown on screen, - * so it stays clear-signable at any calldata size; its transformations[] - * tail legitimately exceeds one 1024-byte chunk. */ + * so it stays clear-signable at any calldata size that fits one chunk. */ if (zx_isZxTransformERC20(msg)) return true; - /* Every other handler parses and displays fixed offsets inside the initial - * chunk only. If the calldata does not fit in that chunk, the remainder - * streams in via EthereumTxAck and is hashed into the signature without - * ever being shown, so refuse to claim the tx and fall through to the - * generic raw-data disclosure path. */ - 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; diff --git a/unittests/firmware/ethereum.cpp b/unittests/firmware/ethereum.cpp index 67f30d32f..0b09d7199 100644 --- a/unittests/firmware/ethereum.cpp +++ b/unittests/firmware/ethereum.cpp @@ -1,12 +1,16 @@ extern "C" { #include "keepkey/firmware/eip712.h" #include "keepkey/firmware/ethereum.h" +#include "keepkey/firmware/ethereum_contracts.h" +#include "keepkey/firmware/ethereum_contracts/zxtransERC20.h" #include "keepkey/firmware/tron.h" #include "trezor/crypto/address.h" +#include "messages-ethereum.pb.h" } #include "gtest/gtest.h" +#include #include static uint8_t bin_from_ascii(char c) { @@ -88,6 +92,23 @@ TEST(Ethereum, StructuredEip712IsDisabledForPointRelease) { EXPECT_FALSE(ethereum_structured_eip712_enabled()); } +TEST(Ethereum, TransformErc20RequiresCompleteCalldataForClearSigning) { + EthereumSignTx msg{}; + msg.has_to = true; + msg.to.size = 20; + std::memcpy(msg.to.bytes, ZXSWAP_ADDRESS, msg.to.size); + msg.has_chain_id = true; + msg.chain_id = 1; + msg.has_data_initial_chunk = true; + msg.data_initial_chunk.size = 4 + 4 * 32; + std::memcpy(msg.data_initial_chunk.bytes, "\x41\x55\x65\xb0", 4); + + EXPECT_TRUE( + ethereum_contractHandled(msg.data_initial_chunk.size, &msg, nullptr)); + EXPECT_FALSE( + ethereum_contractHandled(msg.data_initial_chunk.size + 1, &msg, nullptr)); +} + TEST(Ethereum, Eip712ChainIdRequiresCanonicalUint32) { uint32_t value = 0; EXPECT_TRUE(eip712_parse_canonical_u32("0", &value));