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

Audit: Ensure there are no assumptions that inputs are reasonably sized #292

Closed
briansmith opened this issue Oct 17, 2022 · 12 comments
Closed

Comments

@briansmith
Copy link
Contributor

When reviewing PR #279 I noticed at least one instance where we assume that a Rust object is no more than isize::MAX bytes long. Although that PR was closed, we should still audit that we do not make that assumption elsewhere in the code. See rust-lang/rust#101899 (comment) and more comments in that issue for evidence that such assumptions aren't generally safe. Arguably the user is doing the wrong thing and triggering UB themselves, but we shouldn't exasperate it.

@josephlr
Copy link
Member

I agree with this, it seems like a reasonable idea, and it doesn't seem that hard to do.

@newpavlov
Copy link
Member

It could be worth to ask the WASI guys to clarify the length argument of random_get, whether it MUST be smaller than i32::MAX or is it interpreted as u32.

I don't think we have similar assumptions in other backends, but I agree it's worth to check to be safe.

@newpavlov
Copy link
Member

It looks like use of i32 is a quirk of the wasi crate, which uses i32 for all input parameters and return results of WASI "syscalls". In the WASI spec the length parameter is defined as u32. See bytecodealliance/wasi-rs#67.

@briansmith
Copy link
Contributor Author

briansmith commented Oct 19, 2022

register_custom_getrandom! generates a function with the following implementation:

        extern "C" fn __getrandom_custom(dest: *mut u8, len: usize) -> u32 {
            let f: fn(&mut [u8]) -> Result<(), $crate::Error> = $path;
            let slice = unsafe { ::core::slice::from_raw_parts_mut(dest, len) };
            match f(slice) {
                Ok(()) => 0,
                Err(e) => e.code().get(),
            }
        }

implicit in this implementation is the expectation that len <= isize::MAX since that is a requirement of from_raw_parts_mut: "The total size len * mem::size_of::<T>() of the slice must be no larger than isize::MAX". Probably register_custom_getrandom! should change how it generates the implementation so that it uses chunking, especially since the caller might not be a Rust function passing a pointer to a (validly-sized) Rust object.

@josephlr
Copy link
Member

Probably register_custom_getrandom! should change how it generates the implementation so that it uses chunking, especially since the caller might not be a Rust function passing a pointer to a (validly-sized) Rust object.

I don't think this is necessary. __getrandom_custom can only be called by the custom getrandom_inner implementation. __getrandom_custom is not part of this crate's external API (we do not document it anywhere, and it is prefixed with __).

So not having chunking for the custom implementation is equivalent to saying that the following is always fine:

fn foo(buf: &mut [u8]) -> &mut [u8] {
  slice::from_raw_parts_mut(buf.as_mut_ptr(), buf.len())
}

which IIUC is always valid in Rust.

@josephlr
Copy link
Member

josephlr commented Oct 20, 2022

The fact that a slice can never exceed isize::MAX bytes does seems to be used in the standard library.

@briansmith
Copy link
Contributor Author

The fact that a slice can never exceed isize::MAX bytes does seems to be used in the standard library.

The issue isn't about slices, but about whether a general (*mut u8, usize) pair. Since it's #[no_mangle], __getrandom_custom will be visible globally in the entire executable/library, including from non-Rust code, even if we don't document it's there. I didn't see anything in the documentation that tells people they shouldn't call it directly. And Hyrum's Law says somebody is probably calling it directly even if they shouldn't.

Regarding the extremely unlikely case that len is too large, we can actually safely handle it at basically no cost and without chunking:

// Ensure `len` is a valid slice length.
    let _ = isize::try_from(len).map_err(|_| Error::WHATEVER)?;

@newpavlov
Copy link
Member

I didn't see anything in the documentation that tells people they shouldn't call it directly.

Starting names with __ is common to indicate private items, even if technically they are visible. Even std relied on this convention when #[non_exhaustive] was not implemented yet.

@josephlr
Copy link
Member

Regarding the extremely unlikely case that len is too large, we can actually safely handle it at basically no cost and without chunking:

I like this approach and would be fine with adding it. It's essentially free and properly alerts the user that something has gone wrong in their program.

@newpavlov
Copy link
Member

newpavlov commented Oct 20, 2022

Regarding the extremely unlikely case that len is too large, we can actually safely handle it at basically no cost and without chunking

It introduces branching, which will be hit every getrandom call. The branching can not be optimized by compiler since __getrandom_custom is effectively opaque for it. Granted, the branch overhead is minuscule, but it's not zero.

@briansmith
Assuming __getrandom_custom is completely private (i.e. it can not be called directly, only through the public getrandom functions), do you agree that in such case chunking or error reporting on len > isize::MAX is unnecessary?

@briansmith
Copy link
Contributor Author

It introduces branching, which will be hit every getrandom call. The branching can not be optimized by compiler since __getrandom_custom is effectively opaque for it. Granted, the branch overhead is minuscule, but it's not zero.

In most CPUs the branch will be effectively optimized away since it's never taken. If we look at the other implementations, they (almost?) all have a branch of some kind and it doesn't make a difference.

@briansmith Assuming __getrandom_custom is completely private (i.e. it can not be called directly, only through the public getrandom functions), do you agree that in such case chunking or error reporting on len > isize::MAX is unnecessary?

In some sense already it isn't really "necessary" since I think we all acknowledge that the bad situation is extremely unlikely. I encountered this when trying to write the // SAFETY: comment for the custom.rs use of unsafe { ... }. I didn't (don't) want to write a bunch of caveats when I could just add one line to eliminate them.

@newpavlov
Copy link
Member

I've taken a cursory look over our code base and I think we can close this issue. We can open new issues on case-by-case basis.

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

No branches or pull requests

3 participants