feat(core): Validate script type of change-outputs in Bitcoin signing.

core/.changelog.d/noissue.security

@@ -0,0 +1 @@
+Match and validate script type of change-outputs in Bitcoin signing.

core/src/apps/bitcoin/authorization.py

@@ -9,11 +9,10 @@ if TYPE_CHECKING:
         GetOwnershipProof,
         SignTx,
         TxInput,
+        TxOutput,
     )
     from trezor.protobuf import MessageType
 
-    from apps.common.coininfo import CoinInfo
-
 FEE_RATE_DECIMALS = const(6)
 
 
@@ -38,15 +37,25 @@ class CoinJoinAuthorization:
             and msg.commitment_data.startswith(bytes(coordinator))
         )
 
-    def check_sign_tx_input(self, txi: TxInput, coin: CoinInfo) -> bool:
-        # Check whether the current input matches the parameters of the request.
+    def check_internal_input(self, txi: TxInput) -> bool:
+        # Check whether the input matches the parameters of the request.
         return (
             len(txi.address_n) >= BIP32_WALLET_DEPTH
             and txi.address_n[:-BIP32_WALLET_DEPTH] == self.params.address_n
-            and coin.coin_name == self.params.coin_name
             and txi.script_type == self.params.script_type
         )
 
+    def check_internal_output(self, txo: TxOutput) -> bool:
+        from .common import CHANGE_OUTPUT_TO_INPUT_SCRIPT_TYPES
+
+        # Check whether the output matches the parameters of the request.
+        return (
+            len(txo.address_n) >= BIP32_WALLET_DEPTH
+            and txo.address_n[:-BIP32_WALLET_DEPTH] == self.params.address_n
+            and CHANGE_OUTPUT_TO_INPUT_SCRIPT_TYPES[txo.script_type]
+            == self.params.script_type
+        )
+
     def approve_sign_tx(self, msg: SignTx) -> bool:
         from apps.common import authorization
 

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

@@ -83,7 +83,7 @@ class Approver:
             if txi.orig_hash:
                 self.orig_external_in += txi.amount
 
-    def _add_output(self, txo: TxOutput, script_pubkey: bytes) -> None:
+    async def _add_output(self, txo: TxOutput, script_pubkey: bytes) -> None:
         self.weight.add_output(script_pubkey)
         self.total_out += txo.amount
 
@@ -101,8 +101,8 @@ class Approver:
         self.payment_req_verifier = None
         self.show_payment_req_details = False
 
-    def add_change_output(self, txo: TxOutput, script_pubkey: bytes) -> None:
-        self._add_output(txo, script_pubkey)
+    async def add_change_output(self, txo: TxOutput, script_pubkey: bytes) -> None:
+        await self._add_output(txo, script_pubkey)
         self.change_out += txo.amount
         if self.payment_req_verifier:
             self.payment_req_verifier.add_change_output(txo)
@@ -117,7 +117,7 @@ class Approver:
         script_pubkey: bytes,
         orig_txo: TxOutput | None = None,
     ) -> None:
-        self._add_output(txo, script_pubkey)
+        await self._add_output(txo, script_pubkey)
         if self.payment_req_verifier:
             self.payment_req_verifier.add_external_output(txo)
 
@@ -160,8 +160,21 @@ class BasicApprover(Approver):
         ):
             raise ProcessError("Transaction has changed during signing")
 
-    def add_change_output(self, txo: TxOutput, script_pubkey: bytes) -> None:
-        super().add_change_output(txo, script_pubkey)
+    async def _add_output(self, txo: TxOutput, script_pubkey: bytes) -> None:
+        from ..common import CHANGE_OUTPUT_TO_INPUT_SCRIPT_TYPES
+
+        if txo.address_n and not validate_path_against_script_type(
+            self.coin,
+            address_n=txo.address_n,
+            script_type=CHANGE_OUTPUT_TO_INPUT_SCRIPT_TYPES[txo.script_type],
+            multisig=bool(txo.multisig),
+        ):
+            await helpers.confirm_foreign_address(txo.address_n)
+
+        await super()._add_output(txo, script_pubkey)
+
+    async def add_change_output(self, txo: TxOutput, script_pubkey: bytes) -> None:
+        await super().add_change_output(txo, script_pubkey)
         self.change_count += 1
 
     async def add_external_output(
@@ -389,7 +402,7 @@ class CoinJoinApprover(Approver):
 
     async def add_internal_input(self, txi: TxInput, node: bip32.HDNode) -> None:
         self.our_weight.add_input(txi)
-        if not self.authorization.check_sign_tx_input(txi, self.coin):
+        if not self.authorization.check_internal_input(txi):
             raise ProcessError("Unauthorized path")
 
         # Compute the masking bit for the signable bit in coinjoin flags.
@@ -421,7 +434,7 @@ class CoinJoinApprover(Approver):
         # The main reason for this is that we are not comfortable with using the same private key
         # in multiple signatures schemes (ECDSA and Schnorr) and we want to be sure that the user
         # went through a warning screen before we sign the input.
-        if not self.authorization.check_sign_tx_input(txi, self.coin):
+        if not self.authorization.check_internal_input(txi):
             raise ProcessError("Unauthorized path")
 
     def add_external_input(self, txi: TxInput) -> None:
@@ -433,8 +446,8 @@ class CoinJoinApprover(Approver):
         if input_is_external_unverified(txi):
             raise ProcessError("Unverifiable external input.")
 
-    def add_change_output(self, txo: TxOutput, script_pubkey: bytes) -> None:
-        super().add_change_output(txo, script_pubkey)
+    async def add_change_output(self, txo: TxOutput, script_pubkey: bytes) -> None:
+        await super().add_change_output(txo, script_pubkey)
         self.our_weight.add_output(script_pubkey)
 
     async def approve_orig_txids(
@@ -525,8 +538,11 @@ class CoinJoinApprover(Approver):
         if not self.authorization.approve_sign_tx(tx_info.tx):
             raise ProcessError("Exceeded number of coinjoin rounds.")
 
-    def _add_output(self, txo: TxOutput, script_pubkey: bytes) -> None:
-        super()._add_output(txo, script_pubkey)
+    async def _add_output(self, txo: TxOutput, script_pubkey: bytes) -> None:
+        await super()._add_output(txo, script_pubkey)
+
+        if txo.address_n and not self.authorization.check_internal_output(txo):
+            raise ProcessError("Unauthorized path")
 
         if txo.payment_req_index:
             raise DataError("Unexpected payment request.")

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

@@ -515,7 +515,7 @@ class Bitcoin:
 
         if self.tx_info.output_is_change(txo):
             # Output is change and does not need approval.
-            approver.add_change_output(txo, script_pubkey)
+            await approver.add_change_output(txo, script_pubkey)
         else:
             await approver.add_external_output(txo, script_pubkey, orig_txo)
 

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

@@ -368,7 +368,7 @@ class Decred(Bitcoin):
         if txo.amount != approver.total_in:
             raise DataError("Wrong sstxcommitment amount.")
         script_pubkey = self.process_sstx_commitment_owned(txo)
-        approver.add_change_output(txo, script_pubkey)
+        await approver.add_change_output(txo, script_pubkey)
         tx_info.add_output(txo, script_pubkey)
         if self.serialize:
             self.write_tx_output(self.serialized_tx, txo, script_pubkey)
@@ -386,7 +386,8 @@ class Decred(Bitcoin):
             raise DataError("Only value of 0 allowed for sstx change.")
         if script_pubkey != OUTPUT_SCRIPT_NULL_SSTXCHANGE:
             raise DataError("Only zeroed addresses accepted for sstx change.")
-        approver.add_change_output(txo, script_pubkey)
+        # nothing to approve, just add to tx_weight
+        await approver._add_output(txo, script_pubkey)
         tx_info.add_output(txo, script_pubkey)
         if self.serialize:
             self.write_tx_output(self.serialized_tx, txo, script_pubkey)

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

@@ -112,8 +112,16 @@ class TxInfoBase:
     def output_is_change(self, txo: TxOutput) -> bool:
         if txo.script_type not in common.CHANGE_OUTPUT_SCRIPT_TYPES:
             return False
+
+        # Check the multisig fingerprint only for multisig outputs. This means
+        # that a transfer from a multisig account to a singlesig account is
+        # treated as a change-output as long as all other change-output
+        # conditions are satisfied. This goes a bit against the concept of a
+        # multisig account but the other cosigners will notice that they are
+        # relinquishing control of the funds, so there is no security risk.
         if txo.multisig and not self.multisig_fingerprint.output_matches(txo):
             return False
+
         return (
             self.wallet_path.output_matches(txo)
             and self.script_type.output_matches(txo)

core/tests/test_apps.bitcoin.approver.py

@@ -127,7 +127,7 @@ class TestApprover(unittest.TestCase):
             TxOutput(
                 address="",
                 amount=denomination-fees,
-                script_type=OutputScriptType.PAYTOWITNESS,
+                script_type=OutputScriptType.PAYTOTAPROOT,
                 payment_req_index=0,
             ) for i in range(99)
         ]
@@ -139,7 +139,7 @@ class TestApprover(unittest.TestCase):
                 address="",
                 address_n=[H_(10025), H_(0), H_(0), H_(1), 0, 2],
                 amount=denomination-fees,
-                script_type=OutputScriptType.PAYTOWITNESS,
+                script_type=OutputScriptType.PAYTOTAPROOT,
                 payment_req_index=0,
             )
         )
@@ -149,7 +149,7 @@ class TestApprover(unittest.TestCase):
             TxOutput(
                 address="",
                 amount=coordinator_fee * len(outputs),
-                script_type=OutputScriptType.PAYTOWITNESS,
+                script_type=OutputScriptType.PAYTOTAPROOT,
                 payment_req_index=0,
             )
         )
@@ -168,7 +168,7 @@ class TestApprover(unittest.TestCase):
 
         for txo in outputs:
             if txo.address_n:
-                approver.add_change_output(txo, script_pubkey=bytes(22))
+                await_result(approver.add_change_output(txo, script_pubkey=bytes(22)))
             else:
                 await_result(approver.add_external_output(txo, script_pubkey=bytes(22)))
 

core/tests/test_apps.bitcoin.segwit.signtx.native_p2wpkh.py

@@ -235,6 +235,9 @@ class TestSignSegwitTxNativeP2WPKH(unittest.TestCase):
             TxRequest(request_type=TXOUTPUT, details=TxRequestDetailsType(request_index=1, tx_hash=None), serialized=EMPTY_SERIALIZED),
             TxAckOutput(tx=TxAckOutputWrapper(output=out2)),
 
+            helpers.UiConfirmForeignAddress(address_n=out2.address_n),
+            True,
+
             helpers.UiConfirmTotal(5000000 + 11000, 11000, fee_rate, coin, AmountUnit.BITCOIN),
             True,
 

tests/device_tests/bitcoin/test_multisig_change.py

@@ -143,6 +143,7 @@ def _responses(
     INP1: messages.TxInputType,
     INP2: messages.TxInputType,
     change: int = 0,
+    foreign: bool = False,
 ):
     tt = client.features.model == "T"
     resp = [
@@ -150,15 +151,23 @@ def _responses(
         request_input(1),
         request_output(0),
     ]
+
     if change != 1:
         resp.append(messages.ButtonRequest(code=B.ConfirmOutput))
         if tt:
             resp.append(messages.ButtonRequest(code=B.ConfirmOutput))
+    elif foreign:
+        resp.append(messages.ButtonRequest(code=B.UnknownDerivationPath))
+
     resp.append(request_output(1))
+
     if change != 2:
         resp.append(messages.ButtonRequest(code=B.ConfirmOutput))
         if tt:
             resp.append(messages.ButtonRequest(code=B.ConfirmOutput))
+    elif foreign:
+        resp.append(messages.ButtonRequest(code=B.UnknownDerivationPath))
+
     resp += [
         messages.ButtonRequest(code=B.SignTx),
         (tt, messages.ButtonRequest(code=B.SignTx)),
@@ -233,7 +242,9 @@ def test_external_internal(client: Client):
     )
 
     with client:
-        client.set_expected_responses(_responses(client, INP1, INP2, change=2))
+        client.set_expected_responses(
+            _responses(client, INP1, INP2, change=2, foreign=True)
+        )
         _, serialized_tx = btc.sign_tx(
             client,
             "Bitcoin",
@@ -264,7 +275,9 @@ def test_internal_external(client: Client):
     )
 
     with client:
-        client.set_expected_responses(_responses(client, INP1, INP2, change=1))
+        client.set_expected_responses(
+            _responses(client, INP1, INP2, change=1, foreign=True)
+        )
         _, serialized_tx = btc.sign_tx(
             client,
             "Bitcoin",

tests/device_tests/bitcoin/test_signtx_segwit_native.py

@@ -605,6 +605,7 @@ def test_send_multisig_3_change(client: Client):
     expected_responses = [
         request_input(0),
         request_output(0),
+        messages.ButtonRequest(code=B.UnknownDerivationPath),
         messages.ButtonRequest(code=B.ConfirmOutput),
         (tt, messages.ButtonRequest(code=B.ConfirmOutput)),
         messages.ButtonRequest(code=B.SignTx),
@@ -692,6 +693,7 @@ def test_send_multisig_4_change(client: Client):
     expected_responses = [
         request_input(0),
         request_output(0),
+        messages.ButtonRequest(code=B.UnknownDerivationPath),
         messages.ButtonRequest(code=B.ConfirmOutput),
         (tt, messages.ButtonRequest(code=B.ConfirmOutput)),
         messages.ButtonRequest(code=B.SignTx),

tests/ui_tests/fixtures.json

@@ -850,8 +850,8 @@
 "TT_bitcoin-test_multisig.py::test_attack_change_input": "adcd3cb376bc9dc1de9739f42a539321ff7c6fb2fbeeaf5428bc8a534df4940a",
 "TT_bitcoin-test_multisig.py::test_missing_pubkey": "3fc8eceefb45620d5802324b364162aee8103cbf1a25581e180b7c86c3df9939",
 "TT_bitcoin-test_multisig_change.py::test_external_external": "8d67c21171128a224ce612f47ab3a95533f1c76cd61182146f1b969f382dfd71",
-"TT_bitcoin-test_multisig_change.py::test_external_internal": "63193226f449bae651116b508ec1c5102c2d75de86f4562420f43a33e34ba57d",
-"TT_bitcoin-test_multisig_change.py::test_internal_external": "382865727c59ec85b37c96619717f7f5525d7bdb675a55a7efc1f8150a4e57c9",
+"TT_bitcoin-test_multisig_change.py::test_external_internal": "2f41587169e72a6bc20f1fd6b6804827c278ee6529bd7723183b5515942a1b2b",
+"TT_bitcoin-test_multisig_change.py::test_internal_external": "dd1af10723f4e6bae0b644268591e9aa03e9175b1fcbb70e0dbafd665b3783e7",
 "TT_bitcoin-test_multisig_change.py::test_multisig_change_match_first": "730631a8d21dc5947ecac43a0aa3a1632ec17c9d7e1e5be438c283384d381fce",
 "TT_bitcoin-test_multisig_change.py::test_multisig_change_match_second": "5794c6e0707192bc7a9404bccacbb4bd78ff713ba447252f0104aa0e6bd060f6",
 "TT_bitcoin-test_multisig_change.py::test_multisig_external_external": "2e998ff5310889cbb5b27a1654304ca4851a4c2a0ed8a130c7d3b302454582cc",
@@ -1052,8 +1052,8 @@
 "TT_bitcoin-test_signtx_segwit_native.py::test_send_both": "3eec77e0f79eae561dbaa0468b3743634e09f984a90fba46ba479be032174e9a",
 "TT_bitcoin-test_signtx_segwit_native.py::test_send_multisig_1": "56704b7138799085989a95f7cc8d25ed7535107329c3a2a6524b69bdf48b2dac",
 "TT_bitcoin-test_signtx_segwit_native.py::test_send_multisig_2": "d13ce17eaa563814181aa6201f588fda1c26208fb2bede56cdf1f4f0032d8341",
-"TT_bitcoin-test_signtx_segwit_native.py::test_send_multisig_3_change": "e7146b6b66c8757b1b6cf9b4f1f16745365d094a6389a618d8f8bb390b2dda43",
-"TT_bitcoin-test_signtx_segwit_native.py::test_send_multisig_4_change": "f8dc8c9c56fbe5a568d914bcae1b114b42b40bb6a28af4b95e544246dd8a712d",
+"TT_bitcoin-test_signtx_segwit_native.py::test_send_multisig_3_change": "382d126d6123d2340a253a9ce82927632e3bba6e383060dfa85d9d9c4ce9f707",
+"TT_bitcoin-test_signtx_segwit_native.py::test_send_multisig_4_change": "093db1ae1d609c163c0ac373ecae474e32704e699a5328b6f12a7c8118bd5cc0",
 "TT_bitcoin-test_signtx_segwit_native.py::test_send_native": "3ef4169b93304aa6c0da5f46ffd44feca83d5de1173792e4b7db24b6820f875c",
 "TT_bitcoin-test_signtx_segwit_native.py::test_send_native_change": "3cb6fd481ac140583c7db43d739837a18d0878b916b0139b656a3d4c18e32c48",
 "TT_bitcoin-test_signtx_segwit_native.py::test_send_p2sh": "e1b3f5961ed2f24c997cb4a5abf399324de65af6a1004d9b242ff4e971784ddb",