fuzz: Add fuzzing harness for TorController by practicalswift · Pull Request #19288 · bitcoin/bitcoin

practicalswift · 2020-06-15T21:24:05Z

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

DrahtBot · 2020-06-16T09:52:06Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

torcontrol: Query Tor for correct -onion configuration #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.

laanwj · 2020-07-15T13:43:09Z

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.

Crypt-iQ

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:

bitcoin/src/torcontrol.cpp

Lines 643 to 652 in 9d4b3d8

    
           std::string torpassword = gArgs.GetArg("-torpassword", ""); 
        
           if (!torpassword.empty()) { 
        
               if (methods.count("HASHEDPASSWORD")) { 
        
                   LogPrint(BCLog::TOR, "tor: Using HASHEDPASSWORD authentication\n"); 
        
                   boost::replace_all(torpassword, "\"", "\\\""); 
        
                   _conn.Command("AUTHENTICATE \"" + torpassword + "\"", std::bind(&TorController::auth_cb, this, std::placeholders::_1, std::placeholders::_2)); 
        
               } else { 
        
                   LogPrintf("tor: Password provided with -torpassword, but HASHEDPASSWORD authentication is not available\n"); 
        
               } 
        
           } else if (methods.count("NULL")) {

src/test/fuzz/torcontrol.cpp

jonatack · 2020-08-14T16:56:03Z

Concept ACK

Crypt-iQ · 2020-08-26T09:41:18Z

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 · 2020-08-28T21:29:58Z

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:

/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')
#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
#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
#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
#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
#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
#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
#6 0x55db70ec5bfa in TorController::auth_cb(TorControlConnection&, TorControlReply const&) /root/bitcoin/src/torcontrol.cpp:411:31
#7 0x55db70ebb54b in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) /root/bitcoin/src/test/fuzz/torcontrol.cpp:66:28
#8 0x55db71583916 in LLVMFuzzerTestOneInput /root/bitcoin/src/test/fuzz/fuzz.cpp:45:5
#9 0x55db70e4b641 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/bitcoin/src/test/fuzz/torcontrol+0x649641)
#10 0x55db70e4ad85 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) (/root/bitcoin/src/test/fuzz/torcontrol+0x648d85)
#11 0x55db70e4d6a7 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/root/bitcoin/src/test/fuzz/torcontrol+0x64b6a7)
#12 0x55db70e4da09 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) (/root/bitcoin/src/test/fuzz/torcontrol+0x64ba09)
#13 0x55db70e3c6de in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/bitcoin/src/test/fuzz/torcontrol+0x63a6de)
#14 0x55db70e65522 in main (/root/bitcoin/src/test/fuzz/torcontrol+0x663522)
#15 0x7f13c3c4fb96 in __libc_start_main /build/glibc-2ORdQG/glibc-2.27/csu/../csu/libc-start.c:310
#16 0x55db70e11459 in _start (/root/bitcoin/src/test/fuzz/torcontrol+0x60f459)
SUMMARY: 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 · 2020-08-29T05:34:47Z

ACK 9b825840daf72f20444c2ec13c3aa681df8b594f

test/sanitizer_suppressions/ubsan

maflcko

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

src/test/fuzz/torcontrol.cpp

practicalswift · 2021-01-16T10:18:54Z

Updated to use the excellent CallOneOf!

Should be ready for final review :)

practicalswift · 2021-01-26T10:19:08Z

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 · 2021-01-26T15:13:04Z

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

$ git show --color-moved=dimmed_zebra bb643af8a1bc207935e9ae2c0fe61378ee935bde

practicalswift · 2021-02-28T17:59:08Z

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 · 2021-03-01T10:59:47Z

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

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

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

practicalswift · 2021-03-01T11:18:06Z

@laanwj

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

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

Oh, thanks for noticing!

Now addressed :)

…ler to improve testability

laanwj · 2021-03-03T10:39:52Z

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 10d4477

practicalswift force-pushed the fuzzers-torcontroller branch from a60eecc to 1c96a84 Compare June 15, 2020 22:06

DrahtBot added Build system P2P Tests labels Jun 15, 2020

practicalswift mentioned this pull request Jun 22, 2020

Add seeds bitcoin-core/qa-assets#17

Merged

DrahtBot mentioned this pull request Jun 30, 2020

net: Make DNS lookup mockable, add fuzzing harness #19415

Merged

DrahtBot mentioned this pull request Jul 11, 2020

torcontrol: Create also a V3 ed25519-V3 onion address. #19485

Closed

DrahtBot added the Needs rebase label Jul 11, 2020

practicalswift force-pushed the fuzzers-torcontroller branch from 1c96a84 to 38c5e21 Compare July 11, 2020 18:11

DrahtBot removed the Needs rebase label Jul 11, 2020

Crypt-iQ reviewed Jul 23, 2020

View reviewed changes

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

DrahtBot mentioned this pull request Aug 1, 2020

Replace hidden service with onion service #19638

Merged

DrahtBot added the Needs rebase label Aug 9, 2020

practicalswift force-pushed the fuzzers-torcontroller branch from 38c5e21 to 2c5f1b2 Compare August 14, 2020 16:53

DrahtBot removed the Needs rebase label Aug 14, 2020

practicalswift mentioned this pull request Aug 17, 2020

Intermittent CI failure: rpc_psbt.py --descriptors #19744

Closed

practicalswift force-pushed the fuzzers-torcontroller branch 2 times, most recently from c9d1559 to 9b82584 Compare August 18, 2020 18:04

DrahtBot removed the Needs rebase label Dec 16, 2020

practicalswift force-pushed the fuzzers-torcontroller branch from e4fcaae to 3f3982c Compare December 16, 2020 12:28

This was referenced Dec 17, 2020

Add I2P support using I2P SAM #20685

Merged

rpc: Replace boost::variant with std::variant for RPCArg.m_fallback #20736

Merged

DrahtBot added the Needs rebase label Jan 4, 2021

maflcko reviewed Jan 4, 2021

View reviewed changes

test/sanitizer_suppressions/ubsan Outdated Show resolved Hide resolved

practicalswift force-pushed the fuzzers-torcontroller branch from 3f3982c to bfdc60b Compare January 15, 2021 13:24

DrahtBot removed the Needs rebase label Jan 15, 2021

maflcko reviewed Jan 15, 2021

View reviewed changes

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

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

practicalswift force-pushed the fuzzers-torcontroller branch from bfdc60b to 730e3de Compare January 16, 2021 10:18

practicalswift force-pushed the fuzzers-torcontroller branch 2 times, most recently from 99e4f1e to 9b984fb Compare January 24, 2021 22:20

practicalswift force-pushed the fuzzers-torcontroller branch from 9b984fb to 936cad9 Compare March 1, 2021 11:16

DrahtBot added the Needs rebase label Mar 2, 2021

practicalswift added 2 commits March 2, 2021 12:21

torcontrol: Move TorControlReply, TorControlConnection and TorControl…

64219c0

…ler to improve testability

tests: Add fuzzing harness for TorController

10d4477

practicalswift force-pushed the fuzzers-torcontroller branch from 936cad9 to 10d4477 Compare March 2, 2021 12:26

DrahtBot removed the Needs rebase label Mar 2, 2021

laanwj merged commit dd8f474 into bitcoin:master Mar 3, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 3, 2021

Merge bitcoin#19288: fuzz: Add fuzzing harness for TorController

f763fc3

fanquake mentioned this pull request Mar 4, 2021

test: Move MakeNoLogFileContext to libtest_util, and use it in bench #21003

Merged

practicalswift deleted the fuzzers-torcontroller branch April 10, 2021 19:44

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

	std::string torpassword = gArgs.GetArg("-torpassword", "");
	if (!torpassword.empty()) {
	if (methods.count("HASHEDPASSWORD")) {
	LogPrint(BCLog::TOR, "tor: Using HASHEDPASSWORD authentication\n");
	boost::replace_all(torpassword, "\"", "\\\"");
	_conn.Command("AUTHENTICATE \"" + torpassword + "\"", std::bind(&TorController::auth_cb, this, std::placeholders::_1, std::placeholders::_2));
	} else {
	LogPrintf("tor: Password provided with -torpassword, but HASHEDPASSWORD authentication is not available\n");
	}
	} else if (methods.count("NULL")) {

Conversation

practicalswift commented Jun 15, 2020

Uh oh!

DrahtBot commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

laanwj commented Jul 15, 2020

Uh oh!

Crypt-iQ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Aug 14, 2020

Uh oh!

Crypt-iQ commented Aug 26, 2020

Uh oh!

Crypt-iQ commented Aug 28, 2020

Uh oh!

Crypt-iQ commented Aug 29, 2020

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Jan 16, 2021

Uh oh!

practicalswift commented Jan 26, 2021

Uh oh!

practicalswift commented Jan 26, 2021

Uh oh!

practicalswift commented Feb 28, 2021

Uh oh!

laanwj commented Mar 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Mar 1, 2021

Uh oh!

laanwj commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Jun 16, 2020 •

edited

Loading

laanwj commented Mar 1, 2021 •

edited

Loading

laanwj commented Mar 3, 2021 •

edited

Loading