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

random_seed is UB #2

Closed
nico-abram opened this issue Feb 16, 2022 · 10 comments
Closed

random_seed is UB #2

nico-abram opened this issue Feb 16, 2022 · 10 comments

Comments

@nico-abram
Copy link

nico-abram commented Feb 16, 2022

Here:

temporary/src/lib.rs

Lines 143 to 146 in 6821517

fn random_seed(_: &Path, _: &str) -> [u64; 2] {
use std::mem::uninitialized as rand;
unsafe { [rand::<u64>() ^ 0x12345678, rand::<u64>() ^ 0x87654321] }
}

According to the std::mem::uninitialized docs this is UB: https://doc.rust-lang.org/std/mem/fn.uninitialized.html

As the assume_init documentation explains, the Rust compiler assumes that values are properly initialized. As a consequence, calling e.g. mem::uninitialized::() causes immediate undefined behavior for returning a bool that is not definitely either true or false. Worse, truly uninitialized memory like what gets returned here is special in that the compiler knows that it does not have a fixed value. This makes it undefined behavior to have uninitialized data in a variable even if that variable has an integer type. (Notice that the rules around uninitialized integers are not finalized yet, but until they are, it is advisable to avoid them.)

This crate could possibly give you an easy way to get a real entropy source, if desireable: https://github.com/rust-random/getrandom

Otherwise, you could just use hardcoded values like [0 ^0x12345678, 0 ^ 0x87654321]

If you still want a hacky inbetween, maybe consider using a function address and relying on ASLR being enabled (Like random_seed as usize as u64).

Also notice that sometimes the compiler just replaces uninitialized() with a fixed value (Like 0), so it would be no better than a fixed value entropy-wise despite being UB.

EDIT: Maybe it could also be replaced with inline asm to read the stack, but that would require a different asm block for each ISA and not a real source of entropy.

@IvanUkhov
Copy link
Member

Hello, thank you for the issue! For my understanding, is the concern that there is a usage of unsafe code? I left it that way so that, in some nonoptimized contexts, it would behave less predictably, but it is clear that random_seed is not that random, and that it would probably be some constant returned. For this create, this is not critical. In addition, I wanted to eventually make use of the two arguments passed into random_seed to allow the caller to have some influence.

@5225225
Copy link

5225225 commented Feb 19, 2022

For my understanding, is the concern that there is a usage of unsafe code?

The issue is not unsafe code, the issue is incorrect code.

It's undefined behaviour to use a variable that has not been initialised (there's... some subtle points around what exactly "use" is, but doing a bitwise xor, or returning it from a function is definitely use).

If the uninit variable was being used as a "i will initialise this later" slot, then you could use MaybeUninit. But it's not, it's being used as a value and you're trying to get entropy from there. That will never be allowed, and the compiler is allowed to assume you don't do that (hence it being optimised out to a [0, 0] return in release mode)

Honestly, I'd add a dependency on getrandom and let that do the actual hard part of securely getting good entropy for you.

This crate acts completely predictably with optimised mode (as I noted in my advisory for this crate: rustsec/advisory-db#1196), and some libraries do run their tests in release mode for performance gains.

I'm not terribly concerned about this crate because it's generally used only for tests, so at least this isn't being run "in the wild". Still, I'd make it use getrandom and ideally yank all versions of the crate that use this method of getting entropy.

@IvanUkhov
Copy link
Member

Thank you for the response. Can you please give an example of how it might lead to problems in this case?

@5225225
Copy link

5225225 commented Feb 19, 2022

Well, if you run tests in release, your names are always the same. So you cannot run 2 tests that rely on unique names in release.

And in general you're relying on the compiler to happen to not notice what you're doing. (These might even be turned into unconditional panics in the future, but work towards doing that is ongoing and somewhat slow, because people write stuff that happens to work most of the time.)

@IvanUkhov
Copy link
Member

IvanUkhov commented Feb 19, 2022

Sorry, maybe it was not clear from before. This is the expected behavior. Of course, it will return the same name. This is partially the reason there is a retry loop:

https://github.com/stainless-steel/temporary/blob/master/src/lib.rs#L59

Eventually it will run out of retries, but the probability of this happening is low, especially given we are talking about testing. One can push it even further by updating random_seed how I alluded to above.

@5225225
Copy link

5225225 commented Feb 19, 2022

I guess, but in that case, why not just not have the entropy in the first place?

If you have the retries, and tests run in release will never get any entropy, that's equal to using tmp.00001 and incrementing the counter until you find a free directory.


Edit: either not have entropy or collect it properly (getrandom)

@nico-abram
Copy link
Author

For my understanding, is the concern that there is a usage of unsafe code?

No, there is nothing wrong with using unsafe when you need it. The concern is specifically that the unsafe code is Undefined Behaviour (a.k.a wrong/incorrect). To quote the reference (https://doc.rust-lang.org/reference/behavior-considered-undefined.html):

Rust code is incorrect if it exhibits any of the behaviors in the following list. This includes code within unsafe blocks and unsafe functions. unsafe only means that avoiding undefined behavior is on the programmer; it does not change anything about the fact that Rust programs must never cause undefined behavior.

It also explicitly calls out this case:

An integer (i*/u*), floating point value (f*), or raw pointer obtained from uninitialized memory, or uninitialized memory in a str.

Like 522 mentioned, this is not guaranteed to work. The compiler is free to remove this function, or turn it into an undefined instruction like ud2, or turn it into an unconditional panic. For an example of the compiler turning this kind of thing into an unconditional panic for some types (Specifically types that do not permit zero-initialization) see rust-lang/rust#66059
This example panics at runtime with the message "attempted to leave type core::num::nonzero::NonZeroUsize uninitialized, which is invalid":


fn main() {
    unsafe {
        let _x: core::num::NonZeroUsize = std::mem::uninitialized();
    }
}

I will repeat a part of the quote from the docs in my first comment:

(Notice that the rules around uninitialized integers are not finalized yet, but until they are, it is advisable to avoid them.)

Which means this could be relaxed in the future but it is currently considered UB. As far as I know the last discussion about that question happened here (rust-lang/unsafe-code-guidelines#71). Regardless, it's a good idea to fix this code under the rules from the current documentation instead of assuming it will eventually be considered valid.

@5225225
Copy link

5225225 commented Feb 19, 2022

(Specifically types that do not permit zero-initialization)

It's not just that, char also panics. I think the rules are "if it has no niches we let it pass for now, but we currently don't check arrays because that breaks too much stuff".

Regardless, doing operations on uninit values will never be not UB. You'd need freeze for that.

@IvanUkhov
Copy link
Member

I have updated the function to have no undefined behavior. Thank you for your input!

@5225225
Copy link

5225225 commented Feb 20, 2022

cool, thank you very much!

I updated the advisory to list the patched version, so anyone on older versions can upgrade / get their dependencies to upgrade.

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

No branches or pull requests

3 participants