fix(core): Ensure user is warned about non-standard paths.

core/.changelog.d/noissue.security

@@ -0,0 +1 @@
+Ensure that the user is always warned about non-standard paths.

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

@@ -5,8 +5,8 @@ from trezor.enums import OutputScriptType
 
 from apps.common import safety_checks
 
-from .. import keychain
 from ..authorization import FEE_PER_ANONYMITY_DECIMALS
+from ..keychain import validate_path_against_script_type
 from . import helpers, tx_weight
 from .tx_info import OriginalTxInfo, TxInfo
 
@@ -55,6 +55,9 @@ class Approver:
         if txi.orig_hash:
             self.orig_total_in += txi.amount
 
+    async def check_internal_input(self, txi: TxInput) -> None:
+        pass
+
     def add_external_input(self, txi: TxInput) -> None:
         self.weight.add_input(txi)
         self.total_in += txi.amount
@@ -102,11 +105,22 @@ class BasicApprover(Approver):
         self.change_count = 0  # the number of change-outputs
 
     async def add_internal_input(self, txi: TxInput) -> None:
-        if not keychain.validate_path_against_script_type(self.coin, txi):
+        if not validate_path_against_script_type(self.coin, txi):
             await helpers.confirm_foreign_address(txi.address_n)
 
         await super().add_internal_input(txi)
 
+    async def check_internal_input(self, txi: TxInput) -> None:
+        if not validate_path_against_script_type(self.coin, txi):
+            # The following can be removed once we start validating script_pubkey in step3_verify_inputs().
+            if self.orig_total_in:
+                # Replacement transaction.
+                # This mitigates a cross-coin spending attack when safety checks are disabled.
+                raise wire.ProcessError(
+                    "Non-standard paths not allowed in replacement transactions."
+                )
+            await helpers.confirm_foreign_address(txi.address_n)
+
     def add_change_output(self, txo: TxOutput, script_pubkey: bytes) -> None:
         super().add_change_output(txo, script_pubkey)
         self.change_count += 1
@@ -293,6 +307,11 @@ class CoinJoinApprover(Approver):
 
         await super().add_internal_input(txi)
 
+    async def check_internal_input(self, txi: TxInput) -> None:
+        # The following can be removed once we start validating script_pubkey in step3_verify_inputs().
+        if not self.authorization.check_sign_tx_input(txi, self.coin):
+            raise wire.ProcessError("Unauthorized path")
+
     def add_change_output(self, txo: TxOutput, script_pubkey: bytes) -> None:
         super().add_change_output(txo, script_pubkey)
         self._add_output(txo, script_pubkey)

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

@@ -519,8 +519,9 @@ class Bitcoin:
         self.write_tx_input_derived(self.serialized_tx, txi, key_sign_pub, b"")
 
     def sign_bip143_input(self, i: int, txi: TxInput) -> tuple[bytes, bytes]:
-        self.tx_info.check_input(txi)
         if self.taproot_only:
+            # Prevents an attacker from bypassing prev tx checking by providing a different
+            # script type than the one that was provided during the confirmation phase.
             raise wire.ProcessError("Transaction has changed during signing")
 
         node = self.keychain.derive(txi.address_n)
@@ -547,7 +548,6 @@ class Bitcoin:
         return public_key, signature
 
     def sign_taproot_input(self, i: int, txi: TxInput) -> bytes:
-        self.tx_info.check_input(txi)
         sigmsg_digest = self.tx_info.sig_hasher.hash341(
             i,
             self.tx_info.tx,
@@ -560,6 +560,8 @@ class Bitcoin:
     async def sign_segwit_input(self, i: int) -> None:
         # STAGE_REQUEST_SEGWIT_WITNESS in legacy
         txi = await helpers.request_tx_input(self.tx_req, i, self.coin)
+        self.tx_info.check_input(txi)
+        await self.approver.check_internal_input(txi)
         if txi.script_type not in common.SEGWIT_INPUT_SCRIPT_TYPES:
             raise wire.ProcessError("Transaction has changed during signing")
 
@@ -662,6 +664,8 @@ class Bitcoin:
 
     async def sign_nonsegwit_input(self, i: int) -> None:
         if self.taproot_only:
+            # Prevents an attacker from bypassing prev tx checking by providing a different
+            # script type than the one that was provided during the confirmation phase.
             raise wire.ProcessError("Transaction has changed during signing")
 
         tx_digest, txi, node = await self.get_legacy_tx_digest(i, self.tx_info)

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

@@ -16,6 +16,8 @@ if False:
 class Bitcoinlike(Bitcoin):
     async def sign_nonsegwit_bip143_input(self, i_sign: int) -> None:
         txi = await helpers.request_tx_input(self.tx_req, i_sign, self.coin)
+        self.tx_info.check_input(txi)
+        await self.approver.check_internal_input(txi)
 
         if txi.script_type not in NONSEGWIT_INPUT_SCRIPT_TYPES:
             raise wire.ProcessError("Transaction has changed during signing")

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

@@ -150,6 +150,9 @@ class TestSignSegwitTxNativeP2WPKH(unittest.TestCase):
             )),
             TxAckInput(tx=TxAckInputWrapper(input=inp1)),
 
+            helpers.UiConfirmForeignAddress(address_n=inp1.address_n),
+            True,
+
             TxRequest(request_type=TXFINISHED, details=TxRequestDetailsType(), serialized=TxRequestSerializedType(
                 serialized_tx=unhexlify('02483045022100a7ca8f097525f9044e64376dc0a0f5d4aeb8d15d66808ba97979a0475b06b66502200597c8ebcef63e047f9aeef1a8001d3560470cf896c12f6990eec4faec599b950121033add1f0e8e3c3136f7428dd4a4de1057380bd311f5b0856e2269170b4ffa65bf00000000'),
                 signature_index=0,
@@ -278,6 +281,9 @@ class TestSignSegwitTxNativeP2WPKH(unittest.TestCase):
             )),
             TxAckInput(tx=TxAckInputWrapper(input=inp1)),
 
+            helpers.UiConfirmForeignAddress(address_n=inp1.address_n),
+            True,
+
             TxRequest(request_type=TXFINISHED, details=TxRequestDetailsType(), serialized=TxRequestSerializedType(
                 serialized_tx=unhexlify('02483045022100a7ca8f097525f9044e64376dc0a0f5d4aeb8d15d66808ba97979a0475b06b66502200597c8ebcef63e047f9aeef1a8001d3560470cf896c12f6990eec4faec599b950121033add1f0e8e3c3136f7428dd4a4de1057380bd311f5b0856e2269170b4ffa65bf00000000'),
                 signature_index=0,

tests/ui_tests/fixtures.json

@@ -662,7 +662,7 @@
 "test_msg_signtx_grs.py-test_send_segwit_p2sh": "e59018de5c49f902c6880c2347283b6c1830fcb19e8eab9686938a08abd930b3",
 "test_msg_signtx_grs.py-test_send_segwit_p2sh_change": "08251e3b7e509264dbd89a5ded2deba51544d0fcfce1dc7466b553b73298d423",
 "test_msg_signtx_invalid_path.py-test_invalid_path_fail": "1c100ce4b7c1e47e72428f390de0846c1ff933e9f07894872644a369a9422738",
-"test_msg_signtx_invalid_path.py-test_invalid_path_pass_forkid": "85d3c2f3c85e1bcf774f3067d7eb32396c444f351ad15e68a328f87cf6bdb338",
+"test_msg_signtx_invalid_path.py-test_invalid_path_pass_forkid": "ef98eb752ec5fa948c952def7599f57a2bc5240b2d6b1eec0e02cc9be5c3040f",
 "test_msg_signtx_invalid_path.py-test_invalid_path_prompt": "12e137210397357ed754af0f4618ef03312b3e884930f55727d1b034f969bfd5",
 "test_msg_signtx_komodo.py-test_one_one_fee_sapling": "6286409ed3e62a896c101112f6d1c59559dde677331d08f55ed7d2d43e6dd3b9",
 "test_msg_signtx_komodo.py-test_one_one_rewards_claim": "b3c056df25d639927faaf16dc18c281c1a36b790ea4e77f954f681fb27d3fa1a",
@@ -796,8 +796,8 @@
 "test_nonstandard_paths.py::test_signmessage[m-3'-100'-4-255-script_types1]": "4f73135d2ec9add695e0a22d855816558b4ba9329a2828f9c9930be6245bdc2d",
 "test_nonstandard_paths.py::test_signmessage[m-4-255-script_types0]": "0988cc8bdc5879744bd33190fddc5b5aa137fdd7214abb003c8000a871d98f14",
 "test_nonstandard_paths.py::test_signmessage[m-49-0-63-0-255-script_types4]": "540df94c73a4eed8fe88cdb475e2b31df752dca9e47b102792c01064ee432752",
-"test_nonstandard_paths.py::test_signtx[m-1195487518-6-255-script_types3]": "37cfe119620536464ae42b3fbcae7b89d9272ad904da2bd8e8ae47b1024b4007",
-"test_nonstandard_paths.py::test_signtx[m-1195487518-script_types2]": "27a03a5be542d1f5f76a839e65daec766c1d7de8ae4637404ffcfea8267ea0ec",
+"test_nonstandard_paths.py::test_signtx[m-1195487518-6-255-script_types3]": "3fb1ec777c4c1a4e320740d050444077e118a0fbcfec96cb7e5ead203dfe01a2",
+"test_nonstandard_paths.py::test_signtx[m-1195487518-script_types2]": "e83d90183a5899d8881271e27ce030ec252df9c4a32ca4097cad811431553c37",
 "test_nonstandard_paths.py::test_signtx[m-3'-100'-4-255-script_types1]": "efbe785820901471b0e55f9fd743c84a29fe719c2e1c8e6b2f87b0a20ce43cb2",
 "test_nonstandard_paths.py::test_signtx[m-4-255-script_types0]": "efbe785820901471b0e55f9fd743c84a29fe719c2e1c8e6b2f87b0a20ce43cb2",
 "test_nonstandard_paths.py::test_signtx[m-49-0-63-0-255-script_types4]": "4392475bb51d2dd9316036ed268ee84bafb6f3f7b0d2e1ab6be69a63775d5f66",