From b1c6b4220153c2edb2cac9e3d15fabd6802a69e3 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Fri, 11 Mar 2022 12:26:05 +0100 Subject: [PATCH] fix(core): Stricter Bitcoin transaction checks. --- core/.changelog.d/noissue.security | 1 + core/src/apps/bitcoin/sign_tx/tx_info.py | 7 +++++-- core/src/apps/bitcoin/writers.py | 18 ++++++++++++------ .../tests/test_apps.bitcoin.sign_tx.writers.py | 2 +- 4 files changed, 19 insertions(+), 9 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..552e050cd --- /dev/null +++ b/core/.changelog.d/noissue.security @@ -0,0 +1 @@ +Fix a coin loss vulnerability related to replacement transactions with multisig inputs and unverified external inputs. diff --git a/core/src/apps/bitcoin/sign_tx/tx_info.py b/core/src/apps/bitcoin/sign_tx/tx_info.py index 5272ae50c..8944f3a66 100644 --- a/core/src/apps/bitcoin/sign_tx/tx_info.py +++ b/core/src/apps/bitcoin/sign_tx/tx_info.py @@ -153,8 +153,11 @@ class OriginalTxInfo(TxInfoBase): 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 input that specifies address_n. - if not self.verification_input and txi.address_n: + # 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 diff --git a/core/src/apps/bitcoin/writers.py b/core/src/apps/bitcoin/writers.py index 7bedde242..e0680ae24 100644 --- a/core/src/apps/bitcoin/writers.py +++ b/core/src/apps/bitcoin/writers.py @@ -15,8 +15,6 @@ from apps.common.writers import ( # noqa: F401 write_uint64_le, ) -from .common import input_is_external_unverified - if TYPE_CHECKING: from trezor.messages import ( PrevInput, @@ -48,15 +46,23 @@ def write_tx_input(w: Writer, i: TxInput | PrevInput, script: bytes) -> None: def write_tx_input_check(w: Writer, i: TxInput) -> None: - write_bytes_fixed(w, i.prev_hash, TX_HASH_SIZE) - write_uint32(w, i.prev_index) - write_uint32(w, i.script_type) - write_uint8(w, input_is_external_unverified(i)) + from .multisig import multisig_fingerprint + write_uint32(w, len(i.address_n)) for n in i.address_n: write_uint32(w, n) + write_bytes_fixed(w, i.prev_hash, TX_HASH_SIZE) + write_uint32(w, i.prev_index) + write_bytes_prefixed(w, i.script_sig or b"") write_uint32(w, i.sequence) + write_uint32(w, i.script_type) + multisig_fp = multisig_fingerprint(i.multisig) if i.multisig else b"" + write_bytes_prefixed(w, multisig_fp) write_uint64(w, i.amount or 0) + write_bytes_prefixed(w, i.witness or b"") + write_bytes_prefixed(w, i.ownership_proof or b"") + write_bytes_prefixed(w, i.orig_hash or b"") + write_uint32(w, i.orig_index or 0) write_bytes_prefixed(w, i.script_pubkey or b"") diff --git a/core/tests/test_apps.bitcoin.sign_tx.writers.py b/core/tests/test_apps.bitcoin.sign_tx.writers.py index 65491cccb..0dd6949b8 100644 --- a/core/tests/test_apps.bitcoin.sign_tx.writers.py +++ b/core/tests/test_apps.bitcoin.sign_tx.writers.py @@ -43,7 +43,7 @@ class TestWriters(unittest.TestCase): b = bytearray() writers.write_tx_input_check(b, inp) - self.assertEqual(len(b), 32 + 4 + 4 + 1 + 4 + 4 + 4 + 8 + 26) + self.assertEqual(len(b), 4 + 4 + 32 + 4 + 11 + 4 + 4 + 1 + 8 + 1 + 1 + 1 + 4 + 26) for bad_prevhash in (b"", b"x", b"hello", b"x" * 33): inp.prev_hash = bad_prevhash