Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant