[WIP] Easy User-land CSPRNG - #1119

Congratulations on passing unanimously! One thing: Will you ever support the getrandom(2) syscall on newer Linux flavors?

(I didn't see anything in the RFC and I'm not really awake enough to peruse the code.)

@sarciszewski This is something we have discussed, but decided to leave for future scope. getrandom (and getentropy on BSD) are typically used to seed RNGs, and are not designed for high throughput themselves.

If you compile against LibreSSL and have a Linux Kernel version > 3.17, you will be using getrandom today.

The vote passed, but was never closed. This should be merged soon ;-)

Thanks for the push @ircmaxell! :) I'm coordinating with @lt to get this bad boy ready for merge. :)

Hokay! @lt updated this PR based on feedback. I normalized the return values when errors happen & updated the tests & merged in the lest from master & fixed conflicts. Phew!

The RFC officially passed unanimously and this PR can be merged into master now. W00t! Not sure if @nikic is the one to do that or who we need to ping.

Thanks again @ircmaxell, @lt, @rdlowrey, and all the other kids who help me along on this one! 🐻 Let's do some more of these! :D

Merged via #1268. Thanks everyone who worked on this!

Yay! Thanks @nikic! :)

1. The documentation for random_int could use some improvements:
   • Clarify the behaviour if min<=max is violated
   • Specify if the bounds are inclusive or exclusive (looking at rand and mt_rand it's probably inclusive)
   • It should mention that the distribution is uniform
2. Why does it return FALSE which can easily treated as 0 instead of something you can't ignore, like an exception when the RNG in unavailable?
3. What about a secure polyfill?

Why does it return FALSE which can easily treated as 0 instead of something you can't ignore, like an exception when the RNG in unavailable?

Good point. I think a RuntimeException should be thrown when a proper source of random cannot be detected. That way when errors are turned off we won't have people using false/0 as their CSPRNG. :) Thoughts @lt?

@lt: I just pushed up a branch with this change, can you take a look? :)

@SammyK What should happen if min > max? Also an exception?

I think when we wrote this, exceptions in the engine hadn't yet passed.

I'd be happy with InvalidArgumentException for min>max and len<=0

Should len === 0 be considered invalid? I'd consider it valid and only throw an exception if len < 0.

When someone tries to generate an empty string using the CSPRNG, I think it's likely a programming error (if it were my project, I'd define it as a programming error). So I think it is safer if PHP treats this case as invalid.

And, from the other point of view, I think PHP can reasonably reject bug reports that you can't generate empty strings with the CSPRNG, especially if the conditions for these exceptions are documented.

If it's not a programming error, it's a stupid thing to do. Invalid all the way.

At least min == max on random_int shouldn't be an error, since that can be useful in practice. For example when picking the last card in a shuffling algorithm.

@CodesInChaos I agree.

I'll disagree with practically useful. But I'll agree that it should be allowed, in the "spirit of PHP"

If we're going to allow min == max it should be special cased, no point fetching from the rng for it.

    if (min >= max) {
        if (min == max) {
            RETURN_LONG(min);
        }

        zend_throw_exception(spl_ce_InvalidArgumentException, "Maximum value must not be less than the minimum value", 0);
        return;

    }

@SammyK perhaps do something like this for SPL exceptions:

Please do the change to exceptions separately from the changing any error conditions.

Sounds good. I'll submit 2 separate PR's - one for throwing general exceptions and one for new error conditions. Then I'll update the docs when they are merged and post here for another once-over to make sure the docs cover all the bases. :)