From fd56fa6c72e3e5b81423f65f4b3184be0ee9b18e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Vejpustek?= Date: Tue, 22 Oct 2024 13:04:01 +0200 Subject: [PATCH] fix(core): disallow per-node paths in getaddress --- core/src/apps/bitcoin/common.py | 12 +++++ core/src/apps/bitcoin/get_address.py | 26 +++++++++- .../src/trezor/ui/layouts/mercury/__init__.py | 11 +++++ core/src/trezor/ui/layouts/tr/__init__.py | 8 ++++ core/src/trezor/ui/layouts/tt/__init__.py | 8 ++++ tests/device_tests/bitcoin/test_getaddress.py | 48 +++++++++++++++++++ tests/input_flows.py | 1 + 7 files changed, 113 insertions(+), 1 deletion(-) diff --git a/core/src/apps/bitcoin/common.py b/core/src/apps/bitcoin/common.py index 92f63be59b..d8426375dc 100644 --- a/core/src/apps/bitcoin/common.py +++ b/core/src/apps/bitcoin/common.py @@ -5,6 +5,7 @@ from trezor import wire from trezor.crypto import bech32 from trezor.crypto.curve import bip340 from trezor.enums import InputScriptType, OutputScriptType +from trezor.messages import MultisigRedeemScriptType if TYPE_CHECKING: from enum import IntEnum @@ -263,3 +264,14 @@ def descriptor_checksum(desc: str) -> str: for j in range(0, 8): ret[j] = CHECKSUM_CHARSET[(c >> (5 * (7 - j))) & 31] return "".join(ret) + + +def multisig_uses_single_path(multisig: MultisigRedeemScriptType) -> bool: + if not multisig.pubkeys: + # Pubkeys are specified by multisig.nodes and multisig.address_n, in this case all the pubkeys use the same path + return True + else: + # Pubkeys are specified by multisig.pubkeys, in this case we check that all the pubkeys use the same path + return all( + [hd.address_n == multisig.pubkeys[0].address_n for hd in multisig.pubkeys] + ) diff --git a/core/src/apps/bitcoin/get_address.py b/core/src/apps/bitcoin/get_address.py index e774c38784..2eee86a9b6 100644 --- a/core/src/apps/bitcoin/get_address.py +++ b/core/src/apps/bitcoin/get_address.py @@ -2,6 +2,9 @@ from typing import TYPE_CHECKING from trezor.enums import MultisigPubkeysOrder +from apps.common import safety_checks + +from .common import multisig_uses_single_path from .keychain import with_keychain if TYPE_CHECKING: @@ -35,7 +38,11 @@ def _get_xpubs( async def get_address(msg: GetAddress, keychain: Keychain, coin: CoinInfo) -> Address: from trezor.enums import InputScriptType from trezor.messages import Address - from trezor.ui.layouts import confirm_multisig_warning, show_address + from trezor.ui.layouts import ( + confirm_multisig_different_paths_warning, + confirm_multisig_warning, + show_address, + ) from apps.common.address_mac import get_address_mac from apps.common.paths import address_n_to_str, validate_path @@ -107,6 +114,23 @@ async def get_address(msg: GetAddress, keychain: Keychain, coin: CoinInfo) -> Ad await confirm_multisig_warning() + if not multisig_uses_single_path(multisig): + # An address that uses different derivation paths for different xpubs + # could be difficult to discover if the user did not note all the paths. + # The reason is that each path ends with an address index, which can have + # 1,000,000 possible values. If the address is a t-out-of-n multisig, the + # total number of possible paths is 1,000,000^n. This can be exploited by + # an attacker who has compromised the user's computer. The attacker could + # randomize the address indices and then demand a ransom from the user to + # reveal the paths. To prevent this, we require that all xpubs use the + # same derivation path. + if safety_checks.is_strict(): + raise ValueError( + "Using different paths for different xpubs is not allowed" + ) + else: + await confirm_multisig_different_paths_warning() + if multisig.pubkeys_order == MultisigPubkeysOrder.LEXICOGRAPHIC: account = f"Multisig {multisig.m} of {len(pubnodes)}\n(sorted)" else: diff --git a/core/src/trezor/ui/layouts/mercury/__init__.py b/core/src/trezor/ui/layouts/mercury/__init__.py index ca61bd9be2..0025c417e8 100644 --- a/core/src/trezor/ui/layouts/mercury/__init__.py +++ b/core/src/trezor/ui/layouts/mercury/__init__.py @@ -139,6 +139,17 @@ def confirm_multisig_warning() -> Awaitable[None]: ) +def confirm_multisig_different_paths_warning() -> Awaitable[None]: + return raise_if_not_confirmed( + trezorui2.flow_warning_hi_prio( + title=f"{TR.words__important}!", + description="Using different paths for different XPUBs.", + ), + "warning_multisig_different_paths", + br_code=ButtonRequestType.Warning, + ) + + def confirm_homescreen( image: bytes, ) -> Awaitable[None]: diff --git a/core/src/trezor/ui/layouts/tr/__init__.py b/core/src/trezor/ui/layouts/tr/__init__.py index 4470823307..c8ca94f96c 100644 --- a/core/src/trezor/ui/layouts/tr/__init__.py +++ b/core/src/trezor/ui/layouts/tr/__init__.py @@ -181,6 +181,14 @@ def confirm_multisig_warning() -> Awaitable[ui.UiResult]: ) +def confirm_multisig_different_paths_warning() -> Awaitable[ui.UiResult]: + return show_warning( + "warning_multisig_different_paths", + "Using different paths for different XPUBs.", + TR.words__continue_anyway_question, + ) + + def confirm_homescreen(image: bytes) -> Awaitable[None]: return raise_if_not_confirmed( trezorui2.confirm_homescreen( diff --git a/core/src/trezor/ui/layouts/tt/__init__.py b/core/src/trezor/ui/layouts/tt/__init__.py index aecd69d7ee..ad91c917aa 100644 --- a/core/src/trezor/ui/layouts/tt/__init__.py +++ b/core/src/trezor/ui/layouts/tt/__init__.py @@ -162,6 +162,14 @@ def confirm_multisig_warning() -> Awaitable[None]: ) +def confirm_multisig_different_paths_warning() -> Awaitable[None]: + return show_warning( + "warning_multisig_different_paths", + "Using different paths for different XPUBs.", + TR.words__continue_anyway_question, + ) + + def confirm_homescreen(image: bytes) -> Awaitable[None]: return raise_if_not_confirmed( trezorui2.confirm_homescreen( diff --git a/tests/device_tests/bitcoin/test_getaddress.py b/tests/device_tests/bitcoin/test_getaddress.py index ecdcb58613..73d984a4ce 100644 --- a/tests/device_tests/bitcoin/test_getaddress.py +++ b/tests/device_tests/bitcoin/test_getaddress.py @@ -435,3 +435,51 @@ def test_crw(client: Client): btc.get_address(client, "Crown", parse_path("m/44h/72h/0h/0/0")) == "CRWYdvZM1yXMKQxeN3hRsAbwa7drfvTwys48" ) + + +@pytest.mark.multisig +@pytest.mark.models(skip="legacy", reason="Not fixed") +def test_multisig_different_paths(client: Client): + nodes = [ + btc.get_public_node(client, parse_path(f"m/45h/{i}"), coin_name="Bitcoin").node + for i in range(2) + ] + + multisig = messages.MultisigRedeemScriptType( + pubkeys=[ + messages.HDNodePathType(node=node, address_n=[0, i]) + for i, node in enumerate(nodes) + ], + signatures=[b"", b"", b""], + m=2, + ) + + with pytest.raises( + Exception, match="Using different paths for different xpubs is not allowed" + ): + with client: + if is_core(client): + IF = InputFlowConfirmAllWarnings(client) + client.set_input_flow(IF.get()) + btc.get_address( + client, + "Bitcoin", + parse_path("m/45h/0/0/0"), + show_display=True, + multisig=multisig, + script_type=messages.InputScriptType.SPENDMULTISIG, + ) + + device.apply_settings(client, safety_checks=SafetyCheckLevel.PromptTemporarily) + with client: + if is_core(client): + IF = InputFlowConfirmAllWarnings(client) + client.set_input_flow(IF.get()) + btc.get_address( + client, + "Bitcoin", + parse_path("m/45h/0/0/0"), + show_display=True, + multisig=multisig, + script_type=messages.InputScriptType.SPENDMULTISIG, + ) diff --git a/tests/input_flows.py b/tests/input_flows.py index ff1782a1fe..24cf6a5093 100644 --- a/tests/input_flows.py +++ b/tests/input_flows.py @@ -2264,6 +2264,7 @@ class InputFlowConfirmAllWarnings(InputFlowBase): "witness path", "certificate path", "pool owner staking path", + "using different paths for different xpubs", ) if any(needle.lower() in text for needle in hi_prio): self.debug.click(buttons.CORNER_BUTTON)