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

Initial fuzzing support #422

Merged
merged 3 commits into from
May 2, 2020
Merged

Initial fuzzing support #422

merged 3 commits into from
May 2, 2020

Conversation

zlowram
Copy link
Contributor

@zlowram zlowram commented Apr 22, 2020

I've been auditing this project and, as part of the process, I wrote a fuzzer using libFuzzer for it. It works and it has already discovered some bugs that I'll be reporting to you as soon as I send this PR.

The idea behind this PR is to provide a fuzzer for this project to allow continuous fuzzing in order to improve its security. I will also apply to Google's OSS-Fuzz initiative. If the project gets accepted it will be fuzzed 24/7 in their cluster and the bugs will be automatically reported to you.

I tried to match your style for builds as much as I could, but feel free to adapt it as you wish.

Also, I would highly recommend to integrate regression tests with the samples that trigger bugs in your testing pipelines to avoid uncovering previously fixed bugs. You can do this by doing the linux build with the "--asan" option and calling the fuzzer built (located at Test/Fuzzers/Bin/fuzz_target, once built) passing as argument one of the samples at a time.

Thanks!

@seladb
Copy link
Owner

seladb commented Apr 24, 2020

thanks @zlowram for taking the initiative to integrate PcapPlusPlus into OSS-Fuzz.

I think it's a very important initiative that will definitely improve the security, stability and robustness of this project. Before merging the PR I have a few topics to discuss with you:

  • When running Fuzzer, what is the corpus you used?
  • In the PR you didn't include running Fuzzer as part of the CI. Should we do that? If yes, how do file new bugs that will be discovered during CI run?
  • The fuzzing code includes only simple IPv4 checks.
    • Do you think we should add more protocol checks? Maybe the basic ones first (Eth, IPv6, TCP, UDP, DNS, HTTP, SSL)?
    • Do you think we should also include code that manipulate packets?
  • As you can imagine no one has ever run fuzzer in PcapPlusPlus and I know that some parts in the code (especially the older code) won't handle edge cases very well, so I'm expecting a large number of bugs, at least in the beginning. I'm not sure I'll have the capacity to handle all of them. Will you be able to assist in resolving those bugs? I will obviously help and guide you through the process

@zlowram
Copy link
Contributor Author

zlowram commented Apr 24, 2020

Replying inline:

thanks @zlowram for taking the initiative to integrate PcapPlusPlus into OSS-Fuzz.

I think it's a very important initiative that will definitely improve the security, stability and robustness of this project. Before merging the PR I have a few topics to discuss with you:

  • When running Fuzzer, what is the corpus you used?

I used the corpus from the libpcap fuzzer.

  • In the PR you didn't include running Fuzzer as part of the CI. Should we do that? If yes, how do file new bugs that will be discovered during CI run?

I wouldn't run it aiming to discover bugs as part of the CI. What I would include in the CI, though, is running the fuzzer with the ASAN build against samples that are known to trigger already fixed bugs to avoid regressions. E.g. when one of the reported bugs is fixed, the CI should run the fuzzer with the ASAN build of PcapPlusPlus passing that single sample as parameter (if there were multiple samples, one time for each of them).

  • The fuzzing code includes only simple IPv4 checks.
    • Do you think we should add more protocol checks? Maybe the basic ones first (Eth, IPv6, TCP, UDP, DNS, HTTP, SSL)?
    • Do you think we should also include code that manipulate packets?

Maybe I would just start with this simple one as it already handles parsing of any kind of packet (e.g. one of the bugs reported is DNS related). As time goes and bugs start appearing and being fixed, I would definitely add more checks to allow more coverage within the codebase.

  • As you can imagine no one has ever run fuzzer in PcapPlusPlus and I know that some parts in the code (especially the older code) won't handle edge cases very well, so I'm expecting a large number of bugs, at least in the beginning. I'm not sure I'll have the capacity to handle all of them. Will you be able to assist in resolving those bugs? I will obviously help and guide you through the process

Unfortunately I don't have the time to help you with fixing them. The OSSFuzz initiative will let you know about the bugs privately so they can be fixed prior to disclosure. The good part is that I think that thanks to oss-fuzz the project will have more visibility to atract other devs that can help.

@zlowram
Copy link
Contributor Author

zlowram commented Apr 24, 2020

Good news! It has been accepted

I will work on the integration there, and will try to help with the integration in the CI here. Do you want me to send the commits to this same PR?

@zlowram
Copy link
Contributor Author

zlowram commented Apr 25, 2020

I have created Regression tests with the samples for #423 and #425, added them to both travis-ci and cirrus-ci on a separate task.

Also, I've done a rebase from dev branch so the regression tests are green already and the commit history for the PR looks a little bit better.

BTW, I've used a docker image that I created and uploaded to my docker hub repository but if you'd like to host it on yours, here's the Dockerfile:

FROM ubuntu:19.10

RUN apt update && \
        apt -y upgrade && \
        apt -y install clang gcc g++ make libpcap-dev

Makefile Outdated Show resolved Hide resolved
Tests/RegressionTests/run_tests.sh Outdated Show resolved Hide resolved
configure-linux.sh Outdated Show resolved Hide resolved
configure-linux.sh Outdated Show resolved Hide resolved
configure-linux.sh Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
.cirrus.yml Outdated Show resolved Hide resolved
@zlowram
Copy link
Contributor Author

zlowram commented Apr 27, 2020

Please, do not merge yet. I'm fixing some stuff to the build system to make it easier and compatible with OSS-Fuzz.

Tests/Fuzzers/Makefile Show resolved Hide resolved
Tests/Fuzzers/fuzz_target.cc Outdated Show resolved Hide resolved
@zlowram
Copy link
Contributor Author

zlowram commented Apr 28, 2020

I have slightly modified the build system to honor the make variables: CC, CXX, CFLAGS, and CXXFLAGS.

This is usually a good idea to allow modifying then in the environment variables, and OSS-Fuzz requires it.

@zlowram
Copy link
Contributor Author

zlowram commented Apr 28, 2020

I have added a configure flag for linux to allow linking against libpcap statically. Fuzz targets in OSSFuzz can't have any other dependencies than those installed in the base system, and I think it's also a nice feature to have.

Pcap++/Makefile Outdated Show resolved Hide resolved
Common++/Makefile Show resolved Hide resolved
mk/platform.mk.macosx Show resolved Hide resolved
mk/PcapPlusPlus.mk.common Outdated Show resolved Hide resolved
mk/PcapPlusPlus.mk.linux Outdated Show resolved Hide resolved
* Added support to build with AddressSanitizer and SanitizerCoverage
* Added a first fuzzer
mk/PcapPlusPlus.mk.common Outdated Show resolved Hide resolved
mk/PcapPlusPlus.mk.linux Outdated Show resolved Hide resolved
Pcap++/Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
mk/PcapPlusPlus.mk.fuzzing Outdated Show resolved Hide resolved
In particular:
	* CC
	* CXX
	* CFLAGS
	* CXXFLAGS
@seladb seladb merged commit 61c4b19 into seladb:dev May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants