FrameDecoder panics on malformed input

[pawanjay176]

Encountered a panic thread 'main' panicked at 'attempt to subtract with overflow', while decoding malformed input bytes.

use snap::read::FrameDecoder;
use std::io::Read;

fn main() {
    let data = [255, 6, 0, 0, 115, 78, 97, 80, 112, 89, 0, 0, 0, 0, 38, 1, 255, 0].as_ref();
    let mut reader = FrameDecoder::new(data);
    let mut decoded = Vec::new();
    reader.read_to_end(&mut decoded).unwrap();
}

The panic occurred here https://github.com/BurntSushi/rust-snappy/blob/1951d5d267c2c363c202096f43a026ec0d91f0fc/src/read.rs#L191

The malformed input after providing the correct stream identifier chunk has 4 zero bytes which leads to len becoming 0 and hence panics when we try to do unsafe subtraction in line 191.

The stack trace:

thread 'main' panicked at 'attempt to subtract with overflow', /Users/pawan/.cargo/registry/src/github.com-1ecc6299db9ec823/snap-1.0.0/src/read.rs:191:30
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.44/src/backtrace/libunwind.rs:86
   1: backtrace::backtrace::trace_unsynchronized
             at /Users/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.44/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:1063
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1426
   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:204
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:224
  10: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:470
  11: rust_begin_unwind
             at src/libstd/panicking.rs:378
  12: core::panicking::panic_fmt
             at src/libcore/panicking.rs:85
  13: core::panicking::panic
             at src/libcore/panicking.rs:52
  14: <snap::read::FrameDecoder<R> as std::io::Read>::read
             at /Users/pawan/.cargo/registry/src/github.com-1ecc6299db9ec823/snap-1.0.0/src/read.rs:191
  15: std::io::read_to_end_with_reservation
             at /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libstd/io/mod.rs:394
  16: std::io::read_to_end
             at /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libstd/io/mod.rs:361
  17: std::io::Read::read_to_end
             at /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libstd/io/mod.rs:659
  18: upgrade_test::main
             at src/main.rs:8
  19: std::rt::lang_start::{{closure}}
             at /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libstd/rt.rs:67
  20: std::rt::lang_start_internal::{{closure}}
             at src/libstd/rt.rs:52
  21: std::panicking::try::do_call
             at src/libstd/panicking.rs:303
  22: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:86
  23: std::panicking::try
             at src/libstd/panicking.rs:281
  24: std::panic::catch_unwind
             at src/libstd/panic.rs:394
  25: std::rt::lang_start_internal
             at src/libstd/rt.rs:51
  26: std::rt::lang_start
             at /rustc/4fb7144ed159f94491249e86d5bbd033b5d60550/src/libstd/rt.rs:67
  27: upgrade_test::main

I think we can fix this by doing checked subtraction and returning an error if there's an underflow. Will be happy to raise a PR if you can confirm it's an issue :)

[BurntSushi]

Definitely looks like a bug. Also looks like the bug is here too:

https://github.com/BurntSushi/rust-snappy/blob/1951d5d267c2c363c202096f43a026ec0d91f0fc/src/read.rs#L170

Either checked subtraction or just adding an explicit if len < 4 { return error(...) } sounds great to me! Thanks for catching this!

[pawanjay176]

Sure, no worries. I'm trying to think what Error variant would be appropriate for this case. The closest fit seems to be UnsupportedChunkLength https://github.com/BurntSushi/rust-snappy/blob/1951d5d267c2c363c202096f43a026ec0d91f0fc/src/error.rs#L156-L165

but the docs for this variant says trying to read a chunk with length greater than that supported. which isn't right.

Maybe this requires a new error variant along the lines of InvalidChunkLength?

[BurntSushi]

Ug. I meant to make the enum non-exhaustive when I did the 1.0 release. So that means adding a new variant is a breaking change.

UnsupportedChunkLength looks fine. The name is right anyway. I'd just tweak the description to be a bit more generic, i.e., "a chunk with an unexpected or incorrect length."

[pawanjay176]

Ug. I meant to make the enum non-exhaustive when I did the 1.0 release. So that means adding a new variant is a breaking change.

Sorry, didn't think of this.

I'd just tweak the description to be a bit more generic, i.e., "a chunk with an unexpected or incorrect length."

This sounds great. Will do this.

[flipchan]

Got the same panic today:

use snap::read;
use std::io::Read;

fn main() {
         let data: &[u8] = &[0x10, 0x10, 0x0, 0x0];
         let mut buf = vec![];
         read::FrameDecoder::new(data).read_to_end(&mut buf).unwrap();
}

# ./target/release/testing
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Custom
 { kind: Other, error: StreamHeader { byte: 16 } }', src/libcore/result.rs:11$
8:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace$

found it with the help of https://github.com/rust-fuzz/cargo-fuzz

[BurntSushi]

@flipchan That's not the same panic. And it's not even clear that it's a bug. You're calling unwrap on a fallible operation, which will panic if an error occurs. The only way your code sample is a bug is if the input is a valid Snappy compressed frame. (It doesn't look like it is.)