Skip to content

[security] Verify input scripts against the scriptPubKey they commit to - #1047

Draft
kdmukai wants to merge 6 commits into
SeedSigner:devfrom
kdmukai:2026_09_psbt_input_scripts
Draft

kdmukai wants to merge 6 commits into
SeedSigner:devfrom
kdmukai:2026_09_psbt_input_scripts

Conversation

@kdmukai

@kdmukai kdmukai commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Depends on #1046. Will remain in DRAFT until 1046 is merged.

Description


More precise description below by Claude:


Mechanism

A p2sh or p2wsh scriptPubKey holds only a hash of the script the input spends with. BIP-174 asks
a signer to hash the supplied redeem script or witness script and compare it to that
scriptPubKey. Nested segwit has two layers to check: the redeem script is itself a hash of the
witness script.

_get_policy reads the wallet policy (m-of-n, cosigner keys) out of that script, and every
output is then compared against that policy.

An input must also carry only the scripts it commits to. A legacy multisig or p2sh-p2wpkh redeem
script is not a script hash, so a witness script added to either input has no hash to check
against. Yet _get_policy decides a p2sh input is nested segwit from the mere presence of a
witness script:

if script_type == "p2sh":
    if scope.witness_script is not None:
        script_type = "p2sh-p2wsh"

So an extraneous script is refused on presence alone, on every input type.


Implementation

_verify_input_scripts runs on every input, before _get_policy, whether or not the psbt claims
any of the seed's keys on that input. It has three refusals:

Input supplies Raises Graded as Screen level
Fewer scripts than it commits to PSBTMissingInputScriptError Correctness problem Warning
A script that hashes to the wrong value PSBTInputScriptMismatchError Attack Dire Warning
A script beyond the ones it commits to PSBTExtraneousInputScriptError Ungraded Warning

PSBTOverviewView routes each error to its own screen, and all three screens end the flow.

Outputs are not held to this rule: an honest payment to a stranger's p2wsh legitimately carries
no witness script.

The commit message states the same argument in prose; consult it before the diff if you want the
reasoning in one piece.

Line numbers are for the PR head; the symbol named with each is authoritative if a number has
drifted.

Read for Source
Which scripts each input type must carry, and every refusal _verify_input_scripts, psbt_parser.py:376-434
Why it runs on every input, before the policy is read call site in _parse_inputs, psbt_parser.py:362-365
The step in the process docstring this adds parse process docstring, step 4, psbt_parser.py:268-272
What each error means, and its grade the three error classes, psbt_parser.py:106-140
The presence test that makes an extraneous witness script matter _get_policy, psbt_parser.py:769-776
Routing to the three screens PSBTOverviewView, psbt_views.py:127-137
The three screens PSBTMissingInputScriptView through PSBTExtraneousInputScriptView, psbt_views.py:630-710
Every shape refused, across five fixtures TestPSBTParserInputScripts, test_psbt_parser.py:2258-2406
Another party's input checked the same as the user's own test__parse__checks_input_scripts_whoever_the_input_belongs_to, test_psbt_parser.py:2384

What an unchecked script enables

Not moving funds. Measured on all four p2sh and p2wsh fixtures (p2wsh, p2sh-p2wsh, legacy p2sh
multisig, and p2sh-p2wpkh): sign the untampered psbt and each tampered one, then test the
resulting signature against the real coin's digest. embit signed every case.

Input as supplied Signature valid against the real coin
Untampered Yes
Witness script omitted or wrong, on p2wsh or p2sh-p2wsh No
Redeem script omitted or wrong, on legacy p2sh or p2sh-p2wpkh No
Witness script the input commits to nowhere, on legacy p2sh or p2sh-p2wpkh No
Redeem script omitted or wrong, on p2sh-p2wsh Yes
Redeem script the input commits to nowhere, on p2wsh Yes

Every tampered row is a shape _verify_input_scripts refuses. One rule explains every row: embit
builds the digest from the first of these the psbt supplies, the witness script, then the redeem
script, then the scriptPubKey. The script it lands on decides the signature, and any other
supplied script has no effect on it.

Measured 2026-09-13 against embit 0.8.0, which is still the pinned version.


Design Consideration

How each refusal is graded

  • Missing script: correctness problem. A required field is absent, and an absent field makes
    no claim for anything to contradict.
  • Hash mismatch: attack. The supplied script contradicts the scriptPubKey it is supplied for.
    On outputs, the same contradiction is the first multisig case of
    PSBTOutputOwnershipContradictionError, graded as an attack there. The grade follows the
    contradiction, not the harm: a wrong redeem script on p2sh-p2wsh is inert (a Yes row in the
    table above) and is graded as an attack too.
  • Extra script: ungraded for now. The input's own scripts check out, and the extra one sits
    outside everything the input commits to. It carries the fixed phrase "We don't try to decide
    whether this is an attack or a mistake."

No coordinator survey backs any of the three grades. No honest emitter of a wrong or an extra
input script has been identified.

Two refused shapes are inert

The two Yes rows above are harmless on their own. The signature and the policy both read the
witness script. The redeem script tampering in those rows leaves the witness script intact, so the
tampered field reaches neither.

They are refused anyway. Accepting them means keeping a rule about which supplied script matters
under which policy, and that rule moves with _get_policy. The presence test quoted under
Mechanism could reasonably move to the redeem script's type instead, and a wrong redeem script
would then decide the policy.

A script outside the input's chain of commitments is provably foreign to it. Refusing on that
alone stays correct however the policy logic moves.

Every input, whoever it belongs to

The check runs on every input, including one that claims none of the seed's keys, as another
party's input in a payjoin would. An input looks like someone else's only because of the claims
the psbt makes about it. So an exemption for other parties' inputs would be decided by an
unauthenticated field, and a coordinator could claim it for any input by stripping that input's
derivation paths.

The wallet policy itself is protected another way. Every input's policy has to equal the first
input's (anything else raises "Mixed inputs"), and the seed must be able to sign at least one
input, whose scripts are checked. So a wrong script on another party's input either yields the
same policy as the user's own input or ends the parse. No case was found where it changes the
policy; this was reasoned from the code, not probed. The rule covers every input because the
ownership line it would otherwise draw is the psbt's to move.


Screenshots

Three new screens. Each ends the flow with "Discard transaction".

PSBTMissingInputScriptView (Warning)

PSBTMissingInputScriptView

PSBTInputScriptMismatchView (Dire Warning)

PSBTInputScriptMismatchView

PSBTExtraneousInputScriptView (Warning)

PSBTExtraneousInputScriptView

Testing

pytest passes at 266. Four new parser tests and three new flow tests.

Every existing psbt fixture, in the tests and in the screenshot generator, still parses: each of
their inputs carries exactly the scripts its scriptPubKey commits to.

Mutation checks, full suite each time:

Mutation Tests that fail
Drop every missing-script refusal 3: both missing-script tests and the other-party test
Drop the three hash-mismatch refusals 3: both mismatch tests and the other-party test
Drop the two extraneous-script refusals 2: both extra-script tests
Skip the nested segwit inner layer 14: every honest p2sh-p2wsh input is then refused for its witness script, so 12 existing tests fail along with the missing-script and mismatch parser tests
Remove the _verify_input_scripts call All 7 new tests
Remove any one of the three except clauses in PSBTOverviewView Only that error's flow test

The extraneous-redeem-script refusal is tested on a native p2wpkh input. The p2wsh case in the
measurement table reaches the same line (psbt_parser.py:430-431).

Without its except clause, each error falls through to UnhandledExceptionView, the generic
System Error screen.

Not tested on hardware.


This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Documentation
  • Other

Checklist

I ran pytest locally

  • All tests passed before submitting the PR
  • I couldn't run the tests
  • N/A

I included screenshots of any new or modified screens

Should be part of the PR description above.

  • Yes
  • No
  • 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
  • No, I’m a fool
  • N/A

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.
BIP-174 asks a signer to hash an input's redeem or witness script
against the scriptPubKey being spent. We read the wallet policy (m-of-n,
cosigner keys) out of those scripts and never checked them, so a
coordinator's own script decided which outputs we displayed as the
user's change.

_verify_input_scripts runs on every input before the policy is read, and
each refusal has its own screen:

- A missing script raises PSBTMissingInputScriptError, graded as a
  correctness problem.
- A script that hashes to the wrong value raises
  PSBTInputScriptMismatchError, graded as an attack. It is the same
  contradiction between a supplied script and its scriptPubKey that
  PSBTOutputOwnershipContradictionError grades as an attack on outputs.
- An extraneous script (a witness script on a legacy multisig input, a
  redeem script on a native segwit input) raises
  PSBTExtraneousInputScriptError, left ungraded. It is refused on every
  input type: there is no hash to check it against, and _get_policy
  decides a p2sh input is nested segwit from the mere presence of a
  witness script.

Measured across the four p2sh and p2wsh fixtures: no signature over a
wrong script was valid against the real coin, because embit builds the
digest from the first supplied script (witness, then redeem, then
scriptPubKey). Funds were never movable this way; what the check closes
is the display.
@newtonick newtonick added this to the 0.8.8 milestone Sep 25, 2026
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.

2 participants