psbt: check that various indexes and amounts are within bounds

achow101 commented at 9:32 pm on October 15, 2019: member

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.

DrahtBot added the label RPC/REST/ZMQ on Oct 15, 2019

practicalswift commented at 10:50 pm on October 15, 2019: contributor

Concept ACK

Thanks for addressing these issues quickly! :) Will test.

achow101 force-pushed on Oct 16, 2019

practicalswift commented at 11:06 am on October 17, 2019: contributor

With this patch applied the PSBT fuzzer appears to be unable to trigger the problematic conditions listed in the issue #17149.

However, I noticed that using the following 202 byte input:

070736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f
17cbaa5c8757924f545887bb282dd750000000000ffffffff838d0427d0ec
2650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d01000000
300ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb81
45e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df55
546e8742d202020ffff603016000001012000c2eb0b000000001714a9b7f5
6faff09090010610000db2a077959d07cea1d01000000

The following UBSan condition is triggered:

 0prevector.h:452:19: runtime error: reference binding to misaligned address 0x7fff2c5dc562 for type 'prevector<28, unsigned char, unsigned int, int>::size_type' (aka 'unsigned int'), which requires 4 byte alignment
 10x7fff2c5dc562: note: pointer points here
 2 ff ff  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
 3              ^
 4    [#0](/bitcoin-bitcoin/0/) 0x56183e4f724f in prevector<28u, unsigned char, unsigned int, int>::swap(prevector<28u, unsigned char, unsigned int, int>&) src/./prevector.h:452:9
 5    [#1](/bitcoin-bitcoin/1/) 0x56183e4ec6fa in prevector<28u, unsigned char, unsigned int, int>::operator=(prevector<28u, unsigned char, unsigned int, int>&&) src/./prevector.h:272:9
 6    [#2](/bitcoin-bitcoin/2/) 0x56183e4ec6fa in CScript::operator=(CScript&&) src/./script/script.h:390
 7    [#3](/bitcoin-bitcoin/3/) 0x56183f407fa9 in ProduceSignature(SigningProvider const&, BaseSignatureCreator const&, CScript const&, SignatureData&) src/script/sign.cpp:240:23
 8    [#4](/bitcoin-bitcoin/4/) 0x56183f30c8a3 in SignPSBTInput(SigningProvider const&, PartiallySignedTransaction&, int, int, SignatureData*, bool) src/psbt.cpp:285:24
 9    [#5](/bitcoin-bitcoin/5/) 0x56183e7eb0e8 in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:54:29
10    [#6](/bitcoin-bitcoin/6/) 0x56183e393014 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:28:35
11…
12SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior prevector.h:452:19 in

Might be worth tackling that one as well?

achow101 commented at 5:55 pm on October 17, 2019: member

What do I need to run in order to get the same error?

MarcoFalke commented at 6:06 pm on October 17, 2019: member

Something like this:

0#check out this branch
1./configure --disable-ccache --enable-fuzz --with-sanitizers=fuzzer,address,undefined CC=clang CXX=clang++
2make -j 9
3export LSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/lsan"
4export TSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/tsan"
5export UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1"
6#run this fuzzer with the seed
7./src/test/fuzz/this_fuzzer -runs=1 ./the_input

MarcoFalke commented at 6:07 pm on October 17, 2019: member

Not sure how to translate the base16 seed into a binary seed with the command line, though

practicalswift commented at 6:27 pm on October 17, 2019: contributor

@MarcoFalke

0$ xxd -r -p > seed <<< "70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f
17cbaa5c8757924f545887bb282dd750000000000ffffffff838d0427d0ec
2650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d01000000
300ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb81
45e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df55
546e8742d202020ffff603016000001012000c2eb0b000000001714a9b7f5
6faff09090010610000db2a077959d07cea1d01000000"
7$ head -c4 seed
8psbt

:)

practicalswift commented at 6:39 pm on October 17, 2019: contributor

@achow101

 0$ CC=clang CXX=clang++ ./configure --enable-fuzz \
 1      --with-sanitizers=address,fuzzer,undefined
 2$ make
 3$ mkdir psbt-seeds/
 4$ xxd -r -p > psbt-seeds/seed <<< "70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f
 57cbaa5c8757924f545887bb282dd750000000000ffffffff838d0427d0ec
 6650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d01000000
 700ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb81
 85e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df55
 946e8742d202020ffff603016000001012000c2eb0b000000001714a9b7f5
10faff09090010610000db2a077959d07cea1d01000000"
11$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" src/test/fuzz/psbt psbt-seeds/
12…
13prevector.h:452:19: runtime error: reference binding to misaligned address 0x7fff2c5dc562 for type 'prevector<28, unsigned char, unsigned int, int>::size_type' (aka 'unsigned int'), which requires 4 byte alignment
140x7fff2c5dc562: note: pointer points here
15 ff ff  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
16              ^
17    [#0](/bitcoin-bitcoin/0/) 0x56183e4f724f in prevector<28u, unsigned char, unsigned int, int>::swap(prevector<28u, unsigned char, unsigned int, int>&) src/./prevector.h:452:9
18    [#1](/bitcoin-bitcoin/1/) 0x56183e4ec6fa in prevector<28u, unsigned char, unsigned int, int>::operator=(prevector<28u, unsigned char, unsigned int, int>&&) src/./prevector.h:272:9
19    [#2](/bitcoin-bitcoin/2/) 0x56183e4ec6fa in CScript::operator=(CScript&&) src/./script/script.h:390
20    [#3](/bitcoin-bitcoin/3/) 0x56183f407fa9 in ProduceSignature(SigningProvider const&, BaseSignatureCreator const&, CScript const&, SignatureData&) src/script/sign.cpp:240:23
21    [#4](/bitcoin-bitcoin/4/) 0x56183f30c8a3 in SignPSBTInput(SigningProvider const&, PartiallySignedTransaction&, int, int, SignatureData*, bool) src/psbt.cpp:285:24
22    [#5](/bitcoin-bitcoin/5/) 0x56183e7eb0e8 in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:54:29
23    [#6](/bitcoin-bitcoin/6/) 0x56183e393014 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:28:35
24…
25SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior prevector.h:452:19 in

:)

achow101 commented at 0:00 am on October 18, 2019: member

I’m not really sure why that happens.

 0diff --git a/src/script/sign.cpp b/src/script/sign.cpp
 1index 0ed92e8d5b..be24d002ad 100644
 2--- a/src/script/sign.cpp
 3+++ b/src/script/sign.cpp
 4@@ -237,7 +237,11 @@ bool ProduceSignature(const SigningProvider& provider, const BaseSignatureCreato
 5     if (P2SH) {
 6         result.push_back(std::vector<unsigned char>(subscript.begin(), subscript.end()));
 7     }
 8-    sigdata.scriptSig = PushAll(result);
 9+    if (result.size() == 0) {
10+        sigdata.scriptSig.clear();
11+    } else {
12+        sigdata.scriptSig = PushAll(result);
13+    }
14 
15     // Test solution
16     sigdata.complete = solved && VerifyScript(sigdata.scriptSig, fromPubKey, &sigdata.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, creator.Checker());

fixes the problem, but it’s more signing code than not. ISTM we should see this elsewhere too but it doesn’t seem like it.

practicalswift commented at 3:29 pm on October 19, 2019: contributor

@achow101

ISTM we should see this elsewhere too but it doesn’t seem like it.

Isn’t the difference that the move assignment operator is used in this case whereas the other non-warning cases you’re referring to use the copy assignment operator?

If I’m correct then these two should trigger the UBSan warning:

0sigdata.scriptSig = PushAll(result);

0CString script_sig = PushAll(result);
1sigdata.scriptSig = std::move(script_sig);

Whereas this won’t trigger the UBSan warning:

0CString script_sig = PushAll(result);
1sigdata.scriptSig = script_sig;

Could that be the case?

achow101 commented at 4:31 pm on October 19, 2019: member

Isn’t the difference that the move assignment operator is used in this case whereas the other non-warning cases you’re referring to use the copy assignment operator?

What I meant was that we should see this same error for any fuzzers that hit ProduceSignature. But I think we just don’t have any fuzzers that reach ProduceSignature.

achow101 commented at 4:51 pm on October 19, 2019: member

I’ve added a commit which adds a copy constructor and copy operator= to CScript which fixes the UBSan warning.

practicalswift commented at 7:03 pm on October 20, 2019: contributor

@achow101 Good! Then we should be able to get rid of this UBSan suppression? :)

https://github.com/bitcoin/bitcoin/blob/b8f041af2d2c89afdc14f8c632dfb7fecb535cb5/test/sanitizer_suppressions/ubsan#L2

practicalswift commented at 3:10 pm on October 22, 2019: contributor

Judging from my testing we can get rid of both these UBSan suppressions as part of this PR:

https://github.com/bitcoin/bitcoin/blob/b8f041af2d2c89afdc14f8c632dfb7fecb535cb5/test/sanitizer_suppressions/ubsan#L1-L2

Great!

in src/script/script.h:416 in c8c393b149 outdated

410@@ -411,6 +411,7 @@ class CScript : public CScriptBase
411     CScript(const_iterator pbegin, const_iterator pend) : CScriptBase(pbegin, pend) { }
412     CScript(std::vector<unsigned char>::const_iterator pbegin, std::vector<unsigned char>::const_iterator pend) : CScriptBase(pbegin, pend) { }
413     CScript(const unsigned char* pbegin, const unsigned char* pend) : CScriptBase(pbegin, pend) { }
414+    CScript(const CScript& a) : CScript(a.begin(), a.end()) {}

laanwj commented at 10:24 am on October 27, 2019:

Might want to add a comment in the code describing why this explicit copy constructor (which seems to be redundant) was added, otherwise it might be removed again with the next ‘clean up warnings’ PR.

practicalswift commented at 12:40 pm on October 27, 2019:

Agreed! That is also why it is important also to remove the UBSan suppressions as part of this PR: to catch non-refactoring nature of such a mistaken “clean up” in a future PR :)

These needs to go:

https://github.com/bitcoin/bitcoin/blob/b8f041af2d2c89afdc14f8c632dfb7fecb535cb5/test/sanitizer_suppressions/ubsan#L1-L2

achow101 commented at 8:03 pm on November 4, 2019:

Added a comment and removed the suppressions.

practicalswift commented at 9:20 am on November 4, 2019: contributor

@achow101 Can you remove the UBSan suppressions? That will allow Travis to confirm that you’ve solved the alignment issue. Then this PR should be ready to go AFAICT.

~~@fanquake Would you mind adding waiting for author-tag? :)~~

in src/psbt.cpp:71 in c8c393b149 outdated

68@@ -69,6 +69,9 @@ bool PartiallySignedTransaction::GetInputUTXO(CTxOut& utxo, int input_index) con
69     PSBTInput input = inputs[input_index];
70     int prevout_index = tx->vin[input_index].prevout.n;
71     if (input.non_witness_utxo) {
72+        if (prevout_index >= input.non_witness_utxo->vout.size()) {

practicalswift commented at 10:33 am on November 4, 2019:

I’m afraid this will trigger a sign-compare warning when compiling with Clang due to LHS being int and the RHS being unsigned long (size_t).

achow101 commented at 8:03 pm on November 4, 2019:

Fixed

achow101 force-pushed on Nov 4, 2019

practicalswift commented at 8:19 pm on November 4, 2019: contributor

ACK db30e5bd6fb51913a4ce2177097be3ebcabab146 assuming Travis is happy too – diff looks correct

in src/node/psbt.cpp:99 in db30e5bd6f outdated

83@@ -79,9 +84,16 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
84         // Get the output amount
85         CAmount out_amt = std::accumulate(psbtx.tx->vout.begin(), psbtx.tx->vout.end(), CAmount(0),
86             [](CAmount a, const CTxOut& b) {
87+                if (!MoneyRange(a) || !MoneyRange(b.nValue) || !MoneyRange(a + b.nValue)) {
88+                    return CAmount(-1);
89+                }

MarcoFalke commented at 10:33 pm on November 4, 2019:

Are you sure this is correct? This will reset the intermediate result to -1, and then accumulate all remaining values.

achow101 commented at 10:42 pm on November 4, 2019:

-1 will fail MoneyRange so later values that go through this will also always return -1.

MarcoFalke commented at 10:44 pm on November 4, 2019:

Correct. Thx.

DrahtBot commented at 0:16 am on November 5, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs 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.

in src/script/script.h:415 in db30e5bd6f outdated

411@@ -412,6 +412,9 @@ class CScript : public CScriptBase
412     CScript(std::vector<unsigned char>::const_iterator pbegin, std::vector<unsigned char>::const_iterator pend) : CScriptBase(pbegin, pend) { }
413     CScript(const unsigned char* pbegin, const unsigned char* pend) : CScriptBase(pbegin, pend) { }
414 
415+    // Define a copy constructor so that operator= is a copy instead of a move to avoid undefined behavior

MarcoFalke commented at 8:26 pm on November 15, 2019:

could explain how this would otherwise be undefined behavior and why this can’t be fixed in prevector.h?

achow101 commented at 9:08 pm on November 15, 2019:

I’m not sure why it would be undefined behavior, but UBSan was triggering on it. I think prevector already has a copy constructor?

MarcoFalke commented at 5:47 pm on November 18, 2019:

In that case I’d prefer to leave the third commit for a separate pull request. Changing those parts of the code without understanding why for the sole reason to please a sanitizer seems worrisome

achow101 commented at 6:27 pm on November 18, 2019:

I’ve dropped the commit.

achow101 force-pushed on Nov 18, 2019

laanwj referenced this in commit 7c6dd71664 on Nov 20, 2019

ajtowns referenced this in commit 9d933ef919 on Dec 10, 2019

DrahtBot added the label Needs rebase on Dec 10, 2019

achow101 force-pushed on Dec 10, 2019

practicalswift commented at 10:14 pm on December 10, 2019: contributor

ACK a2800d704870cf120127a98769ed97b06c35042e – diff looks correct

DrahtBot removed the label Needs rebase on Dec 10, 2019

in src/node/psbt.cpp:99 in e769f25031 outdated

94+                    return CAmount(-1);
95+                }
96                 return a += b.nValue;
97             }
98         );
99+        if (out_amt == -1) {

promag commented at 11:13 pm on December 10, 2019:

e769f25031a63c378f2efeb957b90a72a8524a5e

Also use !MoneyRange(out_amt)?

achow101 commented at 3:25 am on December 11, 2019:

Done

in src/node/psbt.h:45 in e769f25031 outdated

40@@ -41,6 +41,14 @@ struct PSBTAnalysis {
41         next = PSBTRole::CREATOR;
42         error = err_msg;
43     }
44+
45+    void Reset() {

promag commented at 11:13 pm on December 10, 2019:

e769f25031a63c378f2efeb957b90a72a8524a5e

nit, { in new line? (if you decide to keep this, see next comment)

achow101 commented at 3:25 am on December 11, 2019:

Removed Reset

in src/node/psbt.cpp:37 in e769f25031 outdated

31@@ -31,6 +32,10 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
32         // Check for a UTXO
33         CTxOut utxo;
34         if (psbtx.GetInputUTXO(utxo, i)) {
35+            if (!MoneyRange(in_amt) || !MoneyRange(utxo.nValue) || !MoneyRange(in_amt + utxo.nValue)) {
36+                result.Reset();
37+                return result;

promag commented at 11:18 pm on December 10, 2019:

e769f25031a63c378f2efeb957b90a72a8524a5e

Why not return {}; instead of Reset? Same below, and then drop PSBTAnalysis::Reset().

achow101 commented at 3:25 am on December 11, 2019:

Removed Reset

in src/rpc/rawtransaction.cpp:1217 in e769f25031 outdated

1213@@ -1203,7 +1214,12 @@ UniValue decodepsbt(const JSONRPCRequest& request)
1214         outputs.push_back(out);
1215 
1216         // Fee calculation
1217-        output_value += psbtx.tx->vout[i].nValue;
1218+        if (MoneyRange(output_value) && MoneyRange(psbtx.tx->vout[i].nValue) && MoneyRange(output_value + psbtx.tx->vout[i].nValue)) {

promag commented at 11:24 pm on December 10, 2019:

e769f25031a63c378f2efeb957b90a72a8524a5e

Unnecessary check MoneyRange(output_value) - starts with zero and next value is already checked. Same with total_in.

achow101 commented at 3:25 am on December 11, 2019:

Done

achow101 force-pushed on Dec 11, 2019

Don't calculate tx fees for PSBTs with invalid money values

In decodepsbt if an invalid amount is seen, don't calculate the fee
but still show the invalid value in the decode.

In analyze psbt, if an invalid amount is seen, set the next step to
be the creator as the creator needs to remake the transaction so that
it is valid.

f1ef7f0aa4

in src/node/psbt.cpp:35 in fc02ce290d outdated

31@@ -31,6 +32,9 @@ PSBTAnalysis AnalyzePSBT(PartiallySignedTransaction psbtx)
32         // Check for a UTXO
33         CTxOut utxo;
34         if (psbtx.GetInputUTXO(utxo, i)) {
35+            if (!MoneyRange(in_amt) || !MoneyRange(utxo.nValue) || !MoneyRange(in_amt + utxo.nValue)) {

promag commented at 0:28 am on December 11, 2019:

Drop first check, same below.

achow101 commented at 3:26 am on December 11, 2019:

Done, but the check below is necessary. It’s how the accumulator is guaranteed to return -1.

promag commented at 8:28 am on December 11, 2019:

but the check below is necessary.

Oh yeah sorry.

achow101 force-pushed on Dec 11, 2019

achow101 commented at 3:27 am on December 11, 2019: member

I’ve added some more to this.

I’ve added a couple of tests. In doing so, I also found a few additional places that we needed checks.

Additionally, I’ve replaced the Reset calls with the new SetInvalid so that we can return proper errors.

practicalswift commented at 7:20 am on December 11, 2019: contributor

ACK 2dd91b86cd7ef09fa3ae280bfe99db8cfb6fe531 – diff looks correct

in src/psbt.cpp:68 in 2dd91b86cd outdated

65@@ -66,8 +66,11 @@ bool PartiallySignedTransaction::AddOutput(const CTxOut& txout, const PSBTOutput
66 bool PartiallySignedTransaction::GetInputUTXO(CTxOut& utxo, int input_index) const
67 {
68     PSBTInput input = inputs[input_index];

Sjors commented at 11:44 am on December 11, 2019:

nit: inputs.at(input_index)

achow101 commented at 5:42 pm on December 11, 2019:

Unrelated style changes are unrelated

Sjors commented at 8:57 am on December 12, 2019:

It’s not a style change, at checks these array indexes “are within bounds” and throws an exception otherwise. There’s a bunch of places in this code where we don’t use iterators and assume that two arrays are the same size.

gwillen commented at 0:24 am on December 14, 2019:

Throwing an exception here would actually be rude, yes? So it would be better to establish invariants that ensure we are in-bounds, or else add a bounds check ourselves otherwise?

promag commented at 11:58 pm on December 16, 2019:

establish invariants that ensure we are in-bounds

Agree, I think it makes no sense to add bound checks here.

In any case, here or followup this could use

0const_reference operator[]( size_type pos ) const;

since PSBTInput is not that trivial.

gwillen commented at 1:09 am on December 17, 2019:

That would just mean changing “PSBTInput input” to “const PSBTInput& input”?

promag commented at 1:27 am on December 17, 2019:

Yes.

in src/node/psbt.cpp:42 in 2dd91b86cd outdated

37+                return result;
38+            }
39             in_amt += utxo.nValue;
40             input_analysis.has_utxo = true;
41         } else {
42+            if (input.non_witness_utxo && psbtx.tx->vin[i].prevout.n >= input.non_witness_utxo->vout.size()) {

Sjors commented at 11:48 am on December 11, 2019:

nit: vin.at(i) (also in the FinalizeAndExtractPSBT loops)

achow101 commented at 5:41 pm on December 11, 2019:

meh. don’t feel like changing

practicalswift commented at 6:20 pm on December 11, 2019: contributor

@Sjors Do you ACK the changes also with the nits unaddressed? If so please consider ACK:ing - it would be good to get this reviewed (and merged).

It bothers me that an RPC call with a malformed PSBT can bring down Bitcoin Core :)

Sjors commented at 8:58 am on December 12, 2019: member

@practicalswift I haven’t reviewed the rest of the code yet. My suggestions for improvements should not hold back merging.

gwillen commented at 0:34 am on December 14, 2019: contributor

@achow101 what would you say to the alternate approach of having something like an all-in-one-place PSBT sanity checker? Could be added to IsSane() (which I would prefer), or if you think it might be too heavyweight (but I don’t think so), a new method.

Then we can immediately check every PSBT at the point of parsing, and not have to worry thereafter.

If you’re amenable, I would be happy to make a candidate CL.

(If you’re not amenable, then utACK to this one. But I’m concerned that the piecemeal approach will both clutter the code with checks that are redundant across different methods, AND still miss stuff that we’ll have to add more checks for later.)

achow101 commented at 3:37 pm on December 16, 2019: member

@gwillen I don’t think that we should have these checks at parse time. It’s useful to have decodepsbt be able to decode psbts that are correctly formatted but logically incorrect (as we are doing here). Doing these checks at parse time would mean that decodepsbt would fail.

gwillen commented at 6:24 pm on December 16, 2019: contributor

That makes sense. What do you think about having two separate checks – a basic validity check at parse time, and a more thorough check that can be run before doing anything with the PSBT (other than displaying it as JSON)?

I think it’s probably a hazard to allow PSBTs to hang around which have inconsistencies that can lead to e.g. reading off the end of vectors, and count on individual use code paths to check this every time. Even if we fix this specific case, I feel like it’s going to hit us again in the future.

achow101 commented at 11:15 pm on December 16, 2019: member

I think it makes to have a general consistency check for other commands. But I think we should still do error checking like this as a belt-and-suspenders. We could add the general consistency check in another pr.

promag commented at 0:19 am on December 17, 2019: member

Code review ACK 2dd91b86cd7ef09fa3ae280bfe99db8cfb6fe531, also ran the tests. However tests still pass if the new checks ... >= input.non_witness_utxo->vout.size() are removed.

achow101 commented at 3:50 pm on December 17, 2019: member

However tests still pass if the new checks ... >= input.non_witness_utxo->vout.size() are removed.

Really? It fails for me. I removed the second commit and kept just the test from it. That test fails.

promag commented at 0:33 am on December 18, 2019: member

Try running the test with

 0--- a/src/wallet/psbtwallet.cpp
 1+++ b/src/wallet/psbtwallet.cpp
 2@@ -44,9 +44,6 @@ TransactionError FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& ps
 3         if (!input.witness_utxo.IsNull()) {
 4             script = input.witness_utxo.scriptPubKey;
 5         } else if (input.non_witness_utxo) {
 6-            if (txin.prevout.n >= input.non_witness_utxo->vout.size()) {
 7-                return TransactionError::MISSING_INPUTS;
 8-            }
 9             script = input.non_witness_utxo->vout[txin.prevout.n].scriptPubKey;
10         } else {
11             // There's no UTXO so we can just skip this now

And I think I’ve tried with another.

psbt: check output index is within bounds before accessing deaa6dd144

achow101 force-pushed on Jan 6, 2020

achow101 commented at 5:58 pm on January 6, 2020: member

I’ve added test cases for the SignPSBTInput and FillPSBT checks. Since the SignPSBTInput check can’t be reached via RPC (it gets caught by FillPSBT or AnalyzePSBT), I added the test case to the psbt_updater_test unit test.

practicalswift commented at 9:33 pm on January 6, 2020: contributor

ACK deaa6dd144f5650b385658a0c4f9a014aff8dde2 – only change since last ACK was the addition of tests

Would be great to have this merged - this PR greatly improves the robustness of the PSBT code :)

gwillen commented at 9:36 pm on January 27, 2020: contributor

tested ACK deaa6dd, would also like to see this merged!

practicalswift commented at 11:29 pm on January 27, 2020: contributor

I really hoped this fix would make it to 0.19.1 :\

It doesn’t feel good to ship with a known heap use-after-free bug (see #17149) which is reachable via RPC.

Now that we that RPC whitelisting is merged (#12763) we no longer have the luxury of assuming that all RPC access is from fully trusted RPC consumers.

Thus the previous “the RPC consumer is also the node operator” assumption that may have allowed us to take RPC flaws somewhat lightly in the past no longer holds in all cases (it may still hold in the case of obviously dangerous RPC functions such as stop which are unlikely to be whitelisted) :)

promag commented at 8:15 am on January 28, 2020: member

IMO we should keep recommending trusted clients regardless of their whitelist. I hope RPC whitelisting doesn’t give the impression that now the RPC interface can be made public..

practicalswift commented at 5:28 pm on January 28, 2020: contributor

@promag I agree and in an ideal world all users would read and follow our recommendations :)

Unfortunately a non-negligible subset of our users still expose their RPC ports publicly as can be verified empirically by running an Internet-wide TCP port scan using say masscan (impressive software: an Internet-wide single-port scan can be as quick as six minutes given a good connection/NIC!) or similar.

One approach to this could be: “OK, those users screwed up badly. They didn’t read our recommendations. Their loss. Not our problem as developers.”

Another could be: “Oh gosh, that’s unfortunate! Let’s try to mitigate the effect of that by trying to make the RPC interface reasonably robust and secure also in the presence user screwups.”

The former approach is certainly more convenient from a developer perspective, but I don’t think that approach is in the best interest of our users :)

sipa commented at 5:35 pm on January 28, 2020: member

These are not in conflict. We should obviously fix bugs in RPC, even if they only affect careless users (by whatever definition). At the same time, we still shouldn’t recommend exposing RPC to the public, because it was not designed for that.

practicalswift commented at 6:26 pm on January 28, 2020: contributor

We should obviously fix bugs in RPC, even if they only affect careless users (by whatever definition).

Agreed.

Let’s get rid of the heap use-after-free bug this PR fixes :)

Please review :)

At the same time, we still shouldn’t recommend exposing RPC to the public, […]

Agreed.

fanquake referenced this in commit 1326092e6c on Jan 29, 2020

fanquake merged this on Jan 29, 2020

fanquake closed this on Jan 29, 2020

practicalswift commented at 9:15 pm on January 29, 2020: contributor

Very glad to see this one merged!

We have another PSBT related issued to resolve too: the misaligned pointer use in ProduceSignature(…) (via prevector). While not PSBT specific the function ProduceSignature(…) is reachable from AnalyzePSBT (via SignPSBTInput):

0prevector.h:453:19: runtime error: reference binding to misaligned address 0x7f63f89a4022 for type 'prevector<28, unsigned char, unsigned int, int>::size_type' (aka 'unsigned int'), which requires 4 byte alignment
1 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00
2              ^ 
3    [#0](/bitcoin-bitcoin/0/) 0x5652760202ce in prevector<28u, unsigned char, unsigned int, int>::swap(prevector<28u, unsigned char, unsigned int, int>&) src/./prevector.h:453:9
4    [#1](/bitcoin-bitcoin/1/) 0x56527601ff0e in prevector<28u, unsigned char, unsigned int, int>::operator=(prevector<28u, unsigned char, unsigned int, int>&&) src/./prevector.h:273:9
5    [#2](/bitcoin-bitcoin/2/) 0x56527601ff0e in CScript::operator=(CScript&&) src/./script/script.h:390:7
6    [#3](/bitcoin-bitcoin/3/) 0x565276094236 in ProduceSignature(SigningProvider const&, BaseSignatureCreator const&, CScript const&, SignatureData&) src/script/sign.cpp:240:23
7    [#4](/bitcoin-bitcoin/4/) 0x565276042ef8 in SignPSBTInput(SigningProvider const&, PartiallySignedTransaction&, int, int, SignatureData*, bool) src/psbt.cpp:285:24
8    [#5](/bitcoin-bitcoin/5/) 0x565275fdad45 in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:64:29

Thus anyone interested in PSBT robustness might want to review #17708 too :)

fanquake referenced this in commit 44c2400bcc on Jan 30, 2020

sidhujag referenced this in commit ef5e467032 on Feb 1, 2020

luke-jr referenced this in commit 1cf77a2dc3 on Feb 5, 2020

luke-jr referenced this in commit 3cae9b69ef on Feb 5, 2020

luke-jr referenced this in commit f5fb7fca96 on Feb 6, 2020

laanwj referenced this in commit ba0b7e1296 on Feb 10, 2020

MarkLTZ referenced this in commit d99bc96496 on Feb 13, 2020

sickpig referenced this in commit 814200589b on Mar 2, 2020

sickpig referenced this in commit 3eb6e4c480 on Mar 2, 2020

sickpig referenced this in commit ef5d7ca74e on Mar 2, 2020

sickpig referenced this in commit d0681e5225 on Mar 3, 2020

sickpig referenced this in commit 05e6a8348e on Mar 7, 2020

sickpig referenced this in commit cdbf702cab on Mar 7, 2020

sickpig referenced this in commit 4647626fac on Mar 7, 2020

sickpig referenced this in commit e2fe02fe51 on Mar 8, 2020

sickpig referenced this in commit 763471f8fe on Mar 10, 2020

sickpig referenced this in commit 5f75fdf9f2 on Mar 12, 2020

sickpig referenced this in commit 328f446d61 on Mar 12, 2020

HashUnlimited referenced this in commit 54a433a126 on Apr 17, 2020

deadalnix referenced this in commit aa22044236 on May 7, 2020

ftrader referenced this in commit aa371a8b08 on Aug 17, 2020

jasonbcox referenced this in commit 781c27cdbd on Oct 24, 2020

sidhujag referenced this in commit d1e8291434 on Nov 10, 2020

silence48 referenced this in commit c8b3fd8939 on Nov 15, 2020

backpacker69 referenced this in commit a3e85ad096 on Mar 28, 2021

Munkybooty referenced this in commit 6696b971e2 on Dec 9, 2021

Munkybooty referenced this in commit 5c1e04fb24 on Dec 9, 2021

Munkybooty referenced this in commit 86284f11f4 on Dec 9, 2021

Munkybooty referenced this in commit b66b23919d on Dec 9, 2021

Munkybooty referenced this in commit 2688da5330 on Dec 9, 2021

Munkybooty referenced this in commit 1ad87663be on Dec 9, 2021

Munkybooty referenced this in commit 2c5ea77983 on Dec 23, 2021

DrahtBot locked this on Feb 15, 2022

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

Conflicts