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

danielhenrymantilla · 2019-09-01T23:18:13Z

(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 😕

`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 · 2019-09-01T23:27:27Z

src/crc.rs

@@ -167,10 +163,6 @@ impl<R: ReadBytes> ReadBytes for Crc16Reader<R> {
        }
    }

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


.read_into is just .read_exact from io::Read

danielhenrymantilla · 2019-09-01T23:27:53Z

src/error.rs

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

-    fn cause(&self) -> Option<&error::Error> {
+    fn cause(&self) -> Option<&dyn error::Error> {


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

danielhenrymantilla · 2019-09-01T23:29:29Z

src/input.rs

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


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.

danielhenrymantilla · 2019-09-01T23:30:58Z

src/metadata.rs

@@ -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)]


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

danielhenrymantilla · 2019-09-01T23:31:36Z

src/metadata.rs

-    // 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));


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

danielhenrymantilla · 2019-09-01T23:32:17Z

src/metadata.rs

-    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));


::uninit is perfect for this kind of patterns

danielhenrymantilla · 2019-09-01T23:32:52Z

src/metadata.rs

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


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

Shnatsel · 2019-09-02T17:42:30Z

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 · 2019-09-02T17:52:43Z

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 · 2021-01-10T23:11:35Z

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 · 2021-01-11T12:59:58Z

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 · 2022-02-11T13:04:08Z

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 · 2022-08-27T12:25:18Z

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.

Daniel Henry-Mantilla and others added 3 commits August 23, 2019 11:27

Fix unsoundness in refresh_reader

e24eb61

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

Fixed soundness issue with vectors of uninit bytes

9c947cc

Fix bare_trait_object usage

979fc90

danielhenrymantilla changed the title ~~[safety-dance] Reduce some unsound usages of unsafe~~ [safety-dance] Remove some unsound usages of unsafe Sep 1, 2019

danielhenrymantilla commented Sep 1, 2019

View reviewed changes

memoryruins mentioned this pull request Jan 10, 2021

Reading on uninitialized memory may cause UB #26

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

danielhenrymantilla commented Sep 1, 2019

danielhenrymantilla Sep 1, 2019

danielhenrymantilla Sep 1, 2019

danielhenrymantilla Sep 1, 2019

danielhenrymantilla Sep 1, 2019

danielhenrymantilla Sep 1, 2019

danielhenrymantilla Sep 1, 2019

danielhenrymantilla Sep 1, 2019

Shnatsel commented Sep 2, 2019 •

edited

Loading

danielhenrymantilla commented Sep 2, 2019

BlackHoleFox commented Jan 10, 2021

danielhenrymantilla commented Jan 11, 2021

est31 commented Feb 11, 2022

est31 commented Aug 27, 2022

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

Are you sure you want to change the base?

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

Conversation

danielhenrymantilla commented Sep 1, 2019

danielhenrymantilla Sep 1, 2019

Choose a reason for hiding this comment

danielhenrymantilla Sep 1, 2019

Choose a reason for hiding this comment

danielhenrymantilla Sep 1, 2019

Choose a reason for hiding this comment

danielhenrymantilla Sep 1, 2019

Choose a reason for hiding this comment

danielhenrymantilla Sep 1, 2019

Choose a reason for hiding this comment

danielhenrymantilla Sep 1, 2019

Choose a reason for hiding this comment

danielhenrymantilla Sep 1, 2019

Choose a reason for hiding this comment

Shnatsel commented Sep 2, 2019 • edited Loading

danielhenrymantilla commented Sep 2, 2019

BlackHoleFox commented Jan 10, 2021

danielhenrymantilla commented Jan 11, 2021

est31 commented Feb 11, 2022

est31 commented Aug 27, 2022

Shnatsel commented Sep 2, 2019 •

edited

Loading