Hey Tom! Thanks for the feedback! :) On Sun, Oct 21, 2018 at 8:09 PM Tom Worster <f...@thefsb.org> wrote: > > At first glance I believed you were proposing that > `openssl_random_pseudo_bytes()` should fail with an exception and that this > would be an improvement. I would agree with that.
Yep! If the function can't gather crypto-strong bytes, this patch will throw an exception: https://github.com/php/php-src/compare/master...SammyK:rfc-improve-openssl-random-pseudo-bytes > With a little more concentration I see you're proposing something less > ambitious that I'm less enthusiastic about. > The function has been obsolete since 7.0 What makes the function obsolete? The addition of the `random_bytes()` CSPRNG (which uses the kernel's CSPRNG) doesn't invalidate OpenSSL's CSPRNG. > and A Bad Choice™ in all versions of PHP except when OS==Windows AND 5.4.0 > <= PHP < 7.0. What makes it a Bad Choice™? :) Historically, it certainly has had it's fair share of implementation disasters: 1. It implemented `RAND_pseudo_bytes()` (PRNG) instead of `RAND_bytes()` (CSPRNG) all the way up until PHP 5.6.11: https://github.com/php/php-src/blob/PHP-5.6.11/ext/openssl/openssl.c#L5408 but that got fixed: https://bugs.php.net/bug.php?id=70014 2. `RAND_bytes()` is not fork safe by default causing major issues for packages like ramsey/uuid: https://benramsey.com/blog/2016/04/ramsey-uuid/#when-uuids-collide but that got fixed in PHP 5.6.24: https://bugs.php.net/bug.php?id=71915 3. It fails open (this patch will fix that) 4. The API is confusing with the second `$crypto_strong` parameter which doesn't do anything (the other thing that this patch will fix) With this patch, `openssl_random_pseudo_bytes()` should finally be implemented properly so it won't be a Bad Choice™ anymore. :) > The only reason to keep this function is BC I'm not sure of the reasons for why this would be the case. :) > but removing the second param breaks BC for ALL conscientious and safe uses, Yes, but it also confuses how to use the function in userland. The OpenSSL extension doesn't seem to be going anywhere anytime soon, so for all future uses of this function, the API should be clear and also fail closed without forcing all the fail checks into userland - especially unnecessary ones. :) > i.e. seeking unpredictable (i.e. crypto strong) randoms from 5.4.0 <= PHP < > 7.0 on Windows. There's no valid reason to ask for predictable randoms from > OpenSSL and, afaik, its not unpredictable (i.e. it's unsafe) on other OSs. `RAND_bytes()` is a proper CSPRNG and isn't set up to be used as a deterministic PRNG. > I'd love to see an RFC along the lines of: "Improve PHP's OpenSSL API by > depreciating and eventually removing openssl_random_pseudo_bytes()". Idk the > right schedule for removing it but how could deprecating it in 7.4 do more > harm than good? My original ping to internals was to alias `openssl_random_pseudo_bytes()` to `random_bytes()`, but as others have pointed out, having more than one CSPRNG isn't necessarily a bad thing to have. :) Thanks again for the feedback! Maybe the vote should be split up into two parts: 1) Fail closed & 2) deprecate the second parameter. :) Thanks, Sammy Kaye Powers sammyk.me -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
