fuzz: BIP 30, CVE-2018-17144

maflcko commented at 7:24 pm on January 3, 2020: member

Add a validation fuzz test for BIP 30 and CVE-2018-17144

DrahtBot added the label Build system on Jan 3, 2020

DrahtBot added the label Tests on Jan 3, 2020

maflcko removed the label Build system on Jan 3, 2020

practicalswift commented at 9:36 pm on January 3, 2020: contributor

Super strong concept ACK: very happy to see more people interested in adding fuzzers. Very welcome! :)

Also: the habit of after fixing a security bug also implementing a fuzz target that could have found the bug is something I think we should strive for. I think @kcc (known for making C++ reasonably sane by introducing the sanitizers, libFuzzer, OSS-Fuzz, CFI, etc.) first coined the term “regression fuzzing” for that :)

A good example of “regression fuzzing” was this OOB read in libc++ <random> which not only was fixed but also got a couple of new fuzz targets added: https://bugs.chromium.org/p/chromium/issues/detail?id=994957

DrahtBot commented at 11:35 pm on January 3, 2020: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, mzumsande
Concept ACK	fanquake, jamesob
Stale ACK	practicalswift, jonatack, fjahr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27491 (refactor: Move chain constants to the util library by TheCharlatan)
#23352 (test: Extend stale_tip_peer_management test by MarcoFalke)
#18470 (test: Extend stale tip test by MarcoFalke)

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/bench/block_assemble.cpp:33 in fa3d9b79c1 outdated

29@@ -30,7 +30,7 @@ static void AssembleBlock(benchmark::State& state)
30     std::array<CTransactionRef, NUM_BLOCKS - COINBASE_MATURITY + 1> txs;
31     for (size_t b{0}; b < NUM_BLOCKS; ++b) {
32         CMutableTransaction tx;
33-        tx.vin.push_back(MineBlock(g_testing_setup->m_node, SCRIPT_PUB));
34+        tx.vin.push_back(CTxIn{MineBlock(g_testing_setup->m_node, SCRIPT_PUB)});

sanjaykdragon commented at 1:18 pm on January 7, 2020:

would emplace_back be more efficient / proper here?

in src/test/fuzz/utxo_total_supply.cpp:151 in fa3d9b79c1 outdated

134+            if (was_valid) {
135+                circulation += GetBlockSubsidy(ChainActive().Height(), Params().GetConsensus());
136+
137+                if (duplicate_coinbase_height == ChainActive().Height()) {
138+                    // we mined the duplicate coinbase
139+                    assert(current_block->vtx.at(0)->vin.at(0).scriptSig == duplicate_coinbase_script);

sanjaykdragon commented at 1:24 pm on January 7, 2020:

use .front() instead of .at(0)

maflcko commented at 6:43 pm on January 7, 2020:

why?

.at() asserts that the element exists, which I like

in src/test/util/mining.cpp:31 in fa3d9b79c1 outdated

24@@ -22,19 +25,62 @@ CTxIn generatetoaddress(const NodeContext& node, const std::string& address)
25     return MineBlock(node, coinbase_script);
26 }
27 
28-CTxIn MineBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
29+void ReGenerateCommitments(CBlock& block)
30+{
31+    CMutableTransaction tx{*block.vtx.at(0)};
32+    tx.vout.erase(tx.vout.begin() + GetWitnessCommitmentIndex(block));

sanjaykdragon commented at 2:01 pm on January 7, 2020:

use .front()

maflcko commented at 6:43 pm on January 7, 2020:

why?

.at() asserts that the element exists, which I like

sanjaykdragon commented at 8:06 pm on January 7, 2020:

why?

.at() asserts that the element exists, which I like

what do you mean by this?

sipa commented at 8:09 pm on January 7, 2020:

.at() will throw an exception when the vector is empty; front() will just cause UB.

sanjaykdragon commented at 8:44 pm on January 7, 2020:

.at() will throw an exception when the vector is empty; front() will just cause UB.

huh, did not know that. thanks for clarifying

Crypt-iQ commented at 1:12 am on January 8, 2020: contributor

This is kind of an aside, but thought I’d post here since this harness makes use of the ConsumeIntegralInRange function and I’m curious what others think.

I modified FuzzedDataProvider.h to ignore byte 48 so that afl-tmin could minimize better. afl-tmin zeros out redundant data with ascii ‘0’ (http://lcamtuf.coredump.cx/afl/technical_details.txt) - this is done to reduce the number of execs. Since the harness uses range (0-2) and all 256 possible fuzz bytes are mapped to this range, ignoring byte 48 should allow for better minimization if running afl-tmin on a corpus. However, with and without ignoring byte 48, afl-tmin was not able to minimize at all, but logically, it SHOULD have regardless of whether the byte was ignored or not (I can look into this more so that there’s more data on this). This is the only harness that used ConsumeIntegralInRange, so I wasn’t able to test afl-tmin in a meaningful way on any other harnesses. Thoughts on ignoring byte 48?

in src/test/fuzz/utxo_total_supply.cpp:96 in fa3d9b79c1 outdated

91+        // Mine the first block with this duplicate
92+        current_block = PrepareNextBlock();
93+        StoreLastTxo();
94+
95+        // Create duplicate (CScript should match exact format as in CreateNewBlock)
96+        CMutableTransaction tx{*current_block->vtx.back()};

instagibbs commented at 9:21 pm on January 9, 2020:

.front() for clarity that this is a coinbase tx?

maflcko commented at 11:27 pm on January 9, 2020:

Thx. Used front in both instances in this scope.

in src/test/fuzz/utxo_total_supply.cpp:97 in fa3d9b79c1 outdated

92+        current_block = PrepareNextBlock();
93+        StoreLastTxo();
94+
95+        // Create duplicate (CScript should match exact format as in CreateNewBlock)
96+        CMutableTransaction tx{*current_block->vtx.back()};
97+        duplicate_coinbase_script = CScript() << duplicate_coinbase_height << OP_0;

instagibbs commented at 9:34 pm on January 9, 2020:

can you remind the reader what the OP_0 push is for? I’ve forgotten as well, or cannot at least find the rule needed for it.

maflcko commented at 11:27 pm on January 9, 2020:

Padding to avoid bad-cb-length. Added comment

in src/test/fuzz/utxo_total_supply.cpp:89 in fa3d9b79c1 outdated

84+    assert(ChainActive().Height() == 0);
85+    // Get at which height we duplicate the coinbase
86+    int64_t duplicate_coinbase_height;
87+    CScript duplicate_coinbase_script;
88+    {
89+        // A chance of 1 in 20 for the duplicate coinbase to be within the first 100 blocks feels about right

instagibbs commented at 9:35 pm on January 9, 2020:

about right… for what? :)

maflcko commented at 11:29 pm on January 9, 2020:

Removed wording and added different doc comment.

The fuzzer mines shorter chains more often than longer ones, so it should be small. Though we also want it to be large sometimes, but not too large.

in src/test/fuzz/utxo_total_supply.cpp:88 in fa3d9b79c1 outdated

83+    UpdateUtxoStats();
84+    assert(ChainActive().Height() == 0);
85+    // Get at which height we duplicate the coinbase
86+    int64_t duplicate_coinbase_height;
87+    CScript duplicate_coinbase_script;
88+    {

instagibbs commented at 9:39 pm on January 9, 2020:

this section needs a description what it’s setting up for

maflcko commented at 11:29 pm on January 9, 2020:

Made scope shorter

maflcko force-pushed on Jan 9, 2020

practicalswift commented at 3:07 pm on January 10, 2020: contributor

Very nice fuzzing harness! Thanks for adding!

Tested ACK 5b90f46b0217af88dd3a88b028c2919361bdade5 modulo squash of last commit

Compared to the other fuzzers this one is very slow (6 executions/second), but I guess that is largely unavoidable given the code being tested :)

Also, a slow fuzzing harness is better than no fuzzing harness.

maflcko force-pushed on Jan 10, 2020

maflcko added the label Review club on Jan 10, 2020

maflcko commented at 3:40 pm on January 10, 2020: member

Compared to the other fuzzers this one is very slow (6 executions/second), but I guess that is largely unavoidable given the code being tested :)

Good question. This is actually a question in the review club next week: https://bitcoincore.reviews/17860.html

You (and all other reviewers) are invited to join and share your thoughts and ideas.

practicalswift commented at 4:12 pm on January 10, 2020: contributor

ACK 8b868f1741d37d97d09218093bc9434bbe5a749d

rajarshimaitra commented at 11:43 am on January 11, 2020: contributor

I am trying fuzzing utxo_total_supply with AFL and the fuzz is returning crash like this: [-] PROGRAM ABORT : Test case 'id:000000,orig:f66a2f2925ab9d377ea5c18ba941e7d1601b7509' results in a crash

One possible cause for crash is memory limit as reported by the fuzzer (52MB), and it suggests doing this: ( ulimit -Sv $[51 << 10]; /path/to/binary [...] <testcase )

While setting ulimit at [51«10] gives a SYSTEM ERROR : Unable to mmap file 'test/fuzz/utxo_total_supply', setting it to [51«100] (and for higher values) gives the crash back.

I am new to fuzzing, and am not very sure whats exactly happening here. Seems like its crashing for the very first test case itself. Would appreciate some guidance to navigate this.

I am at 8b868f174

practicalswift commented at 3:39 pm on January 13, 2020: contributor

@codeShark149 Does it work if you fuzz with libFuzzer instead?

Try this:

0$ make distclean
1$ ./autogen.sh
2$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
3$ make
4$ src/test/fuzz/utxo_total_supply

Crypt-iQ commented at 3:43 pm on January 13, 2020: contributor

@codeShark149 I fixed that error on macOS by turning AFL’s forkserver off export AFL_NO_FORKSRV=1. If AFL continually crashes on the initial input, then it’s probably because the binary isn’t being instrumented correctly.

practicalswift commented at 4:29 pm on January 13, 2020: contributor

I’ve played a bit more with this harness:

I’m currently using a seed corpus of 3247 files. I’m fuzzing at a rate of 6 execs/second which means that it takes almost ten minutes just to initialize libFuzzer with all seeds in a fuzzing session.

A lot of the time is spent setting up (and tearing down) the TestingSetup object for each input.

Can we think of ways to speed up this fuzzing harness?

Perhaps a fully featured TestingSetup object is not needed? Would limiting the setup to only the absolute minimum needed speed up things?

instagibbs commented at 4:32 pm on January 13, 2020: member

@practicalswift I suspected as much, that the testing harness is the heavy part of the test. On my non-beefy machine it looks like I’m getting 1 a second?

Alternative might be to allow it to run longer to build a longer chain?

practicalswift commented at 4:43 pm on January 13, 2020: contributor

Another data point:

As a crude way to quantify the impact of the test setup I made TestingSetup persistent across test inputs (static TestingSetup). That obviously invalidates all assertions (and thus makes the fuzzer meaningless), but the speed jumps to 32 exec/second.

sanjaykdragon commented at 5:30 pm on January 13, 2020: contributor

I’ve played a bit more with this harness:

I’m currently using a seed corpus of 3247 files. I’m fuzzing at a rate of 6 execs/second which means that it takes almost ten minutes just to initialize libFuzzer with all seeds in a fuzzing session.

A lot of the time is spent setting up (and tearing down) the TestingSetup object for each input.

Can we think of ways to speed up this fuzzing harness?

Perhaps a fully featured TestingSetup object is not needed? Would limiting the setup to only the absolute minimum needed speed up things?

You mind posting a profiler view of this? I’ll try to optimize this if I can. (VS has an optimization profiler section).

practicalswift commented at 10:05 pm on January 13, 2020: contributor

@sanjaykdragon I don’t have any profiler view to post :) The quoted numbers are from the output you’ll get when running utxo_total_supply with libFuzzer using the following steps

0$ make distclean
1$ ./autogen.sh
2$ CC=clang CXX=clang++ ./configure --enable-fuzz \
3      --with-sanitizers=address,fuzzer,undefined
4$ make
5$ src/test/fuzz/utxo_total_supply

rajarshimaitra commented at 3:18 pm on January 14, 2020: contributor

@codeShark149 Does it work if you fuzz with libFuzzer instead? @practicalswift yes libfuzzer can fuzz it.

@codeShark149 I fixed that error on macOS by turning AFL’s forkserver off export AFL_NO_FORKSRV=1. @Crypt-iQ thanks for suggesting but for some reason that did’nt work. The problem is simple though. The instrumented binary takes about 103 MB memory to run. So just had to change -m200 while running the fuzzer. Now its fuzzing with AFL.

Some note on execution: libfuzzer reported around 5 exec/sec. AFL reported 3.13/sec. Which is natural given the thing being tested. Any idea on how to make this thing run faster? I will try parallel but i doubt its gonna complete in any reasonable time.

in src/node/coinstats.cpp:35 in 8b868f1741 outdated

31@@ -32,8 +32,7 @@ static void ApplyStats(CCoinsStats &stats, CHashWriter& ss, const uint256& hash,
32     ss << VARINT(0u);
33 }
34 
35-//! Calculate statistics about the unspent transaction output set

jonatack commented at 5:41 pm on January 15, 2020:

Could squash this commit.

jonatack commented at 5:57 pm on January 15, 2020: contributor

ACK 8b868f1741d37d97d09218093bc9434bbe5a749d

Tested with libFuzzer, seeing 6 executions/second on a 2 year old Intel Core i7-6500U CPU 2.50GHz × 4 running Debian Linux.

Edit: Really slowing down as it runs into the thousands of iterations; showing 0 exec/s and making an execution every few seconds.

Am doing further testing with the CVE fix reverted. I’m not sure this test is effective enough.

fjahr commented at 0:54 am on January 17, 2020: contributor

Tested ACK 8b868f1

Reviewed code and successfully ran with AFL.

jonatack commented at 11:09 am on January 17, 2020: contributor

Now running since 2 1l2 days, taking > 1 gb of RAM, and it’s slowed to a crawl of < 10 executions per minute and still has not found the CVE. It also drains my laptop battery at least twice faster than normal.

0[#22184](/bitcoin-bitcoin/22184/)	REDUCE cov: 48346 ft: 261959 corp: 1540/3628Kb exec/s: 0 rss: 1020Mb L: 4077/4096 MS: 1 EraseBytes-
1[#22285](/bitcoin-bitcoin/22285/)	NEW    cov: 48346 ft: 261963 corp: 1541/3632Kb exec/s: 0 rss: 1020Mb L: 4096/4096 MS: 1 ChangeBit-
2[#22296](/bitcoin-bitcoin/22296/)	REDUCE cov: 48346 ft: 261963 corp: 1541/3632Kb exec/s: 0 rss: 1020Mb L: 838/4096 MS: 1 EraseBytes-
3[#22320](/bitcoin-bitcoin/22320/)	REDUCE cov: 48346 ft: 261963 corp: 1541/3632Kb exec/s: 0 rss: 1020Mb L: 4007/4096 MS: 4 ChangeBinInt-ChangeByte-ChangeBit-EraseBytes-
4[#22343](/bitcoin-bitcoin/22343/)	REDUCE cov: 48346 ft: 261963 corp: 1541/3632Kb exec/s: 0 rss: 1020Mb L: 1938/4096 MS: 3 InsertRepeatedBytes-ChangeByte-EraseBytes-
5[#22344](/bitcoin-bitcoin/22344/)	REDUCE cov: 48346 ft: 261963 corp: 1541/3632Kb exec/s: 0 rss: 1020Mb L: 169/4096 MS: 1 EraseBytes-
6[#22370](/bitcoin-bitcoin/22370/)	REDUCE cov: 48346 ft: 261963 corp: 1541/3632Kb exec/s: 0 rss: 1020Mb L: 3804/4096 MS: 1 CrossOver-

Maybe I’ve done something dumb in removing and rebuilding without the CVE fix:

0diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp
1@@ -36,12 +36,6 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
2     // Failure to run this check will result in either a crash or an inflation bug, depending on the implementation of
3     // the underlying coins database.
4-    std::set<COutPoint> vInOutPoints;
5-    for (const auto& txin : tx.vin) {
6-        if (!vInOutPoints.insert(txin.prevout).second)
7-            return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate");
8-    }

I’m not sure why the test runs continually slower. Unless my experience is atypical, this seems to be a real handicap.

This also underscores how necessary it is to run the fuzz tests continuously for them to be most effective.

maflcko commented at 11:06 pm on January 18, 2020: member

@jonatack The increase in memory usage and slow-down is clearly undesired. Which version of clang are you using? I presume 3.8? Could you try with https://packages.debian.org/stretch-backports/clang-6.0 , or even later, if possible?

Your patch to reintroduce the CVE looks correct.

jonatack commented at 12:13 pm on January 19, 2020: contributor

@MarcoFalke thanks, and good idea to verify the clang version. I’ve been using:

0$ clang --version
1clang version 6.0.0 (tags/RELEASE_600/final)
2Target: x86_64-unknown-linux-gnu
3Thread model: posix
4InstalledDir: /usr/local/bin

Which version are you running?

maflcko commented at 3:17 pm on January 19, 2020: member

I’ve been using Ubuntu Focal clang-9:

0$ clang --version 
1clang version 9.0.1-6build1 
2Target: x86_64-pc-linux-gnu

Also, clang-8 on Fedora 30.

Just some more questions:

The exec/s are 0, but the CPU usage is also low?
If you restart the fuzzer, do the exec/s start with a high value again?
What storage are you using hard drive/SSD?

DrahtBot added the label Needs rebase on Jan 20, 2020

fanquake referenced this in commit 20986a2ddf on Jan 21, 2020

fanquake commented at 1:12 am on January 21, 2020: member

Concept ACK.

I’ve been fuzzing this target using libFuzzer (Debian & Clang 9) and have hit the crashes. Took ~half a day of runtime. I’ve posted the container I was using, the crash inputs found and some setup notes here in core-review.

 0[#23497](/bitcoin-bitcoin/23497/)	RELOAD cov: 48632 ft: 251363 corp: 2318/93Kb lim: 192 exec/s: 2 rss: 683Mb
 1[#23557](/bitcoin-bitcoin/23557/)	NEW    cov: 48632 ft: 251369 corp: 2319/94Kb lim: 192 exec/s: 2 rss: 683Mb L: 176/192 MS: 2 ShuffleBytes-CopyPart-
 2utxo_total_supply: test/fuzz/utxo_total_supply.cpp:77: auto test_one_input(const std::vector<uint8_t> &)::(anonymous class)::operator()() const: Assertion "circulation == utxo_stats.nTotalAmount" failed.
 3==16247== ERROR: libFuzzer: deadly signal
 4    [#0](/bitcoin-bitcoin/0/) 0x561d1c715d91  (/bitcoin/src/test/fuzz/utxo_total_supply+0x208bd91)
 5    [#1](/bitcoin-bitcoin/1/) 0x561d1c6603c8  (/bitcoin/src/test/fuzz/utxo_total_supply+0x1fd63c8)
 6    [#2](/bitcoin-bitcoin/2/) 0x561d1c645b33  (/bitcoin/src/test/fuzz/utxo_total_supply+0x1fbbb33)
 7    [#3](/bitcoin-bitcoin/3/) 0x7f510df1551f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1351f)
 8    [#4](/bitcoin-bitcoin/4/) 0x7f510dc0b080  (/lib/x86_64-linux-gnu/libc.so.6+0x3a080)
 9    [#5](/bitcoin-bitcoin/5/) 0x7f510dbf6534  (/lib/x86_64-linux-gnu/libc.so.6+0x25534)
10    [#6](/bitcoin-bitcoin/6/) 0x7f510dbf640e  (/lib/x86_64-linux-gnu/libc.so.6+0x2540e)
11    [#7](/bitcoin-bitcoin/7/) 0x7f510dc03b91  (/lib/x86_64-linux-gnu/libc.so.6+0x32b91)
12    [#8](/bitcoin-bitcoin/8/) 0x561d1c7497c9  (/bitcoin/src/test/fuzz/utxo_total_supply+0x20bf7c9)
13    [#9](/bitcoin-bitcoin/9/) 0x561d1c746e2d  (/bitcoin/src/test/fuzz/utxo_total_supply+0x20bce2d)
14    [#10](/bitcoin-bitcoin/10/) 0x561d1c73f8ca  (/bitcoin/src/test/fuzz/utxo_total_supply+0x20b58ca)
15    [#11](/bitcoin-bitcoin/11/) 0x561d1c6470c1  (/bitcoin/src/test/fuzz/utxo_total_supply+0x1fbd0c1)
16    [#12](/bitcoin-bitcoin/12/) 0x561d1c646905  (/bitcoin/src/test/fuzz/utxo_total_supply+0x1fbc905)
17    [#13](/bitcoin-bitcoin/13/) 0x561d1c648ba7  (/bitcoin/src/test/fuzz/utxo_total_supply+0x1fbeba7)
18    [#14](/bitcoin-bitcoin/14/) 0x561d1c6498c5  (/bitcoin/src/test/fuzz/utxo_total_supply+0x1fbf8c5)
19    [#15](/bitcoin-bitcoin/15/) 0x561d1c6376a8  (/bitcoin/src/test/fuzz/utxo_total_supply+0x1fad6a8)
20    [#16](/bitcoin-bitcoin/16/) 0x561d1c660af2  (/bitcoin/src/test/fuzz/utxo_total_supply+0x1fd6af2)
21    [#17](/bitcoin-bitcoin/17/) 0x7f510dbf7bba  (/lib/x86_64-linux-gnu/libc.so.6+0x26bba)
22    [#18](/bitcoin-bitcoin/18/) 0x561d1c60af49  (/bitcoin/src/test/fuzz/utxo_total_supply+0x1f80f49)
23
24NOTE: libFuzzer has rudimentary signal handlers.
25      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
26SUMMARY: libFuzzer: deadly signal
27MS: 5 CrossOver-ShuffleBytes-ChangeASCIIInt-ChangeBit-EraseBytes-; base unit: d1a5c27d68809f2e9c739d76c69065ff17ae7d6f
280x8f,0x7a,0x5,0xcb,0x7a,0x2,0x2f,0x5,0x5, <trimmed> 0x89,0x89,0x89,0x89,0x0,0x0,0x0,
29\x8fz\x05\xcbz\x02/\x05\x052\x025 <trimmed> \x89\x89\x89\x89\x00\x00\x00
30artifact_prefix='./'; Test unit written to ./crash-9c947d9ff00fa36eca41ad27d337743fd5fee54b
31Base64: j3oFy3oCLwUFMgI1BQIFHS96ekF3uYB6QOLIyAcFjwABenoFZHEFBQFNIMpNBTMFPwXHuYB6CpWVBVBQenp6enpieoB6enoFBQUAplmmDQA9BQV6enoCj/j4+Pj4+Pj4+PgFBQUFBQVQUHp6j4+PjwUFBXpZenp6BVBQenqPj4+PBQUFell6enpZUFCqenp6Anp6eoB6AuZZUFBpenqPelCPeo+JiYmJiQAAAA==
32stat::number_of_executed_units: 23579
33stat::average_exec_per_sec:     2
34stat::new_units_added:          92
35stat::slowest_unit_time_sec:    0
36stat::peak_rss_mb:              683

jonatack commented at 3:26 pm on January 21, 2020: contributor

I’ve been using Ubuntu Focal clang-9:

0$ clang --version 
1clang version 9.0.1-6build1 
2Target: x86_64-pc-linux-gnu

Also, clang-8 on Fedora 30.

Just some more questions:

* The exec/s are 0, but the CPU usage is also low? 
* If you restart the fuzzer, do the exec/s start with a high value again?
* What storage are you using hard drive/SSD?

Thanks @MarcoFalke. Yes, the fuzz test runs faster after restarting before slowing down again. Storage is a 1To nvme 960Pro SSD. I’m building clang master from source and will try again.

maflcko commented at 8:04 pm on January 21, 2020: member

@jonatack clang-6 should be recent enough, no need to compile clang from source when later versions are not in your package manager. That could be a pain (or at least time consuming) to get libFuzzer and everything compiled.

maflcko force-pushed on Jan 21, 2020

maflcko commented at 8:32 pm on January 21, 2020: member

For anyone running into issues with slow executions. What happens if you start the fuzzer with export TMPDIR=/dev/shm?

instagibbs commented at 8:51 pm on January 21, 2020: member

@MarcoFalke significantly faster, something like 2x

DrahtBot removed the label Needs rebase on Jan 21, 2020

jonatack commented at 11:46 pm on January 21, 2020: contributor

Debian / Clang 6: Reproduced the crash with the CVE fix removed and @fanquake’s first crasher:

0$ src/test/fuzz/utxo_total_supply crash-9c947d9ff00fa36eca41ad27d337743fd5fee54b
1INFO: Seed: 38327181
2INFO: Loaded 1 modules   (511594 inline 8-bit counters): 511594 [0x55c2cba38958, 0x55c2cbab57c2), 
3INFO: Loaded 1 PC tables (511594 PCs): 511594 [0x55c2cbab57c8,0x55c2cc283e68), 
4src/test/fuzz/utxo_total_supply: Running 1 inputs 1 time(s) each.
5Running: crash-9c947d9ff00fa36eca41ad27d337743fd5fee54b
6utxo_total_supply: test/fuzz/utxo_total_supply.cpp:77: auto test_one_input(const std::vector<uint8_t> &)::(anonymous class)::operator()() const: Assertion `circulation == utxo_stats.nTotalAmount' failed.
7==14985== ERROR: libFuzzer: deadly signal
8.../...

And no crash (IIUC) with the CVE fix reinstated:

 0$ src/test/fuzz/utxo_total_supply -print_final_stats=1 crash-9c947d9ff00fa36eca41ad27d337743fd5fee54b
 1INFO: Seed: 1310013831
 2INFO: Loaded 1 modules   (512056 inline 8-bit counters): 512056 [0x557a2b535818, 0x557a2b5b2850), 
 3INFO: Loaded 1 PC tables (512056 PCs): 512056 [0x557a2b5b2850,0x557a2bd82bd0), 
 4src/test/fuzz/utxo_total_supply: Running 1 inputs 1 time(s) each.
 5Running: crash-9c947d9ff00fa36eca41ad27d337743fd5fee54b
 6Executed crash-9c947d9ff00fa36eca41ad27d337743fd5fee54b in 3056 ms
 7***
 8*** NOTE: fuzzing was not performed, you have only
 9***       executed the target code on a fixed set of inputs.
10***

Did this a few times as it is quick to run.

Now running export TMPDIR=/dev/shm ; time src/test/fuzz/utxo_total_supply -jobs=10 -print_final_stats=1 -workers=5 and letting it run overnight…

jonatack commented at 11:30 am on January 22, 2020: contributor

Stopping it for now after 12 hours, CPUs running at 100%, here is the bottom of one of the log files:

0[#11181](/bitcoin-bitcoin/11181/)	REDUCE cov: 48212 ft: 257522 corp: 1449/3276Kb exec/s: 0 rss: 878Mb L: 3969/4096 MS: 2 ChangeBit-EraseBytes-
1[#11298](/bitcoin-bitcoin/11298/)	NEW    cov: 48212 ft: 257523 corp: 1450/3279Kb exec/s: 0 rss: 878Mb L: 3302/4096 MS: 2 ChangeBinInt-InsertByte-

maflcko commented at 2:59 am on January 23, 2020: member

Now running export TMPDIR=/dev/shm ; time src/test/fuzz/utxo_total_supply -jobs=10 -print_final_stats=1 -workers=5 and letting it run overnight… @jonatack This is really confusing. One more try, if you don’t mind:

0mkdir /dev/shm/fuzz_temp_seeds
1export TMPDIR=/dev/shm ; time src/test/fuzz/utxo_total_supply /dev/shm/fuzz_temp_seeds

This will:

Run only one fuzzer to rule out interference from the other fuzzers
Specify a seed dir explicitly. This should put the seeds on the same RAM disk that the datadirs are placed in. (I am not sure what the behaviour of libFuzzer even is when you specify multiple workers, but no dir for the inputs)

Could you report back the initial exec/s, to see if they are larger than the 6 exec/s you got initially. I presume initially you ran on the SSD and /dev/shm should be a RAM disk and speed up disk io.

fanquake commented at 12:23 pm on January 23, 2020: member

I’ve tested using /dev/shm/ as the TMPDIR as you suggested, and am also seeing significant speedups. Added some additional notes here as using it with docker requires a larger than the default (only 64mb) /dev/shm.

jonatack commented at 12:29 pm on January 23, 2020: contributor

Thanks, will give it another good run with those suggestions.

maflcko commented at 1:00 pm on January 23, 2020: member

@fanquake Let me explain that the TMPDIR hack will only change the location of the datadir. Ideally, we can completely mock it out for fuzz tests (see #17990).

jonatack commented at 7:08 pm on January 24, 2020: contributor

@MarcoFalke Just launched as per your suggestion; here are the first 1200 iterations: https://gist.github.com/jonatack/fd81eecf390a6c750fd9c2dbd511caa8

It’s running slowly. I use LUKS encryption on the SSD, PureOS (https://www.pureos.net) variant of Debian, and Coreboot (https://www.coreboot.org). 32gb RAM, only 8 used. I’ll let it run overnight and see if it finds the CVE.

Edit: It seems to be running faster than before and sometimes shows 1/s instead of 0/s.

maflcko commented at 7:21 pm on January 24, 2020: member

From that log it looks like it already starts at 0 exec/s, so at least it is not slowing down over time. Sorry, I can’t find anything else to debug the slow executions. Long term you might want to hope for #17990.

maflcko commented at 7:23 pm on January 24, 2020: member

There was a rebase because of a conflict due to the makefile being resorted. Would be nice to get a re-ACK on the code from one of the earlier ACKers.

maflcko closed this on Jan 24, 2020

maflcko reopened this on Jan 24, 2020

jonatack commented at 10:00 am on January 25, 2020: contributor

After ~15 hours, less than 1 exec/minute. Most of the execs occured at the beginning (1200 in the first 30 minues) and it slowed dramatically after. RSS up to 1056Mb from 656Mb at the beginning.

0[#8192](/bitcoin-bitcoin/8192/)	pulse  cov: 52209 ft: 269435 corp: 1186/1943Kb exec/s: 0 rss: 1056Mb
1[#8213](/bitcoin-bitcoin/8213/)	NEW    cov: 52209 ft: 269460 corp: 1187/1947Kb exec/s: 0 rss: 1056Mb L: 4096/4096 MS: 4 ChangeBit-CMP-ChangeASCIIInt-ChangeBit- DE: "T\x00\x00\x00\x00\x00\x00\x00"-
2[#8252](/bitcoin-bitcoin/8252/)	NEW    cov: 52209 ft: 269462 corp: 1188/1950Kb exec/s: 0 rss: 1056Mb L: 3101/4096 MS: 4 InsertByte-ChangeByte-ChangeBit-InsertByte-
3[#8338](/bitcoin-bitcoin/8338/)	NEW    cov: 52209 ft: 269463 corp: 1189/1953Kb exec/s: 0 rss: 1056Mb L: 2376/4096 MS: 1 CopyPart-
4[#8387](/bitcoin-bitcoin/8387/)	NEW    cov: 52210 ft: 269464 corp: 1190/1956Kb exec/s: 0 rss: 1056Mb L: 3219/4096 MS: 4 ShuffleBytes-CrossOver-ChangeBinInt-EraseBytes-
5[#8418](/bitcoin-bitcoin/8418/)	NEW    cov: 52210 ft: 269473 corp: 1191/1960Kb exec/s: 0 rss: 1056Mb L: 4096/4096 MS: 1 PersAutoDict- DE: "\x00\x00\x00\x00\x00\x00\x00U"-
6[#8429](/bitcoin-bitcoin/8429/)	NEW    cov: 52210 ft: 269526 corp: 1192/1960Kb exec/s: 0 rss: 1056Mb L: 63/4096 MS: 1 EraseBytes-
7[#8479](/bitcoin-bitcoin/8479/)	REDUCE cov: 52210 ft: 269526 corp: 1192/1960Kb exec/s: 0 rss: 1056Mb L: 311/4096 MS: 5 CMP-EraseBytes-EraseBytes-ChangeBinInt-InsertRepeatedBytes- DE: "\x99&aS\xbb\xb9\xa5\xce\xcf\xd1\xb2\xd2\xc9#\xc5\xe9S\x7f^\x8cn\x8c.@\xf0x\xce\xf2\xbb\x03QW"-

maflcko commented at 10:34 pm on January 27, 2020: member

@jonatack Would you mind debugging why it takes so long in later stages? I don’t know if libFuzzer-instrumented binaries can be instrumented with other frameworks, but maybe a simple std::cout in a few places would be sufficient to see where it is hanging?

maflcko referenced this in commit 3b5b276734 on Jan 29, 2020

DrahtBot added the label Needs rebase on Mar 11, 2020

practicalswift commented at 5:44 am on June 17, 2020: contributor

Would be happy to re-review after rebase :)

practicalswift commented at 9:11 pm on November 11, 2020: contributor

Needs rebase :)

practicalswift commented at 8:25 pm on February 28, 2021: contributor

@MarcoFalke, are you planning to continue work on this PR? I’d like to review it once rebased :)

maflcko force-pushed on Jul 24, 2022

DrahtBot removed the label Needs rebase on Jul 24, 2022

maflcko force-pushed on Jul 25, 2022

DrahtBot added the label Needs rebase on Aug 30, 2022

maflcko force-pushed on Dec 12, 2022

DrahtBot removed the label Needs rebase on Dec 12, 2022

fanquake requested review from dergoegge on Feb 6, 2023

dergoegge commented at 4:40 pm on February 6, 2023: member

Concept ACK

achow101 commented at 4:10 pm on April 25, 2023: member

Are you still working on this?

achow101 requested review from mzumsande on Apr 25, 2023

achow101 requested review from darosior on Apr 25, 2023

maflcko force-pushed on May 2, 2023

test: Add util to mine invalid blocks

With the current utils it is only possible to mine valid blocks. This
commit adds new util methods to mine invalid blocks.

fa846ee074

maflcko force-pushed on May 2, 2023

DrahtBot added the label CI failed on May 2, 2023

Avoid dereferencing interruption_point if it is nullptr fa26e3462a

maflcko force-pushed on May 2, 2023

jamesob commented at 6:25 pm on May 2, 2023: member

Concept ACK, will review this week

maflcko force-pushed on May 2, 2023

DrahtBot removed the label CI failed on May 2, 2023

in src/test/fuzz/utxo_total_supply.cpp:119 in fa18fe3976 outdated

114+    assert(ActiveHeight() == 1);
115+    UpdateUtxoStats();
116+    current_block = PrepareNextBlock();
117+    StoreLastTxo();
118+
119+    while (fuzzed_data_provider.remaining_bytes()) {

dergoegge commented at 10:20 am on May 3, 2023:

LIMITED_WHILE ?

maflcko commented at 11:36 am on May 5, 2023:

thx, done

in src/test/fuzz/utxo_total_supply.cpp:23 in fa18fe3976 outdated

18+#include <version.h>
19+
20+FUZZ_TARGET(utxo_total_supply)
21+{
22+    /** The testing setup that creates a chainstate and other globals */
23+    TestingSetup test_setup{

dergoegge commented at 9:31 am on May 4, 2023:

Maybe we create a new setup type that doesn’t have all components? (peerman, addrman, banman etc are not needed in this test)

maflcko commented at 11:35 am on May 5, 2023:

Nice. I get “number go down” from 94k to 90k in the last force push with steps to reproduce:

0FUZZ=utxo_total_supply /usr/bin/time --f='%M'  ./src/test/fuzz/fuzz -runs=1 -- --printtoconsole=0

dergoegge commented at 12:42 pm on May 4, 2023: member

Tested ACK fa18fe3976a0f99480ce42dc0c1df7143967bf4d

I reintroduced CVE-2018-17144 and the fuzz target found the bug after ~1 day on 10 cores.

 0[#2133](/bitcoin-bitcoin/2133/)   NEW    cov: 8553 ft: 28450 corp: 659/48Kb lim: 124 exec/s: 19 rss: 110Mb L: 69/124 MS: 2 PersAutoDict-ChangeASCIIInt- DE: "\377\377\377>"-
 1[#2151](/bitcoin-bitcoin/2151/)   NEW    cov: 8553 ft: 28451 corp: 660/48Kb lim: 124 exec/s: 19 rss: 110Mb L: 93/124 MS: 3 CrossOver-EraseBytes-CopyPart-
 2[#2153](/bitcoin-bitcoin/2153/)   NEW    cov: 8553 ft: 28452 corp: 661/48Kb lim: 124 exec/s: 19 rss: 110Mb L: 121/124 MS: 2 ChangeBinInt-ShuffleBytes-
 3[#2158](/bitcoin-bitcoin/2158/)   NEW    cov: 8553 ft: 28463 corp: 662/49Kb lim: 124 exec/s: 19 rss: 110Mb L: 124/124 MS: 5 ShuffleBytes-ChangeBit-ChangeBinInt-ShuffleBytes-PersAutoDict- DE: "\002\000\000\000"-
 4[#2173](/bitcoin-bitcoin/2173/)   NEW    cov: 8553 ft: 28464 corp: 663/49Kb lim: 124 exec/s: 19 rss: 110Mb L: 77/124 MS: 5 ChangeBinInt-CrossOver-ChangeBit-InsertRepeatedBytes-ChangeByte-
 5[#2174](/bitcoin-bitcoin/2174/)   NEW    cov: 8553 ft: 28465 corp: 664/49Kb lim: 124 exec/s: 19 rss: 110Mb L: 99/124 MS: 1 ChangeBinInt-
 6fuzz: test/fuzz/utxo_total_supply.cpp:84: auto utxo_total_supply_fuzz_target(FuzzBufferType)::(anonymous class)::operator()() const: Assertion `circulation == utxo_stats.total_amount' failed.
 7==16109== ERROR: libFuzzer: deadly signal
 8    [#0](/bitcoin-bitcoin/0/) 0x55dc406d0314 in __sanitizer_print_stack_trace (/root/bitcoin/src/test/fuzz/fuzz+0x9c1314) (BuildId: 2dbb839c5abc29bd1bf44537252b5378dcc49df4)
 9    [#1](/bitcoin-bitcoin/1/) 0x55dc406a75d7 in fuzzer::PrintStackTrace() (/root/bitcoin/src/test/fuzz/fuzz+0x9985d7) (BuildId: 2dbb839c5abc29bd1bf44537252b5378dcc49df4)
10    [#2](/bitcoin-bitcoin/2/) 0x55dc4068cc33 in fuzzer::Fuzzer::CrashCallback() (/root/bitcoin/src/test/fuzz/fuzz+0x97dc33) (BuildId: 2dbb839c5abc29bd1bf44537252b5378dcc49df4)
11    [#3](/bitcoin-bitcoin/3/) 0x7fd78765472f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1272f) (BuildId: 1545d6e6c7da9e2e706253a30cbfe720b9101e62)

Minimized input in base64: zhoHDwcH+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4m5ubm/gI+Pj4+Pj4m5ub mwj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+Pj4+A==

Move LoadVerifyActivateChainstate to ChainTestingSetup faae7d5c00

maflcko force-pushed on May 5, 2023

fuzz: BIP 42, BIP 30, CVE-2018-17144 fa2d8b61f9

maflcko force-pushed on May 5, 2023

dergoegge approved

dergoegge commented at 2:01 pm on May 5, 2023: member

Code review ACK fa2d8b61f9343d350b67357a12f39b613c8ee8ad

in src/test/fuzz/utxo_total_supply.cpp:150 in fa2d8b61f9

145+                if (was_valid) {
146+                    circulation += GetBlockSubsidy(ActiveHeight(), Params().GetConsensus());
147+
148+                    if (duplicate_coinbase_height == ActiveHeight()) {
149+                        // we mined the duplicate coinbase
150+                        assert(current_block->vtx.at(0)->vin.at(0).scriptSig == duplicate_coinbase_script);

mzumsande commented at 7:51 pm on May 5, 2023:

why is this check inside the was_valid branch? As far as I understand it, the second block with the duplicate coinbase could be valid (if the coinbase from block 1 was spent), or BIP-30 invalid (otherwise) - but this should hold regardless.

maflcko commented at 12:13 pm on May 8, 2023:

Yes, could move outside

mzumsande commented at 8:32 pm on May 5, 2023: contributor

Tested ACK fa2d8b61f9343d350b67357a12f39b613c8ee8ad

I was also able to catch the reintroduced CVE-2018-17144 (with the previous commit fa18fe3976a0f99480ce42dc0c1df7143967bf4d, current one is still running).

I can’t really see how this actually fuzzes BIP42. Wouldn’t the fuzzer need to create millions of blocks to detect it, so if we’d reintroduce that bug, the fuzzer would never be able to find it in practice?

Finally, I also observed that the fuzzer gets slower with time and after a few hours creates inputs that will take several seconds to run. That seems unavoidable, because the chains that are created are getting longer, and each block must be mined at a small but non-zero difficulty. However, it means that we should probably keep an eye on the qa-corpus seeds once this gets merged.

maflcko renamed this:
~~fuzz: BIP 42, BIP 30, CVE-2018-17144~~
fuzz: BIP 30, CVE-2018-17144
on May 6, 2023

maflcko commented at 7:17 am on May 6, 2023: member

I can’t really see how this actually fuzzes BIP42.

Yeah, it was mostly a joke. Now with LIMITED_WHILE this is no longer possible to fuzz at all.

fanquake merged this on May 6, 2023

fanquake closed this on May 6, 2023

sidhujag referenced this in commit 89dd916e48 on May 7, 2023

maflcko commented at 12:08 pm on May 8, 2023: member

Finally, I also observed that the fuzzer gets slower with time and after a few hours creates inputs that will take several seconds to run. That seems unavoidable, because the chains that are created are getting longer, and each block must be mined at a small but non-zero difficulty. However, it means that we should probably keep an eye on the qa-corpus seeds once this gets merged.

I guess LIMITED_WHILE can be reduced so much that it can catch the CVE only. For BIP 42 and all other purposes, it might be better to add a new fuzz target for that specifically, maybe using a fuzzed assumeutxo state?

maflcko deleted the branch on May 8, 2023

fanquake referenced this in commit 2a786ea349 on May 31, 2023

sidhujag referenced this in commit 2f06d5bef1 on May 31, 2023

bitcoin locked this on May 7, 2024

fuzz: BIP 30, CVE-2018-17144 #17860

Reviews

Conflicts