From 639406b01fc439058e5ecae1ef5441db7d4a9f08 Mon Sep 17 00:00:00 2001 From: matejcik Date: Wed, 1 Sep 2021 12:04:39 +0200 Subject: [PATCH] feat(all): make chain_id mandatory --- common/protob/messages-ethereum.proto | 2 +- core/.changelog.d/1794.incompatible | 1 + core/src/apps/ethereum/sign_tx.py | 19 ++++------ core/src/trezor/messages.py | 4 +- core/tests/test_apps.ethereum.keychain.py | 38 ------------------- .../firmware/.changelog.d/1794.incompatible | 1 + legacy/firmware/ethereum.c | 37 +++++++----------- python/src/trezorlib/messages.py | 6 +-- tests/device_tests/ethereum/test_signtx.py | 1 + 9 files changed, 29 insertions(+), 80 deletions(-) create mode 100644 core/.changelog.d/1794.incompatible create mode 100644 legacy/firmware/.changelog.d/1794.incompatible diff --git a/common/protob/messages-ethereum.proto b/common/protob/messages-ethereum.proto index bbd8fbc824..dcccdb0e53 100644 --- a/common/protob/messages-ethereum.proto +++ b/common/protob/messages-ethereum.proto @@ -65,7 +65,7 @@ message EthereumSignTx { optional bytes value = 6; // <=256 bit unsigned big endian (in wei) optional bytes data_initial_chunk = 7; // The initial data chunk (<= 1024 bytes) optional uint32 data_length = 8; // Length of transaction payload - optional uint64 chain_id = 9; // Chain Id for EIP 155 + required uint64 chain_id = 9; // Chain Id for EIP 155 optional uint32 tx_type = 10; // Used for Wanchain } diff --git a/core/.changelog.d/1794.incompatible b/core/.changelog.d/1794.incompatible new file mode 100644 index 0000000000..5c1e493657 --- /dev/null +++ b/core/.changelog.d/1794.incompatible @@ -0,0 +1 @@ +Ethereum non-EIP-155 cross-chain signing is no longer supported. diff --git a/core/src/apps/ethereum/sign_tx.py b/core/src/apps/ethereum/sign_tx.py index 565ceebfdd..36346ce361 100644 --- a/core/src/apps/ethereum/sign_tx.py +++ b/core/src/apps/ethereum/sign_tx.py @@ -75,10 +75,9 @@ async def sign_tx(ctx, msg, keychain): sha.extend(resp.data_chunk) # eip 155 replay protection - if msg.chain_id: - rlp.write(sha, msg.chain_id) - rlp.write(sha, 0) - rlp.write(sha, 0) + rlp.write(sha, msg.chain_id) + rlp.write(sha, 0) + rlp.write(sha, 0) digest = sha.get_digest() result = sign_digest(msg, keychain, digest) @@ -119,14 +118,12 @@ def get_total_length(msg: EthereumSignTx, data_total: int) -> int: msg.gas_limit, address.bytes_from_address(msg.to), msg.value, + msg.chain_id, + 0, + 0, ): length += rlp.length(item) - if msg.chain_id: # forks replay protection - length += rlp.length(msg.chain_id) - length += rlp.length(0) - length += rlp.length(0) - length += rlp.header_length(data_total, msg.data_initial_chunk) length += data_total @@ -154,7 +151,7 @@ def sign_digest(msg: EthereumSignTx, keychain, digest): req.signature_v = signature[0] if msg.chain_id > MAX_CHAIN_ID: req.signature_v -= 27 - elif msg.chain_id: + else: req.signature_v += 2 * msg.chain_id + 8 req.signature_r = signature[1:33] @@ -217,6 +214,4 @@ def sanitize(msg): msg.to = "" if msg.nonce is None: msg.nonce = b"" - if msg.chain_id is None: - msg.chain_id = 0 return msg diff --git a/core/src/trezor/messages.py b/core/src/trezor/messages.py index 7d0fc5014d..075621691b 100644 --- a/core/src/trezor/messages.py +++ b/core/src/trezor/messages.py @@ -2991,12 +2991,13 @@ if TYPE_CHECKING: value: "bytes | None" data_initial_chunk: "bytes | None" data_length: "int | None" - chain_id: "int | None" + chain_id: "int" tx_type: "int | None" def __init__( self, *, + chain_id: "int", address_n: "list[int] | None" = None, nonce: "bytes | None" = None, gas_price: "bytes | None" = None, @@ -3005,7 +3006,6 @@ if TYPE_CHECKING: value: "bytes | None" = None, data_initial_chunk: "bytes | None" = None, data_length: "int | None" = None, - chain_id: "int | None" = None, tx_type: "int | None" = None, ) -> None: pass diff --git a/core/tests/test_apps.ethereum.keychain.py b/core/tests/test_apps.ethereum.keychain.py index 532703c132..4b28c2ba81 100644 --- a/core/tests/test_apps.ethereum.keychain.py +++ b/core/tests/test_apps.ethereum.keychain.py @@ -155,44 +155,6 @@ class TestEthereumKeychain(unittest.TestCase): ) ) - def test_missing_chain_id(self): - @with_keychain_from_chain_id - async def handler_chain_id(ctx, msg, keychain): - slip44_id = msg.address_n[1] & ~HARDENED - # standard tests - self._check_keychain(keychain, slip44_id) - # provided address should succeed too - keychain.derive(msg.address_n) - - await_result( # Ethereum - handler_chain_id( - wire.DUMMY_CONTEXT, - EthereumSignTx( - address_n=[44 | HARDENED, 60 | HARDENED, 0 | HARDENED], - chain_id=None, - ), - ) - ) - - await_result( # Ethereum Classic - handler_chain_id( - wire.DUMMY_CONTEXT, - EthereumSignTx( - address_n=[44 | HARDENED, 61 | HARDENED, 0 | HARDENED], - ), - ) - ) - - with self.assertRaises(wire.DataError): - await_result( # unknown slip44 id - handler_chain_id( - wire.DUMMY_CONTEXT, - EthereumSignTx( - address_n=[44 | HARDENED, 0 | HARDENED, 0 | HARDENED], - ), - ) - ) - def test_wanchain(self): @with_keychain_from_chain_id async def handler_wanchain(ctx, msg, keychain): diff --git a/legacy/firmware/.changelog.d/1794.incompatible b/legacy/firmware/.changelog.d/1794.incompatible new file mode 100644 index 0000000000..5c1e493657 --- /dev/null +++ b/legacy/firmware/.changelog.d/1794.incompatible @@ -0,0 +1 @@ +Ethereum non-EIP-155 cross-chain signing is no longer supported. diff --git a/legacy/firmware/ethereum.c b/legacy/firmware/ethereum.c index b54b772b70..3fa79b9d36 100644 --- a/legacy/firmware/ethereum.c +++ b/legacy/firmware/ethereum.c @@ -193,12 +193,10 @@ static void send_signature(void) { layoutProgress(_("Signing"), 1000); /* eip-155 replay protection */ - if (chain_id) { - /* hash v=chain_id, r=0, s=0 */ - hash_rlp_number(chain_id); - hash_rlp_length(0, 0); - hash_rlp_length(0, 0); - } + /* hash v=chain_id, r=0, s=0 */ + hash_rlp_number(chain_id); + hash_rlp_length(0, 0); + hash_rlp_length(0, 0); keccak_Final(&keccak_ctx, hash); if (ecdsa_sign_digest(&secp256k1, privkey, hash, sig, &v, @@ -216,10 +214,8 @@ static void send_signature(void) { msg_tx_request.has_signature_v = true; if (chain_id > MAX_CHAIN_ID) { msg_tx_request.signature_v = v; - } else if (chain_id) { - msg_tx_request.signature_v = v + 2 * chain_id + 35; } else { - msg_tx_request.signature_v = v + 27; + msg_tx_request.signature_v = v + 2 * chain_id + 35; } msg_tx_request.has_signature_r = true; @@ -438,17 +434,12 @@ void ethereum_signing_init(EthereumSignTx *msg, const HDNode *node) { if (!msg->has_nonce) msg->nonce.size = 0; /* eip-155 chain id */ - if (msg->has_chain_id) { - if (msg->chain_id < 1) { - fsm_sendFailure(FailureType_Failure_DataError, - _("Chain Id out of bounds")); - ethereum_signing_abort(); - return; - } - chain_id = msg->chain_id; - } else { - chain_id = 0; + if (msg->chain_id < 1) { + fsm_sendFailure(FailureType_Failure_DataError, _("Chain ID out of bounds")); + ethereum_signing_abort(); + return; } + chain_id = msg->chain_id; /* Wanchain txtype */ if (msg->has_tx_type) { @@ -558,11 +549,9 @@ void ethereum_signing_init(EthereumSignTx *msg, const HDNode *node) { if (tx_type) { rlp_length += rlp_calculate_number_length(tx_type); } - if (chain_id) { - rlp_length += rlp_calculate_number_length(chain_id); - rlp_length += rlp_calculate_length(0, 0); - rlp_length += rlp_calculate_length(0, 0); - } + rlp_length += rlp_calculate_number_length(chain_id); + rlp_length += rlp_calculate_length(0, 0); + rlp_length += rlp_calculate_length(0, 0); /* Stage 2: Store header fields */ hash_rlp_list_length(rlp_length); diff --git a/python/src/trezorlib/messages.py b/python/src/trezorlib/messages.py index 6d1283ebb9..932798e90f 100644 --- a/python/src/trezorlib/messages.py +++ b/python/src/trezorlib/messages.py @@ -4264,13 +4264,14 @@ class EthereumSignTx(protobuf.MessageType): 6: protobuf.Field("value", "bytes", repeated=False, required=False), 7: protobuf.Field("data_initial_chunk", "bytes", repeated=False, required=False), 8: protobuf.Field("data_length", "uint32", repeated=False, required=False), - 9: protobuf.Field("chain_id", "uint64", repeated=False, required=False), + 9: protobuf.Field("chain_id", "uint64", repeated=False, required=True), 10: protobuf.Field("tx_type", "uint32", repeated=False, required=False), } def __init__( self, *, + chain_id: "int", address_n: Optional[List["int"]] = None, nonce: Optional["bytes"] = None, gas_price: Optional["bytes"] = None, @@ -4279,10 +4280,10 @@ class EthereumSignTx(protobuf.MessageType): value: Optional["bytes"] = None, data_initial_chunk: Optional["bytes"] = None, data_length: Optional["int"] = None, - chain_id: Optional["int"] = None, tx_type: Optional["int"] = None, ) -> None: self.address_n = address_n if address_n is not None else [] + self.chain_id = chain_id self.nonce = nonce self.gas_price = gas_price self.gas_limit = gas_limit @@ -4290,7 +4291,6 @@ class EthereumSignTx(protobuf.MessageType): self.value = value self.data_initial_chunk = data_initial_chunk self.data_length = data_length - self.chain_id = chain_id self.tx_type = tx_type diff --git a/tests/device_tests/ethereum/test_signtx.py b/tests/device_tests/ethereum/test_signtx.py index ad11d88f82..296e8d4c46 100644 --- a/tests/device_tests/ethereum/test_signtx.py +++ b/tests/device_tests/ethereum/test_signtx.py @@ -178,6 +178,7 @@ def test_data_streaming(client): to=TO_ADDR, value=0, data=b"ABCDEFGHIJKLMNOP" * 256 + b"!!!", + chain_id=1, )