[RFC] Improve openssl_random_pseudo_bytes() #3649
Conversation
|
Quite disgusting BC break for minor version (7.4)... |
|
@Majkl578 The fail-closed BC should rarely occur in the wild, but is an important implementation detail for CSPRNG's. The second param deprecation is a separate vote in the RFC. :) |
|
Well, throwing an exception instead of returning false is a huge BC break in a minor version. This type of stuff is what makes people (and hosting providers) to not upgrade to newer minor versions.
Can you provide some metrics for this statement? |
|
@Majkl578 Those issues should be raised on internals, even more so now that the RFC is in voting |
| } | ||
|
|
||
| if (buffer_length <= 0 | ||
| #ifndef PHP_WIN32 | ||
| || ZEND_LONG_INT_OVFL(buffer_length) | ||
| #endif | ||
| ) { | ||
| RETURN_FALSE; | ||
| zend_throw_exception(zend_ce_error, "Length must be greater than 0", 0); |
There was a problem hiding this comment.
I think this can potentially break stuff. I'm fine with throwing on rand error which happens only if there is something really wrong with the environment but this case can happen more likely. Maybe we should reconsider this case and just return empty string or keep it as false.
There was a problem hiding this comment.
Thanks for the review @bukka! This is to mimic the behavior of random_bytes() like the RFC suggests. This will ensure that openssl_random_pseudo_bytes() can't be used as an attack vector in the event an attacker has access to change $length.
// Let's hope this doesn't exist in the wild! :)
$bytes = openssl_random_pseudo_bytes($_GET['length']);
There was a problem hiding this comment.
I'm also somewhat concerned about this one. Requesting zero bytes from an RNG is not wrong per-se -- it's an operation with a well-defined return value, namely the empty string. The current return value (false) happens to coerce to that.
I feel like there's not much value in throwing an exception for this one specific case (throwing for negative length or overflowing length is fine), and I strongly suspect that this will lead to BC breakage somewhere, particularly in testing code. E.g. I remember that when we merged in ext/sodium and switched from using a sodium-specific random bytes implementation (which allows 0 length) to PHP's that's one of the issues that came up in testing code that was testing random strings of random length (including zero).
That's why I would prefer to continue allowing this particular case. As the RFC does not spell this out either way, I think we could still do so, if we wanted :)
CSPRNG implementations should always fail closed. Now openssl_random_pseudo_bytes() will fail closed by throwing an `\Exception` in fail conditions.
SammyK
force-pushed
the
rfc-improve-openssl-random-pseudo-bytes
branch
from
c53ea27 to
db44309
Compare
|
Just adjusted this PR to match what was voted in via the RFC. :) |
|
Merged as 74c0e58 into master :) |
PR for Improve openssl_random_pseudo_bytes() RFC. :)