reading on uninitialized buffer can cause UB (impl<R> BufRead for GreedyAccessReader<R>)

[JOE1994]

Hello 🦀 ,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

bra-rs/src/greedy.rs

Lines 190 to 225 in 2b3c455

	impl<R> BufRead for GreedyAccessReader<R>
	where
	R: Read,
	{
	fn fill_buf(&mut self) -> IoResult<&[u8]> {
	if self.buf.capacity() == self.consumed {
	self.reserve_up_to(self.buf.capacity() + 16);
	}
	let b = self.buf.len();
	let buf = unsafe {
	// safe because it's within the buffer's limits
	// and we won't be reading uninitialized memory
	std::slice::from_raw_parts_mut(
	self.buf.as_mut_ptr().offset(b as isize),
	self.buf.capacity() - b)
	};
	match self.inner.read(buf) {
	Ok(o) => {
	unsafe {
	// reset the size to include the written portion,
	// safe because the extra data is initialized
	self.buf.set_len(b + o);
	}
	Ok(&self.buf[self.consumed..])
	}
	Err(e) => Err(e),
	}
	}
	fn consume(&mut self, amt: usize) {
	self.consumed += amt;
	}
	}

GreedyAccessReader::fill_buf method creates an uninitialized buffer and passes it to user-provided Read implementation (self.inner.read(buf)). This is unsound, because it allows safe Rust code to exhibit an undefined behavior (read from uninitialized memory).

This part from the Read trait documentation explains the issue:

It is your responsibility to make sure that buf is initialized before calling read. Calling read with an uninitialized buf (of the kind one obtains via MaybeUninit<T>) is not safe, and can lead to undefined behavior.

Suggested Fix

It is safe to zero-initialize the newly allocated u8 buffer before read(), in order to prevent user-provided Read from reading old contents of the newly allocated heap memory.

The version available on Crates.io seems to be different from the latest master branch of this repo,
but the same issue exists in GreedyBufReader::fill_buf() (bra = "0.1.0").

Thank you for checking out this issue 👍

[Enet4]

Hello, and thank you for the detailed report!

It is a bit surprising to find out that the documentation for read was expanded to explicitly require the slice to be initialized. The paragraph quoted from the documentation did not even exist back when I implemented this (the associated commit was released with Rust 1.37.0). Still, it is understandable why this has to be done, considering that there is no unsafe construct barring someone from making a poorly behaved impl of Read.

Although this is a very low-traffic crate, I will take the necessary measures to fix this and release a patched version. 👍

[Enet4]

Thank you for the heads up @JOE1994. v0.1.1 is out with the fix, and v0.1.0 has been yanked from crates.io.