diff --git a/src/seedsigner/models/psbt_parser.py b/src/seedsigner/models/psbt_parser.py index 2f62066d..501c23ca 100644 --- a/src/seedsigner/models/psbt_parser.py +++ b/src/seedsigner/models/psbt_parser.py @@ -249,7 +249,7 @@ class PSBTParser(): _get_policy doesn't propagate cosigner errors, so two such policies match without anything having tied them to the same keys. TODO: don't let a policy with no cosigner information pass as a match between inputs. - Outputs deliberately compare shape alone; see _policy_shape_matches. + Outputs deliberately compare shape alone; see _is_change_candidate. 5. _parse_outputs: organizes the output data (amounts, destination_addresses, etc.) and verifies the ownership of the outputs that come back to this seed @@ -411,7 +411,7 @@ class PSBTParser(): # Is this output change? If this output's policy is superficially similar to # the spending wallet's policy (e.g. they're both 2-of-3 p2wsh), then it's a # candidate for being change. - if PSBTParser._policy_shape_matches(out_policy, self.policy): + if self._is_change_candidate(out, out_policy): # Begin the extensive work to fully verify whether this output is indeed # change. @@ -708,23 +708,49 @@ class PSBTParser(): return policy - @staticmethod - def _policy_shape_matches(policy_a: dict, policy_b: dict) -> bool: + def _is_change_candidate(self, out: OutputScope, out_policy: dict) -> bool: """ - Compares two policies on the shape of the script they describe: the script type, - plus m-of-n for multisig. + Determines whether an output is worth the full ownership check in _parse_outputs. - A policy can also carry the cosigners resolved from the coordinator's global - xpubs. Those are never authoritative here, and comparing them would let a psbt - decide which of its own outputs get verified: one misannotated fingerprint makes - that output's cosigners fail to resolve, and the output then stops matching the - inputs' policy. Shape comes from the scriptPubKey and the supplied script, and the - caller proves ownership rather than assuming it. + Returns True if the output's policy has the same "shape" as the inputs' policy: + the script type, plus m-of-n for multisig. + + One outlier: Nested single sig (p2sh-p2wpkh). Its scriptPubKey is a p2sh hash of + its redeem script, but per BIP-174 the redeem script itself is optional. + When it is omitted, the output is superficially indistinguishable from plain p2sh. + If the inputs are p2sh-p2wpkh, then such an output would fail the policy + comparison test (p2sh != p2sh-p2wpkh) when it may have actually been possible to + verify it as our change. + + So instead, when a p2sh output could be our own nested single sig change we let it + through and leave it to the rebuild process to verify if the output really is our + change. + + Note: A multisig's input or output policy can also include the cosigners if + they're supplied in the global xpubs. But this function does not take the + cosigners into account; comparing the cosigners here would let a psbt decide which + of its own outputs get verified: + * One misannotated derivation path would make that output's cosigners fail + to resolve. + * The output's missing cosigners would mean that it would not match the inputs' + cosigners. + * End result: the output would not be considered a change candidate and would + not go through the same scrutiny that change candidates do. + + Cosigner information, if provided, is evaluated later. """ + # The outlier: a single sig p2sh output when the inputs are p2sh-p2wpkh. + if ( + self.policy["type"] == "p2sh-p2wpkh" # Input is nested single sig + and out_policy["type"] == "p2sh" # Output parses as plain p2sh + and out.redeem_script is None # Output omits its redeem script + ): + return True + + # All other outputs must have the same policy shape as the inputs for field in ("type", "m", "n"): - if policy_a.get(field) != policy_b.get(field): + if out_policy.get(field) != self.policy.get(field): return False - return True diff --git a/tests/test_psbt_parser.py b/tests/test_psbt_parser.py index 588dc393..4bb9103a 100644 --- a/tests/test_psbt_parser.py +++ b/tests/test_psbt_parser.py @@ -2062,6 +2062,113 @@ class TestPSBTParserOutputOwnership(PSBTParserOwnershipTestBase): self._parse(psbt) + def test__parse__counts_nested_single_sig_change_without_its_redeem_script_as_change(self): + """ + A legitimate nested single sig (p2sh-p2wpkh) change output can omit its redeem + script (BIP-174 makes it optional) while still claiming (via bip32_derivations) + that a key owned by our seed will receive the change. + + But the parser's proof of ownership check does not care about the missing redeem + script: the parser rebuilds p2sh(p2wpkh(K)) from the seed's own key at the claimed + path regardless. So the output should still be verifiable as change. + """ + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_CHANGE) + + # The output claims a single key and our seed really does derive it there. + assert len(psbt.outputs[0].bip32_derivations) == 1 + public_key, derivation_path = list(psbt.outputs[0].bip32_derivations.items())[0] + assert PSBTParser.seed_owns_pubkey(self._root(), derivation_path.derivation, public_key, child_key_derivation_cache=None) is True + + psbt.outputs[0].redeem_script = None + + psbt_parser = self._parse(psbt) + assert psbt_parser.change_amount == 10_000 + assert psbt_parser.spend_amount == 0 + + + def test__parse__rejects_a_bare_p2sh_output_that_claims_this_seed_but_pays_someone_else(self): + """ + Variation on the prior test: the redeem script is still omitted but this time the + psbt repoints its output at a stranger's p2sh-p2wpkh. Crucially, the output keeps + its claim on this seed, making this an attempt at deception (if there was no claim + on the output, it would simply be a typical external spend output). + + When the parser rebuilds p2sh(p2wpkh(K)), the resulting scriptPubKey will not + match what the output commits to. The psbt should be refused with + PSBTOutputOwnershipContradictionError. + """ + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_CHANGE) + psbt.outputs[0].redeem_script = None + psbt.outputs[0].script_pubkey = script.p2sh(script.p2wpkh(foreign_public_key())) + + with pytest.raises(PSBTOutputOwnershipContradictionError): + self._parse(psbt) + + + def test__parse__rejects_a_bare_p2sh_output_that_pays_this_seed_but_lists_another_key(self): + """ + Another variation: the redeem script is omitted and the output still pays our key, + but its one derivation path entry lists another seed's key and fingerprint at the + same path. + + The parser rebuilds p2sh(p2wpkh(K)) from our own key at that path and ignores the + fingerprint the entry lists. The result based on our key matches what the output + commits to, so it actually is our output even though the psbt claimed that it paid + to a different key. The psbt should be refused with + PSBTOutputOwnershipContradictionError. + """ + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_CHANGE) + psbt.outputs[0].redeem_script = None + + # Swap the output's entry for another seed's key at the same path. The + # scriptPubKey still pays our key there. + derivation_path = list(psbt.outputs[0].bip32_derivations.values())[0] + path = bip32.path_to_str(derivation_path.derivation) + psbt.outputs[0].bip32_derivations.clear() + claim_seed_owns_key(psbt.outputs[0], path, foreign_public_key(path), seed=PSBTTestData.recipient_seed) + + with pytest.raises(PSBTOutputOwnershipContradictionError): + self._parse(psbt) + + + def test__parse__counts_a_payment_to_another_wallet_of_this_seed_as_a_spend(self): + """ + A psbt can pay another wallet of this seed, such as its native segwit account or a + multisig it belongs to, and annotate that output with our key. Each case below is + such a payment. Each should parse and be counted as a spend. + + The parser makes an exception for nested single sig change that omits its redeem + script. An output must meet three criteria to qualify for the exception: + * inputs: the inputs are nested single sig + * output type: the output is parsed as plain p2sh + * redeem script: the output omits its redeem script + + These outputs should be categorized as external spends if any of the three + criteria are not met. Each scenario in this test sets up one criterion to fail + while the other two are met. + """ + # Fails the inputs condition: a native segwit input instead of nested single sig + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_CHANGE) + psbt.outputs[0].redeem_script = None + psbt_parser = self._parse(psbt) + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + # Fails the output type condition: p2wpkh is not parsed as a plain p2sh output + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.SINGLE_SIG_NATIVE_SEGWIT_CHANGE) + psbt_parser = self._parse(psbt) + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + # Fails the redeem script condition: legacy multisig is parsed as plain p2sh even + # when it supplies its redeem script. + psbt = self._psbt_with_change(PSBTTestData.SINGLE_SIG_NESTED_SEGWIT_1_INPUT, PSBTTestData.MULTISIG_LEGACY_P2SH_CHANGE) + assert psbt.outputs[0].redeem_script is not None + psbt_parser = self._parse(psbt) + assert psbt_parser.change_amount == 0 + assert psbt_parser.spend_amount == 10_000 + + def test_get_cosigners_returns_a_sorted_list(self): """ Two multisig scripts can list the same wallet's keys in different orders, so the