'attempt to add with overflow' panic in webp

[alexanderkjall]

This happens in image::codecs::webp::decoder::WebPDecoder<R>::read_vp8_header

Expected

An Error object should have been returned

Actual behaviour

Stacktrace:

thread 'main' panicked at 'attempt to add with overflow', /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.14/./src/codecs/webp/decoder.rs:114:25
stack backtrace:
   0: rust_begin_unwind
             at /rustc/ce1d5611a28468663e275078649e7ca6eef735a8/library/std/src/panicking.rs:515:5
   1: core::panicking::panic_fmt
             at /rustc/ce1d5611a28468663e275078649e7ca6eef735a8/library/core/src/panicking.rs:92:14
   2: core::panicking::panic
             at /rustc/ce1d5611a28468663e275078649e7ca6eef735a8/library/core/src/panicking.rs:50:5
   3: image::codecs::webp::decoder::WebPDecoder<R>::read_vp8_header
             at /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.14/./src/codecs/webp/decoder.rs:114:25
   4: image::codecs::webp::decoder::WebPDecoder<R>::read_metadata
             at /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.14/./src/codecs/webp/decoder.rs:138:23
   5: image::codecs::webp::decoder::WebPDecoder<R>::new
             at /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.14/./src/codecs/webp/decoder.rs:70:9
   6: image::io::free_functions::load
             at /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.14/./src/io/free_functions.rs:71:64
   7: image::dynimage::load_from_memory_with_format
             at /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.14/./src/dynimage.rs:1320:5
   8: image::dynimage::load_from_memory
             at /home/capitol/.cargo/registry/src/github.com-1ecc6299db9ec823/image-0.23.14/./src/dynimage.rs:1305:5
   9: image_reproduce::main
             at ./src/main.rs:20:13
  10: core::ops::function::FnOnce::call_once
             at /rustc/ce1d5611a28468663e275078649e7ca6eef735a8/library/core/src/ops/function.rs:227:5

Reproduction steps

Can be reproduced with this program:

fn main() {
    let data = vec![0x52, 0x49, 0x46, 0x46, 0xaf, 0x37, 0x80, 0x47, 0x57, 0x45, 0x42, 0x50,
                    0x6c, 0x64, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff, 0xfb, 0x7e, 0x73, 0x00,
                    0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00,
                    0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x40, 0xfb, 0xff, 0xff, 0x65, 0x65,
                    0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x65, 0x00, 0x00, 0x00, 0x00,
                    0x62, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x49, 0x49, 0x54,
                    0x55, 0x50, 0x4c, 0x54, 0x59, 0x50, 0x45, 0x33, 0x37, 0x44, 0x4d, 0x46];

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

[tristanphease]

So this happens because of this function.
What it seems to do is if the RIFF chunk identifier isn't one of the things it recognises as a WebP image type(VP8, VP8L, ALPH, ANIM, ANMF) it skips the whole RIFF chunk and continues looking for a WebP image. This behaviour seems unnecessary in its entirety since the image really ought to immediately follow the WebP file header.

The panic specifically occurs because the 4 bytes of 0xff give an odd value just under the max value, so we could just add a check for that or modify the whole function.

Also we should probably have "VP8X" as one of the formats unsupported.

fn read_vp8_header(&mut self) -> ImageResult<u32> {
    loop {
        let mut chunk = [0; 4];
        self.r.read_exact(&mut chunk)?;

        match &chunk {
            b"VP8 " => {
                let len = self.r.read_u32::<LittleEndian>()?;
                return Ok(len);
            }
            b"ALPH" | b"VP8L" | b"ANIM" | b"ANMF" => {
                // Alpha, Lossless and Animation isn't supported
                return Err(ImageError::Unsupported(UnsupportedError::from_format_and_kind(
                    ImageFormat::WebP.into(),
                    UnsupportedErrorKind::GenericFeature(chunk.iter().map(|&b| b as char).collect()),
                )));
            }
            _ => {
                let mut len = self.r.read_u32::<LittleEndian>()?;
                if len % 2 != 0 {
                    // RIFF chunks containing an uneven number of bytes append
                    // an extra 0x00 at the end of the chunk
                    len += 1;
                }
                io::copy(&mut self.r.by_ref().take(len as u64), &mut io::sink())?;
            }
        }
    }
}

WebP container spec

[tristanphease]

Immediately after posting that, I realised the point of skipping those chunks is to half-support extended file format chunks(VP8X) by just ignoring the extension data and trying to decode the VP8 file as normal. With that, it's probably reasonable to just add a check to prevent the add overflow?

[HeroicKatora]

Yes, that could be a good start. At least it turns the (debug) panic into a proper error as the file will appear to be too short. In any case it may even be okay to perform a saturating_add instead but a check-and-error approach is, on first instinct, cleaner.

[paolobarbolini]

I didn't notice this when it was posted at the time, but this issue was introduced by my PR (#1168) 😅

Another solution could be to move the as u64 from line 115 to line 109

image/src/codecs/webp/decoder.rs

Lines 109 to 115 in 702c1e7

	let mut len = self.r.read_u32::<LittleEndian>()?;
	if len % 2 != 0 {
	// RIFF chunks containing an uneven number of bytes append
	// an extra 0x00 at the end of the chunk
	len += 1;
	}
	io::copy(&mut self.r.by_ref().take(len as u64), &mut io::sink())?;

[5225225]

From looking at it quickly, I'd be inclined to do a checked add and return an error if that fails. More verbose, but clearer as to the intent. As far as I can see, a chunk length that overflows a u32 is invalid anyways, because the whole file size is limited to 4GiB - 2 bytes, according to https://developers.google.com/speed/webp/docs/riff_container. So if it overflows, it's a bad file.

Also, maybe return the same / a similar error if the io::copy is too short (If len is 1000, and we only copy 500 bytes, is that an error that we should be reporting here?)