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

Try /dev/random before using /dev/urandom #338

Merged
merged 8 commits into from
Mar 26, 2018

Conversation

pitdicker
Copy link
Contributor

@dhardy You are maybe not going to like this PR (it is partly similar to dhardy#48), but I try it anyway 😄.

I have removed the custom ReadRng wrapper from OsRng, which was only used by Redox, and the module with Linux and non-specified Unix platforms.

Redox now has a very basic implementation, without caching the file descriptor. I am not sure it is needed there https://github.com/redox-os/redox/issues/1172.

The implementation of ReadRng is mostly folded into the Linux etc. module. It will now attempt to read a single byte from /dev/random to confirm the OS RNG is initialized, before using /dev/urandom.

In two ways this is not entirely optimal:

  • /dev/random may error when the system for some reason thinks it is out of 'entropy', giving a false positive. The change of this happening are unlikely, especially when you consider we only do this test once during the lifetime of the program.
  • We use this method on all Unix platforms, while on some there is no difference between the two random devices. So this could be slightly slower than necessary there.

But I think these problems are minor.

@pitdicker
Copy link
Contributor Author

I'll work a bit more on this one...

@pitdicker
Copy link
Contributor Author

pitdicker commented Mar 26, 2018

I have taken the numbers for NR_GETRANDOM for a few extra platforms from systemd. MIPS_SIM_ABI32 is Rusts mips target (old ABI), and MIPS_SIM_ABI64 the mips64 target.

I know I made a type in the function but I am stumped by this error by the CI for ARM: https://travis-ci.org/rust-lang-nursery/rand/jobs/358410616

getrandom should simply be available for target_arch = "arm", but somehow it isn't...
@dhardy Do you have an idea how to investigate this? I have the same error locally when compiling for armv7-linux-androideabi, and I can't find an obvious typo...

Edit: found the problem. target_os="android" 😄

@pitdicker
Copy link
Contributor Author

I added back storing the fd in a static on Redox, and the CI is finally green 🎉. Gives a little less confidence though...

I think it is ready to took a look at.

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.

Surprise surprise, I do actually like the changes, though it took me a while to review!


Ok(ReadRng {})
}

fn try_fill_bytes(&mut self, dest: &mut [u8]) -> Result<(), Error> {
Copy link
Member

Choose a reason for hiding this comment

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

I wish diffs were smart enough to realise that the only thing changed was a variable name... these tools haven't changed much since the '70s!

src/os.rs Outdated

// Since we have an instance of Self, we can assume that our memory was
// set with a valid object.
let mutex = unsafe{ READ_RNG_FILE.as_ref().unwrap() };
Copy link
Member

Choose a reason for hiding this comment

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

This comment is incorrect; it was there in ReadRng because existence of the self parameter proves the constructor ran. Here we just have to use local-analysis to see that this function only gets called after open_dev_random succeeds.


// Since we have an instance of Self, we can assume that our memory was
// set with a valid object.
let mutex = unsafe { READ_RNG_FILE.as_ref().unwrap() };
Copy link
Member

Choose a reason for hiding this comment

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

Here the old comment actually makes sense — we have self on an object whose constructor initialised the mutex.

@dhardy
Copy link
Member

dhardy commented Mar 26, 2018

I have taken the numbers for NR_GETRANDOM for a few extra platforms from systemd.

I'll just trust you got this right... or wait for error reports.

getrandom should simply be available for target_arch = "arm", but somehow it isn't...

Wait, the full genrandom impl reduced to just {} on ARM? That sounds weird. Or if you had a stub implementation, it should probably be { -1 } like the other stub.


Yep, happy for this to be merged (though one comment ought to be fixed as mentioned).

@pitdicker
Copy link
Contributor Author

😄 Nice!

Wait, the full genrandom impl reduced to just {} on ARM? That sounds weird. Or if you had a stub implementation, it should probably be { -1 } like the other stub.

I forgot to add back the { -1 } part. But what surprised me was that we never actually used genrandom on Android at all. So my mistake was in a way useful to figure if we are using the syscall as assumed.

@pitdicker pitdicker mentioned this pull request Mar 26, 2018
@pitdicker
Copy link
Contributor Author

Ready to merge?

@dhardy dhardy merged commit de231c8 into rust-random:master Mar 26, 2018
@pitdicker pitdicker deleted the osrng_extra_check branch March 26, 2018 17:20
pitdicker pushed a commit that referenced this pull request Apr 4, 2018
 Try /dev/random before using /dev/urandom
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.

2 participants