On Fri, Oct 19, 2018 at 1:38 AM Sammy Kaye Powers <m...@sammyk.me> wrote:
> 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 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. ;) > 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. > > Considering that the fail is really unlikely, it could be changed to exception but that's completely unrelated to the underlying implementations. > 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); > } > > 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. 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. > > 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: > > 1) 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. > 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. > > It is already true but I'm fine with deprecating it. Cheers Jakub
