Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Decoder::read_non_compressed_block() is unsound #31

Closed
Shnatsel opened this issue Jun 19, 2019 · 4 comments
Closed

Decoder::read_non_compressed_block() is unsound #31

Shnatsel opened this issue Jun 19, 2019 · 4 comments

Comments

@Shnatsel
Copy link
Contributor

The following code is unsound:

let old_len = self.buffer.len();
self.buffer.reserve(len as usize);
unsafe { self.buffer.set_len(old_len + len as usize) };
self.bit_reader
.as_inner_mut()
.read_exact(&mut self.buffer[old_len..])?;

The slice passed to read_exact() is uninitialized. This uses a Read implementation supplied by the API user, and there is no guarantee that it will never read from the provided buffer. If it does, it may cause a memory disclosure vulnerability.

Similar bug in Rust MP4 parser for reference: mozilla/mp4parse-rust#172

The equivalent code in stdlib initializes the vector with zeroes before growing it: https://doc.rust-lang.org/src/std/io/mod.rs.html#355-391

There have been some language proposals to create a contract for never reading from the buffer in this case, but they have not been stabilized: rust-lang/rust#42788

For now replacing unsafe { self.buffer.set_len(old_len + len as usize) }; with self.buffer.resize(old_len + len as usize, 0); should fix it.

I have not read all of the unsafe code in libflate, there may be similar issues in other unsafe blocks, which is why I'm opening an issue instead of a PR right away.

@Shnatsel
Copy link
Contributor Author

This code might also be affected:

let mut reader = mem::replace(&mut self.decoder, Err(unsafe { mem::uninitialized() }))

@sile
Copy link
Owner

sile commented Jun 24, 2019

Thank you for your detailed report.
I would like to fix unsound code one by one.

Shnatsel added a commit to Shnatsel/libflate that referenced this issue Jun 24, 2019
@Shnatsel
Copy link
Contributor Author

This seems to be the only occurrence of this issue. Everywhere else read_exact() is passed properly initialized buffers.

sile added a commit that referenced this issue Jun 25, 2019
@Shnatsel
Copy link
Contributor Author

Fixed by the linked PR.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants