out-of-bounds panic in utils::expand_pass

[Shnatsel]

thread '<unnamed>' panicked at 'index out of bounds: the len is 56 but the index is 56', png-0.16.5/src/utils.rs:248:17

Sample file triggering the panic: mysterious-oob-crash

The coolest thing about this is that this bug is not reproducible with png's current cargo-fuzz fuzzing harness for decoding. I get the panic only with the following code:

#![no_main]
use libfuzzer_sys::fuzz_target;

use std::io;
extern crate png;

fn decode_png(data: &[u8])
    -> io::Result<Vec<u8>>
{
    let limits = png::Limits { bytes: 1 << 16 };
    let decoder = png::Decoder::new_with_limits(data, limits);
    let (info, mut reader) = decoder.read_info()?;

    if info.buffer_size() > 5_000_000 {
        return Err(std::io::Error::new(std::io::ErrorKind::Other, "memory limit exceeded"));
    }

    let mut data = vec![0u8; info.buffer_size()];
    reader.next_frame(&mut data)?;

    Ok(data)
}

fuzz_target!(|data: &[u8]| {
    decode_png(data);
});

Backtrace:

thread '<unnamed>' panicked at 'index out of bounds: the len is 56 but the index is 56', /home/shnatsel/.cargo/registry/src/github.com-1ecc6299db9ec823/png-0.16.5/src/utils.rs:248:17
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.46/src/backtrace/libunwind.rs:86
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.46/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:78
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:59
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1069
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1504
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:62
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:49
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:198
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:218
  10: <alloc::boxed::Box<F> as core::ops::function::Fn<A>>::call
             at /home/shnatsel/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/liballoc/boxed.rs:1022
  11: libfuzzer_sys::initialize::{{closure}}
             at /home/shnatsel/.cargo/registry/src/github.com-1ecc6299db9ec823/libfuzzer-sys-0.3.2/src/lib.rs:50
  12: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:515
  13: rust_begin_unwind
             at src/libstd/panicking.rs:419
  14: core::panicking::panic_fmt
             at src/libcore/panicking.rs:111
  15: core::panicking::panic_bounds_check
             at src/libcore/panicking.rs:69
  16: png::utils::expand_pass
             at /home/shnatsel/.cargo/registry/src/github.com-1ecc6299db9ec823/png-0.16.5/src/utils.rs:248
  17: png::decoder::Reader<R>::next_frame
             at /home/shnatsel/.cargo/registry/src/github.com-1ecc6299db9ec823/png-0.16.5/src/decoder/mod.rs:406
  18: roundtrip::decode_png
             at fuzz_targets/roundtrip.rs:23
  19: rust_fuzzer_test_input
             at fuzz_targets/roundtrip.rs:29
  20: libfuzzer_sys::test_input_wrap::{{closure}}
             at /home/shnatsel/.cargo/registry/src/github.com-1ecc6299db9ec823/libfuzzer-sys-0.3.2/src/lib.rs:27
  21: std::panicking::try::do_call
             at /home/shnatsel/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:331
  22: __rust_try
  23: std::panicking::try
             at /home/shnatsel/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panicking.rs:274
  24: std::panic::catch_unwind
             at /home/shnatsel/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/src/libstd/panic.rs:394
  25: LLVMFuzzerTestOneInput
             at /home/shnatsel/.cargo/registry/src/github.com-1ecc6299db9ec823/libfuzzer-sys-0.3.2/src/lib.rs:25
  26: _ZN6fuzzer6Fuzzer15ExecuteCallbackEPKhm
  27: _ZN6fuzzer10RunOneTestEPNS_6FuzzerEPKcm
  28: _ZN6fuzzer12FuzzerDriverEPiPPPcPFiPKhmE
  29: main
             at libfuzzer/FuzzerMain.cpp:19
  30: __libc_start_main
  31: _start
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[Shnatsel]

Ah, I think this is what makes all the difference:

let mut data = vec![0u8; info.buffer_size()]; versus Vec::with_capacity(info.buffer_size()); in png's own fuzzing harness. Apparently the png fuzzing harness has always been passing zero-sized output buffer... because &mut Vec<u8> transparently coerces to &mut [u8] and the types magically work out... despite &mut Vec<u8> being extensible and &mut [u8] not, so zero-lenght Vec makes sense but not zero-lenght &mut [u8].

So much for "if it compiles, it works".

[Shnatsel]

I've fixed the fuzzing harness in #220 but the panic still stands.

[HeroicKatora]

We'll need to rewrite color handling, it's a mess. The 56×1 image with RGBA/2bit has an unexpanded length of 56 bytes but the expansion pass wants to exand each channel to 8bits for quadruple the data. For some reason, output_buffer_size() uses the internal color type to calculate line size and this is of course not enough.

It doesn't not appear to cause an issue in non-interlaced images because here we simply ignore the additional data.