From 5c4703c9bbfb962fbbe409c2e40030f30161e4f0 Mon Sep 17 00:00:00 2001 From: Alois Klink Date: Wed, 22 Dec 2021 23:36:05 +0000 Subject: [PATCH] fix(core,legacy): Fix domain-only ethTypedData When doing Ethereum signTypedData, and the primaryType="EIP712Domain", we completely ignore the "message" part and only sign the domain. According to the community, this is technically allowed by the spec, and may be used by ETH smart contracts to save on gas. Test case generated by @MetaMask/eth-sig-util's library. See: https://ethereum-magicians.org/t/eip-712-standards-clarification-primarytype-as-domaintype/3286 --- common/protob/messages-ethereum.proto | 2 +- .../fixtures/ethereum/sign_typed_data.json | 71 +++++++++++++++++++ core/.changelog.d/2036.fixed | 1 + core/src/apps/ethereum/layout.py | 22 ++++-- core/src/apps/ethereum/sign_typed_data.py | 40 ++++++----- core/src/trezor/messages.py | 4 +- legacy/firmware/.changelog.d/2036.fixed | 1 + legacy/firmware/ethereum.c | 13 +++- legacy/firmware/fsm_msg_ethereum.h | 20 ++++-- python/.changelog.d/2036.fixed | 2 + python/src/trezorlib/cli/ethereum.py | 4 +- python/src/trezorlib/ethereum.py | 5 +- python/src/trezorlib/messages.py | 4 +- .../ethereum/test_sign_typed_data.py | 5 +- tests/ui_tests/fixtures.json | 14 ++-- 15 files changed, 163 insertions(+), 45 deletions(-) create mode 100644 core/.changelog.d/2036.fixed create mode 100644 legacy/firmware/.changelog.d/2036.fixed create mode 100644 python/.changelog.d/2036.fixed diff --git a/common/protob/messages-ethereum.proto b/common/protob/messages-ethereum.proto index 1b0bd52bf..8ac7d768e 100644 --- a/common/protob/messages-ethereum.proto +++ b/common/protob/messages-ethereum.proto @@ -159,7 +159,7 @@ message EthereumVerifyMessage { message EthereumSignTypedHash { repeated uint32 address_n = 1; // BIP-32 path to derive the key from master node required bytes domain_separator_hash = 2; // Hash of domainSeparator of typed data to be signed - required bytes message_hash = 3; // Hash of the data of typed data to be signed + optional bytes message_hash = 3; // Hash of the data of typed data to be signed (empty if domain-only data) } /** diff --git a/common/tests/fixtures/ethereum/sign_typed_data.json b/common/tests/fixtures/ethereum/sign_typed_data.json index dabb9dd49..ae98bb401 100644 --- a/common/tests/fixtures/ethereum/sign_typed_data.json +++ b/common/tests/fixtures/ethereum/sign_typed_data.json @@ -4,6 +4,77 @@ "passphrase": "" }, "tests": [ + { + "name": "bare_minimum", + "comment": "Bare minimum EIP-712 message (domain only)", + "parameters": { + "path": "m/44'/60'/0'/0/0", + "metamask_v4_compat": true, + "data": { + "types": { + "EIP712Domain": [] + }, + "primaryType": "EIP712Domain", + "message": {}, + "domain": {} + }, + "message_hash": null, + "domain_separator_hash": "0x6192106f129ce05c9075d319c1fa6ea9b3ae37cbd0c1ef92e2be7137bb07baa1" + }, + "result": { + "address": "0x73d0385F4d8E00C5e6504C6030F47BF6212736A8", + "sig": "0x18aaea9abed7cd88d3763a9a420e2e7b71a9f991685fbc62d74da86326cffa680644862d459d1973e422777a3933bc74190b1cae9a5418ddaea645a7d7630dd91c" + } + }, + { + "name": "full_domain_empty_message", + "comment": "Domain only EIP-712 message", + "parameters": { + "path": "m/44'/60'/0'/0/0", + "metamask_v4_compat": true, + "data": { + "types": { + "EIP712Domain": [ + { + "name": "name", + "type": "string" + }, + { + "name": "version", + "type": "string" + }, + { + "name": "chainId", + "type": "uint256" + }, + { + "name": "verifyingContract", + "type": "address" + }, + { + "name": "salt", + "type": "bytes32" + } + ] + }, + "primaryType": "EIP712Domain", + "message": {}, + "domain": { + "name": "Trezor", + "version": "Test v0.0.0", + "chainId": 1, + "verifyingContract": "0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC", + "salt": "0x0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + } + }, + "message_hash": null, + "domain_separator_hash": "0xf85aaf157e9a36dc6e12643fff450fdf8d98fd0d0e41c5b42bb1f7aae6c83388" + }, + "result": { + "address": "0x73d0385F4d8E00C5e6504C6030F47BF6212736A8", + "sig": "0x98a3e66f738002da98c70b976ef131c11ed8b94aad872140574ed2a2d4a2bac53a9350e284994274f0a7ce1191cf79bf13f2f0d0a862dcf0dd86ad8141eb90dc1c" + } + }, { "name": "basic_data", "parameters": { diff --git a/core/.changelog.d/2036.fixed b/core/.changelog.d/2036.fixed new file mode 100644 index 000000000..c402937a7 --- /dev/null +++ b/core/.changelog.d/2036.fixed @@ -0,0 +1 @@ +Fix domain-only EIP-712 hashes (i.e. when `primaryType`=`EIP712Domain`) diff --git a/core/src/apps/ethereum/layout.py b/core/src/apps/ethereum/layout.py index 3480c36ec..863661e4f 100644 --- a/core/src/apps/ethereum/layout.py +++ b/core/src/apps/ethereum/layout.py @@ -6,6 +6,7 @@ from trezor.enums import ButtonRequestType, EthereumDataType from trezor.messages import EthereumFieldType, EthereumStructMember from trezor.strings import format_amount, format_plural from trezor.ui.layouts import ( + confirm_action, confirm_address, confirm_amount, confirm_blob, @@ -111,16 +112,27 @@ def require_confirm_data(ctx: Context, data: bytes, data_total: int) -> Awaitabl ) -async def confirm_hash(ctx: Context, message_hash: bytes) -> None: - await confirm_blob( +async def confirm_typed_data_final(ctx: Context) -> None: + await confirm_action( ctx, - "confirm_hash", - title="Confirm hash", - data="0x" + hexlify(message_hash).decode(), + "confirm_typed_data_final", + title="Confirm typed data", + action="Really sign EIP-712 typed data?", + verb="Hold to confirm", hold=True, ) +def confirm_empty_typed_message(ctx: Context) -> Awaitable[None]: + return confirm_text( + ctx, + "confirm_empty_typed_message", + title="Confirm message", + data="", + description="No message field", + ) + + async def should_show_domain(ctx: Context, name: bytes, version: bytes) -> bool: domain_name = decode_typed_data(name, "string") domain_version = decode_typed_data(version, "string") diff --git a/core/src/apps/ethereum/sign_typed_data.py b/core/src/apps/ethereum/sign_typed_data.py index d423068c8..87bd8b31d 100644 --- a/core/src/apps/ethereum/sign_typed_data.py +++ b/core/src/apps/ethereum/sign_typed_data.py @@ -20,7 +20,8 @@ from apps.common import paths from .helpers import address_from_bytes, get_type_name from .keychain import PATTERNS_ADDRESS, with_keychain_from_path from .layout import ( - confirm_hash, + confirm_empty_typed_message, + confirm_typed_data_final, confirm_typed_value, should_show_array, should_show_domain, @@ -82,23 +83,30 @@ async def generate_typed_data_hash( parent_objects=["EIP712Domain"], ) - show_message = await should_show_struct( - ctx, - description=primary_type, - data_members=typed_data_envelope.types[primary_type].members, - title="Confirm message", - button_text="Show full message", - ) - message_hash = await typed_data_envelope.hash_struct( - primary_type=primary_type, - member_path=[1], - show_data=show_message, - parent_objects=[primary_type], - ) + # Setting the primary_type to "EIP712Domain" is technically in spec + # In this case, we ignore the "message" part and only use the "domain" part + # https://ethereum-magicians.org/t/eip-712-standards-clarification-primarytype-as-domaintype/3286 + if primary_type == "EIP712Domain": + await confirm_empty_typed_message(ctx) + message_hash = b"" + else: + show_message = await should_show_struct( + ctx, + description=primary_type, + data_members=typed_data_envelope.types[primary_type].members, + title="Confirm message", + button_text="Show full message", + ) + message_hash = await typed_data_envelope.hash_struct( + primary_type=primary_type, + member_path=[1], + show_data=show_message, + parent_objects=[primary_type], + ) - await confirm_hash(ctx, message_hash) + await confirm_typed_data_final(ctx) - return keccak256(b"\x19" + b"\x01" + domain_separator + message_hash) + return keccak256(b"\x19\x01" + domain_separator + message_hash) def get_hash_writer() -> HashWriter: diff --git a/core/src/trezor/messages.py b/core/src/trezor/messages.py index f59e4b841..71bea4642 100644 --- a/core/src/trezor/messages.py +++ b/core/src/trezor/messages.py @@ -3234,14 +3234,14 @@ if TYPE_CHECKING: class EthereumSignTypedHash(protobuf.MessageType): address_n: "list[int]" domain_separator_hash: "bytes" - message_hash: "bytes" + message_hash: "bytes | None" def __init__( self, *, domain_separator_hash: "bytes", - message_hash: "bytes", address_n: "list[int] | None" = None, + message_hash: "bytes | None" = None, ) -> None: pass diff --git a/legacy/firmware/.changelog.d/2036.fixed b/legacy/firmware/.changelog.d/2036.fixed new file mode 100644 index 000000000..c402937a7 --- /dev/null +++ b/legacy/firmware/.changelog.d/2036.fixed @@ -0,0 +1 @@ +Fix domain-only EIP-712 hashes (i.e. when `primaryType`=`EIP712Domain`) diff --git a/legacy/firmware/ethereum.c b/legacy/firmware/ethereum.c index ae9c88559..66289f393 100644 --- a/legacy/firmware/ethereum.c +++ b/legacy/firmware/ethereum.c @@ -976,14 +976,20 @@ int ethereum_message_verify(const EthereumVerifyMessage *msg) { return 0; } +/* + * EIP-712 hashes might have no message_hash if primaryType="EIP712Domain". + * In this case, set has_message_hash=false. + */ static void ethereum_typed_hash(const uint8_t domain_separator_hash[32], const uint8_t message_hash[32], - uint8_t hash[32]) { + bool has_message_hash, uint8_t hash[32]) { struct SHA3_CTX ctx = {0}; sha3_256_Init(&ctx); sha3_Update(&ctx, (const uint8_t *)"\x19\x01", 2); sha3_Update(&ctx, domain_separator_hash, 32); - sha3_Update(&ctx, message_hash, 32); + if (has_message_hash) { + sha3_Update(&ctx, message_hash, 32); + } keccak_Final(&ctx, hash); } @@ -991,8 +997,9 @@ void ethereum_typed_hash_sign(const EthereumSignTypedHash *msg, const HDNode *node, EthereumTypedDataSignature *resp) { uint8_t hash[32] = {0}; + ethereum_typed_hash(msg->domain_separator_hash.bytes, msg->message_hash.bytes, - hash); + msg->has_message_hash, hash); uint8_t v = 0; if (ecdsa_sign_digest(&secp256k1, node->private_key, hash, diff --git a/legacy/firmware/fsm_msg_ethereum.h b/legacy/firmware/fsm_msg_ethereum.h index 41813a58a..226dbb26e 100644 --- a/legacy/firmware/fsm_msg_ethereum.h +++ b/legacy/firmware/fsm_msg_ethereum.h @@ -216,7 +216,8 @@ void fsm_msgEthereumSignTypedHash(const EthereumSignTypedHash *msg) { CHECK_PIN - if (msg->domain_separator_hash.size != 32 || msg->message_hash.size != 32) { + if (msg->domain_separator_hash.size != 32 || + (msg->has_message_hash && msg->message_hash.size != 32)) { fsm_sendFailure(FailureType_Failure_DataError, _("Invalid hash length")); return; } @@ -256,12 +257,17 @@ void fsm_msgEthereumSignTypedHash(const EthereumSignTypedHash *msg) { layoutHome(); return; } - layoutConfirmHash(&bmp_icon_warning, _("EIP-712 message hash"), - msg->message_hash.bytes, 32); - if (!protectButton(ButtonRequestType_ButtonRequest_Other, false)) { - fsm_sendFailure(FailureType_Failure_ActionCancelled, NULL); - layoutHome(); - return; + + // No message hash when setting primaryType="EIP712Domain" + // https://ethereum-magicians.org/t/eip-712-standards-clarification-primarytype-as-domaintype/3286 + if (msg->has_message_hash) { + layoutConfirmHash(&bmp_icon_warning, _("EIP-712 message hash"), + msg->message_hash.bytes, 32); + if (!protectButton(ButtonRequestType_ButtonRequest_Other, false)) { + fsm_sendFailure(FailureType_Failure_ActionCancelled, NULL); + layoutHome(); + return; + } } ethereum_typed_hash_sign(msg, node, resp); diff --git a/python/.changelog.d/2036.fixed b/python/.changelog.d/2036.fixed new file mode 100644 index 000000000..16f663897 --- /dev/null +++ b/python/.changelog.d/2036.fixed @@ -0,0 +1,2 @@ +Allow passing empty `message_hash` for domain-only EIP-712 hashes +for Trezor T1 (i.e. when `primaryType`=`EIP712Domain`) diff --git a/python/src/trezorlib/cli/ethereum.py b/python/src/trezorlib/cli/ethereum.py index 7d5d4c779..61c5d3ddb 100644 --- a/python/src/trezorlib/cli/ethereum.py +++ b/python/src/trezorlib/cli/ethereum.py @@ -455,10 +455,12 @@ def sign_typed_data_hash( Sign hash of typed data (EIP-712) with Ethereum address. For T1 backward compatibility. + + MESSAGE_HASH_HEX can be set to an empty string '' for domain-only hashes. """ address_n = tools.parse_path(address) domain_hash = ethereum.decode_hex(domain_hash_hex) - message_hash = ethereum.decode_hex(message_hash_hex) + message_hash = ethereum.decode_hex(message_hash_hex) if message_hash_hex else None ret = ethereum.sign_typed_data_hash(client, address_n, domain_hash, message_hash) output = { "domain_hash": domain_hash_hex, diff --git a/python/src/trezorlib/ethereum.py b/python/src/trezorlib/ethereum.py index bc3147e1d..098dbecc2 100644 --- a/python/src/trezorlib/ethereum.py +++ b/python/src/trezorlib/ethereum.py @@ -361,7 +361,10 @@ def verify_message( @expect(messages.EthereumTypedDataSignature) def sign_typed_data_hash( - client: "TrezorClient", n: "Address", domain_hash: bytes, message_hash: bytes + client: "TrezorClient", + n: "Address", + domain_hash: bytes, + message_hash: Optional[bytes], ) -> "MessageType": return client.call( messages.EthereumSignTypedHash( diff --git a/python/src/trezorlib/messages.py b/python/src/trezorlib/messages.py index 9548b0a32..162b78872 100644 --- a/python/src/trezorlib/messages.py +++ b/python/src/trezorlib/messages.py @@ -4742,15 +4742,15 @@ class EthereumSignTypedHash(protobuf.MessageType): FIELDS = { 1: protobuf.Field("address_n", "uint32", repeated=True, required=False), 2: protobuf.Field("domain_separator_hash", "bytes", repeated=False, required=True), - 3: protobuf.Field("message_hash", "bytes", repeated=False, required=True), + 3: protobuf.Field("message_hash", "bytes", repeated=False, required=False), } def __init__( self, *, domain_separator_hash: "bytes", - message_hash: "bytes", address_n: Optional[Sequence["int"]] = None, + message_hash: Optional["bytes"] = None, ) -> None: self.address_n: Sequence["int"] = address_n if address_n is not None else [] self.domain_separator_hash = domain_separator_hash diff --git a/tests/device_tests/ethereum/test_sign_typed_data.py b/tests/device_tests/ethereum/test_sign_typed_data.py index 9b184ee1b..47186cef4 100644 --- a/tests/device_tests/ethereum/test_sign_typed_data.py +++ b/tests/device_tests/ethereum/test_sign_typed_data.py @@ -50,7 +50,10 @@ def test_ethereum_sign_typed_data_blind(client, parameters, result): client, address_n, ethereum.decode_hex(parameters["domain_separator_hash"]), - ethereum.decode_hex(parameters["message_hash"]), + # message hash is empty for domain-only hashes + ethereum.decode_hex(parameters["message_hash"]) + if parameters["message_hash"] + else None, ) assert ret.address == result["address"] assert f"0x{ret.signature.hex()}" == result["sig"] diff --git a/tests/ui_tests/fixtures.json b/tests/ui_tests/fixtures.json index 4add89ba5..5b3e28e93 100644 --- a/tests/ui_tests/fixtures.json +++ b/tests/ui_tests/fixtures.json @@ -602,13 +602,15 @@ "ethereum-test_getpublickey.py::test_ethereum_getpublickey[parameters0-result0]": "095af81ec79e9b510c90d9fa34fed343f3840807190c67bc237af885695ae687", "ethereum-test_getpublickey.py::test_ethereum_getpublickey[parameters1-result1]": "095af81ec79e9b510c90d9fa34fed343f3840807190c67bc237af885695ae687", "ethereum-test_getpublickey.py::test_ethereum_getpublickey[parameters2-result2]": "095af81ec79e9b510c90d9fa34fed343f3840807190c67bc237af885695ae687", -"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[basic_data]": "4f512d6beb0222079aaa878a2bcd2c41ac389fdd47edad3e0b0b5779677d8697", -"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[complex_data]": "275a8630a795c966419f6fc6de834bb576bfc3dbc16a1fd7605aa2b8ceae666e", -"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[struct_list_non_v4]": "7dd23b14bd273b937836a24cf056c745e7b3461139f895caefd4624e0d5545f5", -"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[struct_list_v4]": "b7e3475d4906942bc0e8d62203ae91a13ea0d702c3a7a53b9777bea670c4a7f7", -"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[structs_arrays_v4]": "31cc5b5c1d9d94f0761208f5dc6423d283b9118b12b87cf878d20fa144f4f252", +"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[bare_minimum]": "e13b237f58a9977c6edf1917730748b881749acecc3d040b1fd9d794e6e02299", +"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[basic_data]": "84f1d23e031919fedb4ef6b3f3dc1cb667836b9efcd0c5a0da586010e38e1fd6", +"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[complex_data]": "27ee23894c19e5704d02ae684d50a0f0d14678d3f74a95e67fc4975818a1faa8", +"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[full_domain_empty_message]": "c3c299087e9cac4d7554314abc8637d041da69f0c0ee2863c5526f1f89448ae2", +"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[struct_list_non_v4]": "84f1d23e031919fedb4ef6b3f3dc1cb667836b9efcd0c5a0da586010e38e1fd6", +"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[struct_list_v4]": "84f1d23e031919fedb4ef6b3f3dc1cb667836b9efcd0c5a0da586010e38e1fd6", +"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data[structs_arrays_v4]": "84f1d23e031919fedb4ef6b3f3dc1cb667836b9efcd0c5a0da586010e38e1fd6", "ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data_cancel": "08712efae2d007610289bbfb3a8fe6800547e884636c83c5bf0e25f33728789e", -"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data_show_more_button": "1adbea797586685ce09aae58b0a2b89e1617e4eaad23a8c1ac6fc10b041e57a5", +"ethereum-test_sign_typed_data.py::test_ethereum_sign_typed_data_show_more_button": "72959d95278c136cb2071f6452b2a61782b9ac3e6946d8fb5f8db8f153171e6f", "ethereum-test_sign_verify_message.py::test_signmessage[parameters0-result0]": "7aa14b29e5005d8fdc0a8b497ed5d3ebea15c7017f9c457d09214f2d05fbc532", "ethereum-test_sign_verify_message.py::test_signmessage[parameters1-result1]": "c5fb9393267c3d9b9bf5839aab6c641d3931286411f291cd1d8e937cb224ae2d", "ethereum-test_sign_verify_message.py::test_signmessage[parameters2-result2]": "8499b87474becc06010e9b4356398a3e29ee2ef152e04653f15dcc227fc486d6",