Hello internals friends!
I wanted to propose aliasing openssl_random_pseudo_bytes() to
php_random_bytes_throw() in PHP 7.4 for the following reasons:
- OpenSSL's CSPRNG isn't fork safe:
https://wiki.openssl.org/index.php/Random_fork-safety
This did get fixed last month with the release of v1.1.1 with a
complete rewrite of their CSPRNG, but my guess is it'll be a while
before we see that version landing in a LTS distro.
- The
openssl_random_pseudo_bytes()function fails open which means
code like this:
function genCsrfToken(): string
{
return bin2hex(openssl_random_pseudo_bytes(32));
}
...could return an empty string. This forces the developer to do their
own checks and fail closed in userland.
function genCsrfToken(): string
{
$bytes = openssl_random_pseudo_bytes(32);
if (false === $bytes) {
throw new \Exception('CSPRNG error');
}
return bin2hex($bytes);
}
A quick search in GitHub reveals very little checking of the return
value of openssl_random_pseudo_bytes() in the wild.
CSPRNG implementations should always fail closed.
- It gets worse: there's that confusing pass-by-reference param
zstrong_result_returned that forces yet another check in userland to
determine if the bytes are strong enough for crypto. To be honest, in
checking the implementation, I can't even tell how the function would
ever return a string of bytes and also set the zstrong_result_returned
param to false.
The API is unnecessarily confusing making it easy to get it wrong. The
above userland example isn't even correct, the correct usage in
userland would actually be:
function genCsrfToken(): string
{
$strong = false;
$bytes = openssl_random_pseudo_bytes(32, $strong);
if (false === $bytes || false === $strong) {
throw new \Exception('CSPRNG error');
}
return bin2hex($bytes);
}
- We get to consolidate our CSPRNG code in one place. This would make
it nice to be able to upgrade all the CSPRNG code to libsodium's
CSPRNG if we choose to in the future for example.
The change I'm proposing would be to:
- Make
openssl_random_pseudo_bytes()return bytes from
php_random_bytes_throw() causing the function to fail closed and never
returning false. - Deprecate the usage of the second pass-by-reference parameter and
remove in PHP 8.0. Until then, it always sets the value to true.
Do you think this kind of change would warrant an RFC?
Thanks,
Sammy Kaye Powers
sammyk.me
Hi Sammy
Den fre. 19. okt. 2018 kl. 02.38 skrev Sammy Kaye Powers me@sammyk.me:
- We get to consolidate our CSPRNG code in one place. This would make
it nice to be able to upgrade all the CSPRNG code to libsodium's
CSPRNG if we choose to in the future for example.
I would prefer this, any instead of making it an alias or something,
then just straight down deprecate it instead seems like a more
flawless option to me.
The change I'm proposing would be to:
- Make
openssl_random_pseudo_bytes()return bytes from
php_random_bytes_throw() causing the function to fail closed and never
returning false.- Deprecate the usage of the second pass-by-reference parameter and
remove in PHP 8.0. Until then, it always sets the value to true.Do you think this kind of change would warrant an RFC?
It would.
--
regards,
Kalle Sommer Nielsen
kalle@php.net
Hello internals friends!
I wanted to propose aliasing
openssl_random_pseudo_bytes()to
php_random_bytes_throw() in PHP 7.4 for the following reasons:
- OpenSSL's CSPRNG isn't fork safe:
https://wiki.openssl.org/index.php/Random_fork-safety
This has been already fixed in openssl ext as we now seed it with time as
well. The issue was already fixed in 1.1.0 so it applies to 1.0.2- only.
For more info see:
https://github.com/php/php-src/pull/1857
There also is a link to the bug where you can find even more info.
This did get fixed last month with the release of v1.1.1 with a
complete rewrite of their CSPRNG, but my guess is it'll be a while
before we see that version landing in a LTS distro.
When do you think PHP 7.4 will land in LTS distros..? :) I guess it will be
much later as it's gonna get released 1.5 year later. ;)
- The
openssl_random_pseudo_bytes()function fails open which means
code like this:function genCsrfToken(): string
{
return bin2hex(openssl_random_pseudo_bytes(32));
}...could return an empty string. This forces the developer to do their
own checks and fail closed in userland.function genCsrfToken(): string
{
$bytes = openssl_random_pseudo_bytes(32);
if (false === $bytes) {
throw new \Exception('CSPRNG error');
}
return bin2hex($bytes);
}A quick search in GitHub reveals very little checking of the return
value ofopenssl_random_pseudo_bytes()in the wild.CSPRNG implementations should always fail closed.
Considering that the fail is really unlikely, it could be changed to
exception but that's completely unrelated to the underlying
implementations.
- It gets worse: there's that confusing pass-by-reference param
zstrong_result_returned that forces yet another check in userland to
determine if the bytes are strong enough for crypto. To be honest, in
checking the implementation, I can't even tell how the function would
ever return a string of bytes and also set the zstrong_result_returned
param to false.The API is unnecessarily confusing making it easy to get it wrong. The
above userland example isn't even correct, the correct usage in
userland would actually be:function genCsrfToken(): string
{
$strong = false;
$bytes = openssl_random_pseudo_bytes(32, $strong);
if (false === $bytes || false === $strong) {
throw new \Exception('CSPRNG error');
}
return bin2hex($bytes);
}
That parameter is actually always true for all supported platform by PHP so
it probably makes sense to deprecate it. Again it's unrelated to the
underlying implementations.
- We get to consolidate our CSPRNG code in one place. This would make
it nice to be able to upgrade all the CSPRNG code to libsodium's
CSPRNG if we choose to in the future for example.
Well CSPRNG in OpenSSL 1.1.1 got really improved so I think there is a
value to keep it. I don't really see much reason to drop it.
The change I'm proposing would be to:
- Make
openssl_random_pseudo_bytes()return bytes from
php_random_bytes_throw() causing the function to fail closed and never
returning false.
-1 as there is no valid reason to do that.
- Deprecate the usage of the second pass-by-reference parameter and
remove in PHP 8.0. Until then, it always sets the value to true.
It is already true but I'm fine with deprecating it.
Cheers
Jakub
I wanted to propose aliasing
openssl_random_pseudo_bytes()to
php_random_bytes_throw() in PHP 7.4 for the following reasons:
Sorry, I'm with Jakub here, and for the largely the same reasons, but I'll add:
- Magic. Having something say: "I'm going to call OpenSSL for a
security related reason", then proceed to not call OpenSSL at all is
false advertising, for good or bad. - The point about the openssl function's poor return value stands,
but I would say we can trivially make that function throwing without
having to change its happy-path behavior. - I don't actually think making all sources of randomness the same is
good. There's value in havingrandom_bytes()and OpenSSL and
Sodium to hedge against weaknesses being discovered in any one of
them.
- Make
openssl_random_pseudo_bytes()return bytes from
php_random_bytes_throw() causing the function to fail closed and never
returning false.
-1 for reasons above and what Jakub's already said.
Per #2 above however, I support having openssl_random_psuedo_bytes()
throw on failure rather than merely return false. A failure there
should break any code which isn't explicitly dealing with it.
- Deprecate the usage of the second pass-by-reference parameter and
remove in PHP 8.0. Until then, it always sets the value to true.
+1
-Sara
Thanks for the feedback! That's a good point of having multiple CSPRNG sources.
I'll start drafting an RFC to make openssl_random_pseudo_bytes() fail
closed and deprecate the second parameter. :)
Thanks,
Sammy Kaye Powers
sammyk.me