Additional safety checks in PSBT signer #13917

sipa · 2018-08-08T21:33:09Z

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.

practicalswift · 2018-08-08T22:32:38Z

Excellent! Thanks for tightening PSBT!

Concept ACK

DrahtBot · 2018-08-08T23:06:57Z

Note to reviewers: This pull request conflicts with the following ones:

Additional utility RPCs for PSBT #13932 (Additional utility RPCs for PSBT by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

promag · 2018-08-08T22:46:09Z

src/wallet/rpcwallet.cpp

-            input.non_witness_utxo = nullptr;
-        } else {
-            input.witness_utxo.SetNull();
+        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:

const bool from_wallet = it != pwallet->mapWallet.end(); if (from_wallet) { ... }

promag · 2018-08-08T22:48:30Z

src/script/sign.cpp

    CTxOut utxo;
    if (input.non_witness_utxo) {
+        // 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.

I think they're fine.

achow101 · 2018-08-08T23:18:18Z

utACK

Here is a commit with some more test cases: achow101@9f110e1. It tests for non-matching redeem scripts with both witness and non-witness utxos and non-matching witness scripts (for witness utxo only)

sipa · 2018-08-08T23:41:50Z

Addressed @promag's nit, and included more tests by @achow101.

kallewoof

utACK 2bc1296db7ba4e99f52a4002e67b11c0351819d6

kallewoof · 2018-08-09T06:37:05Z

src/script/sign.cpp

+        if (input.non_witness_utxo->GetHash() != tx.vin[index].prevout.hash) return false;
+        // If both witness and non-witness UTXO are provided, verify that they match. This check shouldn't
+        // matter, as the PSBT deserializer enforces only one of both is provided, and the only way both
+        // can be present is when they're added simultaneously by FillPSBT (in which case they always match).


Is this comment based on the BIP/spec or implementation? What about other implementations that may not do what 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.

@achow101 just opened a PR that adds the actual requirements to test for to BIP 174, including a simple implementation in pseudocode that implements it.

achow101 · 2018-08-09T18:31:45Z

utACK 2bc1296db7ba4e99f52a4002e67b11c0351819d6

kallewoof · 2018-08-13T09:24:41Z

Perhaps unrelated, but either createpsbt is too lenient, or decodepsbt is broken.

$ ./bitcoin-cli createpsbt "[]" "[{\"$(./bitcoin-cli getnewaddress)\":0.01}]"
cHNidP8BACoCAAAAAAFAQg8AAAAAABepFG6Rty1Vk+fUOR4v9E6R6YXDFkHwhwAAAAAAAA==
$ ./bitcoin-cli decodepsbt cHNidP8BACoCAAAAAAFAQg8AAAAAABepFG6Rty1Vk+fUOR4v9E6R6YXDFkHwhwAAAAAAAA==
error code: -22
error message:
TX decode failed CDataStream::read(): end of data: unspecified iostream_category error

fanquake · 2018-08-13T10:33:56Z

@sipa Can you rebase this now that #13666 is merged.

sipa · 2018-08-13T15:42:04Z

Rebased after #13666.

achow101 · 2018-08-13T18:37:10Z

re-utACK

kallewoof · 2018-08-14T05:24:54Z

re-utACK 5df6f08

laanwj · 2018-08-14T16:01:16Z

utACK 5df6f08

GitHub-Pull: bitcoin#13917 Rebased-From: 8254e99

GitHub-Pull: bitcoin#13917 Rebased-From: c05712c

GitHub-Pull: bitcoin#13917 Rebased-From: 7c8bffd

GitHub-Pull: bitcoin#13917 Rebased-From: 5df6f08

fanquake · 2018-08-15T02:04:43Z

Will be backported in #13976

0333914 More tests of signer checks (Andrew Chow) 8935869 Test that a non-witness script as witness utxo is not signed (Andrew Chow) dbaadc9 Only wipe wrong UTXO type data if overwritten by wallet (Pieter Wuille) ad6d845 Additional sanity checks in SignPSBTInput (Pieter Wuille) 517010e Serialize non-witness utxo as a non-witness tx but always deserialize as witness (Andrew Chow) 8c4cd2b Fix PSBT deserialization of 0-input transactions (Andrew Chow) Pull request description: Backports #13917 and #13960 to the 0.17 branch. Tree-SHA512: b3853aff2a13a53aa0a390b6b4b0c539f0ef0d42f2c517e956efd0b135c74c4ddce6a1d00700849a58c696824fa95951d8cac6ca58b426e8dfcb8bb62f680b7c

GitHub-Pull: bitcoin#13917 Rebased-From: 8254e99

GitHub-Pull: bitcoin#13917 Rebased-From: c05712c

GitHub-Pull: bitcoin#13917 Rebased-From: 7c8bffd

GitHub-Pull: bitcoin#13917 Rebased-From: 5df6f08

maflcko added Tests and removed Tests labels Aug 8, 2018

maflcko added this to the 0.17.0 milestone Aug 8, 2018

maflcko added the Wallet label Aug 8, 2018

sipa added the Bug label Aug 8, 2018

promag reviewed Aug 8, 2018

View reviewed changes

sipa force-pushed the 201808_psbtsafesign branch from 1b44970 to 2bc1296 Compare August 8, 2018 23:40

This was referenced Aug 9, 2018

PSBT key path cleanups #13723

Merged

Always create signatures with Low R values #13666

Merged

kallewoof reviewed Aug 9, 2018

View reviewed changes

DrahtBot mentioned this pull request Aug 9, 2018

Additional utility RPCs for PSBT #13932

Merged

DrahtBot added the Needs rebase label Aug 13, 2018

sipa and others added 4 commits August 13, 2018 08:21

Only wipe wrong UTXO type data if overwritten by wallet

c05712c

Additional sanity checks in SignPSBTInput

8254e99

Test that a non-witness script as witness utxo is not signed

7c8bffd

More tests of signer checks

5df6f08

sipa force-pushed the 201808_psbtsafesign branch from 2bc1296 to 5df6f08 Compare August 13, 2018 15:41

sipa mentioned this pull request Aug 13, 2018

decodepsbt fails for PSBT with no inputs #13958

Closed

DrahtBot removed the Needs rebase label Aug 13, 2018

laanwj added the Needs backport label Aug 14, 2018

laanwj merged commit 5df6f08 into bitcoin:master Aug 14, 2018

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Aug 15, 2018

Additional sanity checks in SignPSBTInput

ad6d845

GitHub-Pull: bitcoin#13917 Rebased-From: 8254e99

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Aug 15, 2018

Only wipe wrong UTXO type data if overwritten by wallet

dbaadc9

GitHub-Pull: bitcoin#13917 Rebased-From: c05712c

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Aug 15, 2018

Test that a non-witness script as witness utxo is not signed

8935869

GitHub-Pull: bitcoin#13917 Rebased-From: 7c8bffd

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Aug 15, 2018

More tests of signer checks

0333914

GitHub-Pull: bitcoin#13917 Rebased-From: 5df6f08

fanquake mentioned this pull request Aug 15, 2018

[0.17] Backport #13960 & #13917 #13976

Merged

fanquake removed the Needs backport label Aug 15, 2018

Bushstar mentioned this pull request Sep 4, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#359

Merged

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Sep 14, 2018

Additional sanity checks in SignPSBTInput

f108a9f

GitHub-Pull: bitcoin#13917 Rebased-From: 8254e99

HashUnlimited mentioned this pull request Sep 14, 2018

Additional sanity checks in SignPSBTInput chaincoin/chaincoin#656

Merged

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Sep 14, 2018

Only wipe wrong UTXO type data if overwritten by wallet

34bf962

GitHub-Pull: bitcoin#13917 Rebased-From: c05712c

HashUnlimited mentioned this pull request Sep 14, 2018

Only wipe wrong UTXO type data if overwritten by wallet chaincoin/chaincoin#657

Merged

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Sep 14, 2018

Test that a non-witness script as witness utxo is not signed

b12061c

GitHub-Pull: bitcoin#13917 Rebased-From: 7c8bffd

HashUnlimited mentioned this pull request Sep 14, 2018

Test that a non-witness script as witness utxo is not signed chaincoin/chaincoin#658

Merged

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Sep 14, 2018

More tests of signer checks

8802e10

GitHub-Pull: bitcoin#13917 Rebased-From: 5df6f08

HashUnlimited mentioned this pull request Sep 14, 2018

More tests of signer checks chaincoin/chaincoin#659

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional safety checks in PSBT signer #13917

Additional safety checks in PSBT signer #13917

sipa commented Aug 8, 2018

practicalswift commented Aug 8, 2018

DrahtBot commented Aug 8, 2018 •

edited

promag Aug 8, 2018

sipa Aug 8, 2018

promag Aug 8, 2018

sipa Aug 8, 2018

achow101 commented Aug 8, 2018 •

edited

sipa commented Aug 8, 2018

kallewoof left a comment

kallewoof Aug 9, 2018

sipa Aug 9, 2018

achow101 commented Aug 9, 2018

kallewoof commented Aug 13, 2018

fanquake commented Aug 13, 2018

sipa commented Aug 13, 2018

achow101 commented Aug 13, 2018

kallewoof commented Aug 14, 2018

laanwj commented Aug 14, 2018

fanquake commented Aug 15, 2018

Additional safety checks in PSBT signer #13917

Additional safety checks in PSBT signer #13917

Conversation

sipa commented Aug 8, 2018

practicalswift commented Aug 8, 2018

DrahtBot commented Aug 8, 2018 • edited

promag Aug 8, 2018

Choose a reason for hiding this comment

sipa Aug 8, 2018

Choose a reason for hiding this comment

promag Aug 8, 2018

Choose a reason for hiding this comment

sipa Aug 8, 2018

Choose a reason for hiding this comment

achow101 commented Aug 8, 2018 • edited

sipa commented Aug 8, 2018

kallewoof left a comment

Choose a reason for hiding this comment

kallewoof Aug 9, 2018

Choose a reason for hiding this comment

sipa Aug 9, 2018

Choose a reason for hiding this comment

achow101 commented Aug 9, 2018

kallewoof commented Aug 13, 2018

fanquake commented Aug 13, 2018

sipa commented Aug 13, 2018

achow101 commented Aug 13, 2018

kallewoof commented Aug 14, 2018

laanwj commented Aug 14, 2018

fanquake commented Aug 15, 2018

DrahtBot commented Aug 8, 2018 •

edited

achow101 commented Aug 8, 2018 •

edited