fix(psbt): sanity check psbt before signing

[sdmg15]

Description

This PR attempts to sanity check the PSBT before signing. It closes audit issue #52

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran just p before pushing

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.39%. Comparing base (4b612f5) to head (f0aca23).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #486      +/-   ##
==========================================
+ Coverage   80.30%   80.39%   +0.09%     
==========================================
  Files          24       24              
  Lines        5417     5432      +15     
  Branches      245      246       +1     
==========================================
+ Hits         4350     4367      +17     
+ Misses        989      988       -1     
+ Partials       78       77       -1     

Flag	Coverage Δ
rust	80.39% <100.00%> (+0.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[110CodingP]

Thanks for working on this!
Do you also plan to work on this part of #52 ?
"In the signer module, the previous transaction contained in a PSBT input is not validated against the outpoint for legacy and segwit v0 transactions. This is checked when creating a transaction, but this module may be used to sign a PSBT as an external participant."

[110CodingP]

Plus I am sorry I wasn't able to understand what this test is trying to check. Could you please add some more comments here?

[sdmg15]

This test effectively calls the wallet::sign with a malformed Psbt and expects the wallet to return an error instead of panicking.

So what I did here is crafting a raw transaction that initially just has 1 input then transformed it into a Psbt type to which I added more inputs (3 vs 1 in original tx).

[110CodingP]

I see but how does the input count relate to the error raised?

[sdmg15]

I'm not sure I get your question but that's the essence of the audit report.
When Wallet::sign will be called with the current setup (3 inputs in PSBT but 1 in raw tx), the get_utxo_for will return None and then the SignerError::MissingNonWitnessUtxo will be returned.
Not sure if it answers the question.

[110CodingP]

We get the error even without the other 2 inputs because the first input itself does not contain a non_witness_utxo!

[sdmg15]

You're right about the test, it doesn't check anything unless we modify the function update_psbt_with_descriptor or get_utxo_for to actually return an error type. Maybe the test should just be discarded.

[Musab1258]

Thanks @sdmg15 for working on this!

I hope you are aware that PR #235 by @oleonardolima, when completed and merged, will remove the signers module altogether from bdk_wallet to rely on rust-bitcoin's native Psbt::sign, which will eventually make the changes here obsolete. However, since that PR is still a draft and might take time, this PR can still act as a stopgap fix if it is merged on time.

I reviewed the code changes, and the logic you added to the get_utxo_for should safely prevent the crash. However, I think you should address these two things in this PR.

1. Fixing the Test Setup

Currently, test_wallet_sign_rejects_malformed_psbt_input_count_mismatch does not test the out-of-bounds logic because the first input is missing a non_witness_utxo. Wallet::sign catches that very first input during its trust_witness_utxo validation block and returns the error before it even attempts to sign the out-of-bounds inputs.

To fix this and allow the test to actually reach the signing phase for the malformed inputs, you just need to inject the non_witness_utxo into the first generated input. You can add this right after initializing the PSBT:

let mut psbt = Psbt::from_unsigned_tx(unsigned_tx).unwrap();
psbt.inputs[0].non_witness_utxo = Some(prev_tx.clone()); 

Once you add this, run the test again to see what error the signer actually throws when it hits the out-of-bounds inputs, and update your assert! statement accordingly.

2. Completing the Scope of Issue Sanity check PSBT before signing #52 (Outpoint Validation)

As @110CodingP mentioned earlier, Issue #52 also points out another vulnerability, which is that the signer module currently doesn't validate if the previous transaction contained in the PSBT input actually matches the outpoint for legacy and segwit v0 transactions.

To fix this, you can add a quick validation loop at the beginning of Wallet::sign. It would just need to verify that psbt.inputs[i].non_witness_utxo.txid() == psbt.unsigned_tx.input[i].previous_output.txid.