Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AFL-based fuzzing setup for network modules #13157

Merged
merged 9 commits into from
Apr 18, 2020

Conversation

nmeum
Copy link
Member

@nmeum nmeum commented Jan 17, 2020

Contribution description

This pull requests proposes a simple framework for fuzzing RIOT network
applications. Inspired by the examples/ subdirectory it adds an
additional directory called fuzzing/ with applications for fuzzing
certain network modules. The fuzzing setup is based on Michal
Zalewski's AFL fuzzer.

AFL generates random network packets from an input corpus and writes the
generated data to standard input. As such, each application in fuzzing/
reads from standard input, converts the data to a gnrc_pktsnip_t, and
passes it to the network module under test using RIOT's message passing
mechanism.

The fuzzing applications are terminated as soon as the RIOT network
module under test finished processing the input data (fuzzing packet).
This is achieved differently for applications using GNRC directly and
applications using the sock interface. For the former, RIOT is
terminated using exit(3) as soon as the generated fuzzing
packet is passed to gnrc_pktbuf_release. For the latter, it is
assumed that gnrc_sock_recv is called in a loop, if it is
called again and the previously returned packet equals the
fuzzing packet, RIOT is terminated using exit(3).

I have written fuzzing applications for different modules and discovered
various bugs using these fuzzing applications (see related issues
below). This PR only contains a fuzzing applications for gnrc_tcp.
Additional applications have been omitted for now to ease code review.
If desired, I will submit those separately later.

Needless to say, the fuzzing setup only works on native currently.
Besides, it only sends a single packet and does not fuzz any sending
functions, only receiving functions. I don't think that this setup is
perfect as is, but is definitely usable in the sense that it finds
existing bugs. In my opinion, it is definitely superior to testing
network applications with hardcoded "malcious" packets as done currently
(e.g. #12369), if at all.

Testing procedure

Usage instructions are provided in fuzzing/README.md. The best way to
test the gnrc_tcp fuzzing application and the fuzzing setup in
general is reverting some of the fixes listed below and checking whether
the fuzzing setup finds the underlying bugs again.

For instance:

$ git revert 9e91d21625867f73825c3fc61588749118ad1a6f 0fff1b35ecc06bb2ff7971ebefa4d590ca5c5b7b
$ make -C fuzzing/gnrc_tcp all-asan
$ make -C fuzzing/gnrc_tcp fuzz

At some point AFL will find a crash, keep in mind that this might take
a while though.

Issues/PRs references

The following lists issues found using the proposed fuzzing setup.

gnrc_tftp

gnrc_tcp

asymcute

emcute

dtls-sock


@miri64 you were involved in most issues listed above. Do you have any
though on the changes proposed here?

@miri64 miri64 added Area: tests Area: tests and testing framework Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jan 18, 2020
@nmeum nmeum force-pushed the pr/fuzzing_tcp_only branch 2 times, most recently from 5e07da3 to a405eb9 Compare January 20, 2020 14:08
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Jan 21, 2020
@jvijtiuk
Copy link

Hello,

thank you for your contribution nmeum, I'm pretty happy to see that
someone has managed to setup AFL for RIOT fuzzing.

I'm not sure if this is the best place to ask, so I'm sorry for any potential inconvenience. If an issue would be more appropriate I will open one. Would there be any interest in integrating LLVM's LibFuzzer [1] fuzzing tests into RIOT too?

I have been working on a paper about fuzzing, and have recently
started fuzzing RIOT OS with LibFuzzer. I was considering AFL, but
opted for LibFuzzer as a starting point as it was easier and quicker
to set up than AFL.

The advantage of LibFuzzer is that it is quicker to set up and fuzz
single functions, but somewhat harder to fuzz a whole range of interleaved functions
like in this PR, which is obviously a disadvantage at the
same time, as fuzzing single functions does not, in my experience,
find bugs that are hidden deeper in the code like AFL did in this
case. So far fuzzing with LibFuzzer hasn't resulted in any issues in RIOT.

[1] https://llvm.org/docs/LibFuzzer.html

@miri64
Copy link
Member

miri64 commented Jan 23, 2020

@jvijtiuk if you already have something, go right ahead and make a PR. If not, I recommend an issue as a feature request, and "claiming" it, to prevent other people don't start working on it, while you do your work. In general I think having multiple tools doing the same thing is a good thing, as they might put the focus on different things.

@benpicco benpicco removed the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Feb 17, 2020
@nmeum
Copy link
Member Author

nmeum commented Mar 24, 2020

@benpicco Should I address your comments right away or is there more to come? I just want to avoid spending time on documenting something which doesn't end up being merged in the end. Is there a general interest in having a fuzzing setup? I don't mind if there isn't but this hasn't really received much attention which makes me think that there is not much interest in having something like this integrated into upstream RIOT.

@benpicco
Copy link
Contributor

benpicco commented Mar 24, 2020

There is definitely interest in fuzzing!
Sorry if it sometimes takes a while to respond, we only have a limited maintainer capacity.

Fuzzing is definitely a feature we want - as you have proven with all the fuzzing based bugs you discovered. Thank you a lot for these! (And I think you generally got pretty swift responses there 😉)

It's just that new, larger features take a bit more time to look at, and then sometimes PR are just forgotten. You can always ping maintainers if you feel your PR got swapped out of attention.
I agree that it can be frustrating when there is no consensus on a feature and the discussion just fizzles out instead of coming to a conclusion.

Now with this I think it's pretty good. It's self contained and provides a valuable feature.
But it really lacks a good description on how to use it.

This adds a utility module which is used to write applications for
fuzzing RIOT network modules. The module provides a dummy network
interface which is configured with a static IPv6 addresses for modules
which perform operations on the underlying network interface. Besides,
it contains a utility function for transforming data received on
standard input into a `gnrc_pktsnip_t`.
Since RIOT is an operating system the native binary will never terminate
[0]. The termination condition for fuzzing GNRC is that the packet was
handled by the network stack and therefore freed. If it is never freed
we will deadlock meaning a memory leak was found, afl should be able to
detect this through timeouts.

This is currently only supported for gnrc_pktbuf_malloc since this is
the pktbuf implementation I used for fuzzing. Implementing this in
pktbuf.h is not possible.

[0]: Except NATIVE_AUTO_EXIT is defined, however, even with that define
set RIOT will only terminate when all threads terminated. Unfortunately,
gnrc_udp and other network threads will never terminate.
The termination condition implemented in gnrc_pktbuf_malloc does not
work when using the sock interface as sock copies packet data to a local
buffer and frees the packet afterwards. As such, the fuzzing application
would exit before performing any input processing.

For this reason, the termination condition in gnrc_pktbuf_malloc is
disabled when using sock. Instead, the application terminates if
gnrc_sock_recv previously returned the fuzzing packet. The underlying
assumption of this implementation is that gnrc_sock_recv is called in a
loop.
@nmeum
Copy link
Member Author

nmeum commented Apr 7, 2020

Rebased and made some adjustments for recent API changes of gnrc_netif_raw_create and gnrc_tcp_open_passive. Did some brief testing, still seems to work as expected.

@@ -747,6 +747,11 @@ test-input-hash:
true
endif

.PHONY: fuzz
fuzz:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fuzz:
fuzz: all-asan

So make fuzz will always be enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is theoretically possible, I didn't do this initially because I did not want to enforce usage of the address sanitizer. For instance, to allow instrumentation with other sanitizers (e.g. UBSAN). Besides, some bugs (memory leaks) can also be found without using any sanitizer at all as AFL itself detects a hang in these cases (e.g. #12001).

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally got around to testing this - work like a charm.
Just one minor comment: Can we make the fuzz target depend on all-asan so the user doesn't accidentally fuzz an old binary?

@nmeum
Copy link
Member Author

nmeum commented Apr 17, 2020

It would also be nice to add the fuzzing/ directory to the murdock setup somehow, i.e. compile all fuzzing applications for the native target with murdock. For example, to make sure that the fuzzing applications are also adjusted when function APIs are changed. My knowledge of the RIOT CI setup is unfortunately insufficient for implementing this.

@benpicco
Copy link
Contributor

benpicco commented Apr 17, 2020

It would also be nice to add the fuzzing/ directory to the murdock setup somehow, i.e. compile all fuzzing applications for the native target with murdock.

I agree - I think adding the directory to makefiles/app_dirs.inc.mk should be enough though.

@nmeum
Copy link
Member Author

nmeum commented Apr 17, 2020

I agree - I think adding the directory to makefiles/app_dirs.inc.mk should be enough though.

Kind of, murdock seems to attempt building the applications now but it seems to fail for some reason (maybe missing makefile targets?):

-- running on worker mobi7.inet.haw-hamburg.de thread 10, build number 8365.
./.murdock: compile: error: board directory "boards/Nothing" doesn't exist

-- running on worker mobi8.inet.haw-hamburg.de thread 4, build number 21763.
./.murdock: compile: error: board directory "boards/to" doesn't exist

and more similar messages. Any idea how to fix this?

@benpicco
Copy link
Contributor

Maybe this is caused by a lack of afl-gcc on Murdock?

@nmeum
Copy link
Member Author

nmeum commented Apr 17, 2020

Maybe this is caused by a lack of afl-gcc on Murdock?

Don't think so, seems to me the CI sets TOOLCHAIN=gnu anyhow and thus it shouldn't use AFL. Maybe @kaspar030 can shine some light on this?

@miri64
Copy link
Member

miri64 commented Apr 17, 2020

Don't think so, seems to me the CI sets TOOLCHAIN=gnu anyhow and thus it shouldn't use AFL. Maybe @kaspar030 can shine some light on this?

There are also some TOOLCHAIN=llvm tests. But this does not seem to be the problem here. It seems some output in your application is breaking the murdock script: https://ci.riot-os.org/ (see fails for you PR)

@miri64
Copy link
Member

miri64 commented Apr 17, 2020

Yepp, murdock is unable to get the supported boards for your application:

$ make -C fuzzing/gnrc_tcp/ info-boards-supported 
make: Entering directory '/home/mlenders/Repositories/RIOT-OS/RIOT/fuzzing/gnrc_tcp'
make: Nothing to be done for 'info-boards-supported'.
make: Leaving directory '/home/mlenders/Repositories/RIOT-OS/RIOT/fuzzing/gnrc_tcp'

@miri64
Copy link
Member

miri64 commented Apr 17, 2020

I don't understand Github anymore... in case you missed it: solution is proposed here #13157 (comment)

This adds a new subdirectory called `fuzzing/` which will contain
applications for fuzzing various RIOT network modules in the future.
This subdirectory is heavily inspired by the `examples/` subdirectory.

The fuzzing applications use AFL as a fuzzer. Each application contains
Makefiles, source code, and an input corpus used by AFL to generate
input for fuzzing.
This should ensure that fuzzing applications are build by the CI.
@nmeum
Copy link
Member Author

nmeum commented Apr 17, 2020

I don't understand Github anymore... in case you missed it: #13157 (comment)

Thank you very much, pushed an updated version. While at it, I also improved the documentation a bit. Let's see how this goes... :)

@nmeum
Copy link
Member Author

nmeum commented Apr 17, 2020

Whoo that seems to have worked \o/

Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good and Murdock is happy.

@miri64
Copy link
Member

miri64 commented Apr 18, 2020

Then let's merge this :-)

@miri64 miri64 merged commit 55a7010 into RIOT-OS:master Apr 18, 2020
@miri64 miri64 added this to the Release 2020.07 milestone Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants