Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[safety-dance] Remove some unsound usages of unsafe #19

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

danielhenrymantilla
Copy link

safety-dance

(This is currently a WIP PR, for other members of safety-dance to help audit and improve the code)

This PR aims to tackle the issues raised in rust-secure-code/safety-dance#4

Although the author had done a great job with only few instances of unsafe usage, and all quite well documented / justified, it turns out that the usage of unininitalized bytes has currently no defined behavior in Rust. Instead, the MaybeUninit abstraction can be used, to safely handle uninitialized memory. Sadly, the Read trait of the std library does not yet offer an API to work with MaybeUninit.

Hence the usage of a helper crate, ::uninit, which provides precisely these zero-cost sound constructs.

  • the only thing is that for ::uninit::ReadIntoUninit to be usable with the custom reader of the crate, this unsafe trait has been required to be (re)implemented, resulting in some other unsafe instances. However, these unsafe instances are more of a formality (they aim to guard against a malicious implementation of the trait, which is not the case here).

  • so besides these 3 sound usages of unsafe for the ReadIntoUninit impl, the other unsafe blocks have been defused, except for two very well documented and justified cases of unchecked indexing, that I have therefore let be (I consider them sound).


Sadly, I have not been able to run the unit tests because some files are not in the repository. So I haven't been able to benchmark the impact of these changes 😕

Daniel Henry-Mantilla and others added 3 commits August 23, 2019 11:27
`mem::uninitialized` is unsound; and if refresh reader code panicked
between the enclosing `mem::replace`s, the code would be dropping and
thus reading uninitalized memory.
Taking ownership in and back out is the most simple solution to the
problem.
@danielhenrymantilla danielhenrymantilla changed the title [safety-dance] Reduce some unsound usages of unsafe [safety-dance] Remove some unsound usages of unsafe Sep 1, 2019
@@ -167,10 +163,6 @@ impl<R: ReadBytes> ReadBytes for Crc16Reader<R> {
}
}

fn read_into(&mut self, _buffer: &mut [u8]) -> io::Result<()> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.read_into is just .read_exact from io::Read

@@ -70,7 +70,7 @@ impl error::Error for Error {
}
}

fn cause(&self) -> Option<&error::Error> {
fn cause(&self) -> Option<&dyn error::Error> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this may not have been a great idea if the plan is to support old versions of Rust

buffer: &'buf mut [MaybeUninit<u8>],
) -> io::Result<&'buf mut [u8]>
{
self.read_into_uninit_exact(buffer)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid code repetition for a start, later on this could be improved to use .read_into_uninit_exact's implementation, but replacing the EOF error with a return of the initialized subslice.

@@ -189,7 +191,7 @@ impl<'a> Iterator for GetTag<'a> {
#[inline]
fn next(&mut self) -> Option<&'a str> {
// This import is actually required on Rust 1.13.
#[allow(unused_imports)]
#[allow(deprecated, unused_imports)]
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(imho we shouldn't be targetting such an old version of Rust ...)

// is not exposed. If `read_into` succeeds, it will have overwritten all
// bytes. If not, an error is returned and the memory is never exposed.
unsafe { vendor_bytes.set_len(vendor_len as usize); }
try!(input.read_into(&mut vendor_bytes));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was UB because it was creating a &mut reference to uninitialized bytes

unsafe { vendor_bytes.set_len(vendor_len as usize); }
try!(input.read_into(&mut vendor_bytes));
let mut vendor_bytes = Vec::new();
try!(vendor_bytes.extend_from_reader(vendor_len as usize, &mut input));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

::uninit is perfect for this kind of patterns

where
R : ReadIntoUninit + ReadBytes,
{
let length_minus_four = match (length as usize).checked_sub(4) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's keep a witness of length - 4 not underflowing for later usage.

@Shnatsel
Copy link
Contributor

Shnatsel commented Sep 2, 2019

Why is implementing ReadIntoUninit unsafe? std::io::Read provides a way to write to a Vec yet implementing it is totally safe. Reading into uninit is exactly as safe as reading into a Vec.

@danielhenrymantilla
Copy link
Author

Take this code pattern (which I expect to be kind of reimplemented by crate users for similar but not quite equal needs): https://docs.rs/uninit/0.1.0/src/uninit/lib.rs.html#195-226

The last step, where it is assumed that buf[.. n] has been init thanks to a series of calls to .read_into_init whose returned lengths add up to n, is only sound if .read_into_init "isn't trolling" / malicious.

  • Imagine an implementation of .read_into_init returning a Box::leak(Box::new([0_u8])). A slice with length > 0 has been returned, but the input buffer is still uninit, so assuming that the first length elements have been init would be unsound.

From the docs:

  • buf[.. init_buf.len()] is sound to assume_init

unsafe code can assume this property to skip checks or manual initialization, and that's why incorrectly impl-ementing this marker trait can compromise memory safety.


So, this is one of these cases where the trait needs to be unsafe, although there is no danger if the implementation is not deliberately trying to abuse it; such as with TrustedLen

@BlackHoleFox
Copy link

I'm not sure where this PR left off, but I found this library today but the UB is concerning, especially considering I want to parse potentially untrusted FLAC files. Can I help this one out somehow?

I tried benchmarking this branch vs master, but some new runtime UB checks make it impossible to run the benchmarks. Every benchmark fails with this output:

cargo +nightly bench
...
---- bench_p0_mono_16bit stdout ----
thread 'main' panicked at 'attempted to leave type `claxon::FlacReader<std::io::Cursor<std::vec::Vec<u8>>>` uninitialized, which is invalid', /Users/fox/.rustup/toolchains/nightly-x86_64-apple-darwin/lib/rustlib/src/rust/library/core/src/mem/mod.rs:671:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The danielhenrymantilla:fix_unsoundness fork also fails to compile because uninit v0.1 has been yanked and any future versions have a different public API, resulting in compilation failures.

@danielhenrymantilla
Copy link
Author

Thanks for reminding me of this PR; I now have enough time to go back at tackling and polishing this 🙂 .

For instance, the uninit crate has been updated and improved since (with indeed some necessary API changes to prevent "an unsound troll misusage" (such misusage is not present in this PR) that that v0.1 allowed (hence the yanking).

@est31
Copy link

est31 commented Feb 11, 2022

I'd really love something like this PR. I've reviewed the unsafe uses of this crate and they are, well, not great at all. I think that the uninit crate introduces way too much disruption. Yanking semver versions has an impact on the supply chain of unjustified proportions, especially if done because you think the old version's API design allows for misuse. There are dedicated tools that you can register CVEs with.

@est31
Copy link

est31 commented Aug 27, 2022

As an update, rust is considering to deprecate mem::uninitialized completely: rust-lang/rust#100342 . I think that some PR should be merged so that this crate doesn't use that function any more, ideally sooner rather than later.

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

Successfully merging this pull request may close these issues.

4 participants