Fix panic: Return error before incrementing Reader's index (Reader.i)

[oherrala]

I'm reporting this here (with a fix) because of

Please report bugs either as pull requests or as issues in the issue tracker. untrusted.rs has a full disclosure vulnerability policy. Please do NOT attempt to report any security vulnerability in this code privately to anybody.

https://github.com/briansmith/untrusted/blob/3c842f49cb51fcc72b3656ad063e024d4b725115/README.md#bug-reporting

Proof of Concept code:

extern crate untrusted;

fn main() {
    let buf = vec![1, 2, 3, 4, 5];
    let input = untrusted::Input::from(&buf);
    match input.read_all(untrusted::EndOfInput, reader) {
        Ok(_) => println!("ok"),
        Err(_) => println!("err"),
    }
}

fn reader(input: &mut untrusted::Reader) -> Result<(), untrusted::EndOfInput> {
    let some_input = input.skip_and_get_input(6);
    let and_the_rest = Vec::from(input.skip_to_end().as_slice_less_safe());
    println!("{:?}", some_input);
    println!("{:?}", and_the_rest);
    Ok(())
}

crashes with:

$ RUST_BACKTRACE=1 cargo run                                                                                                               [1/209]
    Finished dev [unoptimized + debuginfo] target(s) in 0.0 secs
     Running `target/debug/untrustedcrash`
thread 'main' panicked at 'attempt to subtract with overflow', /Users/oherrala/.cargo/registry/src/github.1485827954.workers.dev-1ecc6299db9ec823/untrusted-0.6.1/src/untrusted.rs:330:23
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:207
   3: std::panicking::default_hook
             at libstd/panicking.rs:223
   4: std::panicking::begin_panic
             at libstd/panicking.rs:402
   5: std::panicking::try::do_call
             at libstd/panicking.rs:349
   6: std::panicking::try::do_call
             at libstd/panicking.rs:325
   7: core::ptr::drop_in_place
             at libcore/panicking.rs:72
   8: core::ptr::drop_in_place
             at libcore/panicking.rs:51
   9: untrusted::Reader::skip_and_get_input::{{closure}}
             at /Users/oherrala/.cargo/registry/src/github.1485827954.workers.dev-1ecc6299db9ec823/untrusted-0.6.1/src/untrusted.rs:330
  10: untrustedcrash::reader
             at src/main.rs:14
  11: core::ops::function::FnOnce::call_once
             at /Users/travis/build/rust-lang/rust/src/libcore/ops/function.rs:223
  12: untrusted::Input::read_all
             at /Users/oherrala/.cargo/registry/src/github.1485827954.workers.dev-1ecc6299db9ec823/untrusted-0.6.1/src/untrusted.rs:158
  13: untrustedcrash::main
             at src/main.rs:6
  14: std::rt::lang_start::{{closure}}
             at /Users/travis/build/rust-lang/rust/src/libstd/rt.rs:74
  15: std::panicking::try::do_call
             at libstd/rt.rs:59
             at libstd/panicking.rs:306
  16: panic_unwind::dwarf::eh::read_encoded_pointer
             at libpanic_unwind/lib.rs:102
  17: <std::io::Write::write_fmt::Adaptor<'a, T> as core::fmt::Write>::write_str
             at libstd/panicking.rs:285
             at libstd/panic.rs:361
             at libstd/rt.rs:58
  18: std::rt::lang_start
             at /Users/travis/build/rust-lang/rust/src/libstd/rt.rs:74
  19: untrustedcrash::reader

There are actually combination of two errors:

First, there's my mistake: Proof of concept code on line

let some_input = input.skip_and_get_input(6);

doesn't check for error as it should and continues with

let and_the_rest = Vec::from(input.skip_to_end().as_slice_less_safe());

Secondly, Untrusted's error: untrusted's Reader::skip_and_get_input() function doesn't return error before increasing the index pointing to a buffer:

untrusted/src/untrusted.rs

Lines 316 to 324 in 44384f8

	pub fn skip_and_get_input(&mut self, num_bytes: usize)
	-> Result<Input<'a>, EndOfInput> {
	let new_i = try!(self.i.checked_add(num_bytes).ok_or(EndOfInput));
	let ret = self.input.get_slice(self.i..new_i)
	.map(|subslice| Input { value: subslice })
	.ok_or(EndOfInput);
	self.i = new_i;
	ret
	}

here line 318 calculated the new index value, then lines 319-321 try to construct new Input, but since this can fail there's a possibility that ret is Err(EndOfInput). However, this error is not returned when it happens and code continues to line 322 where Reader's index (self.i) is increased and then skip_and_get_input() is done and it returns Err(EndOfInput).

This allows the case where Reader's index can point to outside of buffer thus panicing later.

[briansmith]

Thanks. I can't believe we didn't write it this way originally. I wonder what I was thinking.