diff --git a/core/.changelog.d/noissue.security b/core/.changelog.d/noissue.security new file mode 100644 index 000000000..7318011cc --- /dev/null +++ b/core/.changelog.d/noissue.security @@ -0,0 +1 @@ +Match and validate script type of change-outputs in Bitcoin signing. diff --git a/core/src/apps/bitcoin/authorization.py b/core/src/apps/bitcoin/authorization.py index ea47a55f4..802992da1 100644 --- a/core/src/apps/bitcoin/authorization.py +++ b/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 diff --git a/core/src/apps/bitcoin/sign_tx/approvers.py b/core/src/apps/bitcoin/sign_tx/approvers.py index 701201c47..f51a3c324 100644 --- a/core/src/apps/bitcoin/sign_tx/approvers.py +++ b/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.") diff --git a/core/src/apps/bitcoin/sign_tx/bitcoin.py b/core/src/apps/bitcoin/sign_tx/bitcoin.py index 36b4e862d..e4f80c5af 100644 --- a/core/src/apps/bitcoin/sign_tx/bitcoin.py +++ b/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) diff --git a/core/src/apps/bitcoin/sign_tx/decred.py b/core/src/apps/bitcoin/sign_tx/decred.py index 8054d6ce8..fd3915903 100644 --- a/core/src/apps/bitcoin/sign_tx/decred.py +++ b/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) diff --git a/core/src/apps/bitcoin/sign_tx/tx_info.py b/core/src/apps/bitcoin/sign_tx/tx_info.py index b13ee715b..7176654c2 100644 --- a/core/src/apps/bitcoin/sign_tx/tx_info.py +++ b/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) diff --git a/core/tests/test_apps.bitcoin.approver.py b/core/tests/test_apps.bitcoin.approver.py index 0fa3518fd..3645216f4 100644 --- a/core/tests/test_apps.bitcoin.approver.py +++ b/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))) 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 fef7c997a..c69c715b9 100644 --- a/core/tests/test_apps.bitcoin.segwit.signtx.native_p2wpkh.py +++ b/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, diff --git a/tests/device_tests/bitcoin/test_multisig_change.py b/tests/device_tests/bitcoin/test_multisig_change.py index 450163a76..a54dcded2 100644 --- a/tests/device_tests/bitcoin/test_multisig_change.py +++ b/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", diff --git a/tests/device_tests/bitcoin/test_signtx_segwit_native.py b/tests/device_tests/bitcoin/test_signtx_segwit_native.py index 0a3e994e6..610334d4d 100644 --- a/tests/device_tests/bitcoin/test_signtx_segwit_native.py +++ b/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), diff --git a/tests/ui_tests/fixtures.json b/tests/ui_tests/fixtures.json index b00c87dd2..384fdde05 100644 --- a/tests/ui_tests/fixtures.json +++ b/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",