Skip to content

Derive the PSBT approval screen from the PSBT, not the requesting page - #38

Open
Jossec101 wants to merge 1 commit into
mainfrom
improve-approval-psbt-validation
Open

Derive the PSBT approval screen from the PSBT, not the requesting page#38
Jossec101 wants to merge 1 commit into
mainfrom
improve-approval-psbt-validation

Conversation

@Jossec101

Copy link
Copy Markdown
Contributor

The problem

ApprovePSBT displayed four fields. Only Tx Id and Fee came from the PSBT — Operation Type and Amount were scraped by the content script from #request-type and #channel-amount on the page requesting the signature:

data["request_type"] = requestTypeField.innerHTML;
data["amount"] = channelAmountField.innerHTML;

Meanwhile PSBTDetails carried only tx_id and fee, so no destination address, per-output amount, or total was ever derived from the PSBT and shown. The two fields a user actually reads to decide were authored by the party asking for the signature, and a transaction paying a substituted recipient rendered byte-for-byte identically to an honest one — the only difference being an opaque txid the extension never cross-references.

Two further display failures understated risk: PSBTDetails::from_str(&psbt).unwrap_or_default() rendered an empty txid and Fee: 0 when the PSBT failed to parse for display, while the Sign button stayed enabled and signed the raw string anyway; and fee_amount().unwrap_or_default() showed Fee: 0 SATS whenever the fee was uncomputable.

The fix

Everything on the approval screen now derives from the PSBT.

  • PSBTDetails carries outputs: Vec<PSBTOutput> (destination + amount), fee: Option<u64>, and the network. display_fields() produces the exact rows the UI draws, so the component and the tests cannot drift apart.
  • content.ts no longer reads #request-type or #channel-amount. findPSBT returns {psbt} and nothing else, and OperationRequestData lost both fields — the page has no channel onto the approval screen.
  • The parse-failure path fails closed. ApprovalScreen::for_psbt returns Refuse { reason } for an undecodable PSBT; ApprovePSBT renders the reason with only a Go back button, and ApprovePastedPSBT keeps Sign disabled. The screen decision lives in signer so it is unit-testable at the layer where the original defect lived.
  • Uncomputable fees and overflowing totals render as unknown, never 0.

Two things worth flagging beyond the original finding, both found while reviewing this change:

  • The fee is computed locally with checked arithmetic instead of bdk::psbt::PsbtUtils::fee_amount, which is not panic-free: it does .expect("input amount must be greater than output amount") and indexes non_witness_utxo.output[vout] unchecked, both driven entirely by attacker-supplied PSBT fields. In wasm a panic traps mid-render, which would have taken out the refusal screen this PR adds. Reproduced both cases; they are covered by t7.
  • The network is displayed. Scripts carry no network, and SettingsStorage::get_network() falls back to Network::Bitcoin when the setting is unset, so a regtest PSBT would render bc1q… addresses matching nothing NodeGuard shows. Stating the network makes the mismatch visible instead of silently training users to click through it. The underlying mainnet default is a separate issue and is untouched here.

Verification

signer/tests/ngc01_blind_confirmation.rs — 7 tests, run with cargo test --test ngc01_blind_confirmation -- --nocapture:

t1 every destination, amount, total, fee and network reaches the user
t2 a substituted recipient now changes what the user reads
t3 an undecodable PSBT refuses signing instead of rendering blank
t4 a missing witness_utxo shows unknown, not 0
t5 every output of a multi-output PSBT is rendered; Total is their sum
t6 a non-standard script shows the script, never a blank destination
t7 crafted amounts render unknown instead of panicking the parse

extension/tests/ngc01_content_script_poc.mjs compiles and executes the real content.ts against a hostile DOM and asserts findPSBT returns the PSBT and nothing else. Run with node extension/tests/ngc01_content_script_poc.mjs.

Also passing: full cargo test in signer (36 tests), cargo clippy --target wasm32-unknown-unknown clean for extension, tsc --noEmit clean.

Notes for review

  • t2's output shows Total and Fee are identical between the honest and substituted PSBT — that is expected and is the point: only the destination distinguishes them, which is precisely what used to be invisible.
  • extension/dist/ and extension/pkg/ are stale build outputs still containing the pre-fix content script. Run just build-extension before just sign-extension.
  • This addresses the display half. The signature's SIGHASH scope and the auto-submit behaviour in pastePSBT are separate concerns and are unchanged.

🤖 Generated with Claude Code

The approval screen showed "Operation Type" and "Amount" scraped by the
content script from the DOM of the page asking for the signature, while
the PSBT's own destinations and amounts were never displayed. Two
transactions paying different recipients rendered identically, so the
screen offered no verification value.

- PSBTDetails now carries every output (destination + amount) and the
  network, and both approval screens render only those PSBT-derived
  rows via PSBTDetails::display_fields.
- content.ts no longer reads #request-type or #channel-amount; the page
  can supply nothing but the PSBT itself.
- An undecodable PSBT yields ApprovalScreen::Refuse and no Sign button,
  replacing unwrap_or_default() which rendered a blank screen while
  leaving Sign enabled and signing the raw string anyway.
- An uncomputable fee and an overflowing total render as "unknown"
  rather than 0.
- The fee is computed locally with checked arithmetic instead of bdk's
  PsbtUtils::fee_amount, which panics on crafted PSBTs (outputs above
  inputs, out-of-range non_witness_utxo vout); in wasm that panic traps
  mid-render and would bypass the refusal screen.
- The network the addresses were rendered for is stated on screen, since
  scripts carry no network and the setting defaults to mainnet.

Regression tests cover recipient substitution, multi-output PSBTs,
non-standard scripts, unknown fees, the crafted-amount panic cases, and
that the content script hands over the PSBT and nothing else.

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

@RodriFS RodriFS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants