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

achow101 · 2019-10-15T21:32:25Z

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.

practicalswift · 2019-10-15T22:50:48Z

Concept ACK

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

practicalswift · 2019-10-17T11:06:20Z

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:

70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f
7cbaa5c8757924f545887bb282dd750000000000ffffffff838d0427d0ec
650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d01000000
00ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb81
5e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df55
46e8742d202020ffff603016000001012000c2eb0b000000001714a9b7f5
faff09090010610000db2a077959d07cea1d01000000

The following UBSan condition is triggered:

prevector.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
0x7fff2c5dc562: note: pointer points here
 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
              ^
    #0 0x56183e4f724f in prevector<28u, unsigned char, unsigned int, int>::swap(prevector<28u, unsigned char, unsigned int, int>&) src/./prevector.h:452:9
    #1 0x56183e4ec6fa in prevector<28u, unsigned char, unsigned int, int>::operator=(prevector<28u, unsigned char, unsigned int, int>&&) src/./prevector.h:272:9
    #2 0x56183e4ec6fa in CScript::operator=(CScript&&) src/./script/script.h:390
    #3 0x56183f407fa9 in ProduceSignature(SigningProvider const&, BaseSignatureCreator const&, CScript const&, SignatureData&) src/script/sign.cpp:240:23
    #4 0x56183f30c8a3 in SignPSBTInput(SigningProvider const&, PartiallySignedTransaction&, int, int, SignatureData*, bool) src/psbt.cpp:285:24
    #5 0x56183e7eb0e8 in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:54:29
    #6 0x56183e393014 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:28:35
…
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior prevector.h:452:19 in

Might be worth tackling that one as well?

achow101 · 2019-10-17T17:55:20Z

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

maflcko · 2019-10-17T18:06:27Z

Something like this:

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

maflcko · 2019-10-17T18:07:00Z

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

practicalswift · 2019-10-17T18:27:01Z

@MarcoFalke

$ xxd -r -p > seed <<< "70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f
7cbaa5c8757924f545887bb282dd750000000000ffffffff838d0427d0ec
650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d01000000
00ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb81
5e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df55
46e8742d202020ffff603016000001012000c2eb0b000000001714a9b7f5
faff09090010610000db2a077959d07cea1d01000000"
$ head -c4 seed
psbt

:)

practicalswift · 2019-10-17T18:39:55Z

@achow101

$ CC=clang CXX=clang++ ./configure --enable-fuzz \
      --with-sanitizers=address,fuzzer,undefined
$ make
$ mkdir psbt-seeds/
$ xxd -r -p > psbt-seeds/seed <<< "70736274ff01009a020000000258e87a21b56daf0c23be8e7070456c336f
7cbaa5c8757924f545887bb282dd750000000000ffffffff838d0427d0ec
650a68aa46bb0b098aea4422c071b2ca78352a077959d07cea1d01000000
00ffffffff0270aaf00800000000160014d85c2b71d0060b09c9886aeb81
5e50991dda124d00e1f5050000000016001400aea9a2e5f0f876a588df55
46e8742d202020ffff603016000001012000c2eb0b000000001714a9b7f5
faff09090010610000db2a077959d07cea1d01000000"
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1" src/test/fuzz/psbt psbt-seeds/
…
prevector.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
0x7fff2c5dc562: note: pointer points here
 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
              ^
    #0 0x56183e4f724f in prevector<28u, unsigned char, unsigned int, int>::swap(prevector<28u, unsigned char, unsigned int, int>&) src/./prevector.h:452:9
    #1 0x56183e4ec6fa in prevector<28u, unsigned char, unsigned int, int>::operator=(prevector<28u, unsigned char, unsigned int, int>&&) src/./prevector.h:272:9
    #2 0x56183e4ec6fa in CScript::operator=(CScript&&) src/./script/script.h:390
    #3 0x56183f407fa9 in ProduceSignature(SigningProvider const&, BaseSignatureCreator const&, CScript const&, SignatureData&) src/script/sign.cpp:240:23
    #4 0x56183f30c8a3 in SignPSBTInput(SigningProvider const&, PartiallySignedTransaction&, int, int, SignatureData*, bool) src/psbt.cpp:285:24
    #5 0x56183e7eb0e8 in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:54:29
    #6 0x56183e393014 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:28:35
…
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior prevector.h:452:19 in

:)

achow101 · 2019-10-18T00:00:25Z

I'm not really sure why that happens.

diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index 0ed92e8d5b..be24d002ad 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -237,7 +237,11 @@ bool ProduceSignature(const SigningProvider& provider, const BaseSignatureCreato
     if (P2SH) {
         result.push_back(std::vector<unsigned char>(subscript.begin(), subscript.end()));
     }
-    sigdata.scriptSig = PushAll(result);
+    if (result.size() == 0) {
+        sigdata.scriptSig.clear();
+    } else {
+        sigdata.scriptSig = PushAll(result);
+    }
 
     // Test solution
     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 · 2019-10-19T15:29:25Z

@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:

sigdata.scriptSig = PushAll(result);

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

Whereas this won't trigger the UBSan warning:

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

Could that be the case?

achow101 · 2019-10-19T16:31:12Z

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 · 2019-10-19T16:51:12Z

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

practicalswift · 2019-10-20T19:03:04Z

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

bitcoin/test/sanitizer_suppressions/ubsan

Line 2 in b8f041a

alignment:prevector.h

practicalswift · 2019-10-22T15:10:35Z

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

bitcoin/test/sanitizer_suppressions/ubsan

Lines 1 to 2 in b8f041a

    
           alignment:move.h 
        
           alignment:prevector.h

Great!

src/script/script.h

practicalswift · 2019-11-04T09:20:08Z

@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? :)~~

src/psbt.cpp

practicalswift · 2019-11-04T20:19:15Z

ACK db30e5b assuming Travis is happy too -- diff looks correct

src/node/psbt.cpp

DrahtBot · 2019-11-05T00:16:52Z

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.

src/script/script.h

`#pragma pack(1)` prevents aligning the struct and its members to their required alignment. This can result in code that performs non-aligned reads and writes to integers and pointers, which is problematic on some architectures. It also triggers UBsan — see bitcoin#17156 (comment) and bitcoin#17510.

… within bounds f5fb7fc psbt: check output index is within bounds before accessing (Andrew Chow) 1cf77a2 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow) Pull request description: Backport of #17156, non-trivial due to crossing the refactor in #17371 ACKs for top commit: laanwj: ACK f5fb7fc Tree-SHA512: 7aabf9a6b8a8e287a26dfbf73a437a3bc55177bef8fc5149d822ef81b8ef2458e1d718c3a19c73532c5cef0f9bd8144574c7fad90ca89f13a08b44edf3a2656d

- [0.19] wallet: Reset reused transactions cache bitcoin#18083 - 0.19: Backports bitcoin#17792 - psbt: handle unspendable psbts bitcoin#17524 - qt: Fix comparison function signature bitcoin#17634 - psbt: check that various indexes and amounts are within bounds bitcoin#17156 - [0.19] psbt: check that various indexes and amounts are within bounds bitcoin#18079 - [0.19] Final backports for 0.19.1 bitcoin#17988 - Bug: IsUsedDestination shouldn't use key id as script id for ScriptHash bitcoin#17924 - qt: Fix deprecated QCharRef usage bitcoin#18101 - gui: Throttle GUI update pace when -reindex bitcoin#18121 - gui: Fix race in WalletModel::pollBalanceChanged bitcoin#18123 - gui: Fix unintialized WalletView::progressDialog bitcoin#18062 - Bugfix: GUI: Hide the HD/encrypt icons earlier so they get re-shown if another wallet is open bitcoin#18007 - bug-fix macos: give free bytes to F_PREALLOCATE bitcoin#17887 - test: add missing #include to fix compiler errors bitcoin#17980 - zmq: Fix due to invalid argument and multiple notifiers bitcoin#17445

`#pragma pack(1)` prevents aligning the struct and its members to their required alignment. This can result in code that performs non-aligned reads and writes to integers and pointers, which is problematic on some architectures. It also triggers UBsan — see bitcoin/bitcoin#17156 (comment) and #17510. This is a partial port of Core #17708. Pull request description: Ensure prevector data is appropriately aligned. Earlier discussion in #17530. In contrast to #17530, it does this without increase in size of any of the coin cache data structures (x86_64, clang) | Struct | (size,align) before | (size,align) after | | ---------------- | ------------------- | ------------------- | | Coin | 48, 8 | 48, 8 | | CCoinsCacheEntry | 56, 8 | 56, 8 | | CScript | 32, 1 | 32, 8 |

`#pragma pack(1)` prevents aligning the struct and its members to their required alignment. This can result in code that performs non-aligned reads and writes to integers and pointers, which is problematic on some architectures. It also triggers UBsan — see bitcoin#17156 (comment) and bitcoin#17510.

Summary: ``` `#pragma pack(1)` prevents aligning the struct and its members to their required alignment. This can result in code that performs non-aligned reads and writes to integers and pointers, which is problematic on some architectures. It also triggers UBsan — see bitcoin/bitcoin#17156 (comment) and #17510. ``` Backport of core [[bitcoin/bitcoin#17708 | PR17708]]. Test Plan: With and without UBSAN: ninja check Reviewers: #bitcoin_abc, nakihito Reviewed By: nakihito Differential Revision: https://reviews.bitcoinabc.org/D5986

Summary: * 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. * psbt: check output index is within bounds before accessing This is a backport of Core [[bitcoin/bitcoin#17156 | PR17156]] Because we only have segwit style UTXO due to our signature algorithm, some checks and test case are not relevent to us. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: PiRK Differential Revision: https://reviews.bitcoinabc.org/D8092

… within bounds deaa6dd psbt: check output index is within bounds before accessing (Andrew Chow) f1ef7f0 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow) Pull request description: Fixes bitcoin#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 deaa6dd -- only change since last ACK was the addition of tests gwillen: tested ACK deaa6dd, would also like to see this merged! Tree-SHA512: 06c36720bbb5a7ab1c29f7d15878bf9f0d3e5760c06bff479d412e1bf07bb3e0e9ab6cca820a4bfedaab71bfd7af813807e87cbcdf0af25cc3f66a53a06dbcfd

`#pragma pack(1)` prevents aligning the struct and its members to their required alignment. This can result in code that performs non-aligned reads and writes to integers and pointers, which is problematic on some architectures. It also triggers UBsan — see bitcoin/bitcoin#17156 (comment) and #17510.

…nts are within bounds f5fb7fc psbt: check output index is within bounds before accessing (Andrew Chow) 1cf77a2 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow) Pull request description: Backport of bitcoin#17156, non-trivial due to crossing the refactor in bitcoin#17371 ACKs for top commit: laanwj: ACK f5fb7fc Tree-SHA512: 7aabf9a6b8a8e287a26dfbf73a437a3bc55177bef8fc5149d822ef81b8ef2458e1d718c3a19c73532c5cef0f9bd8144574c7fad90ca89f13a08b44edf3a2656d

…and amounts are within bounds f5fb7fc psbt: check output index is within bounds before accessing (Andrew Chow) 1cf77a2 Don't calculate tx fees for PSBTs with invalid money values (Andrew Chow) Pull request description: Backport of bitcoin#17156, non-trivial due to crossing the refactor in bitcoin#17371 ACKs for top commit: laanwj: ACK f5fb7fc Tree-SHA512: 7aabf9a6b8a8e287a26dfbf73a437a3bc55177bef8fc5149d822ef81b8ef2458e1d718c3a19c73532c5cef0f9bd8144574c7fad90ca89f13a08b44edf3a2656d

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

achow101 force-pushed the psbt-fuzz-fix branch 2 times, most recently from fcb2421 to 545e0e5 Compare October 16, 2019 13:38

practicalswift mentioned this pull request Oct 21, 2019

Make all tests pass UBSan without using any UBSan suppressions #17208

Closed

laanwj reviewed Oct 27, 2019

View reviewed changes

src/script/script.h Outdated Show resolved Hide resolved

practicalswift reviewed Nov 4, 2019

View reviewed changes

src/psbt.cpp Show resolved Hide resolved

achow101 force-pushed the psbt-fuzz-fix branch 2 times, most recently from 9668444 to db30e5b Compare November 4, 2019 20:04

maflcko reviewed Nov 4, 2019

View reviewed changes

src/node/psbt.cpp Show resolved Hide resolved

maflcko reviewed Nov 15, 2019

View reviewed changes

src/script/script.h Outdated Show resolved Hide resolved

achow101 force-pushed the psbt-fuzz-fix branch from db30e5b to d447b94 Compare November 18, 2019 18:27

achow101 mentioned this pull request Nov 18, 2019

Add copy constructor and copy operator= to CScript to remove ubsan suppression #17510

Closed

laanwj mentioned this pull request Nov 20, 2019

Use natural alignment for prevector #17530

Closed

practicalswift mentioned this pull request Feb 10, 2020

Security: remove or restrict rpcbind #18105

Closed

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

achow101 commented Oct 15, 2019

Uh oh!

practicalswift commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Oct 17, 2019

Uh oh!

achow101 commented Oct 17, 2019

Uh oh!

maflcko commented Oct 17, 2019

Uh oh!

maflcko commented Oct 17, 2019

Uh oh!

practicalswift commented Oct 17, 2019

Uh oh!

practicalswift commented Oct 17, 2019

Uh oh!

achow101 commented Oct 18, 2019

Uh oh!

practicalswift commented Oct 19, 2019

Uh oh!

achow101 commented Oct 19, 2019

Uh oh!

achow101 commented Oct 19, 2019

Uh oh!

practicalswift commented Oct 20, 2019

Uh oh!

practicalswift commented Oct 22, 2019

Uh oh!

Uh oh!

practicalswift commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Nov 4, 2019

Uh oh!

Uh oh!

DrahtBot commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Oct 15, 2019 •

edited

Loading

practicalswift commented Nov 4, 2019 •

edited

Loading

DrahtBot commented Nov 5, 2019 •

edited

Loading