From 9052133fca1eef245d97d3481590659bf5d96a8a Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Wed, 17 Nov 2021 11:20:45 +0100 Subject: [PATCH] fix(core): Ensure user is warned about non-standard paths. --- core/.changelog.d/noissue.security | 1 + core/src/apps/bitcoin/sign_tx/approvers.py | 23 +++++++++++++++++-- core/src/apps/bitcoin/sign_tx/bitcoin.py | 8 +++++-- core/src/apps/bitcoin/sign_tx/bitcoinlike.py | 2 ++ ...pps.bitcoin.segwit.signtx.native_p2wpkh.py | 6 +++++ tests/ui_tests/fixtures.json | 6 ++--- 6 files changed, 39 insertions(+), 7 deletions(-) create mode 100644 core/.changelog.d/noissue.security diff --git a/core/.changelog.d/noissue.security b/core/.changelog.d/noissue.security new file mode 100644 index 000000000..dc6ae541f --- /dev/null +++ b/core/.changelog.d/noissue.security @@ -0,0 +1 @@ +Ensure that the user is always warned about non-standard paths. diff --git a/core/src/apps/bitcoin/sign_tx/approvers.py b/core/src/apps/bitcoin/sign_tx/approvers.py index 03e97a4f1..56542a4e0 100644 --- a/core/src/apps/bitcoin/sign_tx/approvers.py +++ b/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) diff --git a/core/src/apps/bitcoin/sign_tx/bitcoin.py b/core/src/apps/bitcoin/sign_tx/bitcoin.py index c0a79a0de..7242eaaa9 100644 --- a/core/src/apps/bitcoin/sign_tx/bitcoin.py +++ b/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) diff --git a/core/src/apps/bitcoin/sign_tx/bitcoinlike.py b/core/src/apps/bitcoin/sign_tx/bitcoinlike.py index c0369af78..9cc55880c 100644 --- a/core/src/apps/bitcoin/sign_tx/bitcoinlike.py +++ b/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") diff --git a/core/tests/test_apps.bitcoin.segwit.signtx.native_p2wpkh.py b/core/tests/test_apps.bitcoin.segwit.signtx.native_p2wpkh.py index 7133e21f9..d0feb364e 100644 --- a/core/tests/test_apps.bitcoin.segwit.signtx.native_p2wpkh.py +++ b/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, diff --git a/tests/ui_tests/fixtures.json b/tests/ui_tests/fixtures.json index dd444134c..8a3e15ac9 100644 --- a/tests/ui_tests/fixtures.json +++ b/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",