From 8c4cd2bd895fe7467307867fefc3cd45a685367c Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 13 Aug 2018 14:59:31 -0700 Subject: Fix PSBT deserialization of 0-input transactions 0-input transactions can be ambiguously deserialized as being witness transactions. Since the unsigned transaction is never serialized as a witness transaction as it has no witnesses, we should always deserialize it as a non-witness transaction and set the serialization flags as such. Also always serialize the unsigned transaction as a non-witness transaction. GitHub-Pull: #13960 Rebased-From: 43811e6 --- src/script/sign.h | 7 +++++-- src/streams.h | 1 + 2 files changed, 6 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/script/sign.h b/src/script/sign.h index 24cddda51b..86188cbb38 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -547,7 +547,8 @@ struct PartiallySignedTransaction SerializeToVector(s, PSBT_GLOBAL_UNSIGNED_TX); // Write serialized tx to a stream - SerializeToVector(s, *tx); + OverrideStream os(&s, s.GetType(), s.GetVersion() | SERIALIZE_TRANSACTION_NO_WITNESS); + SerializeToVector(os, *tx); // Write the unknown things for (auto& entry : unknown) { @@ -601,7 +602,9 @@ struct PartiallySignedTransaction throw std::ios_base::failure("Global unsigned tx key is more than one byte type"); } CMutableTransaction mtx; - UnserializeFromVector(s, mtx); + // Set the stream to serialize with non-witness since this should always be non-witness + OverrideStream os(&s, s.GetType(), s.GetVersion() | SERIALIZE_TRANSACTION_NO_WITNESS); + UnserializeFromVector(os, mtx); tx = std::move(mtx); // Make sure that all scriptSigs and scriptWitnesses are empty for (const CTxIn& txin : tx->vin) { diff --git a/src/streams.h b/src/streams.h index 2dcca6646d..096ebfc9c2 100644 --- a/src/streams.h +++ b/src/streams.h @@ -61,6 +61,7 @@ public: int GetVersion() const { return nVersion; } int GetType() const { return nType; } + size_t size() const { return stream->size(); } }; template -- cgit v1.2.3 From 517010e30e4ac52868fcfd537b644515f3081f88 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 13 Aug 2018 15:00:06 -0700 Subject: Serialize non-witness utxo as a non-witness tx but always deserialize as witness Strip out the witnesses when serializing the non-witness utxo. However witness serializations are allowed, so make sure we always deserialize as witness. GitHub-Pull: #13960 Rebased-From: bd19cc7 --- src/script/sign.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/script/sign.h b/src/script/sign.h index 86188cbb38..80fda617e9 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -223,7 +223,8 @@ struct PSBTInput // If there is a non-witness utxo, then don't add the witness one. if (non_witness_utxo) { SerializeToVector(s, PSBT_IN_NON_WITNESS_UTXO); - SerializeToVector(s, non_witness_utxo); + OverrideStream os(&s, s.GetType(), s.GetVersion() | SERIALIZE_TRANSACTION_NO_WITNESS); + SerializeToVector(os, non_witness_utxo); } else if (!witness_utxo.IsNull()) { SerializeToVector(s, PSBT_IN_WITNESS_UTXO); SerializeToVector(s, witness_utxo); @@ -297,13 +298,17 @@ struct PSBTInput // Do stuff based on type switch(type) { case PSBT_IN_NON_WITNESS_UTXO: + { if (non_witness_utxo) { throw std::ios_base::failure("Duplicate Key, input non-witness utxo already provided"); } else if (key.size() != 1) { throw std::ios_base::failure("Non-witness utxo key is more than one byte type"); } - UnserializeFromVector(s, non_witness_utxo); + // Set the stream to unserialize with witness since this is always a valid network transaction + OverrideStream os(&s, s.GetType(), s.GetVersion() & ~SERIALIZE_TRANSACTION_NO_WITNESS); + UnserializeFromVector(os, non_witness_utxo); break; + } case PSBT_IN_WITNESS_UTXO: if (!witness_utxo.IsNull()) { throw std::ios_base::failure("Duplicate Key, input witness utxo already provided"); -- cgit v1.2.3 From ad6d845ac9d43aac663fa46912b49aaf837a1a9e Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 8 Aug 2018 14:18:55 -0700 Subject: Additional sanity checks in SignPSBTInput GitHub-Pull: #13917 Rebased-From: 8254e99 --- src/script/sign.cpp | 16 ++++++++++++++++ src/script/sign.h | 2 +- 2 files changed, 17 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/script/sign.cpp b/src/script/sign.cpp index 1e4102c20f..1ab5051ff7 100644 --- a/src/script/sign.cpp +++ b/src/script/sign.cpp @@ -244,17 +244,33 @@ bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& t input.FillSignatureData(sigdata); // Get UTXO + bool require_witness_sig = false; CTxOut utxo; if (input.non_witness_utxo) { + // If we're taking our information from a non-witness UTXO, verify that it matches the prevout. + 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). + // Still, check in order to not rely on callers to enforce this. + if (!input.witness_utxo.IsNull() && input.non_witness_utxo->vout[tx.vin[index].prevout.n] != input.witness_utxo) return false; utxo = input.non_witness_utxo->vout[tx.vin[index].prevout.n]; } else if (!input.witness_utxo.IsNull()) { utxo = input.witness_utxo; + // When we're taking our information from a witness UTXO, we can't verify it is actually data from + // the output being spent. This is safe in case a witness signature is produced (which includes this + // information directly in the hash), but not for non-witness signatures. Remember that we require + // a witness signature in this situation. + require_witness_sig = true; } else { return false; } MutableTransactionSignatureCreator creator(&tx, index, utxo.nValue, sighash); + sigdata.witness = false; bool sig_complete = ProduceSignature(provider, creator, utxo.scriptPubKey, sigdata); + // Verify that a witness signature was produced in case one was required. + if (require_witness_sig && !sigdata.witness) return false; input.FromSignatureData(sigdata); return sig_complete; } diff --git a/src/script/sign.h b/src/script/sign.h index 80fda617e9..7ade715ee2 100644 --- a/src/script/sign.h +++ b/src/script/sign.h @@ -686,7 +686,7 @@ bool ProduceSignature(const SigningProvider& provider, const BaseSignatureCreato bool SignSignature(const SigningProvider &provider, const CScript& fromPubKey, CMutableTransaction& txTo, unsigned int nIn, const CAmount& amount, int nHashType); bool SignSignature(const SigningProvider &provider, const CTransaction& txFrom, CMutableTransaction& txTo, unsigned int nIn, int nHashType); -/** Signs a PSBTInput */ +/** Signs a PSBTInput, verifying that all provided data matches what is being signed. */ bool SignPSBTInput(const SigningProvider& provider, const CMutableTransaction& tx, PSBTInput& input, SignatureData& sigdata, int index, int sighash = 1); /** Extract signature data from a transaction input, and insert it. */ -- cgit v1.2.3 From dbaadc9ea92fd0e221c60584a5d1ca871fdd9c66 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 8 Aug 2018 14:27:20 -0700 Subject: Only wipe wrong UTXO type data if overwritten by wallet GitHub-Pull: #13917 Rebased-From: c05712c --- src/wallet/rpcwallet.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 4e539a85de..92e6931645 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4504,10 +4504,11 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const C // If we don't know about this input, skip it and let someone else deal with it const uint256& txhash = txin.prevout.hash; - const auto& it = pwallet->mapWallet.find(txhash); + const auto it = pwallet->mapWallet.find(txhash); if (it != pwallet->mapWallet.end()) { const CWalletTx& wtx = it->second; CTxOut utxo = wtx.tx->vout[txin.prevout.n]; + // Update both UTXOs from the wallet. input.non_witness_utxo = wtx.tx; input.witness_utxo = utxo; } @@ -4524,11 +4525,13 @@ bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const C complete &= SignPSBTInput(PublicOnlySigningProvider(pwallet), *psbtx.tx, input, sigdata, i, sighash_type); } - // Drop the unnecessary UTXO - if (sigdata.witness) { - input.non_witness_utxo = nullptr; - } else { - input.witness_utxo.SetNull(); + if (it != pwallet->mapWallet.end()) { + // Drop the unnecessary UTXO if we added both from the wallet. + if (sigdata.witness) { + input.non_witness_utxo = nullptr; + } else { + input.witness_utxo.SetNull(); + } } // Get public key paths -- cgit v1.2.3