Opened 2 years ago

Closed 2 years ago

#20893 closed enhancement (implemented)

Add a fuzzing harness for Tor

Reported by: teor Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: fuzz, review-group-15
Cc: Actual Points:
Parent ID: Points: 2
Reviewer: Sponsor:

Description

I have a branch for this, we held off on releasing it publicly because it made the underlying bug in #20384 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.

Child Tickets

Change History (23)

comment:1 Changed 2 years ago by nickm

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

comment:2 Changed 2 years ago by teor

Status: newneeds_revision

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

  • remove the test case for #20894 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.

comment:3 Changed 2 years ago by teor

Milestone: Tor: 0.3.???Tor: 0.3.0.x-final
Status: needs_revisionneeds_review

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.

comment:4 Changed 2 years ago by nickm

Owner: set to nickm
Status: needs_reviewaccepted

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

comment:5 Changed 2 years ago by nickm

Status: acceptedneeds_revision

comment:6 Changed 2 years ago by nickm

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

comment:7 Changed 2 years ago by teor

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

comment:8 Changed 2 years ago by teor

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

comment:9 Changed 2 years ago by nickm

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.

comment:10 Changed 2 years ago by teor

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.

comment:11 Changed 2 years ago by nickm

Status: needs_revisionneeds_review

Okay; I think this approach is ready for reviews.

comment:12 Changed 2 years ago by nickm

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

comment:13 Changed 2 years ago by teor

I opened #21179, #21180, and #21181 for the other fuzzing targets I could think of.

comment:14 Changed 2 years ago by nickm

Summary: Add a fuzzing harness for the Tor directory protocolAdd a fuzzing harness for Tor

comment:15 Changed 2 years ago by nickm

Keywords: review-group-15 added

comment:16 Changed 2 years ago by chelseakomlo

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

comment:17 Changed 2 years ago by chelseakomlo

Status: needs_reviewneeds_revision

comment:18 Changed 2 years ago by nickm

Status: needs_revisionneeds_review

oops; better now?

comment:19 in reply to:  18 Changed 2 years ago by chelseakomlo

Replying to nickm:

oops; better now?

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

comment:20 Changed 2 years ago by asn

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/.

Last edited 2 years ago by asn (previous) (diff)

comment:21 Changed 2 years ago by nickm

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.

comment:22 Changed 2 years ago by asn

Status: needs_reviewmerge_ready

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

comment:23 Changed 2 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

Thank you! Merged, and fixed the doc issue.

Note: See TracTickets for help on using tickets.