tests: Add fuzzing harness for various PSBT related functions

practicalswift commented at 2:47 pm on October 14, 2019: contributor

Add fuzzing harness for various PSBT related functions.

Testing this PR

Run:

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

fanquake added the label Tests on Oct 14, 2019

DrahtBot commented at 3:21 pm on October 14, 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:

#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 commented at 6:27 am on October 15, 2019: member

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 commented at 9:17 am on October 15, 2019: contributor

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

 0$ src/test/fuzz/psbt
 1…
 2==8112==ERROR: AddressSanitizer: heap-use-after-free on address 0x6070001083f0 at pc 0x55a5d650f41b bp 0x7ffe03cc3c90 sp 0x7ffe03cc3c88
 3READ of size 8 at 0x6070001083f0 thread T0
 4    [#0](/bitcoin-bitcoin/0/) 0x55a5d650f41a in CTxOut::operator=(CTxOut const&) src/./primitives/transaction.h:133:7
 5    [#1](/bitcoin-bitcoin/1/) 0x55a5d8362e8d in PartiallySignedTransaction::GetInputUTXO(CTxOut&, int) const src/psbt.cpp:72:14
 6    [#2](/bitcoin-bitcoin/2/) 0x55a5d6dae419 in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:32:19
 7    [#3](/bitcoin-bitcoin/3/) 0x55a5d64b27d7 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:28:35
 8…
 9$ src/test/fuzz/psbt
10…
11AddressSanitizer:DEADLYSIGNAL
12=================================================================
13==22238==ERROR: AddressSanitizer: SEGV on unknown address 0x6098001079e0 (pc 0x55880d55c50f bp 0x7fff8d03d8f0 sp 0x7fff8d03d860 T0)
14==22238==The signal is caused by a READ memory access.
15    [#0](/bitcoin-bitcoin/0/) 0x55880d55c50e in CTxOut::operator=(CTxOut const&) src/./primitives/transaction.h:133:7
16    [#1](/bitcoin-bitcoin/1/) 0x55880f3b6a83 in SignPSBTInput(SigningProvider const&, PartiallySignedTransaction&, int, int, SignatureData*, bool) src/psbt.cpp:262:14
17    [#2](/bitcoin-bitcoin/2/) 0x55880f3b81ff in FinalizePSBT(PartiallySignedTransaction&) src/psbt.cpp:311:21
18    [#3](/bitcoin-bitcoin/3/) 0x55880d500ee9 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:62:11
19…
20$ src/test/fuzz/psbt
21…
22node/psbt.cpp:82:26: runtime error: signed integer overflow: 7857790368227518552 + 6582955728264977243 cannot be represented in type 'long'
23    [#0](/bitcoin-bitcoin/0/) 0x55a8b66b13ee in AnalyzePSBT(PartiallySignedTransaction)::$_0::operator()(long, CTxOut const&) const src/node/psbt.cpp:82:26
24    [#1](/bitcoin-bitcoin/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
25    [#2](/bitcoin-bitcoin/2/) 0x55a8b66aeecf in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:80:27
26    [#3](/bitcoin-bitcoin/3/) 0x55a8b5db1a17 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:28:35
27…
28$ src/test/fuzz/psbt
29…
30policy/feerate.cpp:18:34: runtime error: signed integer overflow: -7497025602362688435 * 1000 cannot be represented in type 'long'
31    [#0](/bitcoin-bitcoin/0/) 0x5587f480b863 in CFeeRate::CFeeRate(long const&, unsigned long) src/policy/feerate.cpp:18:34
32    [#1](/bitcoin-bitcoin/1/) 0x5587f327051f in AnalyzePSBT(PartiallySignedTransaction) src/node/psbt.cpp:116:22
33    [#2](/bitcoin-bitcoin/2/) 0x5587f2971ac7 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/psbt.cpp:28:35
34…

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

 0diff --git a/src/psbt.cpp b/src/psbt.cpp
 1index fe74002e8..515410534 100644
 2--- a/src/psbt.cpp
 3+++ b/src/psbt.cpp
 4@@ -69,6 +69,9 @@ bool PartiallySignedTransaction::GetInputUTXO(CTxOut& utxo, int input_index) con
 5     PSBTInput input = inputs[input_index];
 6     int prevout_index = tx->vin[input_index].prevout.n;
 7     if (input.non_witness_utxo) {
 8+        if (prevout_index >= input.non_witness_utxo->vout.size()) {
 9+            return false;
10+        }
11         utxo = input.non_witness_utxo->vout[prevout_index];
12     } else if (!input.witness_utxo.IsNull()) {
13         utxo = input.witness_utxo;
14@@ -259,6 +262,9 @@ bool SignPSBTInput(const SigningProvider& provider, PartiallySignedTransaction&
15         if (input.non_witness_utxo->GetHash() != prevout.hash) {
16             return false;
17         }
18+        if (prevout.n >= input.non_witness_utxo->vout.size()) {
19+            return false;
20+        }
21         utxo = input.non_witness_utxo->vout[prevout.n];
22     } else if (!input.witness_utxo.IsNull()) {
23         utxo = input.witness_utxo;

laanwj commented at 9:20 am on October 15, 2019: member

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 commented at 6:32 pm on October 15, 2019: member

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

practicalswift commented at 12:38 pm on October 17, 2019: contributor

These are the places where we process raw PSBT:

0analyzepsbt(JSONRPCRequest const&) → DecodeBase64PSBT(PartiallySignedTransaction&, std::string const&, std::string&) → DecodeRawPSBT(PartiallySignedTransaction&, std::string const&, std::string&)
1combinepsbt(JSONRPCRequest const&) → DecodeBase64PSBT(PartiallySignedTransaction&, std::string const&, std::string&) → DecodeRawPSBT(PartiallySignedTransaction&, std::string const&, std::string&)
2decodepsbt(JSONRPCRequest const&) → DecodeBase64PSBT(PartiallySignedTransaction&, std::string const&, std::string&) → DecodeRawPSBT(PartiallySignedTransaction&, std::string const&, std::string&)
3finalizepsbt(JSONRPCRequest const&) → DecodeBase64PSBT(PartiallySignedTransaction&, std::string const&, std::string&) → DecodeRawPSBT(PartiallySignedTransaction&, std::string const&, std::string&)
4joinpsbts(JSONRPCRequest const&) → DecodeBase64PSBT(PartiallySignedTransaction&, std::string const&, std::string&) → DecodeRawPSBT(PartiallySignedTransaction&, std::string const&, std::string&)
5utxoupdatepsbt(JSONRPCRequest const&) → DecodeBase64PSBT(PartiallySignedTransaction&, std::string const&, std::string&) → DecodeRawPSBT(PartiallySignedTransaction&, std::string const&, std::string&)
6walletprocesspsbt(JSONRPCRequest const&) → DecodeBase64PSBT(PartiallySignedTransaction&, std::string const&, std::string&) → DecodeRawPSBT(PartiallySignedTransaction&, std::string const&, std::string&)

DrahtBot added the label Needs rebase on Oct 25, 2019

practicalswift force-pushed on Oct 25, 2019

DrahtBot removed the label Needs rebase on Oct 25, 2019

DrahtBot added the label Needs rebase on Oct 30, 2019

practicalswift force-pushed on Oct 30, 2019

practicalswift commented at 9:18 pm on October 30, 2019: contributor

Rebased!

DrahtBot removed the label Needs rebase on Oct 30, 2019

practicalswift force-pushed on Oct 31, 2019

EthanHeilman commented at 2:45 pm on November 13, 2019: contributor

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

Concept ACK

in src/test/fuzz/psbt.cpp:35 in ed94e0a167 outdated

30+    if (!DecodeRawPSBT(psbt_mut, raw_psbt, error)) {
31+        return;
32+    }
33+    const PartiallySignedTransaction psbt = psbt_mut;
34+
35+    const PSBTAnalysis analysis = AnalyzePSBT(psbt_mut);

MarcoFalke commented at 5:54 pm on November 15, 2019:

0    const PSBTAnalysis analysis = AnalyzePSBT(psbt);

practicalswift commented at 4:53 pm on November 18, 2019:

Fixed!

in src/test/fuzz/psbt.cpp:46 in ed94e0a167 outdated

41+    (void)psbt.IsNull();
42+    (void)psbt.IsSane();
43+
44+    Optional<CMutableTransaction> tx = psbt.tx;
45+    if (tx) {
46+        const CMutableTransaction mtx = *tx;

MarcoFalke commented at 5:58 pm on November 15, 2019:

Or just inline the *tx further down?

0        const CMutableTransaction& mtx = *tx;

practicalswift commented at 4:52 pm on November 18, 2019:

Oh, good catch. Thanks!

in test/fuzz/test_runner.py:18 in ed94e0a167 outdated

11@@ -12,6 +12,10 @@
12 import subprocess
13 import logging
14 
15+# Fuzzers known to lack a seed corpus in https://github.com/bitcoin-core/qa-assets/tree/master/fuzz_seed_corpus
16+FUZZERS_MISSING_CORPORA = [
17+    "psbt",
18+]

MarcoFalke commented at 6:34 pm on November 15, 2019:

:eye:

practicalswift commented at 4:53 pm on November 18, 2019:

Removed!

MarcoFalke commented at 6:34 pm on November 15, 2019: member

ACK ed94e0a167505cf46ae98beb13babc496acf1eeb 🌳

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK ed94e0a167505cf46ae98beb13babc496acf1eeb 🌳
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgY6Qv6A5eczsPZ3gg2eLu1N/BofXKDUaRdN7oRm2zUQOvZXy+1uspKYs5rncZD
 8t2jcma6bLPYB1yYSZz1XGRpBR+I7F5f5oKVa6tig82MrDdI5HXqd4JyYn/10MvyJ
 9ln5hfGObv0S5xHsj8ut0wP7O6iahVtdKbTUBTSzLOsK8cZhOBYwGAhGR54i++71G
10+0kdqGPUfMBkEqbcLdGSsoGpVnZ2RAHBaX4N1u9UuFhNQlnNcQu0ogR6NrgjA2zk
11f6pWCSeZ1HgdAs3CpuWZupsp4jAYQ1GnVGB67Wr9swrhNAC2tZ0OaaEeFZ3KPP8J
12g0OkeidMtDXY4xZME7UvqS/XVuSrvCRIdl97BVKmg1UF0IEGUcCmwZYXGIJ3F+6P
134HvmQhMy0jEL0bWtIGXhiONaRgru3iI1yJmjcjx/qvKqUAOhC+LfmCkLtMAvUjQs
14q4fvyLQU4kAJz4UHSnMklnC87pJTwsCyy6gBHKvCCIbn9Uy1v90X9MKRawCxND6l
15V05fe33X
16=zQ9x
17-----END PGP SIGNATURE-----

Timestamp of file with hash 6cb134b5529a1cecaf9838fe5e9d153f0bbc035bbfa3db1bb4b21f0dcd6aef96 -

tests: Add fuzzing harness for various PSBT related functions 49f4c7f069

practicalswift force-pushed on Nov 18, 2019

practicalswift commented at 4:54 pm on November 18, 2019: contributor

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

MarcoFalke commented at 5:16 pm on November 18, 2019: member

re-ACK 49f4c7f0699e5e19ac6e41ef5b607392dd7a2983 🐟

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 49f4c7f0699e5e19ac6e41ef5b607392dd7a2983 🐟
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgzewwAyitA7w1F5QgES5+wQunGnKQ4JT865NoAoBWVFg41msIC0fcXP2V+i7ED
 8uAhV6hRv8J31ZnFfX6tI3/ySa9tfGC5dsTdZkwPp0yxSnXk6y3O/pA0iXONycipB
 9ilWMkPcVTopYYXwh4nMCZegTadcou3SaiGPJtDL+9xMusvSM9gVW0/D3nMKvUTXo
10yfVpsqSYXw9g6vkyi1OVOi4HNKyQSbjCxAwmWrIZ2zZvpDjQehO4l/jmqpmfFY5S
11eRmNIasWJ/NM0lhl2xotGSbgUKX9+w0vD8qEBt6nJRmFtIyMY9/98UfaUPhPzU/u
12nuNd8T/76wrsXW7oZYMJzC54S+ZRRb7alWCULPedb2FjY23T+v2FgHXXPFhQL+iz
13I2J8qtKaWkqfWvOphz+3yKY+x+9WtnMzOMShoR8RKDVj1Kds2z51tAolVQbbVmZt
14NHkKgYViZUhcjdtEuDBkEehj6gU0Hz02npA4Sid1N4p248SjiHfvsJhgHJxQJNiZ
152G4KrTli
16=NFnN
17-----END PGP SIGNATURE-----

Timestamp of file with hash 33b3bc228d3e4851067ee685da6a61b50c83146ed4d27450cc02cae21b16be1b -

practicalswift commented at 5:18 pm on November 18, 2019: contributor

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

MarcoFalke commented at 5:19 pm on November 18, 2019: member

@practicalswift I think you mean @jonatack ? :)

MarcoFalke referenced this in commit 30521302f9 on Nov 18, 2019

MarcoFalke merged this on Nov 18, 2019

MarcoFalke closed this on Nov 18, 2019

practicalswift commented at 5:23 pm on November 18, 2019: contributor

Oh, sorry! Same mistake again! :)

Thanks for merging!

jonatack commented at 5:26 pm on November 18, 2019: member

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

practicalswift commented at 5:35 pm on November 18, 2019: contributor

@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 commented at 5:38 pm on November 18, 2019: member

Thanks @practicalswift! :heart_eyes:

sidhujag referenced this in commit 1f87a08c74 on Nov 18, 2019

jasonbcox referenced this in commit ceda1dd005 on Jul 16, 2020

sidhujag referenced this in commit cabc6e14a7 on Nov 10, 2020

practicalswift deleted the branch on Apr 10, 2021

Munkybooty referenced this in commit 97e59a0653 on May 23, 2022

Munkybooty referenced this in commit 6fe14d0e82 on May 23, 2022

Munkybooty referenced this in commit 2e57dcd7e1 on May 23, 2022

Munkybooty referenced this in commit be03563038 on May 30, 2022

Munkybooty referenced this in commit 0fdb34b253 on Jun 7, 2022

Munkybooty referenced this in commit d24b34c48b on Jun 8, 2022

Munkybooty referenced this in commit c4d6a441c3 on Jun 16, 2022

Munkybooty referenced this in commit a1a904895f on Jun 21, 2022

Munkybooty referenced this in commit 13caee52e1 on Jun 21, 2022

DrahtBot locked this on Aug 16, 2022

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

Conflicts