Sectors::get accesses unclaimed/uninitialized memory

[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.

calamine/src/cfb.rs

Lines 243 to 261 in 2391d0c

	fn get<R: Read>(&mut self, id: u32, r: &mut R) -> Result<&[u8], CfbError> {
	let start = id as usize * self.size;
	let end = start + self.size;
	if end > self.data.len() {
	let mut len = self.data.len();
	unsafe {
	self.data.set_len(end);
	}
	// read_exact or stop if EOF
	while len < end {
	let read = r.read(&mut self.data[len..end]).map_err(CfbError::Io)?;
	if read == 0 {
	return Ok(&self.data[start..len]);
	}
	len += read;
	}
	}
	Ok(&self.data[start..end])
	}

Issue#1: Write to unclaimed memory.

self.data.set_len(end) is done without reserving extra memory for self.data.
After that, new data is written to the unclaimed memory via r.read(&mut self.data[len..end]).
This seems to be a critical security error, as r.read(&mut self.data[len..end]) can potentially write to memory that is actively occupied by another entity.

Issue#2: read() on uninitialized memory can cause undefined behavior

r.read(&mut self.data[len..end]) can potentially pass uninitialized memory to user-provided Read implementation. 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 would be safe to zero-initialize the newly allocated u8 buffer before read()
(probably via self.data.resize(end - len, 0)),
in order to prevent user-provided Read from accessing old contents of the newly allocated heap memory.

Thank you for checking out this issue 👍

[Shnatsel]

Heads up: this issue has been included in the RustSec advisory database. It will be surfaced by tools such as cargo-audit or cargo-deny from now on.

Once a fix is released to crates.io, please open a pull request to update the advisory with the patched version, or file an issue on the advisory database repository.

[tafia]

PR opened on RustSec repo.