[RFC] [VOTE] Improve openssl_random_pseudo_bytes() View as Markdown

@Niklas: Ooops! Thanks for pointing that out. Done! :)
https://github.com/php/php-src/pull/3649

@Nikita

IMHO we should just document that it is guaranteed to always be true if the call succeeds and checking it is not necessary. But we should not penalize existing code that is checking the flag -- it may no longer necessary nowadays, but previously that was the correct use of the function.

That's fair. I'm just thinking of devs that will encounter the second
param outside of the docs (i.e. via their IDE hints). And since
ext/openssl seems to be here for the long haul, the goal would be to
have a non-confusing API going forward. But I can totally see your
point as well which is the main reason I went for two separate votes.
:)

Thanks,
Sammy Kaye Powers
sammyk.me

Hey internals!

Given there has been at least two weeks of discussion around the RFC,
"Improve openssl_random_pseudo_bytes()" and there doesn't seem to be
any outstanding issues, I have moved the RFC into voting phase.

The voting period lasts two weeks; from now until 2018-11-16.

The RFC can be found here:
https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes

Happy voting! :)

I've voted against the deprecation of the $crypto_strong param. IMHO we
should just document that it is guaranteed to always be true if the call
succeeds and checking it is not necessary. But we should not penalize
existing code that is checking the flag -- it may no longer necessary
nowadays, but previously that was the correct use of the function.

I think this is a good point so voted against it too.

Hey Michael!

I really appreciate you fighting against BC's in minor versions. We
certainly need to always be hyper aware of the impact BC's have on the
community.

First, I'll address your concerns you raised on GitHub here:
https://github.com/php/php-src/pull/3649#issuecomment-435684017

Well, throwing an exception instead of returning false is a huge BC break in a minor version.

Indeed and it is. And it's an important and very much needed one since
we're dealing with cryptographic contexts. As infosec people say, "Bad
cryptography is not backwards compatible with good cryptography." If
we don't make the CSPRNG fail closed, we're allowing a potential
attack vector in every single PHP script that doesn't have a fail
check.

This type of stuff is what makes people (and hosting providers) to not upgrade to newer minor versions.

I think it's important to point out, we're talking about two very
distinct BC's here. 1) Failing closed which is a security-related BC
(a must break IMO). 2) A confusing API clean-up. I'm not sure there's
a 100% right answer to fixing the second BC; whether we keep the
second param or deprecate it, both options have tradeoffs. Perhaps
this second deprecation BC should wait until 8.0, but that's why I
made it a separate vote (and at this point the deprecation BC looks
like it will fail anyway). It's really tough to get this stuff right -
everything is a tradeoff. :)

"should rarely occur" Can you provide some metrics for this statement?

I don't have stats on how often RAND_bytes() returns 0. But even
if it failed often, that's even more reason to have
openssl_random_pseudo_bytes() fail closed and shut down that
potential attack vector.

the "Backward Incompatible Changes" section fails to mention that this
function will start throwing an exception in next MINOR PHP version (7.4)

Failing closed is an inherent BC break and the RFC explicitly targets
PHP 7.4. The entire RFC after all is a big "Backward Incompatible
Change". :) So I'm not really following how that is unclear.

But again - thanks for fighting against BC's! I totally agree with you
that we gotta keep those BC's to a minimum; even in major versions. <3

Thanks,
Sammy Kaye Powers
sammyk.me