sikkienl/dash
1
0
Fork 0
mirror of https://github.com/dashpay/dash.git synced 2024-12-25 12:02:48 +01:00
Code Issues Actions Packages Projects Releases Wiki Activity

Merge #17156: psbt: check that various indexes and amounts are within bounds

Browse Source 
deaa6dd144f5650b385658a0c4f9a014aff8dde2 psbt: check output index is within bounds before accessing (Andrew Chow)
f1ef7f0aa46338f4cd8de79696027a1bf868f359 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow)

Pull request description:

  Fixes #17149

  Two classes of issues were found by the psbt fuzzer: values out of range and causing overflows, and prevout indexes being out of range. This PR fixes both.

  When accessing a specific output using the index given in the tx, check that it is actually a possible output before trying to access the output.

  When summing and checking amounts for `decodepsbt` and `analyzepsbt`, make sure that the values are actually valid money values.. Otherwise, stop summing and don't show the fee. For `analyzepsbt`, return that the next role is the Creator since the Creator needs to remake the transaction to be valid.

ACKs for top commit:
  practicalswift:
    ACK deaa6dd144f5650b385658a0c4f9a014aff8dde2 -- only change since last ACK was the addition of tests
  gwillen:
    tested ACK deaa6dd, would also like to see this merged!

Tree-SHA512: 06c36720bbb5a7ab1c29f7d15878bf9f0d3e5760c06bff479d412e1bf07bb3e0e9ab6cca820a4bfedaab71bfd7af813807e87cbcdf0af25cc3f66a53a06dbcfd
This commit is contained in:
fanquake 2020-01-29 19:26:17 +08:00 committed by PastaPastaPasta
parent bd0f27310f
commit 63ed912c73
5 changed files with 62 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
src/psbt.cpp
View File

@ -72,8 +72,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 {
return false;
@ -225,6 +228,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;
}
@ -330,9 +336,17 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
// Check for a UTXO
CTxOut utxo;
if (psbtx.GetInputUTXO(utxo, i)) {
if (!MoneyRange(utxo.nValue) || !MoneyRange(in_amt + utxo.nValue)) {
result.SetInvalid(strprintf("PSBT is not valid. Input %u has invalid value", i));
return result;
}
in_amt += utxo.nValue;
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;
@ -383,9 +397,16 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
// Get the output amount
CAmount out_amt = std::accumulate(psbtx.tx->vout.begin(), psbtx.tx->vout.end(), CAmount(0),
[](CAmount a, const CTxOut& b) {
if (!MoneyRange(a) || !MoneyRange(b.nValue) || !MoneyRange(a + b.nValue)) {
return CAmount(-1);
}
return a += b.nValue;
}
);
if (!MoneyRange(out_amt)) {
result.SetInvalid(strprintf("PSBT is not valid. Output amount invalid"));
return result;
}
// Get the fee
CAmount fee = in_amt - out_amt;

15
src/rpc/rawtransaction.cpp
View File

@ -1067,7 +1067,13 @@ UniValue decodepsbt(const JSONRPCRequest& request)
UniValue non_wit(UniValue::VOBJ);
TxToUniv(*input.non_witness_utxo, uint256(), non_wit, false);
in.pushKV("non_witness_utxo", non_wit);
total_in += input.non_witness_utxo->vout[psbtx.tx->vin[i].prevout.n].nValue;
CAmount utxo_val = input.non_witness_utxo->vout[psbtx.tx->vin[i].prevout.n].nValue;
if (MoneyRange(utxo_val) && MoneyRange(total_in + utxo_val)) {
total_in += utxo_val;
} else {
// Hack to just not show fee later
have_all_utxos = false;
}
} else {
have_all_utxos = false;
}
@ -1166,7 +1172,12 @@ UniValue decodepsbt(const JSONRPCRequest& request)
outputs.push_back(out);
// Fee calculation
output_value += psbtx.tx->vout[i].nValue;
if (MoneyRange(psbtx.tx->vout[i].nValue) && MoneyRange(output_value + psbtx.tx->vout[i].nValue)) {
output_value += psbtx.tx->vout[i].nValue;
} else {
// Hack to just not show fee later
have_all_utxos = false;
}
}
result.pushKV("outputs", outputs);
if (have_all_utxos) {

3
src/wallet/psbtwallet.cpp
View File

@ -39,6 +39,9 @@ TransactionError FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& ps
// Get the scriptPubKey to know which SigningProvider to use
CScript script;
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

@ -67,6 +67,15 @@ BOOST_AUTO_TEST_CASE(psbt_updater_test)
ssTx << psbtx;
std::string final_hex = HexStr(ssTx);
BOOST_CHECK_EQUAL(final_hex, "70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f7cbaa5c8757924f545887bb2abdd750000000000ffffffff838d0427d0ec650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d0100000000ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb815e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df5546e8742d1d87008f00000000000100bb0200000001aad73931018bd25f84ae400b68848be09db706eac2ac18298babee71ab656f8b0000000048473044022058f6fc7c6a33e1b31548d481c826c015bd30135aad42cd67790dab66d2ad243b02204a1ced2604c6735b6393e5b41691dd78b00f0c5942fb9f751856faa938157dba01feffffff0280f0fa020000000017a9140fb9463421696b82c833af241c78c17ddbde493487d0f20a270100000017a91429ca74f8a08f81999428185c97b5d852e4063f6187650000000104475221029583bf39ae0a609747ad199addd634fa6108559d6c5cd39b4c2183f1ab96e07f2102dab61ff49a14db6a7d02b0cd1fbb78fc4b18312b5b4e54dae4dba2fbfef536d752ae2206029583bf39ae0a609747ad199addd634fa6108559d6c5cd39b4c2183f1ab96e07f049c4942a9220602dab61ff49a14db6a7d02b0cd1fbb78fc4b18312b5b4e54dae4dba2fbfef536d704b9147fd300000000");
// 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)

15
test/functional/rpc_psbt.py
View File

@ -310,5 +310,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('cHNidP8BAHECAAAAAQXbPuwX0yi0oRvSfIXqeruM5+ibuXvP92RCdIEsJR6jAAAAAAD/////AgD5ApUAAAAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XL87QKVAAAAABYAFPck4gF7iL4NL4wtfRAKgQbghiTUAAAAAAABABMCAAAAAAEBQAda8HUHAAAAAAAAAQcCagABAR8AgIFq49AHABYAFJUDtxf2PHo641HEOBOAIvFMNTr2AAAA')
assert_equal(analysis['next'], 'creator')
assert_equal(analysis['error'], 'PSBT is not valid. Input 0 has invalid value')
analysis = self.nodes[0].analyzepsbt('cHNidP8BAHECAAAAAbmNr2X0Nc+2AtaXYjPu3kIFc3Cmj1aYy5WQs5yaUfRvAAAAAAD/////AgCAgWrj0AcAFgAUKNw0x8HRctAgmvoevm4u1SbN7XL87QKVAAAAABYAFPck4gF7iL4NL4wtfRAKgQbghiTUAAAAAAABABQCAAAAAAGghgEAAAAAAAFRAAAAAAEBHwDyBSoBAAAAFgAUlQO3F/Y8ejrjUcQ4E4Ai8Uw1OvYAAAA=')
assert_equal(analysis['next'], 'creator')
assert_equal(analysis['error'], 'PSBT is not valid. Output amount invalid')
analysis = self.nodes[0].analyzepsbt('cHNidP8BAJoCAAAAAkvEW8NnDtdNtDpsmze+Ht2LH35IJcKv00jKAlUs21RrAwAAAAD/////S8Rbw2cO1020OmybN74e3Ysffkglwq/TSMoCVSzbVGsBAAAAAP7///8CwLYClQAAAAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQD5ApUAAAAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XIAAAAAAAEAnQIAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP7///9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/v///wIA+QKVAAAAABl2qRT9zXUVA8Ls5iVqynLHe5/vSe1XyYisQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAAAAAQEfQM0ClQAAAAAWABRmWQUcjSjghQ8/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, 'cHNidP8BAJoCAAAAAkvEW8NnDtdNtDpsmze+Ht2LH35IJcKv00jKAlUs21RrAwAAAAD/////S8Rbw2cO1020OmybN74e3Ysffkglwq/TSMoCVSzbVGsBAAAAAP7///8CwLYClQAAAAAWABSNJKzjaUb3uOxixsvh1GGE3fW7zQD5ApUAAAAAFgAUKNw0x8HRctAgmvoevm4u1SbN7XIAAAAAAAEAnQIAAAACczMa321tVHuN4GKWKRncycI22aX3uXgwSFUKM2orjRsBAAAAAP7///9zMxrfbW1Ue43gYpYpGdzJwjbZpfe5eDBIVQozaiuNGwAAAAAA/v///wIA+QKVAAAAABl2qRT9zXUVA8Ls5iVqynLHe5/vSe1XyYisQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAAAAAQEfQM0ClQAAAAAWABRmWQUcjSjghQ8/uH4Bn/zkakwLtAAAAA==')
if __name__ == '__main__':
PSBTTest().main()