GlcoinCore/elements
1
0
Fork 0
mirror of https://github.com/ElementsProject/elements.git synced 2026-08-17 13:07:54 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions

Merge 1326092e6c into merged_master (Bitcoin PR #17156)

Browse source 
This fixes a segfault I introduced in 981df590811 (#17371) a couple hundred PRs
ago, where we might out-of-bounds access the vout arrray of a non_witness_utxo
transaction in a PSBT.

This logic should probably be refactored after the rebase -- we sift through
the non_witness_utxos to find a scriptpubkey, which we use to get a wallet
provider, so we can pass this to SignPSBTInput, which then repeats the same
logic (for its own reasons, and without the segfault bug)....and we aren't
even calling SignPSBTInput for signing reasons! It has something to do with
blinding.
This commit is contained in:
Andrew Poelstra 2020-11-14 16:52:12 +00:00
commit 68d0a25901
6 changed files with 65 additions and 5 deletions
Download patch file Download diff file Expand all files Collapse all files

27
src/node/psbt.cpp
View file

@ -2,6 +2,7 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <amount.h>
#include <coins.h>
#include <consensus/tx_verify.h>
#include <node/psbt.h>
@ -32,9 +33,17 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
CTxOut utxo;
if (psbtx.GetInputUTXO(utxo, i)) {
//TODO(gwillen) do PSBT inputs always have explicit assets & amounts?
if (!MoneyRange(utxo.nValue.GetAmount()) || !MoneyRange(in_amts[utxo.nAsset.GetAsset()] + utxo.nValue.GetAmount())) {
result.SetInvalid(strprintf("PSBT is not valid. Input %u has invalid value", i));
return result;
}
in_amts[utxo.nAsset.GetAsset()] += utxo.nValue.GetAmount();
input_analysis.has_utxo = true;
} else {
if (input.non_witness_utxo && psbtx.tx->vin[i].prevout.n >= input.non_witness_utxo->vout.size()) {
result.SetInvalid(strprintf("PSBT is not valid. Input %u specifies invalid prevout", i));
return result;
}
input_analysis.has_utxo = false;
input_analysis.is_final = false;
input_analysis.next = PSBTRole::UPDATER;
@ -85,11 +94,23 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
if (calc_fee) {
// Get the output amount
CAmountMap out_amts = std::accumulate(psbtx.tx->vout.begin(), psbtx.tx->vout.end(), CAmountMap(),
[](CAmountMap map, const CTxOut& b) {
map[b.nAsset.GetAsset()] += b.nValue.GetAmount();
return map;
[](CAmountMap a, const CTxOut& b) {
CAmount acc = a[b.nAsset.GetAsset()];
CAmount add = b.nValue.GetAmount();
if (!MoneyRange(acc) || !MoneyRange(add) || !MoneyRange(acc + add)) {
CAmountMap invalid;
invalid[::policyAsset] = CAmount(-1);
return invalid;
}
a[b.nAsset.GetAsset()] += add;
return a;
}
);
if (!MoneyRange(out_amts)) {
result.SetInvalid(strprintf("PSBT is not valid. Output amount invalid"));
return result;
}
// Get the fee
CAmountMap fee = in_amts - out_amts;

8
src/psbt.cpp
View file

@ -68,8 +68,11 @@ bool PartiallySignedTransaction::AddOutput(const CTxOut& txout, const PSBTOutput
bool PartiallySignedTransaction::GetInputUTXO(CTxOut& utxo, int input_index) const
{
PSBTInput input = inputs[input_index];
int prevout_index = tx->vin[input_index].prevout.n;
uint32_t prevout_index = tx->vin[input_index].prevout.n;
if (input.non_witness_utxo) {
if (prevout_index >= input.non_witness_utxo->vout.size()) {
return false;
}
utxo = input.non_witness_utxo->vout[prevout_index];
} else if (!input.witness_utxo.IsNull()) {
utxo = input.witness_utxo;
@ -276,6 +279,9 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction&
if (input.non_witness_utxo) {
// If we're taking our information from a non-witness UTXO, verify that it matches the prevout.
COutPoint prevout = tx.vin[index].prevout;
if (prevout.n >= input.non_witness_utxo->vout.size()) {
return false;
}
if (input.non_witness_utxo->GetHash() != prevout.hash) {
return false;
}

3
src/rpc/rawtransaction.cpp
View file

@ -1269,7 +1269,8 @@ UniValue decodepsbt(const JSONRPCRequest& request)
UniValue out(UniValue::VOBJ);
if (txout.nValue.IsExplicit()) {
out.pushKV("amount", ValueFromAmount(txout.nValue.GetAmount()));
CAmount nValue = txout.nValue.GetAmount();
out.pushKV("amount", ValueFromAmount(nValue));
} else {
out.pushKV("amountcommitment", txout.nValue.GetHex());
}

6
src/wallet/psbtwallet.cpp
View file

@ -49,6 +49,9 @@ TransactionError FillPSBTInputsData(const CWallet* pwallet, PartiallySignedTrans
if (!input.witness_utxo.IsNull()) {
script = input.witness_utxo.scriptPubKey;
} else if (input.non_witness_utxo) {
if (txin.prevout.n >= input.non_witness_utxo->vout.size()) {
return TransactionError::MISSING_INPUTS;
}
script = input.non_witness_utxo->vout[txin.prevout.n].scriptPubKey;
} else {
// There's no UTXO so we can just skip this now
@ -177,6 +180,9 @@ TransactionError SignPSBT(const CWallet* pwallet, PartiallySignedTransaction& ps
if (!input.witness_utxo.IsNull()) {
script = input.witness_utxo.scriptPubKey;
} else if (input.non_witness_utxo) {
if (txin.prevout.n >= input.non_witness_utxo->vout.size()) {
return TransactionError::MISSING_INPUTS;
}
script = input.non_witness_utxo->vout[txin.prevout.n].scriptPubKey;
} else {
// There's no UTXO so we can just skip this now

9
src/wallet/test/psbt_wallet_tests.cpp
View file

@ -66,6 +66,15 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test)
ssTx << psbtx;
std::string final_hex = HexStr(ssTx.begin(), ssTx.end());
BOOST_CHECK_EQUAL(final_hex, "70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd750000000000ffffffff838d0427d0ec650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d0100000000ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb815e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df5546e8742d1d87008f00000000000100bb0200000001aad73931018bd25f84ae400b68848be09db706eac2ac18298babee71ab656f8b0000000048473044022058f6fc7c6a33e1b31548d481c826c015bd30135aad42cd67790dab66d2ad243b02204a1ced2604c6735b6393e5b41691dd78b00f0c5942fb9f751856faa938157dba01feffffff0280f0fa020000000017a9140fb9463421696b82c833af241c78c17ddbde493487d0f20a270100000017a91429ca74f8a08f81999428185c97b5d852e4063f6187650000000104475221029583bf39ae0a609747ad199addd634fa6108559d6c5cd39b4c2183f1ab96e07f2102dab61ff49a14db6a7d02b0cd1fbb78fc4b18312b5b4e54dae4dba2fbfef536d752ae2206029583bf39ae0a609747ad199addd634fa6108559d6c5cd39b4c2183f1ab96e07f10d90c6a4f000000800000008000000080220602dab61ff49a14db6a7d02b0cd1fbb78fc4b18312b5b4e54dae4dba2fbfef536d710d90c6a4f00000080000000800100008001090880f0fa02000000000001012000c2eb0b0000000017a914b7f5faf40e3d40a5a459b1db3535f2b72fa921e88701042200208c2353173743b595dfb4a07b72ba8e42e3797da74e87fe7d9d7497e3b2028903010547522103089dc10c7ac6db54f91329af617333db388cead0c231f723379d1b99030b02dc21023add904f3d6dcf59ddb906b0dee23529b7ffb9ed50e5e86151926860221f0e7352ae2206023add904f3d6dcf59ddb906b0dee23529b7ffb9ed50e5e86151926860221f0e7310d90c6a4f000000800000008003000080220603089dc10c7ac6db54f91329af617333db388cead0c231f723379d1b99030b02dc10d90c6a4f00000080000000800200008001090800c2eb0b0000000000220203a9a4c37f5996d3aa25dbac6b570af0650394492942460b354753ed9eeca5877110d90c6a4f000000800000008004000080010521010000000000000000000000000000000000000000000000000000000000000000002202027f6399757d2eff55a136ad02c684b1838b6556e5f1b6b34282a94b6b5005109610d90c6a4f00000080000000800500008001052101000000000000000000000000000000000000000000000000000000000000000000");
// Mutate the transaction so that one of the inputs is invalid
psbtx.tx->vin[0].prevout.n = 2;
// Try to sign the mutated input
SignatureData sigdata;
psbtx.inputs[0].FillSignatureData(sigdata);
const SigningProvider* provider = m_wallet.GetSigningProvider(ws1, sigdata);
BOOST_CHECK(!SignPSBTInput(*provider, psbtx, 0, SIGHASH_ALL));
}
BOOST_AUTO_TEST_CASE(parse_hd_keypath)

17
test/functional/rpc_psbt.py
View file

@ -549,6 +549,8 @@ class PSBTTest(BitcoinTestFramework):
self.nodes[0].generate(200)
self.sync_all()
assert_raises_rpc_error(-25, 'Missing inputs', self.nodes[0].walletprocesspsbt, 'cHNldP8BAOECAAAAAAK2GUhaoOw0nV0BJhZg0a8G7vb9PQvlTds8GA/N+l+peAMAAAAA/////7YZSFqg7DSdXQEmFmDRrwbu9v09C+VN2zwYD836X6l4AQAAAAD/////AgEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAAAlQK2wAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAAAlQL5AAAWABQo3DTHwdFy0CCa+h6+bi7VJs3tcgAAAAAAAQDhAgAAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP////9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/////wIBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAJUC+QAAFgAU/c11FQPC7OYlaspyx3uf70ntV8kBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAJUCzUAAFgAUZlkFHI0o4IUPP7h+AZ/85GpMC7QAAAAAAAEBQgEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAAAlQLNQAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAA==')
# Run all the pre-Elements, tests first with non-confidential addresses, then again with confidential addresses
self.run_basic_tests(False)
self.run_basic_tests(True)
@ -668,5 +670,20 @@ class PSBTTest(BitcoinTestFramework):
assert_equal(analysis['next'], 'creator')
assert_equal(analysis['error'], 'PSBT is not valid. Input 0 spends unspendable output')
self.log.info("PSBT with invalid values should have error message and Creator as next")
analysis = self.nodes[0].analyzepsbt('cHNldP8BALgCAAAAAAHwNNARYAJurafOkaMMB+gTCJkDS+c11HE0/e16Cxs9AQAAAAAA/////wIBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAJUC+QAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XIBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAJUC7fwAFgAU9yTiAXuIvg0vjC19EAqBBuCGJNQAAAAAAAEBQgEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAB9DjaoGAAAAWABSVA7cX9jx6OuNRxDgTgCLxTDU69gAAAA==')
assert_equal(analysis['next'], 'creator')
assert_equal(analysis['error'], 'PSBT is not valid. Input 0 has invalid value')
analysis = self.nodes[0].analyzepsbt('cHNldP8BALgCAAAAAAHwNNARYAJurafOkaMMB+gTCJkDS+c11HE0/e16Cxs9AQAAAAAA/////wIBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAfQ42qBgAAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XIBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAJUC7fwAFgAU9yTiAXuIvg0vjC19EAqBBuCGJNQAAAAAAAEBQgEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAABKgXyAAAWABSVA7cX9jx6OuNRxDgTgCLxTDU69gAAAA==')
assert_equal(analysis['next'], 'creator')
assert_equal(analysis['error'], 'PSBT is not valid. Output amount invalid')
analysis = self.nodes[0].analyzepsbt('cHNldP8BAOECAAAAAAK2GUhaoOw0nV0BJhZg0a8G7vb9PQvlTds8GA/N+l+peAMAAAAA/////7YZSFqg7DSdXQEmFmDRrwbu9v09C+VN2zwYD836X6l4AQAAAAD/////AgEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAAAlQK2wAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAAAlQL5AAAWABQo3DTHwdFy0CCa+h6+bi7VJs3tcgAAAAAAAQDhAgAAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP////9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/////wIBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAJUC+QAAFgAU/c11FQPC7OYlaspyx3uf70ntV8kBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAJUCzUAAFgAUZlkFHI0o4IUPP7h+AZ/85GpMC7QAAAAAAAEBQgEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAAAlQLNQAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAA==')
assert_equal(analysis['next'], 'creator')
assert_equal(analysis['error'], 'PSBT is not valid. Input 0 specifies invalid prevout')
assert_raises_rpc_error(-25, 'Missing inputs', self.nodes[0].walletprocesspsbt, 'cHNldP8BAOECAAAAAAK2GUhaoOw0nV0BJhZg0a8G7vb9PQvlTds8GA/N+l+peAMAAAAA/////7YZSFqg7DSdXQEmFmDRrwbu9v09C+VN2zwYD836X6l4AQAAAAD/////AgEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAAAlQK2wAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAAAlQL5AAAWABQo3DTHwdFy0CCa+h6+bi7VJs3tcgAAAAAAAQDhAgAAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP////9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/////wIBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAJUC+QAAFgAU/c11FQPC7OYlaspyx3uf70ntV8kBAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAJUCzUAAFgAUZlkFHI0o4IUPP7h+AZ/85GpMC7QAAAAAAAEBQgEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAAAlQLNQAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAA==')
if __name__ == '__main__':
PSBTTest().main()