Skip to content

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

Draft
kdmukai wants to merge 5 commits into
SeedSigner:devfrom
kdmukai:2026_09_psbt_nested_singlesig_change
Draft

kdmukai wants to merge 5 commits into
SeedSigner:devfrom
kdmukai:2026_09_psbt_nested_singlesig_change

Conversation

@kdmukai

@kdmukai kdmukai commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Depends on #1044 so this PR will remain in DRAFT until 1044 is merged.


Human-friendly overview:

The Problem

A nested single sig (p2sh-p2wpkh) output can omit its redeem script (it's optional per BIP-174). Such outputs are interpreted as plain p2sh and so they fail the initial possible change check (p2sh fails to match p2sh-p2wpkh). That creates two undesirable possible outcomes:

  • The user's own change would be displayed as an external spend.
    • Note: BlueWallet produces such psbts.
  • A specific type of deceptive claim that we could have detected is not inspected and is allowed to pass as an external spend.
    • I don't think there's a clear reason to structure an attack this way, but it would require:
      • The output's bip32_derivations specify a key that the user's seed does actually own, making it look like it really is their own change.
      • But the redeem script is omitted and the output's actual scriptPubKey is a p2sh-p2wpkh hash that pays to an attacker's key.

The Solution

Allow nested single sig outputs to be considered possible change when the redeem script is omitted AND the output's bip32_derivations claim a key that is derived from the user's seed. Those outputs will then be subject to our deeper validity checks.

The vital insight here is that the redeem script for nested single sig is trivially reproducible. Even when the psbt supplies it, we ignore it during our change verification and just regenerate it ourselves. And once we rebuild the redeem script, we can use it to produce the output's expected scriptPubKey and see if it matches the scriptPubKey in the psbt.

In this case the scriptPubKey will not match. The deceptive bip32_derivations claim will have been found and an exception will be raised.


More precise description below by Claude, primarily intended to be consumed by other AIs

Mechanism

_get_policy types an output from what that output supplies. A p2sh-p2wpkh output's scriptPubKey
is a bare p2sh hash, so the redeem script is the only field separating it from plain p2sh. Drop it
and the output's policy is {type: p2sh}, which does not match the wallet's p2sh-p2wpkh policy.

Nested single sig is the only script type where this can happen, because it is the only one whose
identity depends on a field its proof never reads. The proof rebuilds p2sh(p2wpkh(K)) from
our own seed at the claimed path and from the inputs' policy, so the omitted field is a
discriminator rather than evidence. For p2sh-p2wsh the discriminator and the proof material are
the same field, so its absence is genuine silence and correctly out of reach. For p2wpkh, p2pkh
and p2tr the scriptPubKey announces the type on its own.


Implementation

_policy_shape_matches becomes _is_change_candidate, an instance method, since it now reads the
inputs' policy and the output's verified derivation paths alongside the output's own policy.

Beside the unchanged shape comparison it admits an output on all five of:

self.policy["type"] == "p2sh-p2wpkh"    # Input policy criteria
and out_policy["type"] == "p2sh"        # Output policy criteria
and "m" not in out_policy               # Exclude multisig
and len(out.bip32_derivations) == 1     # Nested single sig pays just one key
and len(verified_derivation_paths) == 1 # And that one key must be ours

The middle three are how "this is plausibly a nested single sig output" gets expressed in code
that cannot see the redeem script. Everything below the candidacy test is unchanged.

Two files change: src/seedsigner/models/psbt_parser.py and tests/test_psbt_parser.py. The
commit message states the same argument in prose; consult it before the diff if you want the
reasoning in one piece.

Links below are pinned to this PR's head commit, so they stay correct as the branch moves.

Read for Source
The five clauses, each with its rationale as a trailing comment _is_change_candidate, L721-729
Why the candidacy test exists at all and what it does not decide that method's docstring, L699-720
The trust model this rests on and its verdict table parse's process docstring, L216-274
Where an admitted output is proved or refused, single sig arm _parse_outputs, L431-446
p2sh(p2wpkh(K)) construction _build_singlesig_script, L738-758
The contradiction refusal this change makes reachable, with the taproot exemption beside it _parse_outputs, L576-596
Honest change counted as change test, L1949
Repointed output refused test, L1973

Which coordinators emit this

The opening names BlueWallet. Source-level survey, repos read at HEAD 2026-09-24, asking one
question: does the coordinator write PSBT_OUT_REDEEM_SCRIPT on a p2sh-p2wpkh change output?

Coordinator Writes it Where
BlueWallet (BIP 49) No abstract-hd-electrum-wallet.ts, createTransaction
Bitcoin Core Yes FromSignatureData, from ProduceSignature's sigdata
Specter Desktop Yes drives Core's engine
Nunchuk Yes drives Core's engine
Sparrow Yes drongo's PSBT constructor, for WalletNodeOutput change
Electrum Yes add_output_info sets script_descriptor, whose setter fills redeem_script

BlueWallet's HDsegwitP2SHWallet is segwitType: "p2sh(p2wpkh)" at m/49'/0'/0'. It extends
AbstractHDElectrumWallet and overrides _addPsbtInput() but not createTransaction(), so its
change output takes the inherited path, which passes bip32Derivation and no redeemScript.

Two limits on that survey. Bitcoin Keeper's single-sig vault change output has the same shape
(addOutput({ ...output, bip32Derivation }), no redeem script), but I did not verify whether
those vaults are p2sh-p2wpkh rather than native segwit or taproot, so it is not counted; its
non-vault wallets annotate no output at all, which puts them outside this entirely. Zeus and
Caravan were not reached; Caravan is legacy multisig only in any case. This is a read of source,
not a capture of real psbts.


Design Consideration

Why the admission is narrow

  • No m-of-n leaves a legacy p2sh multisig to the multisig path, which has its own redeem
    script to work from.
  • Exactly one derivation path entry avoids a false refusal with a real cost: the single sig
    arm raises PSBTSurplusDerivationPathsError on an output claiming more than one path, so
    without this clause an ordinary p2sh payment annotated with two of the recipient's own entries
    ends the review on PSBTSurplusDerivationPathsView.
  • A verified claim on this seed keeps the case tied to the ownership scan's result.

The first two are also what keeps the resulting refusal safe. A genuine multisig output carries
one derivation path entry per cosigner, so it never reaches the contradiction check by this route.

What this does not defend against

An output paying a script type this wallet never scans is still outside the change check, because
the type genuinely differs. Catching that would mean rebuilding the seed's key under every script
type.


Screenshots

No screen changes. The refusal reaches the existing PSBTOutputOwnershipContradictionView.


Testing

Note that testqrs.com has been updated with this PR's test scenarios.

pytest passes at 259. Two new parser tests, one for the honest output and one for the repointed
one.

Before this change, measured on a nested single sig fixture with its redeem script removed: change
0, spend 10,000, with the user's own change address listed as the destination.

Mutation checks:

Mutation Result
Remove the new p2sh case from _is_change_candidate Both new tests fail
Drop the m-of-n exclusion Suite passes
Drop the one-derivation-path condition Suite passes
Drop the verified-claim condition Suite passes

The three narrowing conditions are not pinned individually: no fixture in the suite pays a legacy
p2sh multisig or an unclaimed p2sh output under p2sh-p2wpkh inputs.

I have not tested this on hardware.


Other Notes

  • The _parse_outputs process docstring's cross-reference follows the rename.

This pull request is categorized as a:

  • Other: Security hardening

Checklist

I ran pytest locally

  • All tests passed before submitting the PR

I included screenshots of any new or modified screens

Should be part of the PR description above.

  • N/A

I added or updated tests

Any new or altered functionality should be covered in a unit test. Any new or updated sequences require FlowTests.

  • Yes

I tested this PR hands-on on the following platform(s):


I have reviewed these notes:

  • Keep your changes limited in scope.
  • If you uncover other issues or improvements along the way, ideally submit those as a separate PR.
  • The more complicated the PR, the harder it is to review, test, and merge.
  • We appreciate your efforts, but we're a small team of volunteers so PR review can be a very slow process.
  • Please only "@" mention a contributor if their input is truly needed to enable further progress.
  • I understand

Thank you! Please join our Devs' Telegram group to get more involved.

Names the input shape: a list of child indices below parent_key, which
is what the cache is keyed on. A companion that takes a psbt entry's
DerivationPath object follows; the two names then say which one a caller
holds.
A multisig output lists one derivation path entry per cosigner. The
output check verified only the first entry claiming this seed against
the committed script, so a second claim of ours whose key the script has
no use for went unreported when listed behind our real entry or in the
place of another cosigner's entry (which keeps the entry count at n and
passes the surplus count).

Every entry claiming this seed is now held to the script. To feed that,
the ownership scan keeps every entry it proved per input and output, as
the psbt's own DerivationPath objects, instead of the first one's path.
_derive_with_cache_via_derivation_path is added beside the index-taking
primitive for callers holding such an entry; it reads only the entry's
path, since the single-sig rebuild and the multisig fallback derive at
entries carrying a foreign fingerprint on purpose. change_data keeps the
index-list path the views read.
A p2sh-p2wpkh output's scriptPubKey is a bare p2sh hash. BIP-174 leaves the
redeem script optional on an output, and without it _get_policy types the output
as plain p2sh, which does not match a p2sh-p2wpkh wallet policy. The output then
never reaches the ownership check at all. Two things follow. The user's own
change is displayed as a payment out to a stranger. And an output that keeps a
genuine claim on this seed while repointing its scriptPubKey escapes the
ownership-contradiction refusal by dropping one optional field.

Nested single sig is the only script type whose identity depends on an optional
field its proof does not read: the rebuild is p2sh(p2wpkh(K)) from our own seed
at the claimed path and the inputs' policy, so the missing field is a
discriminator rather than evidence. For p2sh-p2wsh the discriminator and the
proof material are the same field, so its absence is genuine silence and
correctly out of reach.

_policy_shape_matches becomes _is_change_candidate, an instance method, since it
now reads the inputs' policy and the output's verified derivation paths. Beside
the unchanged shape comparison it admits a bare p2sh output under a p2sh-p2wpkh
wallet that carries no m-of-n, supplies exactly one derivation path entry, and
holds one verified claim on this seed. Everything below is unchanged: the
rebuild decides, honest change is counted as change, and a repointed output
raises PSBTOutputOwnershipContradictionError.

The single-entry conditions are what keep that refusal safe. A genuine multisig
output carries one derivation path entry per cosigner, so it never reaches the
contradiction check by this route. The shape that would is a bare p2sh output
withholding both scripts while annotating a single key of a multisig, and no
surveyed coordinator emits one. Every coordinator but Bitcoin Core annotates
only its own change output. Core annotates an output because a descriptor solved
its script, so it holds all n key origins and writes them.
@kdmukai
kdmukai force-pushed the 2026_09_psbt_nested_singlesig_change branch 2 times, most recently from eb3091c to 81ae37a Compare September 25, 2026 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 0.8.8 Needs Review

Development

Successfully merging this pull request may close these issues.

1 participant