Image: attempt to multiply with overflow in image::load_from_memory

[alexanderkjall]

This happens in the function image::load_from_memory

Expected

An error about that the image format was invalid

Actual behaviour

Full output:

$ RUST_BACKTRACE=1 ./target/debug/image-reproduce 
thread 'main' panicked at 'attempt to multiply with overflow', /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.12/./src/image.rs:485:9
stack backtrace:
   0: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
   1: core::fmt::write
   2: std::io::Write::write_fmt
   3: std::panicking::default_hook::{{closure}}
   4: std::panicking::default_hook
   5: std::panicking::rust_panic_with_hook
   6: rust_begin_unwind
   7: core::panicking::panic_fmt
   8: core::panicking::panic
   9: image::image::ImageDecoder::total_bytes
             at /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.12/./src/image.rs:485
  10: image::image::decoder_to_vec
             at /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.12/./src/image.rs:421
  11: image::dynimage::decoder_to_image
             at /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.12/./src/dynimage.rs:1140
  12: image::dynimage::DynamicImage::from_decoder
             at /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.12/./src/dynimage.rs:194
  13: image::io::free_functions::load
             at /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.12/./src/io/free_functions.rs:85
  14: image::dynimage::load_from_memory_with_format
             at /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.12/./src/dynimage.rs:1323
  15: image::dynimage::load_from_memory
             at /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.12/./src/dynimage.rs:1308
  16: image_reproduce::main
             at src/main.rs:5
  17: std::rt::lang_start::{{closure}}
             at /usr/src/rustc-1.43.0/src/libstd/rt.rs:67
  18: std::panicking::try::do_call
  19: __rust_maybe_catch_panic
  20: std::rt::lang_start_internal
  21: std::rt::lang_start
             at /usr/src/rustc-1.43.0/src/libstd/rt.rs:67
  22: main
  23: __libc_start_main
  24: _start

Reproduction steps

    let data = vec![0x66, 0x61, 0x72, 0x62, 0x66, 0x65, 0x6c, 0x64, 0xc9, 0xc9,
                    0xc9, 0x36, 0x39, 0x30, 0x38, 0x31];

    let _ = image::load_from_memory(&data);

In version: image = "0.23.12"

[fseegraeber]

I had a look at this.
This is a valid farbfeld header with very big image dimensions (3,385,444,662 x 959,461,425) but missing image data.

If you run a test with image dimensions such that width*height*8 < u64::MAX e.g. 640 x 400:

let data = vec![0x66, 0x61, 0x72, 0x62, 0x66, 0x65, 0x6c, 0x64, 0x00, 0x00,
0x02, 0x80, 0x00, 0x00, 0x01, 0x90];

assert!(super::load_from_memory(&data).is_err());

you will get an std::io::Error with UnexpectedEof kind. So that scenario seems covered.

For handling very big dimensions: Changing ImageDecoder::total_bytes to return a Result would be a breaking change. One could check parsed dimensions in e.g. dynimage::decoder_to_image. Something like this seems to work:

if (u64::from(w) * u64::from(h)).checked_mul(u64::from(color_type.bytes_per_pixel())).is_none() {
    return Err(ImageError::Limits(LimitError::from_kind(LimitErrorKind::DimensionError)));
}

Perhaps using a decoding error instead?

The question is though, do you want to handle image dimensions that cause an u64 overflow? If so I'd be happy to open a PR.

[HeroicKatora]

Returning an error for image data exceeding u64::MAX bytes seems fine. Due to the signature it would need to be checked somewhere else, as you've observed. I would try to do this check early, e.g. in the constructor? (As for error kind, either the DimensionError or make an additional case distinction returning UnsupportedError for the edge case where the size exceeds u64 but is okay according to the format). I would also not be surprised if this happens to affect other decoders as well, maybe just not quite as obvious due to a more complicated header structure.

[fintelia]

I think that returning an error from the constructor is the right approach. Realistically, there should be very few legitimate images with a width or height that don't fit in a u32 or whose uncompressed size doesn't fit in a u64.

[fseegraeber]

Makes sense, the decoders can be used directly after all.

So basically:

1. Check the allowed dimensions for every supported file format.
2. Make sure all Decoders enforce that limit by returning a DimensionError if exceeded.
3. In the case that the format doesnt limit the size below u64::MAX make sure width*height*byte_per_pixel doesn't exceed u64::MAX by returning an UnsupportedError otherwise.

Ensuring that width and height fit into an u32 should be covered since that is the supported type for dimension. Having a quick glance, some decoders e.g. the BMP one, already seem to enforce point 3.

Seems alot :smile:, but ill have a look in the next days.