From 43f1d67289a30cb4af535b6a418d842562f7d4df Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Thu, 3 Mar 2022 19:02:53 +0100 Subject: [PATCH] feat(core): Support unverified external inputs. --- core/.changelog.d/2144.added | 1 + core/src/apps/bitcoin/common.py | 9 ++++++ core/src/apps/bitcoin/sign_tx/approvers.py | 20 +++++++++++-- core/src/apps/bitcoin/sign_tx/bitcoin.py | 9 ++++-- core/src/apps/bitcoin/writers.py | 3 ++ core/tests/test_apps.bitcoin.approver.py | 1 + .../test_apps.bitcoin.sign_tx.writers.py | 2 +- docs/common/communication/bitcoin-signing.md | 30 +++++++++++++------ 8 files changed, 60 insertions(+), 15 deletions(-) create mode 100644 core/.changelog.d/2144.added diff --git a/core/.changelog.d/2144.added b/core/.changelog.d/2144.added new file mode 100644 index 000000000..d89148122 --- /dev/null +++ b/core/.changelog.d/2144.added @@ -0,0 +1 @@ +Support unverified external inputs. diff --git a/core/src/apps/bitcoin/common.py b/core/src/apps/bitcoin/common.py index fe95edc86..616d6de86 100644 --- a/core/src/apps/bitcoin/common.py +++ b/core/src/apps/bitcoin/common.py @@ -161,6 +161,15 @@ def input_is_external(txi: TxInput) -> bool: return txi.script_type == InputScriptType.EXTERNAL +def input_is_external_unverified(txi: TxInput) -> bool: + return ( + txi.script_type == InputScriptType.EXTERNAL + and txi.ownership_proof is None + and txi.witness is None + and txi.script_sig is None + ) + + def tagged_hashwriter(tag: bytes) -> HashWriter: tag_digest = sha256(tag).digest() ctx = sha256(tag_digest) diff --git a/core/src/apps/bitcoin/sign_tx/approvers.py b/core/src/apps/bitcoin/sign_tx/approvers.py index 1a0145758..f08487f98 100644 --- a/core/src/apps/bitcoin/sign_tx/approvers.py +++ b/core/src/apps/bitcoin/sign_tx/approvers.py @@ -8,6 +8,7 @@ from trezor.ui.components.common.confirm import INFO from apps.common import safety_checks from ..authorization import FEE_PER_ANONYMITY_DECIMALS +from ..common import input_is_external_unverified from ..keychain import validate_path_against_script_type from . import helpers, tx_weight from .payment_request import PaymentRequestVerifier @@ -68,10 +69,16 @@ class Approver: def add_external_input(self, txi: TxInput) -> None: self.weight.add_input(txi) self.total_in += txi.amount - self.external_in += txi.amount if txi.orig_hash: self.orig_total_in += txi.amount - self.orig_external_in += txi.amount + + if input_is_external_unverified(txi): + if safety_checks.is_strict(): + raise wire.ProcessError("Unverifiable external input.") + else: + self.external_in += txi.amount + if txi.orig_hash: + self.orig_external_in += txi.amount def _add_output(self, txo: TxOutput, script_pubkey: bytes) -> None: self.weight.add_output(script_pubkey) @@ -351,6 +358,15 @@ class CoinJoinApprover(Approver): if not self.authorization.check_sign_tx_input(txi, self.coin): raise wire.ProcessError("Unauthorized path") + def add_external_input(self, txi: TxInput) -> None: + super().add_external_input(txi) + + # External inputs should always be verifiable in CoinJoin. This check + # is not critical for security, we are just being cautious, because + # CoinJoin is automated and this is not a very legitimate use-case. + if input_is_external_unverified(txi): + raise wire.ProcessError("Unverifiable external input.") + def add_change_output(self, txo: TxOutput, script_pubkey: bytes) -> None: super().add_change_output(txo, script_pubkey) self.our_weight.add_output(script_pubkey) diff --git a/core/src/apps/bitcoin/sign_tx/bitcoin.py b/core/src/apps/bitcoin/sign_tx/bitcoin.py index 1dd68dd7e..9a533392b 100644 --- a/core/src/apps/bitcoin/sign_tx/bitcoin.py +++ b/core/src/apps/bitcoin/sign_tx/bitcoin.py @@ -15,6 +15,7 @@ from ..common import ( bip340_sign, ecdsa_sign, input_is_external, + input_is_external_unverified, input_is_segwit, ) from ..ownership import verify_nonownership @@ -205,8 +206,10 @@ class Bitcoin: progress.advance() txi = await helpers.request_tx_input(self.tx_req, i, self.coin) writers.write_tx_input_check(h_check, txi) - assert txi.script_pubkey is not None # checked in sanitize_tx_input - await self.verify_external_input(i, txi, txi.script_pubkey) + if not input_is_external_unverified(txi): + assert txi.script_pubkey is not None # checked in sanitize_tx_input + await self.verify_external_input(i, txi, txi.script_pubkey) + progress.advance(self.tx_info.tx.inputs_count - len(self.external)) else: # There are internal non-Taproot inputs. We need to verify all inputs, because we can't @@ -232,7 +235,7 @@ class Bitcoin: if script_pubkey != self.input_derive_script(txi): raise wire.DataError("Input does not match scriptPubKey") - if i in self.external: + if i in self.external and not input_is_external_unverified(txi): await self.verify_external_input(i, txi, script_pubkey) # check that the inputs were the same as those streamed for approval diff --git a/core/src/apps/bitcoin/writers.py b/core/src/apps/bitcoin/writers.py index 50ee6e6ce..7bedde242 100644 --- a/core/src/apps/bitcoin/writers.py +++ b/core/src/apps/bitcoin/writers.py @@ -15,6 +15,8 @@ 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, @@ -49,6 +51,7 @@ 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)) write_uint32(w, len(i.address_n)) for n in i.address_n: write_uint32(w, n) diff --git a/core/tests/test_apps.bitcoin.approver.py b/core/tests/test_apps.bitcoin.approver.py index 46d670eeb..10e7c7a0f 100644 --- a/core/tests/test_apps.bitcoin.approver.py +++ b/core/tests/test_apps.bitcoin.approver.py @@ -49,6 +49,7 @@ class TestApprover(unittest.TestCase): script_pubkey=bytes(22), script_type=InputScriptType.EXTERNAL, sequence=0xffffffff, + witness="", ) for i in range(99) ] diff --git a/core/tests/test_apps.bitcoin.sign_tx.writers.py b/core/tests/test_apps.bitcoin.sign_tx.writers.py index 562c4f36c..65491cccb 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 + 4 + 4 + 4 + 8 + 26) + self.assertEqual(len(b), 32 + 4 + 4 + 1 + 4 + 4 + 4 + 8 + 26) for bad_prevhash in (b"", b"x", b"hello", b"x" * 33): inp.prev_hash = bad_prevhash diff --git a/docs/common/communication/bitcoin-signing.md b/docs/common/communication/bitcoin-signing.md index 46c297464..7054319d7 100644 --- a/docs/common/communication/bitcoin-signing.md +++ b/docs/common/communication/bitcoin-signing.md @@ -150,16 +150,28 @@ Full documentation for multisig is TBD. #### External inputs -Trezor T can include inputs that it will not sign, typically because they are owned by -another party. Such inputs are of type `EXTERNAL` and the host does not specify a -derivation path for the key. Instead, these inputs must either already have a valid -signature or they must come with an ownership proof. If the input already has a valid -signature, then the host provides the `script_sig` and/or `witness` fields. If the other -signing party hasn't signed their input yet (i.e., with two Trezors, one must sign first -so that the other can include a pre-signed input), they can instead provide a +Trezor can include inputs that it will not sign, typically because they are +owned by another party. Such inputs are of type `EXTERNAL` and the host does +not specify a derivation path for the key, but sets the `script_pubkey` field +to the scriptPubKey of the previous output that is being spent by the external +input. An external input can either be *verified* or *unverified*. The amounts +supplied to the transaction by verified external inputs are subtracted from the +transaction total that the user is asked to confirm, whereas unverified +external inputs are not subtracted. + +Verified external inputs are only supported on the Trezor T. They must either +already have a valid signature or they must come with an ownership proof. If +the input already has a valid signature, then the host provides the +`script_sig` and/or `witness` fields. If the other signing party hasn't signed +their input yet (i.e., with two Trezors, one must sign first so that the other +can include a pre-signed input), they can instead provide a [SLIP-19](https://github.com/satoshilabs/slips/blob/master/slip-0019.md) -ownership proof in the `ownership_proof` field, with optional commitment data in -`commitment_data`. The `script_pubkey` field is required for all external inputs. +ownership proof in the `ownership_proof` field, with optional commitment data +in `commitment_data`. + +Unverified external inputs are accepted by Trezor only if +[safety checks](https://wiki.trezor.io/Using_trezorctl_commands_with_Trezor#Safety-checks) +are disabled on the device. ### Transaction output