Merge pull request #1046 from kdmukai/2026_09_psbt_nested_singlesig_change

[security] Verify nested single sig change that omits its redeem script

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 _is_change_candidate(self, out: OutputScope, out_policy: dict) -> bool:
-    def _policy_shape_matches(policy_a: dict, policy_b: dict) -> bool:
         """
-        Compares two policies on the shape of the script they describe: the script type,
+        Determines whether an output is worth the full ownership check in _parse_outputs.
-        plus m-of-n for multisig.
 
-        A policy can also carry the cosigners resolved from the coordinator's global
+        Returns True if the output's policy has the same "shape" as the inputs' policy:
-        xpubs. Those are never authoritative here, and comparing them would let a psbt
+        the script type, plus m-of-n for multisig.
-        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
+        One outlier: Nested single sig (p2sh-p2wpkh). Its scriptPubKey is a p2sh hash of
-        inputs' policy. Shape comes from the scriptPubKey and the supplied script, and the
+        its redeem script, but per BIP-174 the redeem script itself is optional.
-        caller proves ownership rather than assuming it.
+        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
 
 

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