Several OsRng improvements #484

pitdicker · 2018-05-28T11:39:42Z

Documentation links to the relevant man pages.
A bit more explanation around wasm, as it turns out to be a bit confusing.
Removed the blanket Unix implementation of reading from /dev/urandom. Besides Linux (which it is pretty specialized for), it was only 'correct' for NetBSD. I think it is better to not compile, and add support for a platform, than to have a default implementation. Then we can make sure it is tested, and matches the recommendations for that platform.
NetBSD: now uses a simple zero-sized struct for OsRng.
Dragonfly BSD: reads from /dev/random. /dev/urandom is faster, but using IBAA, the predecessor of ISAAC, of which the security is unknown.
Added support for Bitrig. It is obsolete, but costs us nothing, as it is the same as OpenBSD.
Emscripten: reads from /dev/urandom, without testing /dev/random, and splits the read up in chunks.
stdweb split the read up in chunks of 65536 bytes.
Moved shared code around reading from a random device to a unix module.
Removed the WASM without stdweb stub (never understood why we should have it).
Added test that reading 0 bytes works.

Fixes #332.

vks · 2018-05-28T13:23:59Z

Did you test this for all those operating systems?

pitdicker · 2018-05-28T15:39:03Z

Did you test this for all those operating systems?

I wish 😄. But most of the changes are around /dev/random and /dev/urandom. I did test those changes by faking the module to be Linux, and made the choice between those, or to use chunks, from the man pages.

I finished digging though the Haiku source code (with help from a virtual machine). There is just about no documentation, but /dev/random and /dev/urandom seem to be the same, and it uses the Yarrow RNG. So added support for that.

And I am trying to test Solaris with a syscall interface. Trying to cross-compile from a virtual machine with Ubuntu, to test it on a virtual machine with Solaris. 😢

dhardy

There are some very nice clean-ups here!

Removed the WASM without stdweb stub (never understood why we should have it).

Initially this was added to allow compiling to WASM, possibly with the goal of making remaining functionality available (not quite sure). By removing this I think we'll now get a compile-failure? Another option would be to feature-gate like with no_std. Or maybe WASM without stdweb should only work in no_std mode, I don't know.

Regardless, I'm a little concerned about this change, especially for the 0.5 release, though it may be the best option in the end. (Also that snippet in JitterRng which just fails — ideally we would not include features which just fail at run-time.)

dhardy · 2018-05-28T15:46:25Z

src/rngs/os.rs

+/// `wasm32-unknown-emscripten` and `wasm32-experimental-emscripten` use
+/// Emscripten's emulation of `/dev/random` on web browsers and Node.js.
+/// Unfortunately it falls back to the insecure `Math.random()` if a browser
+/// doesn't support [`Crypto.getRandomValues`][12].a browser doesn't support Crypto.getRandomValues.


Copy+paste error?

dhardy · 2018-05-28T15:52:19Z

src/rngs/os.rs

+    // Read a single byte from `/dev/random` in non-blocking mode, to determine
+    // if the OS RNG is already seeded.
+    fn try_dev_random() -> Result<(), io::Error> {
+        info!("OsRng: opening random device /dev/random");


"testing random device"?

dhardy · 2018-05-28T15:58:47Z

src/rngs/os.rs

+    #[cfg(any(target_arch = "x86_64", target_arch = "x86",
+              target_arch = "arm", target_arch = "aarch64",
+              target_arch = "s390x", target_arch = "powerpc",
+              target_arch = "mips", target_arch = "mips64"))]


It would be nicer still if this repetition could be avoided. Perhaps const NR_GETRANDOM: Option<libc::c_long> and a single definition of getrandom?

dhardy · 2018-05-28T15:59:42Z

src/rngs/os.rs

+    #[cfg(any(target_arch = "x86_64", target_arch = "x86",
+              target_arch = "arm", target_arch = "aarch64",
+              target_arch = "s390x", target_arch = "powerpc",
+              target_arch = "mips", target_arch = "mips64"))]


Same, seems like this function doesn't need two definitions.

dhardy · 2018-05-28T16:01:04Z

src/rngs/os.rs

-        Ok(())
+    impl OsRng {
+        pub fn new() -> Result<OsRng, Error> {
+            open_random_device("/dev/urandom", false)?;


Comment above says read from /dev/random. Which is correct?

pitdicker · 2018-05-28T17:29:02Z

Initially this was added to allow compiling to WASM, possibly with the goal of making remaining functionality available (not quite sure)

I think that explaines it. We didn't have a no_std mode at the time, so that only the traits and PRNGs could be used, and also no rand_core crate.

Regardless, I'm a little concerned about this change, especially for the 0.5 release, though it may be the best option in the end.

I am not completely used to the idea of having some sort of stability 😄. But I don't think this PR really makes anything worse on the systems that Rust supports.

(Also that snippet in JitterRng which just fails — ideally we would not include features which just fail at run-time.)

I think we need to test someday that is using a good timer on all platforms Rust supports. And then we may even re-implement that little part that std takes care of now ourselves to. But I am not sure what to do if there is no good timer available, as with WASM.

pitdicker · 2018-05-28T17:39:21Z

I am not sure adding the commit to add support for Solaris is the right thing to add.

I think it is correct or pretty close, as it is mostly the same as the one for Linux. It compiles, but I just can't get it to link against my copy of the libraries from Solaris...

Is having something better than nothing?

dhardy

Looks good! How much of this is tested? Still, we do the best we can...

dhardy · 2018-05-29T08:32:12Z

src/rngs/os.rs

+                } else if kind == io::ErrorKind::WouldBlock {
+                    // Potentially this would waste bytes, but since we use
+                    // /dev/urandom blocking only happens if not initialised.
+                    // Also, wasting the bytes in dest doesn't matter very much.


Did you understand the man page? Apparently the function returns -1 when it would block, and may set 0 or the number of bytes requested, and one should check the return value — but this is -1!

That is an old comment, I noticed it after making the commit. I was going over the documentation a bit more right now.

Hmm, the manual is not very clear. Both -1 and 0 can indicate an error. We should ask for bytes in chunks of 1024, while it are chunks of 1040 for /dev/random. And apparently it is guaranteed the chunk it either not or completely filled.

1040 is an unusual chunk size. Yeah, I love confusing manuals (not).

pitdicker · 2018-05-29T10:24:19Z

One thing that I think still needs a better look is what happens when the OS RNG is not yet seeded. Does it block, return an error, return lower-quality bytes, unknown, or n/a? And does that happen in OsRng::new() or OsRng::try_fill_bytes?

And I wonder if it makes sense to add a little more abstraction in this file, not sure yet.

pitdicker · 2018-05-29T13:35:18Z

Sorry, I keep playing with this file.

I have tried to refactor the OsRng implementations, using this trait:

trait OsRngImpl where Self: Sized {
    fn new() -> Result<Self, Error>;
    fn fill_chunk(&mut self, dest: &mut [u8]) -> Result<(), Error>;
    fn max_chunk_size(&self) -> Option<usize> { None }
    fn method_str(&self) -> &'static str;
}

In my opinion it helped making things easier to understand. Logging and dividing into chunks is now done in one place (and thus uniformly), in try_fill_bytes.

I have tried cross-compiling almost all targets. It turned out Fuchsia and CloudABI didn't work before...

Redox doesn't work yet. The unix module depends on libc, for test opening /dev/random. Maybe it is best to move that piece elsewhere, but not sure how yet. It is quite platform-specific.

Edit: one disadvantage of the changes here is that read_random_device has to keep locking the mutex again for every chunk it has to read. This matters only on Solaris and Emscripten (which set a maximum buffer size), but in my benchmarks it didn't cause any real overhead.

dhardy

Sensible idea to use a trait. Still not entirely convinced about replacing try_fill_bytes with three other methods but seems okay.

dhardy · 2018-05-29T09:57:48Z

src/rngs/os.rs

+        }
+        for slice in dest.chunks_mut(max) {
+            let result = getrandom(&mut slice);
+            if result == -1 || result == 0 {


Might as well just do result <= 0 in this case since on success this is always > 0, though according to the manual (can we trust it?) it only returns 0 on error.

I think in this case I like to point out both, so it is clearly not a typo.

dhardy · 2018-05-29T15:07:07Z

src/rngs/os.rs

+trait OsRngImpl where Self: Sized {
+    fn new() -> Result<Self, Error>;
+    fn fill_chunk(&mut self, dest: &mut [u8]) -> Result<(), Error>;
+    fn max_chunk_size(&self) -> Option<usize> { None }


May as well just use core::usize::MAX and drop the Option since this is the default impl.

dhardy · 2018-05-29T15:12:13Z

src/rngs/os.rs

-                OsRngMethod::RandomDevice => read_random_device(slice, Some(1040)),
-             }
+                OsRngMethod::GetRandom => Some(1024),
+                OsRngMethod::RandomDevice => Some(1040),


Seriously? Save the logic and just use 1024.

dhardy · 2018-05-29T15:18:45Z

src/rngs/os.rs

            }
            Ok(())
        }
+
+        fn max_chunk_size(&self) -> Option<usize> {
+            Some(<ULONG>::max_value() as usize)


I assume this is u32::MAX. Should probably be the default; it's not like anyone will use more anyway.

It costs us nothing to do the right thing.

pitdicker · 2018-05-29T16:44:20Z

I am mostly happy with it now. And thank you for also looking carefully 😄.

I am not sure yet what the right thing to do is around blocking. On Linux, NetBSD and Solaris we have the choice between an error and blocking. Other systems always block, or don't document what happens when the OS RNG hasn't yet collected enough entropy.

And Linux, FreeBSD (not NetBSD) and Solaris have the ability to poll on /dev/random to wait until it becomes available.

We currently choose to get an error, and have some approximation of blocking in the fill_bytes wrapper using thread::sleep().

Would it make sense to store whether we know the OS RNG is not yet initialized, and to use a blocking read in fill_bytes instead of trying to schedule ourselves? And to expose some functions like this for users?

pitdicker · 2018-05-29T20:23:07Z

Added one more commit. This is not getting easier...

I added a test_initialized methods. On most systems this is a no-op. On Linux, NetBSD and Solaris is can be used to return an error if the OS RNG it not yet initialized, or to block instead. Once we know the OS RNG is initialized, this is stored in a static and not checked again.

fill_bytes will use the blocking variant, try_fill_bytes the error variant. A small advantage over doing the check in OsRng::new() is that the random bytes generated are not wasted, and that we don't have to do our own primitive scheduling.

Edit: Still doesn't work quite right on Solaris...

pitdicker · 2018-05-30T07:26:50Z

Update to use the correct open flags on Solaris.

I'll stop touching this now 😄.

dhardy · 2018-05-30T10:50:06Z

I might wait a couple of days before reviewing just in case 😆

dhardy · 2018-06-01T08:23:15Z

I had a look at the recent changes — generally looks good, but goes a bit beyond what I can easily review 😄 . I'll leave it to you @pitdicker; if you think it's ready the merge this.

Several OsRng improvements

cb458cf

pitdicker mentioned this pull request May 28, 2018

Add wasm_bindgen support #478

Closed

Add support for Solaris and Haiku

43b4c1b

dhardy reviewed May 28, 2018

View reviewed changes

Clean up

fbc4002

dhardy reviewed May 29, 2018

View reviewed changes

Better handle and log buffer limits

a60cd53

Refactor OsRng

8c67fc4

dhardy reviewed May 29, 2018

View reviewed changes

pitdicker added 2 commits May 29, 2018 18:06

Move the /dev/random tests to their platform modules

8b550f3

Simplify max_chunk_size

246a278

Test whether the OS RNG is initialized in {try_}fill_bytes

a69c6d5

pitdicker added 3 commits May 30, 2018 08:37

Use correct open flags

07f86f1

Update OsRng documentation

f824d91

Update changelog

8ae5b43

pitdicker force-pushed the osrng_improvements branch from 026489a to 8ae5b43 Compare May 30, 2018 07:26

Remove duplication

a87c65e

pitdicker merged commit 3b0a884 into rust-random:master Jun 1, 2018

pitdicker deleted the osrng_improvements branch June 1, 2018 09:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several OsRng improvements #484

Several OsRng improvements #484

pitdicker commented May 28, 2018 •

edited

Loading

vks commented May 28, 2018

pitdicker commented May 28, 2018

dhardy left a comment

dhardy May 28, 2018

dhardy May 28, 2018

dhardy May 28, 2018

dhardy May 28, 2018

dhardy May 28, 2018

pitdicker commented May 28, 2018

pitdicker commented May 28, 2018

dhardy left a comment

dhardy May 29, 2018

pitdicker May 29, 2018

pitdicker May 29, 2018

dhardy May 29, 2018

pitdicker commented May 29, 2018

pitdicker commented May 29, 2018 •

edited

Loading

dhardy left a comment

dhardy May 29, 2018

pitdicker May 29, 2018

dhardy May 29, 2018

dhardy May 29, 2018

dhardy May 29, 2018

pitdicker May 29, 2018

pitdicker commented May 29, 2018 •

edited

Loading

pitdicker commented May 29, 2018 •

edited

Loading

pitdicker commented May 30, 2018 •

edited

Loading

dhardy commented May 30, 2018

dhardy commented Jun 1, 2018

Several OsRng improvements #484

Several OsRng improvements #484

Conversation

pitdicker commented May 28, 2018 • edited Loading

vks commented May 28, 2018

pitdicker commented May 28, 2018

dhardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker commented May 28, 2018

pitdicker commented May 28, 2018

dhardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker commented May 29, 2018

pitdicker commented May 29, 2018 • edited Loading

dhardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker commented May 29, 2018 • edited Loading

pitdicker commented May 29, 2018 • edited Loading

pitdicker commented May 30, 2018 • edited Loading

dhardy commented May 30, 2018

dhardy commented Jun 1, 2018

pitdicker commented May 28, 2018 •

edited

Loading

pitdicker commented May 29, 2018 •

edited

Loading

pitdicker commented May 29, 2018 •

edited

Loading

pitdicker commented May 29, 2018 •

edited

Loading

pitdicker commented May 30, 2018 •

edited

Loading