tests: Add fuzzing harness for various PSBT related functions #17136

practicalswift · 2019-10-14T14:47:24Z

Add fuzzing harness for various PSBT related functions.

Testing this PR

Run:

$ CC=clang CXX=clang++ ./configure --enable-fuzz \
      --with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/psbt

DrahtBot · 2019-10-14T15:21:24Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17229 (tests: Add fuzzing harnesses for various Base{32,58,64} and hex related functions by practicalswift)
#17225 (tests: Test serialisation as part of deserialisation fuzzing. Test round-trip equality where possible. by practicalswift)
#17109 (tests: Add fuzzing harness for various functions consuming only integrals by practicalswift)
#17093 (tests: Add fuzzing harness for various CTx{In,Out} related functions by practicalswift)
#17071 (tests: Add fuzzing harness for CheckBlock(...) and other CBlock related functions by practicalswift)
#17051 (tests: Add deserialization fuzzing harnesses by practicalswift)
#17050 (tests: Add fuzzing harnesses for functions parsing scripts, numbers, JSON and HD keypaths (bip32) by practicalswift)

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.

laanwj · 2019-10-15T06:27:38Z

Thanks for adding fuzzing tests! However I think the PR list is getting saturated this way. Maybe it makes more sense to add these to a single PR. I suppose people will intend to review and merge them together anyway.

practicalswift · 2019-10-15T09:17:50Z

@laanwj I try to separate so that fuzzers stressing a specific subsystem go in to the same PR.

The reason I do that is that if the fuzzers uncover potential problems in our code base the fuzzing PR might need attention not only from test/ maintainers but also from experts on the specific subsystem being stressed.

This PR is a good example: this PR adds a PSBT fuzzer which means that an expert on that part of the code base (such as @achow101?) might want to take a look at any issues the fuzzer uncovers.

Since I submitted this PR the PSBT fuzzer in this PR has managed to hit the following conditions which might need some investigation from a PSBT expert:

$ src/test/fuzz/psbt
…
==8112==ERROR: AddressSanitizer: heap-use-after-free on address 0x6070001083f0 at pc 0x55a5d650f41b bp 0x7ffe03cc3c90 sp 0x7ffe03cc3c88
READ of size 8 at 0x6070001083f0 thread T0
    #0 0x55a5d650f41a in CTxOut::operator=(CTxOut const&) src/./primitives/transaction.h:133:7
    #1 0x55a5d8362e8d in PartiallySignedTransaction::GetInputUTXO(CTxOut&, int) const src/psbt.cpp:72:14
    #2 0x55a5d6dae419 in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:32:19
    #3 0x55a5d64b27d7 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:28:35
…
$ src/test/fuzz/psbt
…
AddressSanitizer:DEADLYSIGNAL
=================================================================
==22238==ERROR: AddressSanitizer: SEGV on unknown address 0x6098001079e0 (pc 0x55880d55c50f bp 0x7fff8d03d8f0 sp 0x7fff8d03d860 T0)
==22238==The signal is caused by a READ memory access.
    #0 0x55880d55c50e in CTxOut::operator=(CTxOut const&) src/./primitives/transaction.h:133:7
    #1 0x55880f3b6a83 in SignPSBTInput(SigningProvider const&, PartiallySignedTransaction&, int, int, SignatureData*, bool) src/psbt.cpp:262:14
    #2 0x55880f3b81ff in FinalizePSBT(PartiallySignedTransaction&) src/psbt.cpp:311:21
    #3 0x55880d500ee9 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:62:11
…
$ src/test/fuzz/psbt
…
node/psbt.cpp:82:26: runtime error: signed integer overflow: 7857790368227518552 + 6582955728264977243 cannot be represented in type 'long'
    #0 0x55a8b66b13ee in AnalyzePSBT(PartiallySignedTransaction)::$_0::operator()(long, CTxOut const&) const src/node/psbt.cpp:82:26
    #1 0x55a8b66b1043 in long std::accumulate<__gnu_cxx::__normal_iterator<CTxOut*, std::vector<CTxOut, std::allocator<CTxOut> > >, long, AnalyzePSBT(PartiallySignedTransaction)::$_0>(__gnu_cxx::__normal_iterator<CTxOut*, std::vector<CTxOut, std::allocator<CTxOut> > >, __gnu_cxx::__normal_iterator<CTxOut*, std::vector<CTxOut, std::allocator<CTxOut> > >, long, AnalyzePSBT(PartiallySignedTransaction)::$_0) /usr/bin/../lib/gcc/x86_64-linux-gnu/7.4.0/../../../../include/c++/7.4.0/bits/stl_numeric.h:154:11
    #2 0x55a8b66aeecf in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:80:27
    #3 0x55a8b5db1a17 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:28:35
…
$ src/test/fuzz/psbt
…
policy/feerate.cpp:18:34: runtime error: signed integer overflow: -7497025602362688435 * 1000 cannot be represented in type 'long'
    #0 0x5587f480b863 in CFeeRate::CFeeRate(long const&, unsigned long) src/policy/feerate.cpp:18:34
    #1 0x5587f327051f in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:116:22
    #2 0x5587f2971ac7 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:28:35
…

The day before submitting the PSBT fuzzer I submitted a Miniscript fuzzer (#17129) which means that Miniscript expert @sipa might want to take a look at the potential issues it manages to hit (heap out-of-bounds read in Node::CalcOps and assertion failure in ComputeType).

In summary I think it is a good idea to group related fuzzers in to the same PR.

OTOH I'm not entirely convinced that it is a good idea to group unrelated fuzzers (such as for example the PSBT fuzzers and Miniscript fuzzers) in to the same PR in order to keep the PR list short :)

diff --git a/src/psbt.cpp b/src/psbt.cpp
index fe74002e8..515410534 100644
--- a/src/psbt.cpp
+++ b/src/psbt.cpp
@@ -69,6 +69,9 @@ bool PartiallySignedTransaction::GetInputUTXO(CTxOut& utxo, int input_index) con
     PSBTInput input = inputs[input_index];
     int 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;
@@ -259,6 +262,9 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction&
         if (input.non_witness_utxo->GetHash() != prevout.hash) {
             return false;
         }
+        if (prevout.n >= input.non_witness_utxo->vout.size()) {
+            return false;
+        }
         utxo = input.non_witness_utxo->vout[prevout.n];
     } else if (!input.witness_utxo.IsNull()) {
         utxo = input.witness_utxo;

laanwj · 2019-10-15T09:20:49Z

OTOH I'm not entirely convinced that it is a good idea to group unrelated fuzzers (such as for example the PSBT fuzzers and Miniscript fuzzers) in to the same PR in order to keep the PR list short :)

I don't think anyone but the maintainers ever cares about keeping the PR list clean. Feel free to ignore my remark, but don't expect attention from me any time soon on these then.

Also, how you group them would be up to you, I just think that there should be a limit to the number of open PRs by one author, let alone similar ones.

jb55 · 2019-10-15T18:32:57Z

Concept ACK. I think the granular fuzz PRs are ok, it allows individuals from each subsystem to comment on them independently.

practicalswift · 2019-10-17T12:38:15Z

These are the places where we process raw PSBT:

analyzepsbt(JSONRPCRequest const&) → DecodeBase64PSBT(PartiallySignedTransaction&, std::string const&, std::string&) → DecodeRawPSBT(PartiallySignedTransaction&, std::string const&, std::string&)
combinepsbt(JSONRPCRequest const&) → DecodeBase64PSBT(PartiallySignedTransaction&, std::string const&, std::string&) → DecodeRawPSBT(PartiallySignedTransaction&, std::string const&, std::string&)
decodepsbt(JSONRPCRequest const&) → DecodeBase64PSBT(PartiallySignedTransaction&, std::string const&, std::string&) → DecodeRawPSBT(PartiallySignedTransaction&, std::string const&, std::string&)
finalizepsbt(JSONRPCRequest const&) → DecodeBase64PSBT(PartiallySignedTransaction&, std::string const&, std::string&) → DecodeRawPSBT(PartiallySignedTransaction&, std::string const&, std::string&)
joinpsbts(JSONRPCRequest const&) → DecodeBase64PSBT(PartiallySignedTransaction&, std::string const&, std::string&) → DecodeRawPSBT(PartiallySignedTransaction&, std::string const&, std::string&)
utxoupdatepsbt(JSONRPCRequest const&) → DecodeBase64PSBT(PartiallySignedTransaction&, std::string const&, std::string&) → DecodeRawPSBT(PartiallySignedTransaction&, std::string const&, std::string&)
walletprocesspsbt(JSONRPCRequest const&) → DecodeBase64PSBT(PartiallySignedTransaction&, std::string const&, std::string&) → DecodeRawPSBT(PartiallySignedTransaction&, std::string const&, std::string&)

practicalswift · 2019-10-30T21:18:52Z

Rebased!

EthanHeilman · 2019-11-13T14:45:30Z

I did some fuzzing of Bitcoin. I wish this had been around back then.

Concept ACK

maflcko

ACK ed94e0a 🌳

Show signature and timestamp

Signature:

Timestamp of file with hash 6cb134b5529a1cecaf9838fe5e9d153f0bbc035bbfa3db1bb4b21f0dcd6aef96 -

src/test/fuzz/psbt.cpp

test/fuzz/test_runner.py

practicalswift · 2019-11-18T16:54:13Z

@MarcoFalke Thanks a lot for reviewing! Feedback addressed. Please re-review :)

maflcko · 2019-11-18T17:16:05Z

re-ACK 49f4c7f 🐟

Show signature and timestamp

Signature:

Timestamp of file with hash 33b3bc228d3e4851067ee685da6a61b50c83146ed4d27450cc02cae21b16be1b -

practicalswift · 2019-11-18T17:18:20Z

@jonasnick You've shown interest in previous fuzzing harnesses - would you mind reviewing this one too? :)

maflcko · 2019-11-18T17:19:02Z

@practicalswift I think you mean @jonatack ? :)

…ctions 49f4c7f tests: Add fuzzing harness for various PSBT related functions (practicalswift) Pull request description: Add fuzzing harness for various PSBT related functions. **Testing this PR** Run: ``` $ CC=clang CXX=clang++ ./configure --enable-fuzz \ --with-sanitizers=address,fuzzer,undefined $ make $ src/test/fuzz/psbt ``` ACKs for top commit: MarcoFalke: re-ACK 49f4c7f 🐟 Tree-SHA512: 4cebe62bd8c244ee40a43e829f5bd175ab40e1dfbbab1affb1529374858225820d6c9fa9ba45862bf56c1522845422fd96d620cedbdec52a67ac1449dec4e1b2

practicalswift · 2019-11-18T17:23:31Z

Oh, sorry! Same mistake again! :)

Thanks for merging!

jonatack · 2019-11-18T17:26:04Z

Yep, don't hesitate to ping me, I'm for the moment only a part-time contributor and often don't see things :)

practicalswift · 2019-11-18T17:35:34Z

@jonatack Thanks! :)

(BTW: It has been nice to follow your Bitcoin Core activity and progression during 2019 and I really enjoyed the recent podcast interview. I hope you become a full-time contributor during 2020!)

jonatack · 2019-11-18T17:38:30Z

Thanks @practicalswift! 😍

…ted functions 49f4c7f tests: Add fuzzing harness for various PSBT related functions (practicalswift) Pull request description: Add fuzzing harness for various PSBT related functions. **Testing this PR** Run: ``` $ CC=clang CXX=clang++ ./configure --enable-fuzz \ --with-sanitizers=address,fuzzer,undefined $ make $ src/test/fuzz/psbt ``` ACKs for top commit: MarcoFalke: re-ACK 49f4c7f 🐟 Tree-SHA512: 4cebe62bd8c244ee40a43e829f5bd175ab40e1dfbbab1affb1529374858225820d6c9fa9ba45862bf56c1522845422fd96d620cedbdec52a67ac1449dec4e1b2

…functions Summary: 49f4c7f0699e5e19ac6e41ef5b607392dd7a2983 tests: Add fuzzing harness for various PSBT related functions (practicalswift) Pull request description: Add fuzzing harness for various PSBT related functions. **Testing this PR** Run: ``` $ CC=clang CXX=clang++ ./configure --enable-fuzz \ --with-sanitizers=address,fuzzer,undefined $ make $ src/test/fuzz/psbt ``` --- Backport of Core [[bitcoin/bitcoin#17136 | PR17136]] Test Plan: cmake -GNinja .. -DENABLE_SANITIZERS="address;fuzzer" -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ ninja bitcoin-fuzzers ./srt/test/fuzz/psbt Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6945

…ted functions 49f4c7f tests: Add fuzzing harness for various PSBT related functions (practicalswift) Pull request description: Add fuzzing harness for various PSBT related functions. **Testing this PR** Run: ``` $ CC=clang CXX=clang++ ./configure --enable-fuzz \ --with-sanitizers=address,fuzzer,undefined $ make $ src/test/fuzz/psbt ``` ACKs for top commit: MarcoFalke: re-ACK 49f4c7f 🐟 Tree-SHA512: 4cebe62bd8c244ee40a43e829f5bd175ab40e1dfbbab1affb1529374858225820d6c9fa9ba45862bf56c1522845422fd96d620cedbdec52a67ac1449dec4e1b2

fanquake added the Tests label Oct 14, 2019

practicalswift mentioned this pull request Oct 15, 2019

Potential PSBT issues found by the PSBT fuzzer #17149

Closed

DrahtBot added the Needs rebase label Oct 25, 2019

practicalswift force-pushed the fuzzers-psbt branch from b5c8aa6 to 60af6cd Compare October 25, 2019 17:05

DrahtBot added Needs rebase and removed Needs rebase labels Oct 25, 2019

practicalswift force-pushed the fuzzers-psbt branch from 60af6cd to 78033db Compare October 30, 2019 21:18

DrahtBot removed the Needs rebase label Oct 30, 2019

practicalswift force-pushed the fuzzers-psbt branch from 78033db to ed94e0a Compare October 31, 2019 11:35

practicalswift mentioned this pull request Oct 31, 2019

Add fuzzing seeds to recently contributed fuzzing harnesses bitcoin-core/qa-assets#2

Closed

maflcko reviewed Nov 15, 2019

View reviewed changes

src/test/fuzz/psbt.cpp Outdated Show resolved Hide resolved

src/test/fuzz/psbt.cpp Outdated Show resolved Hide resolved

test/fuzz/test_runner.py Outdated Show resolved Hide resolved

tests: Add fuzzing harness for various PSBT related functions

49f4c7f

practicalswift force-pushed the fuzzers-psbt branch from ed94e0a to 49f4c7f Compare November 18, 2019 16:53

maflcko merged commit 49f4c7f into bitcoin:master Nov 18, 2019

practicalswift mentioned this pull request Nov 19, 2019

psbt: handle unspendable psbts #17524

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

practicalswift deleted the fuzzers-psbt branch April 10, 2021 19:39

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

tests: Add fuzzing harness for various PSBT related functions #17136

tests: Add fuzzing harness for various PSBT related functions #17136

Uh oh!

Conversation

practicalswift commented Oct 14, 2019

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

laanwj commented Oct 15, 2019

Uh oh!

practicalswift commented Oct 15, 2019

Uh oh!

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

Uh oh!

Uh oh!

jb55 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!

practicalswift commented Oct 30, 2019

Uh oh!

EthanHeilman commented Nov 13, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Nov 18, 2019

Uh oh!

maflcko commented Nov 18, 2019

Uh oh!

practicalswift commented Nov 18, 2019

Uh oh!

maflcko commented Nov 18, 2019

Uh oh!

practicalswift commented Nov 18, 2019

Uh oh!

jonatack commented Nov 18, 2019

Uh oh!

practicalswift commented Nov 18, 2019

Uh oh!

jonatack commented Nov 18, 2019

Uh oh!

Uh oh!

DrahtBot commented Oct 14, 2019 •

edited

Loading

laanwj commented Oct 15, 2019 •

edited

Loading

jb55 commented Oct 15, 2019 •

edited

Loading