diff --git a/core/src/apps/bitcoin/sign_tx/bitcoin.py b/core/src/apps/bitcoin/sign_tx/bitcoin.py index ec7321557..2acdefefc 100644 --- a/core/src/apps/bitcoin/sign_tx/bitcoin.py +++ b/core/src/apps/bitcoin/sign_tx/bitcoin.py @@ -122,9 +122,9 @@ class Bitcoin: # stable device tests. self.orig_txs: list[OriginalTxInfo] = [] - # The digests of the inputs streamed for approval in Step 1. These are used to ensure that - # the inputs streamed for verification in Step 3 are the same as those in Step 1. - self.h_inputs: bytes | None = None + # The digests of the external inputs streamed for approval in Step 1. These are used + # to ensure that the inputs streamed for verification in Step 3 are the same as + # those in Step 1. self.h_external_inputs: bytes | None = None # The index of the payment request being processed. @@ -165,11 +165,12 @@ class Bitcoin: if txi.orig_hash: await self.process_original_input(txi, script_pubkey) - self.h_inputs = self.tx_info.get_tx_check_digest() + self.tx_info.h_inputs_check = self.tx_info.get_tx_check_digest() self.h_external_inputs = h_external_inputs_check.get_digest() # Finalize original inputs. for orig in self.orig_txs: + orig.h_inputs_check = orig.get_tx_check_digest() if orig.index != orig.tx.inputs_count: raise wire.ProcessError("Removal of original inputs is not supported.") @@ -194,7 +195,7 @@ class Bitcoin: await orig.finalize_tx_hash() async def step3_verify_inputs(self) -> None: - # should come out the same as h_inputs, checked before continuing + # should come out the same as h_inputs_check, checked before continuing h_check = HashWriter(sha256()) if self.taproot_only: @@ -220,7 +221,7 @@ class Bitcoin: # inputs or to falsely claim that a transaction is a replacement of an already approved # transaction or to construct a valid transaction by combining signatures obtained in # multiple rounds of the attack. - expected_digest = self.h_inputs + expected_digest = self.tx_info.h_inputs_check for i in range(self.tx_info.tx.inputs_count): progress.advance() txi = await helpers.request_tx_input(self.tx_req, i, self.coin) @@ -406,29 +407,34 @@ class Bitcoin: async def verify_original_txs(self) -> None: for orig in self.orig_txs: - if orig.verification_input is None: - raise wire.ProcessError( - "Each original transaction must specify address_n for at least one input." + # should come out the same as h_inputs_check, checked before continuing + h_check = HashWriter(sha256()) + + for i in range(orig.tx.inputs_count): + txi = await helpers.request_tx_input( + self.tx_req, i, self.coin, orig.orig_hash + ) + writers.write_tx_input_check(h_check, txi) + script_pubkey = self.input_derive_script(txi) + verifier = SignatureVerifier( + script_pubkey, txi.script_sig, txi.witness, self.coin + ) + verifier.ensure_hash_type( + (SigHashType.SIGHASH_ALL_TAPROOT, self.get_sighash_type(txi)) ) + tx_digest = await self.get_tx_digest( + i, + txi, + orig, + verifier.public_keys, + verifier.threshold, + script_pubkey, + ) + verifier.verify(tx_digest) - assert orig.verification_index is not None - txi = orig.verification_input - script_pubkey = self.input_derive_script(txi) - verifier = SignatureVerifier( - script_pubkey, txi.script_sig, txi.witness, self.coin - ) - verifier.ensure_hash_type( - (SigHashType.SIGHASH_ALL_TAPROOT, self.get_sighash_type(txi)) - ) - tx_digest = await self.get_tx_digest( - orig.verification_index, - txi, - orig, - verifier.public_keys, - verifier.threshold, - script_pubkey, - ) - verifier.verify(tx_digest) + # check that the inputs were the same as those streamed for approval + if h_check.get_digest() != orig.h_inputs_check: + raise wire.ProcessError("Transaction has changed during signing") async def approve_output( self, diff --git a/core/src/apps/bitcoin/sign_tx/tx_info.py b/core/src/apps/bitcoin/sign_tx/tx_info.py index 8944f3a66..e91620e83 100644 --- a/core/src/apps/bitcoin/sign_tx/tx_info.py +++ b/core/src/apps/bitcoin/sign_tx/tx_info.py @@ -72,6 +72,11 @@ class TxInfoBase: # in Steps 1 and 2 and the ones streamed for signing legacy inputs in Step 4. self.h_tx_check = HashWriter(sha256()) # not a real tx hash + # The digests of the inputs streamed for approval in Step 1. These are used to + # ensure that the inputs streamed for verification in Step 3 are the same as + # those in Step 1. + self.h_inputs_check: bytes | None = None + # BIP-0143 transaction hashing. self.sig_hasher = signer.create_sig_hasher(tx) @@ -145,22 +150,10 @@ class OriginalTxInfo(TxInfoBase): signer.write_tx_header(self.h_tx, tx, witness_marker=False) writers.write_compact_size(self.h_tx, tx.inputs_count) - # The input which will be used for verification and its index in the original transaction. - self.verification_input: TxInput | None = None - self.verification_index: int | None = None - def add_input(self, txi: TxInput, script_pubkey: bytes) -> None: super().add_input(txi, script_pubkey) writers.write_tx_input(self.h_tx, txi, txi.script_sig or bytes()) - # For verification use the first original non-multisig input that specifies address_n. - # NOTE: Supporting replacement transactions where all internal inputs are multisig would - # require checking the signatures of all of the original internal inputs or not allowing - # unverified external inputs in transactions where multisig inputs are present. - if not self.verification_input and txi.address_n and not txi.multisig: - self.verification_input = txi - self.verification_index = self.index - def add_output(self, txo: TxOutput, script_pubkey: bytes) -> None: super().add_output(txo, script_pubkey) diff --git a/docs/common/communication/bitcoin-signing.md b/docs/common/communication/bitcoin-signing.md index 7054319d7..7b484e038 100644 --- a/docs/common/communication/bitcoin-signing.md +++ b/docs/common/communication/bitcoin-signing.md @@ -286,10 +286,10 @@ set to `tx.extra_data_chunk`. Trezor sets `request_type` to `TXORIGINPUT`. `request_details.tx_hash` is the transaction hash of the original transaction. -Host must respond with a `TxAckInput` message. All relevant data must be set in +The host must respond with a `TxAckInput` message. All relevant data must be set in `tx.input`. The derivation path and `script_type` are mandatory for all original -internal inputs. For each original transaction, one of its original internal inputs must -be accompanied with a valid signature in the `script_sig` and/or `witness` fields. +internal inputs. All original internal inputs must also be accompanied with full +transaction signature data in the `script_sig` and/or `witness` fields. ### Original transaction output diff --git a/tests/device_tests/bitcoin/test_signtx_replacement.py b/tests/device_tests/bitcoin/test_signtx_replacement.py index f9086221e..ea31895a3 100644 --- a/tests/device_tests/bitcoin/test_signtx_replacement.py +++ b/tests/device_tests/bitcoin/test_signtx_replacement.py @@ -133,6 +133,7 @@ def test_p2pkh_fee_bump(client: Client): request_meta(TXHASH_beafc7), request_input(0, TXHASH_beafc7), request_output(0, TXHASH_beafc7), + request_orig_input(0, TXHASH_50f6f1), (tt, request_orig_input(0, TXHASH_50f6f1)), (tt, request_orig_output(0, TXHASH_50f6f1)), (tt, request_orig_output(1, TXHASH_50f6f1)), @@ -257,6 +258,8 @@ def test_p2tr_fee_bump(client: Client): request_output(1), request_orig_output(1, TXHASH_8e4af7), messages.ButtonRequest(code=B.SignTx), + request_orig_input(0, TXHASH_8e4af7), + request_orig_input(1, TXHASH_8e4af7), request_input(0), request_input(1), request_output(0), @@ -328,6 +331,7 @@ def test_p2wpkh_finalize(client: Client): request_output(0, TXHASH_43d273), request_output(1, TXHASH_43d273), request_output(2, TXHASH_43d273), + request_orig_input(0, TXHASH_70f987), request_input(0), request_output(0), request_output(1), @@ -464,6 +468,7 @@ def test_p2wpkh_payjoin( request_input(0, TXHASH_70f987), request_output(0, TXHASH_70f987), request_output(1, TXHASH_70f987), + request_orig_input(0, TXHASH_65b768), request_input(0), request_input(1), request_output(0), @@ -538,6 +543,8 @@ def test_p2wpkh_in_p2sh_remove_change(client: Client): request_meta(TXHASH_efaa41), request_input(0, TXHASH_efaa41), request_output(0, TXHASH_efaa41), + request_orig_input(0, TXHASH_334cd7), + request_orig_input(1, TXHASH_334cd7), request_input(0), request_input(1), request_output(0), @@ -618,6 +625,8 @@ def test_p2wpkh_in_p2sh_fee_bump_from_external(client: Client): request_meta(TXHASH_efaa41), request_input(0, TXHASH_efaa41), request_output(0, TXHASH_efaa41), + request_orig_input(0, TXHASH_334cd7), + request_orig_input(1, TXHASH_334cd7), request_input(0), request_input(1), request_output(0), @@ -759,6 +768,10 @@ def test_tx_meld(client: Client): request_input(1, TXHASH_927784), request_input(2, TXHASH_927784), request_output(0, TXHASH_927784), + request_orig_input(0, TXHASH_334cd7), + request_orig_input(1, TXHASH_334cd7), + request_orig_input(0, TXHASH_ed89ac), + request_orig_input(1, TXHASH_ed89ac), request_input(0), request_input(1), request_input(2), diff --git a/tests/txcache/testnet/334cd7ad982b3b15d07dd1c84e939e95efb0803071648048a7f289492e7b4c8a.json b/tests/txcache/testnet/334cd7ad982b3b15d07dd1c84e939e95efb0803071648048a7f289492e7b4c8a.json index dbac3f2f8..f2afa38d0 100644 --- a/tests/txcache/testnet/334cd7ad982b3b15d07dd1c84e939e95efb0803071648048a7f289492e7b4c8a.json +++ b/tests/txcache/testnet/334cd7ad982b3b15d07dd1c84e939e95efb0803071648048a7f289492e7b4c8a.json @@ -17,6 +17,7 @@ "amount": 998060, "script_type": "SPENDP2SHWITNESS", "script_sig": "160014209297fb46272a0b7e05139440dbd39daea3e25a", + "witness":"0247304402206f5b2032601278685921b397416cecd5f70cde48b7203bf2a953283c0cd05f4002205fd21a2c318330f11e8433f8fae6b1c4c257a79da55be9712961214ba2752a1f012103c2c2e65556ca4b7371549324b99390725493c8a6792e093a0bdcbb3e2d7df4ab", "sequence": 4294967295 } ], diff --git a/tests/txcache/testnet/8e4af74cee65dac2615f7befea76bab4955707cd216011d918b526f5e86d85ba.json b/tests/txcache/testnet/8e4af74cee65dac2615f7befea76bab4955707cd216011d918b526f5e86d85ba.json index c7daeb3d0..53cb26d4f 100644 --- a/tests/txcache/testnet/8e4af74cee65dac2615f7befea76bab4955707cd216011d918b526f5e86d85ba.json +++ b/tests/txcache/testnet/8e4af74cee65dac2615f7befea76bab4955707cd216011d918b526f5e86d85ba.json @@ -17,7 +17,8 @@ "prev_index": 0, "script_sig": "", "script_type": "SPENDTAPROOT", - "sequence": 4294967293 + "sequence": 4294967293, + "witness": "0140d961a3fc97561c8a305d0ba78488e8d1efed520ac36b226db76277c38e5ab112236a2f821b6dde82dc4013f74148d1ec6e1e471b952755b5265b7ed594afb723" } ], "lock_time": 0, diff --git a/tests/txcache/testnet/ed89acb52cfa438e3653007478e7c7feae89fdde12867943eec91293139730d1.json b/tests/txcache/testnet/ed89acb52cfa438e3653007478e7c7feae89fdde12867943eec91293139730d1.json index b780aa60e..60f3a0369 100644 --- a/tests/txcache/testnet/ed89acb52cfa438e3653007478e7c7feae89fdde12867943eec91293139730d1.json +++ b/tests/txcache/testnet/ed89acb52cfa438e3653007478e7c7feae89fdde12867943eec91293139730d1.json @@ -27,6 +27,7 @@ "amount": 839318869, "script_type": "SPENDP2SHWITNESS", "script_sig": "160014681ea49259abb892460bf3373e8a0b43d877fa18", + "witness": "02473044022039bb1710e041e9a936bd3adf39f61906ac4363a67d1c29b1a264ff8cf5a0c95102202829307d82141792125480abe6491f0f0c7d675c3d1a06daa383d617afcb53380121028cbc37e1816a23086fa738c8415def477e813e20f484dbbd6f5a33a37c322251", "sequence": 4294967295 } ],