fuzz: Add fuzzing harness for TorController

practicalswift commented at 9:24 pm on June 15, 2020: contributor

Add fuzzing harness for TorController.

See doc/fuzzing.md for information on how to fuzz Bitcoin Core. Don’t forget to contribute any coverage increasing inputs you find to the Bitcoin Core fuzzing corpus repo.

Happy fuzzing :)

practicalswift force-pushed on Jun 15, 2020

DrahtBot added the label Build system on Jun 15, 2020

DrahtBot added the label P2P on Jun 15, 2020

DrahtBot added the label Tests on Jun 15, 2020

DrahtBot commented at 9:52 am on June 16, 2020: 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:

#15423 (torcontrol: Query Tor for correct -onion configuration by luke-jr)

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.

DrahtBot added the label Needs rebase on Jul 11, 2020

practicalswift force-pushed on Jul 11, 2020

DrahtBot removed the label Needs rebase on Jul 11, 2020

laanwj commented at 1:43 pm on July 15, 2020: member

Concept ACK I don’t really like moving all kinds of internal details to the public header, but testability and encapsuation are always a bit at odds.

in src/test/fuzz/torcontrol.cpp:63 in 38c5e21d38 outdated

54+        }
55+        tor_control_reply.lines = ConsumeRandomLengthStringVector(fuzzed_data_provider);
56+        if (tor_control_reply.lines.empty()) {
57+            break;
58+        }
59+        DummyTorControlConnection dummy_tor_control_connection;

Crypt-iQ commented at 6:24 am on July 23, 2020:

Can’t this be replaced by TorControlConnection tor_control_connection{nullptr}; and then you can get rid of the DummyTorControlConnection class?

practicalswift commented at 4:46 pm on August 14, 2020:

We don’t want TorControlConnection::{Connect,Disconnect,Command} to be called, right? :) My intention with adding DummyTorControlConnection was to make sure {Connect,Disconnect,Command} return immediately (and thus be guaranteed to be safe regardless of the state of things).

Crypt-iQ commented at 5:16 pm on August 14, 2020:

Yes I think that’s the right approach, I think my coverage confused me again as it showed DummyTorControlConnection as unused.

Crypt-iQ commented at 6:44 am on July 23, 2020: contributor

48 hour coverage with libfuzzer here: https://crypt-iq.github.io/torcontrol_cov/src/torcontrol.cpp.gcov.html

Currently two functions are coverage lacking from the above output:

authchallenge_cb
protocolinfo_cb

authchallenge_cb coverage is a bit tricky to get coverage for because it has to pass the HMAC check. protocolinfo_cb just needs better seeds and it should be able to hit every part but this one: https://github.com/bitcoin/bitcoin/blob/9d4b3d86b694ac6e56495e1955f6bf5ff584cbb9/src/torcontrol.cpp#L643-L652

DrahtBot added the label Needs rebase on Aug 9, 2020

practicalswift force-pushed on Aug 14, 2020

jonatack commented at 4:56 pm on August 14, 2020: member

Concept ACK

DrahtBot removed the label Needs rebase on Aug 14, 2020

practicalswift force-pushed on Aug 17, 2020

practicalswift force-pushed on Aug 18, 2020

Crypt-iQ commented at 9:41 am on August 26, 2020: contributor

Runs without complaint on Ubuntu 18: ./configure --enable-fuzz --with-sanitizers=address,undefined,fuzzer CC=clang-10 CXX=clang++-10

Will build on macOS and report back.

Crypt-iQ commented at 9:29 pm on August 28, 2020: contributor

Builds on macOS: ./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined no actionable complaints ./configure --enable-fuzz --with-sanitizers=fuzzer,integer,undefined no actionable complaints

Ran with Ubuntu 18 again: ./configure --enable-fuzz --with-sanitizers=fuzzer,integer:

 0/usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/basic_string.tcc:1271:25: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::size_type' (aka 'unsigned long')
 1[#0](/bitcoin-bitcoin/0/) 0x55db7101bde3 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::rfind(char, unsigned long) const /usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/basic_string.tcc:1271:25
 2[#1](/bitcoin-bitcoin/1/) 0x55db7101bde3 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::find_last_of(char, unsigned long) const /usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/basic_string.h:2636:22
 3[#2](/bitcoin-bitcoin/2/) 0x55db7101bde3 in SplitHostPort(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, int&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) /root/bitcoin/src/util/strencodings.cpp:111:23
 4[#3](/bitcoin-bitcoin/3/) 0x55db70fc30f6 in Lookup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<CService, std::allocator<CService> >&, int, bool, unsigned int) /root/bitcoin/src/netbase.cpp:213:5
 5[#4](/bitcoin-bitcoin/4/) 0x55db70fc36ca in Lookup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, CService&, int, bool) /root/bitcoin/src/netbase.cpp:237:17
 6[#5](/bitcoin-bitcoin/5/) 0x55db70fc389b in LookupNumeric(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int) /root/bitcoin/src/netbase.cpp:262:9
 7[#6](/bitcoin-bitcoin/6/) 0x55db70ec5bfa in TorController::auth_cb(TorControlConnection&, TorControlReply const&) /root/bitcoin/src/torcontrol.cpp:411:31
 8[#7](/bitcoin-bitcoin/7/) 0x55db70ebb54b in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) /root/bitcoin/src/test/fuzz/torcontrol.cpp:66:28
 9[#8](/bitcoin-bitcoin/8/) 0x55db71583916 in LLVMFuzzerTestOneInput /root/bitcoin/src/test/fuzz/fuzz.cpp:45:5
10[#9](/bitcoin-bitcoin/9/) 0x55db70e4b641 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/bitcoin/src/test/fuzz/torcontrol+0x649641)
11[#10](/bitcoin-bitcoin/10/) 0x55db70e4ad85 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) (/root/bitcoin/src/test/fuzz/torcontrol+0x648d85)
12[#11](/bitcoin-bitcoin/11/) 0x55db70e4d6a7 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/root/bitcoin/src/test/fuzz/torcontrol+0x64b6a7)
13[#12](/bitcoin-bitcoin/12/) 0x55db70e4da09 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/root/bitcoin/src/test/fuzz/torcontrol+0x64ba09)
14[#13](/bitcoin-bitcoin/13/) 0x55db70e3c6de in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/bitcoin/src/test/fuzz/torcontrol+0x63a6de)
15[#14](/bitcoin-bitcoin/14/) 0x55db70e65522 in main (/root/bitcoin/src/test/fuzz/torcontrol+0x663522)
16[#15](/bitcoin-bitcoin/15/) 0x7f13c3c4fb96 in __libc_start_main /build/glibc-2ORdQG/glibc-2.27/csu/../csu/libc-start.c:310
17[#16](/bitcoin-bitcoin/16/) 0x55db70e11459 in _start (/root/bitcoin/src/test/fuzz/torcontrol+0x60f459)
18SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /usr/bin/../lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/basic_string.tcc:1271:25

Crypt-iQ commented at 5:34 am on August 29, 2020: contributor

ACK 9b825840daf72f20444c2ec13c3aa681df8b594f

practicalswift commented at 9:49 am on August 29, 2020: contributor

@Crypt-iQ Thanks for reviewing! I’ve now added relevant -fsanitize=integer suppressions (all non-UB). Please re-review :)

in test/sanitizer_suppressions/ubsan:77 in e87294918c outdated

77+implicit-integer-sign-change:util/strencodings.cpp
78+implicit-integer-sign-change:util/strencodings.h
79 implicit-integer-sign-change:validation.cpp
80 implicit-integer-sign-change:zmq/zmqpublishnotifier.cpp
81-implicit-signed-integer-truncation,implicit-integer-sign-change:chain.h
82-implicit-signed-integer-truncation,implicit-integer-sign-change:test/skiplist_tests.cpp

Crypt-iQ commented at 2:57 pm on August 29, 2020:

chain.h & test/skiplist_tests.cpp missing the implicit-signed-integer-truncation suppression in the new version. Then CI should pass.

practicalswift commented at 9:36 am on September 1, 2020:

Thanks! Should be fixed now.

in test/sanitizer_suppressions/ubsan:9 in e87294918c outdated

11@@ -12,6 +12,7 @@ float-divide-by-zero:wallet/wallet.cpp
12 # contains files in which we expect unsigned integer overflows to occur. The
13 # list is used to suppress -fsanitize=integer warnings when running our CI UBSan
14 # job.
15+unsigned-integer-overflow:*/include/c++/*/bits/basic_string.tcc

Crypt-iQ commented at 6:03 pm on August 29, 2020:

This will hide other unsigned int overflows that occur from different call sites. I think it should eventually be deleted, no?

practicalswift commented at 9:41 am on September 1, 2020:

AFAICT we can’t do anything meaningful about these unsigned integer wraparounds in basic_string.tcc. I think that suppressing them is fine: unsigned integer wraparound is not UB after all :)

practicalswift force-pushed on Sep 1, 2020

Crypt-iQ commented at 2:31 pm on September 5, 2020: contributor

ACK f87801d2fc0e27271aa71861979a8ef3e1fe935f

MarcoFalke removed the label Build system on Sep 6, 2020

practicalswift commented at 5:15 pm on September 12, 2020: contributor

Ready for merge? :)

jonatack commented at 5:28 pm on September 12, 2020: member

Oh, nice – will try to review this soon.

practicalswift commented at 1:22 pm on September 17, 2020: contributor

Had to rebase after the merge of 15c27c44417ab77a660b53b8574f7eb5261b19f8. @Crypt-iQ Thanks a lot for reviewing. May I ask you a re-review and hopefully give your third(!) ACK for this change? :)

practicalswift force-pushed on Sep 17, 2020

Crypt-iQ commented at 10:50 am on September 20, 2020: contributor

ACK 2a5c8188209cc6d6654ef3fefb80acbb87c134a1

DrahtBot added the label Needs rebase on Oct 2, 2020

practicalswift force-pushed on Oct 6, 2020

practicalswift commented at 7:41 pm on October 6, 2020: contributor

Had to rebase again due to a merge conflict. @Crypt-iQ Again: thanks a lot for your efforts in reviewing the fuzzing harnesses. May I ask you a re-review and hopefully give your fourth(!) ACK for this change? :)

Fuzzing review beg: If we only had two or three more reviewers (like @Crypt-iQ!) who were willing to review fuzzing harnesses that would make such a huge difference for the state of fuzzing in the project. Please consider reviewing! ❤️

Some fuzzing harnesses awaiting review:

#19065: tests: Add fuzzing harness for CAddrMan. Fill some fuzzing coverage gaps.
#19203: net: Add regression fuzz harness for CVE-2017-18350. Add FuzzedSocket. Add thin SOCKET wrapper.
#19259: tests: Add fuzzing harness for LoadMempool(…) and DumpMempool(…)
#19288: tests: Add fuzzing harness for TorController
#19415: net: Make DNS lookup mockable, add fuzzing harness
#19972: tests/net: Add fuzzing harness for node eviction logic. Make node eviction logic testable.

DrahtBot removed the label Needs rebase on Oct 6, 2020

practicalswift renamed this:
~~tests: Add fuzzing harness for TorController~~
test: Add fuzzing harness for TorController
on Nov 24, 2020

practicalswift renamed this:
~~test: Add fuzzing harness for TorController~~
fuzz: Add fuzzing harness for TorController
on Nov 24, 2020

Crypt-iQ commented at 3:44 pm on December 5, 2020: contributor

Will review again @practicalswift

Crypt-iQ commented at 10:32 am on December 14, 2020: contributor

ACK 00e48011c3e1235077353ca7d209661de66e9aab

Reviewed code and ran locally on macOS 10.15.4 with clang-10.

DrahtBot added the label Needs rebase on Dec 15, 2020

practicalswift force-pushed on Dec 16, 2020

DrahtBot removed the label Needs rebase on Dec 16, 2020

practicalswift force-pushed on Dec 16, 2020

DrahtBot added the label Needs rebase on Jan 4, 2021

in test/sanitizer_suppressions/ubsan:61 in 3f3982c4e1 outdated

57@@ -56,6 +58,7 @@ implicit-integer-sign-change:script/interpreter.cpp
58 implicit-integer-sign-change:serialize.h
59 implicit-integer-sign-change:test/arith_uint256_tests.cpp
60 implicit-integer-sign-change:test/coins_tests.cpp
61+implicit-integer-sign-change:test/fuzz/FuzzedDataProvider.h

MarcoFalke commented at 11:01 am on January 4, 2021:

what are the steps to reproduce this?

practicalswift commented at 1:23 pm on January 15, 2021:

I don’t think is needed now that we’ve updated FuzzedDataProvider.h to the most recent version. Suppression removed! :)

Crypt-iQ commented at 0:25 am on January 16, 2021:

test_runner.py on macOS, clang-11, --with-sanitizers=fuzzer,undefined,integer, commit 6d81d7aa87a15edd87175c33d1fd4a2accb3549d:

0test/fuzz/FuzzedDataProvider.h:211:49: runtime error: implicit conversion from type 'int' of value -2147483648 (32-bit, signed) to type 'unsigned long long' changed the value to 18446744071562067968 (64-bit, unsigned)
1SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior test/fuzz/FuzzedDataProvider.h:211:49 in 
2test/fuzz/FuzzedDataProvider.h:211:47: runtime error: unsigned integer overflow: 2147483647 - 18446744071562067968 cannot be represented in type 'unsigned long long'
3SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior test/fuzz/FuzzedDataProvider.h:211:47 in

practicalswift force-pushed on Jan 15, 2021

DrahtBot removed the label Needs rebase on Jan 15, 2021

in src/test/fuzz/torcontrol.cpp:60 in bfdc60bbed outdated

55+        tor_control_reply.lines = ConsumeRandomLengthStringVector(fuzzed_data_provider);
56+        if (tor_control_reply.lines.empty()) {
57+            break;
58+        }
59+        DummyTorControlConnection dummy_tor_control_connection;
60+        switch (fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 3)) {

MarcoFalke commented at 1:38 pm on January 15, 2021:

nit: CallOneOf?

practicalswift commented at 10:15 am on January 16, 2021:

Good point! Fixed!

in src/test/fuzz/torcontrol.cpp:54 in bfdc60bbed outdated

49+            tor_control_reply.code = 250;
50+        } else if (fuzzed_data_provider.ConsumeBool()) {
51+            tor_control_reply.code = 510;
52+        } else {
53+            tor_control_reply.code = fuzzed_data_provider.ConsumeIntegral<int>();
54+        }

MarcoFalke commented at 1:39 pm on January 15, 2021:

nit: Are the constants from our source code or from somewhere else? Like https://src-ref.docs.torproject.org/tor/control__cmd_8c_source.html ?

Not sure, but if you want, you can also use CallOneOf here:

0        CallOneOf (fuzzed_data_provider,
1        [&] {
2            tor_control_reply.code = 250;
3        } ,
4        [&]{
5            tor_control_reply.code = 510;
6        } ,
7        [&]{
8            tor_control_reply.code = fuzzed_data_provider.ConsumeIntegral<int>();
9        });

practicalswift commented at 10:17 am on January 16, 2021:

Good point. Now using CallOneOf.

250 and 510 are from our source:

0$ git grep reply.code
1src/test/fuzz/torcontrol.cpp:                tor_control_reply.code = 250;
2src/test/fuzz/torcontrol.cpp:                tor_control_reply.code = 510;
3src/test/fuzz/torcontrol.cpp:                tor_control_reply.code = fuzzed_data_provider.ConsumeIntegral<int>();
4src/torcontrol.cpp:    if (reply.code == 250) {
5src/torcontrol.cpp:    } else if (reply.code == 510) { // 510 Unrecognized command
6src/torcontrol.cpp:        LogPrintf("tor: Add onion failed; error code %d\n", reply.code);
7src/torcontrol.cpp:    if (reply.code == 250) {
8src/torcontrol.cpp:    if (reply.code == 250) {
9src/torcontrol.cpp:    if (reply.code == 250) {

MarcoFalke commented at 1:39 pm on January 15, 2021: member

haven’t reviewed, but it would probably be good to use the new style here

practicalswift force-pushed on Jan 16, 2021

practicalswift commented at 10:18 am on January 16, 2021: contributor

Updated to use the excellent CallOneOf!

Should be ready for final review :)

practicalswift force-pushed on Jan 24, 2021

practicalswift commented at 10:19 am on January 26, 2021: contributor

Compared to other fuzzing PRs this PR is relatively cumbersome to keep up to date over time due to the code move from torcontrol.cpp to torcontrol.h which must be kept in sync. Final review welcome :)

Current status:

Concept ACK

ACKs

@Crypt-iQ: counting four now stale ACK:s - thanks for being patient! :)

practicalswift commented at 3:13 pm on January 26, 2021: contributor

The first commit which is move-only is best viewed dimmed 🦓 style:

0$ git show --color-moved=dimmed_zebra bb643af8a1bc207935e9ae2c0fe61378ee935bde

practicalswift commented at 5:59 pm on February 28, 2021: contributor

This PR has one stale ACK and three Concept ACKs, and I believe all feedback has been addressed.

Ready for merge after re-review?

laanwj commented at 10:59 am on March 1, 2021: member

Sorry, forgot about this a bit. I’ll have a look.

Looks like this has a silent merge conflict on current master:

0…/bitcoin/src/test/fuzz/torcontrol.cpp:38:39: error: use of undeclared identifier 'MakeFuzzingContext'
1    static const auto testing_setup = MakeFuzzingContext<>();
2                                      ^
31 error generated.

practicalswift force-pushed on Mar 1, 2021

practicalswift commented at 11:18 am on March 1, 2021: contributor

@laanwj

Looks like this has a silent merge conflict on current master:

0…/bitcoin/src/test/fuzz/torcontrol.cpp:38:39: error: use of undeclared identifier 'MakeFuzzingContext'
1    static const auto testing_setup = MakeFuzzingContext<>();
2                                      ^
31 error generated.

Oh, thanks for noticing!

Now addressed :)

DrahtBot added the label Needs rebase on Mar 2, 2021

torcontrol: Move TorControlReply, TorControlConnection and TorController to improve testability 64219c01dc

tests: Add fuzzing harness for TorController 10d4477dae

practicalswift force-pushed on Mar 2, 2021

DrahtBot removed the label Needs rebase on Mar 2, 2021

laanwj commented at 10:39 am on March 3, 2021: member

I want to say this one more time before merging: I dislike moving internal implementation details to the header for testing purposes. It kind of violates encapsulation and makes the headers larger and less usefully self-documenting.

That said, I do not see any other solution in the immediate context here. A longer-term direction would be to have test-specific headers that expose internal functionality for unit testing[, benching, fuzzing] but are never included in production code.

ACK 10d4477dae663631411a1bd5a92e4fa941d3a96c

laanwj merged this on Mar 3, 2021

laanwj closed this on Mar 3, 2021

sidhujag referenced this in commit f763fc3eb5 on Mar 3, 2021

practicalswift deleted the branch on Apr 10, 2021

DrahtBot locked this on Aug 16, 2022

fuzz: Add fuzzing harness for TorController #19288

Conflicts