The current PSBT signing code can end up producing a non-segwit signature, while only the UTXO being spent is provided in the PSBT (as opposed to the entire transaction being spent). This may be used to trick a user to incorrectly decide a transaction has the semantics he intends to sign.
Fix this by refusing to sign if there is any mismatch between the provided data and what is being signed.
Excellent! Thanks for tightening PSBT!
Concept ACK
4526@@ -4525,10 +4527,12 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const C
4527 }
4528
4529 // Drop the unnecessary UTXO
4530- if (sigdata.witness) {
4531- input.non_witness_utxo = nullptr;
4532- } else {
4533- input.witness_utxo.SetNull();
4534+ if (from_wallet) {
Commit “Only wipe wrong UTXO type data if overwritten by wallet”
Suggestion, either reuse above condition if (it != pwallet->mapWallet.end())or above do:
0const bool from_wallet = it != pwallet->mapWallet.end();
1if (from_wallet) {
2 ...
3}
243@@ -244,17 +244,33 @@ bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& t
244 input.FillSignatureData(sigdata);
245
246 // Get UTXO
247+ bool require_witness_sig = false;
248 CTxOut utxo;
249 if (input.non_witness_utxo) {
250+ // If we're taking our information from a non-witness UTXO, verify that it matches the prevout.
Commit “Additional sanity checks in SignPSBTInput”
nit, could reflow comments.
utACK
Here is a commit with some more test cases: https://github.com/achow101/bitcoin/commit/9f110e1acbb1dc882ea871043fd03cf9064a1063. It tests for non-matching redeem scripts with both witness and non-witness utxos and non-matching witness scripts (for witness utxo only)
249 if (input.non_witness_utxo) {
250+ // If we're taking our information from a non-witness UTXO, verify that it matches the prevout.
251+ if (input.non_witness_utxo->GetHash() != tx.vin[index].prevout.hash) return false;
252+ // If both witness and non-witness UTXO are provided, verify that they match. This check shouldn't
253+ // matter, as the PSBT deserializer enforces only one of both is provided, and the only way both
254+ // can be present is when they're added simultaneously by FillPSBT (in which case they always match).
FillPSBT does in Bitcoin Core? I understand that the check is done anyway, but the comment sounds like it could be skipped due to an implementation detail, which sounds error-prone.
Perhaps unrelated, but either createpsbt is too lenient, or decodepsbt is broken.
0$ ./bitcoin-cli createpsbt "[]" "[{\"$(./bitcoin-cli getnewaddress)\":0.01}]"
1cHNidP8BACoCAAAAAAFAQg8AAAAAABepFG6Rty1Vk+fUOR4v9E6R6YXDFkHwhwAAAAAAAA==
2$ ./bitcoin-cli decodepsbt cHNidP8BACoCAAAAAAFAQg8AAAAAABepFG6Rty1Vk+fUOR4v9E6R6YXDFkHwhwAAAAAAAA==
3error code: -22
4error message:
5TX decode failed CDataStream::read(): end of data: unspecified iostream_category error
