[RFC] Improve openssl_random_pseudo_bytes()

I'd like to start a discussion on the "Improve
openssl_random_pseudo_bytes()" RFC:
https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes

lgtm; though a small nit: s/RETURN_BOOL(pzv, 1)/RETURN_TRUE(pzv)/g
Slightly more readable that way.

Or rather: ZVAL_BOOL(pzv, 1) => ZVAL_TRUE(pzv)

Good call - done! :)

Hi Sammy,

What makes the function obsolete? The addition of the random_bytes()

Yes.

What makes the function obsolete? The addition of the random_bytes()
CSPRNG (which uses the kernel's CSPRNG) doesn't invalidate OpenSSL's
CSPRNG.

According to one argument that has a lot of currency, it does.

From the point of view of a consumer of unpredictable randoms comparing
two APIs: a CSPRNG implemented in user-space memory vs. the system call
to the kernel's non-blocking CSPRNG, the user-space CSPRNG 1) cannot do
anything you need that the system call cannot, and 2) relies on the
kernel for its entropy and periodic reseeding. Hence the user-space
CSPRNG adds potential failure modes and adds to the attack surface and
is therefore less trustworthy.

The same thing stated from a different point of view: Nobody knows how
to verify CSPRNG code so staking CSPRNGs is a bad idea.

This argument has perhaps most famously been made by Thomas Ptacek
https://sockpuppet.org/blog/2014/02/25/safely-generate-random-numbers/

The argument seems to win a lot including on this list where I've never
seen it fail.

Personally, I find the argument generally convincing and I wouldn't dare
argue with Thomas Ptacek. While it's a broad argument with sweeping
consequences, I think it is sound general advice for most consumers and
probably all PHP coders.

and A Bad Choice™ in all versions of PHP except when OS==Windows
AND 5.4.0 <= PHP < 7.0.

What makes it a Bad Choice™? :) Historically, it certainly has had
it's fair share of implementation disasters:

1. It implemented RAND_pseudo_bytes() (PRNG) instead of
   RAND_bytes() (CSPRNG) all the way up until PHP 5.6.11:
   https://github.com/php/php-src/blob/PHP-5.6.11/ext/openssl/openssl.c#L5408
   but that got fixed: https://bugs.php.net/bug.php?id=70014
2. RAND_bytes() is not fork safe by default causing major issues for
   packages like ramsey/uuid:
   https://benramsey.com/blog/2016/04/ramsey-uuid/#when-uuids-collide but
   that got fixed in PHP 5.6.24: https://bugs.php.net/bug.php?id=71915
3. It fails open (this patch will fix that)
4. The API is confusing with the second $crypto_strong parameter
   which doesn't do anything (the other thing that this patch will fix)

Such a list supports concern over potential failure modes and attack
surface. It's good to fix bugs but it's even better to avoid them. The
only thing you can loose by avoiding openssl_random_pseudo_bytes is
hazard.

Personal story: sometime early in the 7.0 epoc, I tried to argue that
random_compat should not use openssl_random_pseudo_bytes except on
Windows. I failed and, iirc, item 2 in your list was a consequence.

The only reason to keep this function is BC

I'm not sure of the reasons for why this would be the case. :)

It's because anyone authoring new code should use the safer option.

RAND_bytes() is a proper CSPRNG and isn't set up to be used as a
deterministic PRNG.

Yes.

(Btw, "a proper CSPRNG" might be misinterpreted as a bold claim.
Some CSPRNG implementations have a relatively good record (so far) but
that's about as much as you can confidently say. I saw some interesting
research on formal verification at 33c3 but I believe it's still true
that code review is the state of the art.
https://fahrplan.events.ccc.de/congress/2016/Fahrplan/events/8099.html
https://formal.iti.kit.edu/klebanov/software/entroposcope/ from which I
also learned to my shame that statistical tests are useless as they only
test the output bit mixing function.)

My original ping to internals was to alias
openssl_random_pseudo_bytes() to random_bytes(), but as others
have pointed out, having more than one CSPRNG isn't necessarily a bad
thing to have. :)

I missed those remarks. I think differently that it is a good thing if
PHP offers only one CSRNG.

Thanks again for the feedback! Maybe the vote should be split up into
two parts: 1) Fail closed & 2) deprecate the second parameter. :)

Idk. It's your RFC and I kinda hijacked the thread.

Tom