Add a fuzzing harness for Tor

I have a branch for this, we held off on releasing it publicly because it made the underlying bug in legacy/trac#20384 (moved) obvious.

Now we've had time for people to upgrade to 0.2.8.9 (and various other patched versions), we can release the fuzzing code.

changed milestone to %Tor: 0.3.0.x-final in legacy/trac

added component::core tor/tor in Legacy / Trac fuzz in Legacy / Trac milestone::Tor: 0.3.0.x-final in Legacy / Trac owner::nickm in Legacy / Trac points::2 in Legacy / Trac priority::medium in Legacy / Trac resolution::implemented in Legacy / Trac review-group-15 in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac type::enhancement in Legacy / Trac labels

+1. Seriously, though, relays should upgrade, it's been more than a month now.

Given the conversation on #tor-dev, here's how I want to proceed:

• remove the test case for legacy/trac#20894 (moved) before posting the branch, (it can be regenerated in a few minutes using the fuzzer instructions, but that's ok),

Here's what I'm thinking of doing, but I could be persuaded either way:

• run the fuzzing code on a 24-core machine for a week, to see if we can shake loose any more bugs.

In any case, I want to have posted this code by Wednesday 14 December at the latest. I think it's been kept secret long enough.

Trac:
Status: new to needs_revision

I have pushed a fuzz-dir-v2 branch to my github.

Putting this in 0.3.0 so we can get it into master for the google fuzzer.

Trac:
Milestone: Tor: 0.3.??? to Tor: 0.3.0.x-final
Status: needs_revision to needs_review

I'm working on merging teor's harness with my harness for a combined harness. Thanks here!

Trac:
Status: needs_review to accepted
Owner: N/A to nickm

Trac:
Status: accepted to needs_revision

My combined-fuzzing branch is a work in progress at getting both fuzzers into a single system.

Also see a few extra commits on my fuzz-dir-extras branch. We might want to keep the test cases separate, as the directory entry point expects HTTP headers.

Let me know when you think the branch is ready for testing, and I will deploy it on a 20 core machine I have access to.

(It would be nice to have a valid HTTP request for each descriptor GET and POST, and then we could strip out the descriptor content for the descriptor entry point. I'm thinking relay, bridge, HSDir, etc. Perhaps adding debug logging then running one of the comprehensive chutney networks would be the way to go here.)

(And yes, the approach looks sensible. Thanks!)

Teor: I'm now up to combined-fuzzing-v3. I've pulled out the corpora, since they were getting unwieldy. I'm wondering if we should just have tiny seed corpora in the tor repo, and larger corpora kept separately.

Makes sense to me: I want to add a full dump of everything seen in a chutney network with all features active, so we should probably create a tor patch, chutney network, and build script for that.

Okay; I think this approach is ready for reviews.

Trac:
Status: needs_revision to needs_review

I've got this going in google's oss-fuzz infrastructure now. Let's see what explodes!

I opened legacy/trac#21179 (moved), legacy/trac#21180 (moved), and legacy/trac#21181 (moved) for the other fuzzing targets I could think of.

Trac:
Summary: Add a fuzzing harness for the Tor directory protocol to Add a fuzzing harness for Tor

Trac:
Keywords: N/A deleted, review-group-15 added

On nickm's combined-fuzzing-v3 branch, I see the following error when running make:

src/or/routerparse.c:3717:9: warning: implicit declaration of function ‘vote_routerstatus_free’ [-Wimplicit-function-declaration]
         vote_routerstatus_free(rs);
         ^
src/or/routerparse.c:3717:9: warning: nested extern declaration of ‘vote_routerstatus_free’ [-Wnested-externs]

It looks like this is due to vote_routerstatus_free being wrapped by#ifdef NETWORKSTATUS_PRIVATE in routerstatus.h, although I do see more errors when vote_routerstatus_free is defined...

I don't see this error on master (but I did rebase combined-fuzzing-v3 with master).

Trac:
Status: needs_review to needs_revision

oops; better now?

Trac:
Status: needs_revision to needs_review

Replying to nickm:

oops; better now?

Yep, now it looks good! I'll run the fuzzer and test cases as well, will report back.

Hello. I tried to test run the fuzzer but it seems like make fuzz does not exist on purpose but that's what Fuzzing.md is suggesting to use... Shouldn't we also include make fuzz? Otherwise how do we test this feature?

Also are we sure we want an src/or/dirsplit file? Maybe it should be shoved away in docs/.

Hi! I tried to improve the documentation.

"Make fuzz" works now, but it doesn't fuzz: as documented, it runs each of the fuzzers against its corpus. (You'll need to download the corpora.)

To actually fuzz, you need libfuzzer or afl installed. Those can be a little tricky; I'd recommend whichever you have good distribution support for.

I also added a fixup commit to remove src/or/dirsplit.

OK, I followed the guide and now it works fine. I fuzzed with fuzz-http and fuzz-consensus for a little while. I think this feature is ready to merge now.

BTW, I think the docs are a bit vague here:

To Run:
  mkdir -p src/test/fuzz/fuzz_http_findings
  ../afl/afl-fuzz -i ${TOR_FUZZ_CORPORA}/http -o src/test/fuzz/fuzz_http_findings -m <asan-memory-limit> -- src/test/fuzz_dir

since the last arg should be a fuzzing executable (like src/test/fuzz/fuzz-http) and not a non-existent directory (src/test/fuzz_dir).

Trac:
Status: needs_review to merge_ready

Thank you! Merged, and fixed the doc issue.

Trac:
Resolution: N/A to implemented
Status: merge_ready to closed

closed

changed time estimate to 16h

mentioned in issue legacy/trac#20894 (moved)

moved from legacy/trac#20893 (moved)

added Feature label and removed 1 deleted label

removed 1 deleted label