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

Panic on malformed input #79

Closed
Shnatsel opened this issue Jun 21, 2018 · 5 comments
Closed

Panic on malformed input #79

Shnatsel opened this issue Jun 21, 2018 · 5 comments

Comments

@Shnatsel
Copy link
Contributor

Previous fuzzing attempts did not bypass crc32 checks in png and adler32 checks in inflate crates. Thus they never actually exercised the png decoding code.

I have disabled checksum verification in fuzzing mode via conditional compilation and ran cargo-fuzz. I got a crash in less than a second.


Details on this specific panic: it happens at line https://github.com/PistonDevelopers/image-png/blob/1266ec2/src/decoder/mod.rs#L450

Steps to reproduce:

git clone https://github.com/Shnatsel/png-leak.git
cd png-leak
env RUST_BACKTRACE=1 RUSTFLAGS='--cfg fuzzing' cargo run

This repo uses my fork of image-png with crc32 conditionally disabled. --cfg fuzzing in the above command disables it so that fuzzer-generated files can be tested. The file that's causing the crash is included in the repo and can be downloaded here.

Shnatsel added a commit to Shnatsel/image-png that referenced this issue Jun 27, 2018
@Shnatsel
Copy link
Contributor Author

Shnatsel commented Jun 27, 2018

Another panic on malformed input: out of bounds access on line https://github.com/PistonDevelopers/image-png/blob/1266ec2967b49a4c5c0a80605e19e3268a7b7bef/src/filter.rs#L79 when bpp > len

Found with cargo-fuzz. Testcase: in.png

I have a feeling that there's gonna be a lot of these, so I'm just going to put them all in this issue instead of creating a separate issue for each panic.

Shnatsel added a commit to Shnatsel/image-png that referenced this issue Jun 27, 2018
@Shnatsel
Copy link
Contributor Author

Shnatsel commented Jun 27, 2018

There is an integer overflow in https://github.com/PistonDevelopers/image-png/blob/99383650e1a440bb14c54987938676c8f54d3bc6/src/decoder/mod.rs#L51

Any unsafe code relying on this value being correct has a security vulnerability - either information disclosure or arbitrary code execution.

The worst part is, fixing this correctly requires changing the external API: the function should use checked_mul() which returns Option<usize>. For now I'll have to make it panic on overflow, which is bad, but not as bad as the other options. I will document that unsafe code cannot rely on this value being correct.

Testcase: integer_overflow_in_multiplication found via afl-rs

Update: this issue is complicated and fixing it requires a breaking change, so I have filed it separately as #80

Shnatsel added a commit to Shnatsel/image-png that referenced this issue Jun 27, 2018
… the previous one. Fixes panic on malformed files (image-rs#79) and also likely fixes decoding of some exotic PNGs out there. Found via afl.rs
@Shnatsel
Copy link
Contributor Author

Shnatsel commented Jun 27, 2018

There is a panic on .unwrap() in https://github.com/PistonDevelopers/image-png/blob/99383650e1a440bb14c54987938676c8f54d3bc6/src/decoder/mod.rs#L280

The code reads info for previous chunk instead of the current one. The fix in e221ae9 likely also fixes decoding of some real-world PNGs.

Testcase: faulty_unwrap found via found via afl-rs.

I've also committed updated AFL integration to my fork; it uses in-process fuzzing which is ~10x faster.

@Shnatsel
Copy link
Contributor Author

There is overflow in left shift in https://github.com/PistonDevelopers/image-png/blob/99383650e1a440bb14c54987938676c8f54d3bc6/src/utils.rs#L18

However, I do not know what behavior is correct in this case. Perhaps the overflow is expected and the operator should be replaced with overflowing left shift?

Testcase: shift_left_with_overflow found via afl.rs

@Shnatsel
Copy link
Contributor Author

Fixed by #81

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

No branches or pull requests

1 participant