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

Comments

Projects
None yet
1 participant
@Shnatsel
Copy link
Contributor

Shnatsel commented Jun 21, 2018

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

This comment has been minimized.

Copy link
Contributor

Shnatsel commented Jun 27, 2018

Another panic on malformed input: out of bounds access on line

for i in 0..bpp {
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

This comment has been minimized.

Copy link
Contributor

Shnatsel commented Jun 27, 2018

There is an integer overflow in

self.line_size * self.height as usize

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

When decoding interlaced files use info from current chunk instead of…
… the previous one. Fixes panic on malformed files (PistonDevelopers#79) and also likely fixes decoding of some exotic PNGs out there. Found via afl.rs
@Shnatsel

This comment has been minimized.

Copy link
Contributor

Shnatsel commented Jun 27, 2018

There is a panic on .unwrap() in

let trns = get_info!(self).trns.as_ref().unwrap();

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

This comment has been minimized.

Copy link
Contributor

Shnatsel commented Jun 27, 2018

There is overflow in left shift in

let mask = ((1u16 << bit_depth) - 1) as u8;

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

This comment has been minimized.

Copy link
Contributor

Shnatsel commented Aug 15, 2018

Fixed by #81

@Shnatsel Shnatsel closed this Aug 15, 2018

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