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

Use appropriate RNG for version 4 generation #52

Closed
veeti opened this issue Nov 22, 2015 · 11 comments
Closed

Use appropriate RNG for version 4 generation #52

veeti opened this issue Nov 22, 2015 · 11 comments

Comments

@veeti
Copy link

veeti commented Nov 22, 2015

Version 4 identifiers are generally expected to be globally random, unique and collision free. To ensure this, a cryptographically secure random number generator should be used. Most if not all implementations (for example, libuuid, Java and Python) do this.

Secure defaults are important. The current code uses thread_rng, which is not suitable for this purpose according to the rand documentation.

@lilyball
Copy link

That's a good point. Version 4 UUIDs are supposed to be un-guessable, and there are potentially many scenarios where an attacker being able to guess at generated UUIDs would be a security vulnerability.

@lhecker
Copy link

lhecker commented Feb 28, 2016

Well... While I agree that uuid could theoretically be made better by using the OsRng I kinda question if that would improve the security at all...

thread_rng() uses the StdRng by default which currently is only a wrapper around IsaacRng which is in turn a cryptographically secure PRNG. And that PRNG is furthermore reseeded every 32KiB.
I think it's very unlikely that the probability for collisions is substantially affected by this.

P.S.: The rand crate is not subject to the issues described in the ISAAC+ paper et. al., because they correctly call isaac() before generating random data. The attack space for the ISAAC cipher is still at 10^1240 and thus probably completely sufficient for this.

@veeti
Copy link
Author

veeti commented Feb 28, 2016

Is that a guarantee, or an implementation detail? Has the implementation been verified for correctness? The rand documentation explicitly calls out OsRng as the right choice for cryptographic requirements:

An application that requires an entropy source for cryptographic purposes must use OsRng, which reads randomness from the source that the operating system provides... The other random number generators provided by this module are not suitable for such purposes.

Many experts recommend that using the system RNG is always a better, less error-prone choice.

@lhecker
Copy link

lhecker commented Feb 28, 2016

In the order of your questions:

Is that a guarantee, or an implementation detail?

Both. The Rust developers have decided to not include insecure RNGs by default some time ago. Thus the StdRng is always a CSPRNG.

Has the implementation been verified for correctness?

Unfortunately they don't have unit tests yet as it seems. I guess someone should create a PR for it. On the other hand the ISAAC RNG in the code is a near identical translation of the code in the original paper which in turn has been verified. The only thing I'd worry about now is how the PRNG is seeded by the OsRng.

The rand documentation explicitly calls out OsRng as the right choice for cryptographic requirements

Just like the man page recommends random over urandom, because it's a good recommendation if you are not familiar with the topic.

Many experts recommend that using the system RNG is always a better, less error-prone choice.

Well I'd argue that guys like him are as much an "expert" as anyone else though…
(I think this is a better article btw: http://www.2uo.de/myths-about-urandom/)

But anyways: The article you linked basically tells you only 2 things:

  • Use /dev/urandom instead of /dev/random
  • Use the OS RNG instead of one implemented in user-land or otherwise you have two points of failure instead of one

I absolutely agree about both points. This is actually a good recommendation. The problem now is that I still don't think that the hefty performance hit you take that way is worth it in this case, since the IsaacRng is a very strong and correctly implemented CSPRNG and IMHO enough for this.
The issues mentioned in your linked article definitely and proofably do not apply to this issue.

@lilyball
Copy link

I originally believed that the rand crate claimed that thread_rng is not suitable for anything that needs a CSPRNG, but that doesn't actually appear to be the case (at least not today, I don't know if it used to say something else). What it actually says on the subject is

An application that requires an entropy source for cryptographic purposes must use OsRng, which reads randomness from the source that the operating system provides (e.g. /dev/urandom on Unixes or CryptGenRandom() on Windows). The other random number generators provided by this module are not suitable for such purposes.

Note that it's talking here about using it as an entropy source, rather than just using it as a CSPRNG.

Given this, my only concern about using thread_rng is that the documentation does not make any guarantees about what PRNG it uses, although it does guarantee that it's periodically reseeded from the OS. Based on the documentation, it's plausible for it to be implemented using an insecure PRNG.

The Rust developers have decided to not include insecure RNGs by default some time ago.

I'm not sure what you mean by this. The rand crate does have one non-cryptographically-secure PRNG (XorShiftRng). Right now thread_rng is always the same thing as StdRng, which is either IsaacRng or Isaac64Rng depending on platform, but the documentation makes no guarantees here and it could default to some other PRNG in the future

At this point it's probably a bad idea for the rand crate to ever use a non-cryptographically-secure PRNG for StdRng or thread_rng, given that clients like uuid assume it's secure (and just in general it's a good idea to default to something secure), so really what should happen is the rand crate should update its documentation to guarantee that StdRng and thread_rng are using some CSPRNG, even if it doesn't define which particular CSPRNG it uses.

@lhecker
Copy link

lhecker commented Feb 29, 2016

I'm not sure what you mean by this.

I mean: If I remember correctly there where multiple points in time when official Rust developers said that using a insecure PRNG by default would be a bad idea. Instead they said, that using a CSPRNG by default is always preferable. Breaking that change retroactively in the rand crate would be major breaking change.

So yeah: I 💯 agree that the rand crate should guarantee the security of the thread_rng and StdRng. Who wants to create an issue? 😄

@Dylan-DPC-zz
Copy link
Member

We need to find a way to use os::OsRng but rand crate doesn't provide a funtionality.

@sameer
Copy link

sameer commented Aug 7, 2018

More recently, rand considered switching to HC-128 and it was merged.

It seems that thread_rng is more appropriate now, as it is being called a CSPRNG.

@kinggoesgaming kinggoesgaming added this to the 0.7.1 milestone Aug 7, 2018
@kinggoesgaming kinggoesgaming modified the milestones: 0.7.1, 0.7.2 Sep 9, 2018
@kinggoesgaming kinggoesgaming modified the milestones: 0.7.2, 0.8.0 Jan 22, 2019
@kinggoesgaming
Copy link
Member

@uuid-rs/uuid what is blocking this issue.. if anything...

@KodrAus
Copy link
Member

KodrAus commented Feb 17, 2019

@kinggoesgaming we've made it possible to build a v4 uuid using externally generated bytes, so could punt on this. How random bytes are generated is an implementation detail though, so there's nothing really blocking this issue and this issue doesn't really block anything else.

@kinggoesgaming
Copy link
Member

close this based on @KodrAus's explanation

@kinggoesgaming kinggoesgaming removed this from the 0.8.0 milestone Feb 18, 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

No branches or pull requests

7 participants