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

Fix panics on malformed inputs, support fuzzing #81

Merged
merged 12 commits into from Aug 13, 2018

Conversation

Projects
None yet
2 participants
@Shnatsel
Copy link
Contributor

Shnatsel commented Jun 27, 2018

Bypass crc32 and adler32 checks when compiled with fuzzing instrumentation. This lets fuzzers reach actually interesting code.

Add a starting corpus for fuzzing, obtained initially by fuzzing other tools, then fed to afl-fuzz on image-png to get more inputs specific to it, and subsequently minified with afl-cmin.

Fix panics on malformed inputs discovered via fuzzing (#79):

  • Panic on malformed paletted images in expand_paletted(): return Result instead of calling unwrap()
  • Out of bounds access in unfilter(): now checks bounds and returns Result
  • Panic in next_interlaced_row(): info from previous row was erroneously used to decode the current row. Fixing this no longer triggers a panic and likely fixes decoding of some real-world PNGs.
  • Overflow on left shift described in #79 (comment)

Not fixed in this PR:

  • Overflow in calculation of output buffer size (#80), which allows an attacker to exhaust system memory and/or the address space of the process. Fixing this would be a breaking change for the external API.
  • Possibly some other overflows and debug mode panics that I cannot reach because of the above overflow happening before that code is reached

Shnatsel added some commits Jun 21, 2018

Fuzzing support: do not check crc32 or adler32 checksums in fuzzing m…
…ode using conditional compilation. Enables fuzzers to actually reach PNG decoding code instead of never going beyond checksums
When decoding interlaced files use info from current chunk instead of…
… the previous one. Fixes panic on malformed files (#79) and also likely fixes decoding of some exotic PNGs out there. Found via afl.rs
Drop cargo-fuzz intergration; this crate already has afl in-tree whic…
…h does pretty much the same thing a bit better. Integration with other fuzzers will be done via a generic harness in https://github.com/rust-fuzz/targets
Commit fuzzing seeds to afl folder. These were aggregated from fuzzin…
…g a bunch of tools (libpng, lodepng-rust), then used for fuzzing image-png with afl, and the resulting corpus minified with afl-cmin. As such they provide good starting coverage for afl and can serve as seeds for more computationally expensive tools.
Validate that paletted images have bit depth of 8 or less. Fixes debu…
…g mode panic on overflow in left shift (#79)
version = "0.1.0"
authors = ["nwin <nwin@users.noreply.github.com>"]
version = "0.2.0"
authors = ["Sergey Davidoff <shnatsel@gmail.com>", "Paul Grandperrin <paul.grandperrin@gmail.com>"]

This comment has been minimized.

@bvssvni

bvssvni Jul 5, 2018

Member

I think nwin removed as author here by accident.

This comment has been minimized.

@Shnatsel

Shnatsel Jul 6, 2018

Contributor

I have entirely replaced the afl/ subfolder without using anything that's in there previously. Its current incarnation is based only on https://github.com/rust-fuzz/targets, which is why Paul Grandperrin is credited.

I agree nwin should be credited for his contribution in some way, but I have removed him from the copyright notice for the source files the current afl/ folder is not based on his work in any way, and such misattribution is deliberately prohibited under some licenses, e.g. some BSD variants. This crate is dual-licensed under MIT and Apache, which do not have such a clause, so crediting nwin here would not be in conflict with the license.

I will re-add him as an author if you believe that is the best way to go about it.

This comment has been minimized.

@bvssvni

bvssvni Jul 6, 2018

Member

Ah, it's under the "png-afl" directory, didn't saw that.

@nwin OK with this?

This comment has been minimized.

@Shnatsel

Shnatsel Jul 11, 2018

Contributor

Should I split the panic fixes to a separate PR so they would not be held up by a copyright notice?

This comment has been minimized.

@bvssvni

bvssvni Jul 12, 2018

Member

I think it's OK. @nwin can just add himself back as author if he disagrees.

@bvssvni

This comment has been minimized.

Copy link
Member

bvssvni commented Aug 13, 2018

Merging.

@bvssvni bvssvni merged commit fc46e33 into PistonDevelopers:master Aug 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Shnatsel Shnatsel referenced this pull request Aug 15, 2018

Closed

Panic on malformed input #79

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