Skip to content

[sanity check] Reject a PSBT whose fingerprint records disagree - #1040

Draft
kdmukai wants to merge 5 commits into
SeedSigner:devfrom
kdmukai:psbt_fingerprint_consistency
Draft

kdmukai wants to merge 5 commits into
SeedSigner:devfrom
kdmukai:psbt_fingerprint_consistency

Conversation

@kdmukai

@kdmukai kdmukai commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Builds on top of changes in #1044. Kept in DRAFT until its parent is merged.

This is a LOW priority follow-up to the _get_cosigners change in #1032 (45a5eab), which stopped using the claimed fingerprint to match a key to its xpub. It occurred to me that while we can't rely on the fingerprints being correct from the coordinator, we can verify that they're at least internally consistent when supplied (global xpubs' fingerprints match the fingerprint for the key that they derive).

Written by Claude, revised by Keith:


Description

Problem or Issue being addressed

A psbt carries two claims about each key's origin:

  • the fingerprint on the key's derivation path entry (bip32_derivations)
  • the fingerprint on the global xpub that derives that key (PSBT_GLOBAL_XPUB)

Both are the coordinator's own claims about the same master key, so they should agree. Currently a psbt where they disagree parses without complaint: since #1032 matches keys to xpubs by derivation, a mislabeled fingerprint would be completely ignored.

There's no attack risk in allowing mismatched fingerprints: cosigners resolve by derivation, and a mislabel on a cosigner's key changes nothing downstream (though a mislabel that claims OUR fingerprint is already caught by the ownership scan).

It's evidence of a self-contradictory psbt and we already reject psbts for similar correctness issues without calling them attacks (PSBTMixedDerivationPathTypesError, the single-sig case of PSBTSurplusDerivationPathsError).


Solution

A new final step in parse(), _reject_inconsistent_fingerprints, iterates through every input and output entry, finds the global xpub that derives its key (through the existing derivation cache, so the re-derivation is basically free), and raises PSBTInconsistentFingerprintError if the two fingerprints differ.

  • All-zero fingerprints are skipped on either side. Coordinators write 00000000 for a fingerprint they do not know (xpub-only keystores; see _fill_missing_fingerprints), so that is a missing value, not a second answer.
  • Runs last so that a more serious finding about our own keys (PSBTOutputOwnershipContradictionError, the ownership claim errors) is still the one reported.

The view layer routes the new error to PSBTInconsistentFingerprintView, a Warning-level "Transaction Problem" screen in the same grade as the mixed-derivation-map error: one record is wrong, but nothing says which, so it is not graded as an attack.


Additional Information

  • _get_cosigners is where the two records meet and would be the natural place for the check, but it is a static helper under _get_policy's catch-all except, and raising there would pre-empt the more serious errors that run later. A TODO on the new method records the refactor (instance methods, record the mismatch, raise after the outputs are read).
  • Also in this PR: a TODO in mnemonic_generation.py for a seed whose real fingerprint is 00000000. Such a seed cannot be told apart from one the coordinator never identified, so a multisig psbt that also carries zero-labeled cosigner keys is refused for it. One seed in 2^32; no code change.

Tests

  • New tests added for single sig and multisig fingerprint mismatch scenarios, including the all-zero case.
  • Minor changes to affected existing tests.

Screenshots

PSBTInconsistentFingerprintView

This pull request is categorized as a:

  • Other: Insist on internal consistency in psbt claims.

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.

  • Yes

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.

@kdmukai
kdmukai marked this pull request as draft September 16, 2026 17:03
@Chaitanya-Keyal Chaitanya-Keyal moved this to 0.8.8 Needs Review in @SeedSigner Development Board Sep 16, 2026
3rdIteration added a commit to 3rdIteration/seedsigner that referenced this pull request Sep 18, 2026
…ia descriptor

From a review of open upstream SeedSigner PRs and their discussions:

- Previous-tx proof (upstream SeedSigner#995/SeedSigner#977): a non_witness_utxo must hash to the
  txid its input spends (UTXO_MISMATCH), and a legacy p2sh input may not rely
  on a witness_utxo alone (INVALID_WITNESS_UTXO). Legacy signatures commit to
  no amount, so either lie hides fee in a single signing.
- Multisig change without global xpubs (review thread on upstream SeedSigner#1032): a
  change-shaped m-of-n containing our key is not provably this wallet's. It is
  now a payment unless a loaded descriptor identifies it; the new
  PSBTIdentifyChangeView offers "Load descriptor to identify change", and
  returning with a descriptor re-reads the psbt. verify_multisig_output's
  per-entry check is shared as _descriptor_owns_output.
- OP_RETURN payload read from its push opcode (SeedSigner#965/SeedSigner#1003): direct pushes lost
  their first byte on screen.
- INCONSISTENT_FINGERPRINTS (SeedSigner#1040): a global xpub whose fingerprint disagrees
  with the keys it derives.
- Untrusted-QR crashes (SeedSigner#967/SeedSigner#968/SeedSigner#970/SeedSigner#971): SeedQR checksum and whole-payload
  detection, signmessage bounds, parse_derivation_path on short/48h paths, and
  a UR2 seq_len bound.

New psbt_faker vectors cover each (PREV_TX_TXID_MISMATCH, LEGACY_P2SH_WITNESS_ONLY,
OP_RETURN_DIRECT_PUSH, TX-24.xpub_fingerprint_mismatch, TX-21.ms_foreign_quorum*,
TX-21.ms_honest_change_no_xpubs). TX-20 and XTRAS.MIXED_INPUT_TYPES are
re-vendored after generator fixes; TX-20 is no longer vacuous.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3rdIteration added a commit to 3rdIteration/seedsigner that referenced this pull request Sep 18, 2026
* test+fix: resync psbt_faker corpus and close the gaps it exposes

Re-vendor the regenerated psbt_faker corpus (97 vectors, fixed BIP-371
taproot encoding) and cover every vector in psbt_suite_util.

Parser fixes for vectors that previously parsed:
- FORGED_OUTPUT_OWNERSHIP also when a genuine key of ours annotates an
  output whose script does not pay it (TX-21.claims_us_pays_other,
  TX-21.ms_repointed_*, TX-22.ms_decoy_*).
- MISLABELED_OUTPUT_OWNERSHIP: an output paying our key but attributed to
  a foreign fingerprint (TX-21.pays_us_claims_other, taproot_claims_other,
  ms_fp_relabeled).
- SURPLUS_DERIVATIONS / MIXED_DERIVATION_MAPS for contradictory claims
  (TX-22.surplus_paths, taproot_surplus_internal_key, mixed_path_maps*).
- SCRIPT_TIMELOCK warning for CLTV/CSV in an output's committed script
  (TX-20.cltv_time).
- verify_multisig_output checks every derivation entry instead of embit's
  first-match Descriptor.owns (decoy-last used to verify as ours).

TX-20.cltv_height/csv_relative are vacuous upstream (scriptPubKey is still
p2wpkh) and marked so.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test: stop flow tests hanging when collected after the refusal-screen tests

tests/test_psbt_refusal_screens.py imported the GUI before tests/base.py
installed its renderer and hardware-button doubles. Any flow test collected
after it then ran against the real, never-configured Renderer (every View and
the error View raise, so the Controller loops forever) or against real buttons
(real-screen flows wait for a key press forever). The full suite only passed
because alphabetical order puts the flow tests first.

The refusal-screen test now imports base first, and base.py also rebinds
seedsigner.gui.Renderer when the package was already imported by some other
module collected earlier.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix: port upstream PSBT/QR hardening; identify multisig change only via descriptor

From a review of open upstream SeedSigner PRs and their discussions:

- Previous-tx proof (upstream SeedSigner#995/SeedSigner#977): a non_witness_utxo must hash to the
  txid its input spends (UTXO_MISMATCH), and a legacy p2sh input may not rely
  on a witness_utxo alone (INVALID_WITNESS_UTXO). Legacy signatures commit to
  no amount, so either lie hides fee in a single signing.
- Multisig change without global xpubs (review thread on upstream SeedSigner#1032): a
  change-shaped m-of-n containing our key is not provably this wallet's. It is
  now a payment unless a loaded descriptor identifies it; the new
  PSBTIdentifyChangeView offers "Load descriptor to identify change", and
  returning with a descriptor re-reads the psbt. verify_multisig_output's
  per-entry check is shared as _descriptor_owns_output.
- OP_RETURN payload read from its push opcode (SeedSigner#965/SeedSigner#1003): direct pushes lost
  their first byte on screen.
- INCONSISTENT_FINGERPRINTS (SeedSigner#1040): a global xpub whose fingerprint disagrees
  with the keys it derives.
- Untrusted-QR crashes (SeedSigner#967/SeedSigner#968/SeedSigner#970/SeedSigner#971): SeedQR checksum and whole-payload
  detection, signmessage bounds, parse_derivation_path on short/48h paths, and
  a UR2 seq_len bound.

New psbt_faker vectors cover each (PREV_TX_TXID_MISMATCH, LEGACY_P2SH_WITNESS_ONLY,
OP_RETURN_DIRECT_PUSH, TX-24.xpub_fingerprint_mismatch, TX-21.ms_foreign_quorum*,
TX-21.ms_honest_change_no_xpubs). TX-20 and XTRAS.MIXED_INPUT_TYPES are
re-vendored after generator fixes; TX-20 is no longer vacuous.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* test(screenshots): give the multisig PSBT global xpubs; add PSBTIdentifyChangeView

The screenshot generator's multisig PSBT carries no PSBT_GLOBAL_XPUB, so since
multisig change is only identified through global xpubs or a loaded descriptor,
it had no change output and the PSBTChangeDetailsView_multisig_* screenshots
crashed (get_change_data returned None), failing CI's "Generate screenshots".

- The multisig screenshots now use a copy of that PSBT with global xpubs taken
  from its own wallet descriptor, like a coordinator that includes them.
- The parser is built with the loaded descriptor, and the descriptor is loaded
  before the PSBT, so the "verified" screenshot parses the way the device would.
- New screenshots for PSBTIdentifyChangeView (no descriptor / descriptor loaded),
  from the original xpub-less PSBT.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
When a global xpub derives a key, the fingerprint beside the key and
the fingerprint beside the xpub both describe that key's origin, so
they are expected to agree. A multisig psbt where they differ is now
refused with a Warning-level screen rather than an attack grading:
one record is wrong, but nothing says which.

The check runs as a final pass after the ownership checks so that a
false claim about our own keys is still the finding reported.
_get_cosigners is where the two records meet, but it is a static
helper under a catch-all except, and raising there pre-empts the more
serious errors; a TODO records the refactor that would fold the check
in. All-zero fingerprints are skipped, as in _fill_missing_fingerprints,
because xpub-only keystores produce them legitimately.
A seed whose fingerprint is 00000000 cannot be told apart from one a
coordinator never identified, and a multisig psbt that also carries
zero-labeled cosigner keys is refused for it. Record where a warning
at seed creation would go and why.
The consistency check skipped single-sig policies because nothing on
that path reads the global xpubs, so the comparison would add
derivations the parser did not otherwise make. But a single-sig psbt
whose xpub record and key entry disagree is just as self-contradictory,
and the cost is two cached derivations per key, small next to what a
multisig parse already performs. A test pins the single-sig refusal,
and the two derivation-count tests account for the xpub levels by name.
@kdmukai
kdmukai force-pushed the psbt_fingerprint_consistency branch from 23c4957 to ccaf1a5 Compare September 20, 2026 14:36
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