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

adler32 check in fuzz target interferes with fuzzing #29

Closed
Shnatsel opened this Issue Jun 28, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@Shnatsel
Copy link
Contributor

Shnatsel commented Jun 28, 2018

The fuzz target currently invokes miniz_oxide like this:

let _ = miniz_oxide::inflate::decompress_to_vec(data);

It does not seem to disable alder32 checks. Because of checksum verification fuzzers cannot reach the actually interesting decoding code.

Rust fuzzer integration provides special configuration option fuzzing which is set when a binary is built with fuzzing instrumentation; you can use it to disable checksums via conditional compilation (example from lodepng-rust). Alternatively, you can adapt your fuzzing target to set the required configuration parameters to disable checksum verification (inflate crate provides a *_no_checksum function).

cc @oyvindln who has authored the fuzz target.

I can also provide you with an initial fuzzing corpus to kickstart fuzzing, both with and without zlib headers, obtained by fuzzing inflate crate with afl-fuzz, libfuzzer and honggfuzz. If you're interested, let me know and I'll attach it here.

honggfuzz-rs provides further documentation, including enabling sanitizers to catch bugs in code under unsafe. Or ping me or https://github.com/rust-fuzz if you have any questions regarding fuzzing.

@oyvindln

This comment has been minimized.

Copy link
Collaborator

oyvindln commented Jun 28, 2018

Checksum validation happens at the very end, after decoding, so I'm not sure how that would be blocking anything. Could probably add a flag to disable it though, more fuzzing coverage won't hurt in any case. Didn't do much with the fuzzing code for a long time due to a rustc bug that triggered an LLVM assertion, but that's been fixed now. What's there currently is very basic, so adding your fuzzing corpus sounds like a good idea. We should be fuzzing streaming decoding as well.

I've been working on adding some basic coverage tests ported from zlib-ng, which I will commit soon.

@Shnatsel

This comment has been minimized.

Copy link
Contributor

Shnatsel commented Jun 28, 2018

Here are the fuzzing seeds, with and without zlib headers respectively. You will probably want to minify them before running the fuzzing. Just don't forget to add these to your fuzzing set and minify again after adding any new features.

min_starting_points.tar.gz
nozlib_min_starting_points.tar.gz

@oyvindln

This comment has been minimized.

Copy link
Collaborator

oyvindln commented Jul 13, 2018

Thanks.

As miniz_oxide seems to have made its way into gecko, getting a proper fuzzing setup has become even more important.

@Shnatsel

This comment has been minimized.

Copy link
Contributor

Shnatsel commented Jul 17, 2018

I've disabled checksum verification and run the existing "inflate_nonwrapping" fuzz target through a total of over 1 billion iterations under afl-fuzz, honggfuzz and cargo-fuzz. That found no panics, crashes or memory leaks, including under address sanitizer. Kudos!

I've opened a pull request to disable alder32 and add the fuzzing seeds to kickstart future fuzzing.

@oyvindln

This comment has been minimized.

Copy link
Collaborator

oyvindln commented Jul 17, 2018

That sounds promising, nice work! Next up we should probably add some more fuzzing targets for different flags and for streaming configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment