Simplify random number implementation #2

Open
timoh6 opened this Issue Aug 23, 2013 · 1 comment

Projects

None yet

1 participant

@timoh6
timoh6 commented Aug 23, 2013

First of all, this is a "must have" addition to the PHP core. Great it is being worked on!

Looking at the current implementation, I think there are a few important design decisions we should re-think.

Now, the Random constructor looks like: __construct($secure = false). I think we should remove this "unnecessary" parameter. This is just a hassle for the end user (for no practical reasons).

I propose we should have just one good source for random bytes. That is /dev/urandom (and alike). No /dev/random, because it is basically not any better than /dev/urandom, but it is most probably unusable on an interactive web app scenario (due to its blocking nature).

I have got previously disagreements with this from various PHP folks, but I'd like to show some comments from expers on cryptography:
https://news.ycombinator.com/item?id=6216101
http://security.stackexchange.com/a/3939

And finally, If you look at NaCl source code, you see it is relying /dev/urandom for its random numbers:
http://nacl.cr.yp.to/index.html

If we use just one straight forward method to get random numbers, random_compat library code can be simplified alot. Which is critical for secure design and implementation.

We should trust the experts on this, and not roll our own.

This way, we can get rid of the constructor parameter, different random byte fetch implementations and the mergeBuffers() method.

Having just one "genNormal()" method is enough to do the job.

And finally, regarding the current implementation of genNormal(), it is critical to exit immediately if random bytes could not be generated (currently it falls back to a mt_rand(), which is no good).

Also, it is all fine if we return the bytes from the first source that was available (mcrypt_create_iv() or openssl_random_pseudo_bytes() or /dev/urandom). No need to fetch bytes from all of them.

Timo

@timoh6
timoh6 commented Aug 29, 2013

It turned out that openssl_random_pseudo_bytes() might not be a reliable source of randomness (at least) due to the "PID wrapping bug". For more info, see http://emboss.github.io/blog/2013/08/21/openssl-prng-is-not-really-fork-safe/

However, I'm not sure does this practically affect the usage of openssl_random_pseudo_bytes() in PHP, but for the safety, I personally stopped relying on openssl_random_pseudo_bytes(), at least for now.

The underlying OpenSSL code is also a mess to read and understand due to its complexity, so this is as well a solid reason not to use it.

So my final proposal is to fetch the random bytes using mcrypt_create_iv (with URANDOM) or read straight from /dev/urandom (or possibly /dev/arandom).

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment