Fuzzing tcpdump and netdissect #700
Conversation
|
Could you address the missing header issue with AppVeyor and make this change ready for merging (even if it does not make it into OSS-Fuzz)? |
...which, as with libpcap, means "don't build the |
fuzz/fuzz_pcap.c
Outdated
| Ndo.ndo_printf=fuzz_ndo_printf; | ||
|
|
||
| //initialize output file | ||
| if (outfile == NULL) { |
There was a problem hiding this comment.
Unless there's some mechanism to send the output to a file rather than /dev/null, this could be done more simply by having fuzz_ndo_printf() do nothing.
There was a problem hiding this comment.
I think that we would miss the format string bugs if we do not call some print functions with the real arguments.
A mechanism to send the output to a regular file would be good for debugging purposes. I will try to add that.
There was a problem hiding this comment.
Before I noticed your changes, I started some work with the same goal - to get tcpdump printers fuzzed. I ended up going a different way: I created different fuzzers for different printers, because my goal is to help a contributor who is modifying a given printer. This makes it harder to create a corpus, since you can't just throw in pcap files, but it makes it easier to target a fuzz process at a given printer.
I think there are benefits to both paths: obviously going through libpcap means that the whole end to end is fuzzed, and that's good; I like my more-targeted process because as I said, it allows targeting a specific printer. Given that, I plan to keep developing my specific fuzzers but will use your infrastructure as a starting point.
| Ndo.ndo_printf=fuzz_ndo_printf; | ||
| init_print(&Ndo, 0, 0); | ||
|
|
||
| //rewrite buffer to a file as libpcap does not have buffer inputs |
There was a problem hiding this comment.
The clusterfuzz infrastructure really wants fuzzing to be fast - writing to a file and then reading it is a lot of overhead.
There was a problem hiding this comment.
writing to a file and then reading it is a lot of overhead.
Indeed.
But as the comment said libpcap does not have buffer inputs
So the other solution would be to modify libpcap : have a new pcap_open_buffer interface and modify pcap_open_file so that both use the same internal logic.
For a first integration, I did not want to change the internal code.
That can be an optimization that comes afterwards.
Do you see any other way ?
There was a problem hiding this comment.
I have to take my comment about speed back. pcap_fuzz is about 10x as fast as bgp_print_fuzzer right now. It's possible that this is due to vasprintf but as often happens, gut reactions about code speed can be wrong :-)
There was a problem hiding this comment.
I suspect oss-fuzz people use a ramdisk...
I am interested if you can get profiling data.
cf google/oss-fuzz#2180 (comment)
| u_int packets_captured = 0; | ||
| netdissect_options Ndo; | ||
|
|
||
| //initialize output file |
There was a problem hiding this comment.
Why not do initialization in LLVMFuzzerInitialize()?
There was a problem hiding this comment.
The libfuzzer documentation https://llvm.org/docs/LibFuzzer.html#id38 states that
Do this only if you really need to access argv/argc.
So, why do you remand the use of LLVMFuzzerInitialize ?
There was a problem hiding this comment.
I hadn't seen that documentation. I don't understand that statement (because why is it better to use a static local variable rather than an explicit initialize-time function call?), but, ok, that's what they want.
There was a problem hiding this comment.
One reason that I want to use the Initialize function is the way I structured my individual fuzzers; the common code in common.c has the common initialization.
The alternative is to add code like "if (!initialized) { intialize(); initialized=1; }" to each of the individual fuzzers, but using the LLVMFuzzerInitialize function means that I don't have to duplicate boilerplate code like that.
There was a problem hiding this comment.
I do not know why they prefer it this way.
Another alternative could be to use the static initialized variable in LLVMFuzzerInitialize
| int ret; | ||
|
|
||
| va_start(args, fmt); | ||
| ret = vfprintf(outfile, fmt, args); |
There was a problem hiding this comment.
I had trouble with getting EINTR when running a long fuzz, so I switched to using vasprintf() (and freeing the resulting string) so that the arguments got formatted but we could discard them. I also have an option triggered by an environment variable to leave ndo->ndo_printf alone, so that you get the output when you run a replication case.
There was a problem hiding this comment.
Interesting, I did not get this EINTR.
Was you file /dev/null or some regular file for logging ?
Your solution looks good.
|
Thanks for your review
I totally agree
|
I ended up using scapy and a short python script to take the existing pcap files and turn them into individual per-fuzzer corpora, and another script to do the inverse to convert the kind of file that oss-fuzz would provide with a bug report into a pcap file. |
|
Looks great. Can you share these scripts ? |
Sorry, I thought I had posted a link. https://github.com/fenner/tcpdump/tree/fuzz/fuzz I still want to move some of the build.sh to the tcpdump repo, but for now I added this to the oss-fuzz one that you wrote: $CC $CFLAGS -I.. -I. -c ../fuzz/common.c -o common.o
for p in ip ip6 ether bgp
do
$CC $CFLAGS -I.. -I. -c ../fuzz/${p}_print_fuzzer.c -o ${p}_print_fuzzer.o
$CXX $CXXFLAGS ${p}_print_fuzzer.o common.o -o $OUT/${p}_print_fuzzer libnetdissect.a ../../libpcap/build/libpcap.a -lFuzzingEngine
done
mkdir corpus
cd corpus
$SRC/tcpdump/fuzz/corpus/pcap2corpus $SRC/tcpdump/tests/*.pcap
for d in *
do
zip -r $OUT/${d}_print_fuzzer_seed_corpus.zip $d/*
done
cd ..and added |
For use with oss-fuzz