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:
1) 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. 2) 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. 3) 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); } 4) 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: 1) Make openssl_random_pseudo_bytes() return bytes from php_random_bytes_throw() causing the function to fail closed and never returning false. 2) 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
