fix(core): disallow per-node paths in getaddress

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]
+        )

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()
 
+            # 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 not multisig_uses_single_path(multisig):
+                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:

core/src/apps/bitcoin/sign_tx/change_detector.py

@@ -52,7 +52,21 @@ class ChangeDetector:
         if txo.script_type not in common.CHANGE_OUTPUT_SCRIPT_TYPES:
             return False
 
-        if txo.multisig and not self.multisig_fingerprint.output_matches(txo):
+        if txo.multisig:
+            if not (
+                self.multisig_fingerprint.output_matches(txo)
+                and common.multisig_uses_single_path(
+                    txo.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.
+            ):
                 return False
 
         return (

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]:

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(

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(

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,
+        )

tests/input_flows.py

@@ -2273,6 +2273,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):