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

Seed weak_rng with the current time if OsRng fails #181

Merged
merged 3 commits into from
Oct 27, 2017

Conversation

cpeterso
Copy link
Contributor

And add a test for weak_rng.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

The main thing I want to say here is that Xorshift is a terrible PRNG to use to seed other PRNGs with. If you want a fallback source of entropy to use when seeding stuff, I'd recommend still using a better PRNG internally (e.g. Isaac: StdRng).

Xorshift is really so bad for seeding stuff that if you seed one XorshiftRng from another, they essentially end up being clones. The refactor in progress will very likely remove this algorithm.

Possibly my recommendation here would be to adapt thread_rng instead, and make weak_rng seed from thread_rng. This does make thread_rng weaker, but it's not supposed to be used for crypto anyway. Not 100% sure if this is agreeable; alternatively you could add a new function.

src/lib.rs Outdated
Err(e) => panic!("weak_rng: failed to create seeded RNG: {:?}", e)
Err(_) => {
// Set a bit because XorShiftRng will panic if the seed is
// entirely zero.
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you don't want this to panic if precise_time_ns is so broken it returns zero? You've got zero entropy in this case... for some uses that doesn't matter but not necessarily all.

@dhardy
Copy link
Member

dhardy commented Oct 23, 2017

See also dhardy#21 which makes a similar change on my branch of rand

@cpeterso
Copy link
Contributor Author

cpeterso commented Oct 23, 2017

Possibly my recommendation here would be to adapt thread_rng instead, and make weak_rng seed from thread_rng. This does make thread_rng weaker, but it's not supposed to be used for crypto anyway. Not 100% sure if this is agreeable; alternatively you could add a new function.

Thanks. I see that fixing thread_rng instead of just weak_rng would avoid these panics for other callers. I will need to check that thread_rng's periodic reseeding also avoids these panics.

@cpeterso
Copy link
Contributor Author

This PR moves the OsRng fallback into thread_rng and then uses thread_rng to seed weak_rng.

I construct the weak fallback seed from the current system time using std::time, as suggested in dhardy#21 (comment), instead of adding a new crate dependency just for time::precise_time_ns().

src/lib.rs Outdated
let unix_time = now.duration_since(time::UNIX_EPOCH).unwrap();
let secs = unix_time.as_secs() as usize;
let nanos = unix_time.subsec_nanos() as usize;
[(secs << 32) | nanos]
Copy link
Member

Choose a reason for hiding this comment

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

1s = 10^9 ns, not 2^32 ns

It doesn't matter precisely, but if you want a fast approximation use << 30 to avoid leaving 2 empty bits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If usize is 32 bits, then shifting secs << 32 (or 30) will lose data. Instead of shifting any bits, I could return them all like [secs, nanos] and let StdRng use the bits as it sees fit. A comment in isaac.rs says reseed() will "make the seed into [seed[0], seed[1], ..., seed[seed.len() - 1], 0, 0, ...], to fill rng.rsl."

@alexcrichton alexcrichton merged commit 81a5422 into rust-random:master Oct 27, 2017
@alexcrichton
Copy link
Contributor

Thanks @cpeterso!

@cpeterso cpeterso deleted the cpeterso-weak_rng-seed branch October 30, 2017 05:08
pitdicker pushed a commit to pitdicker/rand that referenced this pull request Apr 4, 2018
 Seed weak_rng with the current time if OsRng fails
@dhardy dhardy mentioned this pull request Jan 17, 2019
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.

3 participants