-
-
Notifications
You must be signed in to change notification settings - Fork 484
Seed weak_rng with the current time if OsRng fails #181
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
Conversation
dhardy
left a comment
There was a problem hiding this 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. |
There was a problem hiding this comment.
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.
|
See also dhardy#21 which makes a similar change on my branch of rand |
Thanks. I see that fixing |
030c961 to
b008e1d
Compare
|
This PR moves the 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 |
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] |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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."
b008e1d to
0812927
Compare
0812927 to
6d297d5
Compare
|
Thanks @cpeterso! |
Seed weak_rng with the current time if OsRng fails
And add a test for
weak_rng.