Validate sender input UTXO data - #1836
Conversation
Coverage Report for CI Build 33793175073Coverage increased (+0.07%) to 86.706%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
caarloshenriq
left a comment
caarloshenriq
left a comment
There was a problem hiding this comment.
Concept ACK d576e05
The security logic is sound. The outpoint pre-check correctly uses a single advancing iterator with any() to enforce that all original outpoints appear as an ordered subsequence in the proposal, catching modified outpoints early instead of misclassifying them as receiver inputs. The UTXO validation with is_none_or correctly accepts stripped fields (per BIP 78) while rejecting modified or injected data. The fix to the existing receiver-input tests (operating on input[1] instead of mangling input[0]) is a genuine improvement: those tests now exercise the actual receiver input.
Two commit hygiene points:
This commit does three things: outpoint pre-check, UTXO data validation, and existing test fixes. Per project conventions each logical unit should be its own commit. At minimum the test fixes should be separate, since they're independently correct and useful regardless of the new validation.
The commit message has no body. For a security-relevant change to PSBT validation, it would be worth explaining the threat model (receiver modifying UTXO data to invalidate signatures or cause the sender to sign a different transaction). The context in #678 covers this well and the body could reference it.
These tests mangled the first input's outpoint so that the sender's own input would be classified as receiver-contributed, then asserted the receiver checks against it. The proposal already carries a real receiver input at index 1, so the setup was both indirect and never exercised the input it claimed to cover. Point the assertions at input 1 and drop the outpoint mangling. The checks under test are unchanged; only the input they run against is.
d576e05 to
bd2186a
Compare
Thanks — split into three commits: the test fixes first (they pass on master's validation logic unchanged, so they stand alone as you said), then the outpoint pre-check, then the UTXO data validation with its error variant. |
caarloshenriq
left a comment
caarloshenriq
left a comment
There was a problem hiding this comment.
tACK bd2186a
All sender-input validation tests (test_sender_input_outpoint_changed, test_sender_input_witness_utxo_changed, test_sender_input_non_witness_utxo_changed) and the corrected receiver-input tests pass.
xstoicunicornx
left a comment
xstoicunicornx
left a comment
There was a problem hiding this comment.
utACK bd2186a.
PR addresses some deficiencies in accurate error reporting when the sender is validating the payjoin proposal. None of these changes are absolutely necessary but do improve the clarity of our tests and error reporting.
Provided some small feedback.
Input classification treats any proposed input whose outpoint does not match an original one as receiver-contributed. A receiver that alters an original outpoint therefore escapes the sender-side checks entirely: the input is validated as though the receiver had contributed it, and the sender only learns something is wrong later, from a finalization error that points at the wrong thing. Check up front that every original outpoint is still present in the proposal, in its original relative order, before classifying any input. is_ordered_subsequence enforces that with a single advancing iterator, so dropped and reordered inputs are both rejected as MissingOrShuffledInputs. Keeping it a free function lets the ordering rule be tested directly, without a proposal vector carrying several sender inputs. See payjoin#678.
bd2186a to
6c62fb2
Compare
updated bd2186a → 6c62fb2: extracted is_ordered_subsequence with a unit test, split the UTXO check into two ensures, and added a test for unchanged/stripped data. Both folded into their existing commits, so the three-commit structure is unchanged. Re-ACK when you have a moment. |
| InternalProposalError::SenderTxinNonAllSighashType, | ||
| )?; | ||
| } | ||
| // BIP 78 lets the receiver strip these fields, but never alter them. |
There was a problem hiding this comment.
BIP 78 doesn't currently say anything about these fields, we are enforcing this arbitrarily. Which I think is fine, but thinking about it more would like others to weigh in too.
There was a problem hiding this comment.
Agreed, this is policy, not spec, and the comment now says so. It also doesn't close a hole: restore_original_utxos overwrites these fields two lines after the check, so the sender signs its own values either way. The gain is a named error instead of a silent overwrite. Happy to drop this commit if the preference is status quo.
There was a problem hiding this comment.
Hmm... yes could we please drop this commit for now? Feel free to open as an issue to follow up on this question of whether we should enforce this policy even if not in the specification. Careful, I think some updates for the Reject proposals missing original sender inputs commit got entangled in this commit.
The sender restores UTXO information onto its own inputs after the receiver returns a proposal, because many wallets need it to sign. Restoration overwrote whatever came back unconditionally, so a receiver could return altered witness_utxo or non_witness_utxo data and have it silently replaced rather than reported. The sender lost the one signal that the receiver had tampered with its inputs. Compare the returned UTXO data against the original PSBT before restoring it, and reject any mismatch as SenderTxinUtxoInfoChanged. Absent fields are still accepted, tolerating receivers that strip data they do not need, and the two field checks stay separate so each reads on its own. BIP 78 says nothing about altering these fields: its reference implementation overwrites them without reading them, and its proposal vectors return them unchanged. Rejecting a mismatch is a policy choice, not a spec requirement. See payjoin#678.
6c62fb2 to
6db2b1d
Compare
4 participants
Closes #678.
What
Validate that every original sender outpoint remains present and ordered before classifying additional proposal inputs as receiver-owned. For matched sender inputs, reject returned
witness_utxoornon_witness_utxodata when it differs from the original PSBT while continuing to accept fields stripped according to BIP 78.Add a dedicated proposal error for modified sender UTXO information and regression tests for altered outpoints, witness UTXOs, and non-witness UTXOs. Existing receiver-input tests now exercise the proposal's actual receiver input.
Why
Previously, an altered sender outpoint was misclassified as a receiver input and could produce a misleading finalization error. Modified sender UTXO metadata was silently overwritten during restoration instead of reporting receiver misbehavior.
Tests
cargo test -p payjoin test_sender_input_ --all-featurescargo test -p payjoin test_receiver_input_ --all-featurescargo test -p payjoin --all-featurescargo fmt --all -- --checkcargo clippy -p payjoin --all-targets --all-features -- -D warningscodespellDisclosure: co-authored by Codex