# [RFC] [VOTE] Improve openssl_random_pseudo_bytes() - Thread: https://externals.io/message/103415 - Original archive: https://news-web.php.net/php.internals/103415 - Started by Sammy Kaye Powers on 2018-11-02T19:30:10+00:00 - Messages: 8 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #103415 — Sammy Kaye Powers — 2018-11-02T19:30:10+00:00 URL: https://externals.io/message/103415#103415 Hey internals! Given there has been at least two weeks of discussion around the RFC, "Improve `openssl_random_pseudo_bytes()`" and there doesn't seem to be any outstanding issues, I have moved the RFC into voting phase. The voting period lasts two weeks; from now until 2018-11-16. The RFC can be found here: https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes Happy voting! :) Thanks, Sammy Kaye Powers sammyk.me --- ## #103416 — Niklas Keller — 2018-11-03T09:38:09+00:00 URL: https://externals.io/message/103415#103416 · In reply to: #103415 Hey Sammy, could you please turn your patch into a PR, so it's easier to comment? Thanks, Niklas Am Fr., 2. Nov. 2018 um 20:30 Uhr schrieb Sammy Kaye Powers : > > Hey internals! > > Given there has been at least two weeks of discussion around the RFC, > "Improve `openssl_random_pseudo_bytes()`" and there doesn't seem to be > any outstanding issues, I have moved the RFC into voting phase. > > The voting period lasts two weeks; from now until 2018-11-16. > > The RFC can be found here: > https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes > > Happy voting! :) > > Thanks, > Sammy Kaye Powers > sammyk.me --- ## #103417 — Nikita Popov — 2018-11-03T10:26:54+00:00 URL: https://externals.io/message/103415#103417 · In reply to: #103415 > Hey internals! > > Given there has been at least two weeks of discussion around the RFC, > "Improve `openssl_random_pseudo_bytes()`" and there doesn't seem to be > any outstanding issues, I have moved the RFC into voting phase. > > The voting period lasts two weeks; from now until 2018-11-16. > > The RFC can be found here: > https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes > > Happy voting! :) I've voted against the deprecation of the $crypto_strong param. IMHO we should just document that it is guaranteed to always be true if the call succeeds and checking it is not necessary. But we should not penalize existing code that is checking the flag -- it may no longer necessary nowadays, but previously that was the correct use of the function. Nikita --- ## #103418 — Sammy Kaye Powers — 2018-11-03T17:48:22+00:00 URL: https://externals.io/message/103415#103418 · In reply to: #103417 @Niklas: Ooops! Thanks for pointing that out. Done! :) https://github.com/php/php-src/pull/3649 @Nikita > IMHO we should just document that it is guaranteed to always be true if the call succeeds and checking it is not necessary. But we should not penalize existing code that is checking the flag -- it may no longer necessary nowadays, but previously that was the correct use of the function. That's fair. I'm just thinking of devs that will encounter the second param outside of the docs (i.e. via their IDE hints). And since ext/openssl seems to be here for the long haul, the goal would be to have a non-confusing API going forward. But I can totally see your point as well which is the main reason I went for two separate votes. :) Thanks, Sammy Kaye Powers sammyk.me --- ## #103419 — Jakub Zelenka — 2018-11-04T17:01:17+00:00 URL: https://externals.io/message/103415#103419 · In reply to: #103417 > > > > […] > > > I've voted against the deprecation of the $crypto_strong param. IMHO we > should just document that it is guaranteed to always be true if the call > succeeds and checking it is not necessary. But we should not penalize > existing code that is checking the flag -- it may no longer necessary > nowadays, but previously that was the correct use of the function. > > I think this is a good point so voted against it too. --- ## #103420 — Michael Moravec — 2018-11-04T20:12:12+00:00 URL: https://externals.io/message/103415#103420 · In reply to: #103415 > > Hey internals! > > Given there has been at least two weeks of discussion around the RFC, > "Improve `openssl_random_pseudo_bytes()`" and there doesn't seem to be > any outstanding issues, I have moved the RFC into voting phase. > > The voting period lasts two weeks; from now until 2018-11-16. > > The RFC can be found here: > https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes > > Happy voting! :) Hi, the "Backward Incompatible Changes" section fails to mention that this function will start throwing an exception in next MINOR PHP version (7.4) in all existing code. As such this is quite heavy BC break. I think this should be clarified in the "Backward Incompatible Changes" section, the RFC postponed to 8.0 (as majors are for BC breaks) and restarted. Thanks, Michael --- ## #103435 — Sammy Kaye Powers — 2018-11-10T04:26:09+00:00 URL: https://externals.io/message/103415#103435 · In reply to: #103420 Hey Michael! I really appreciate you fighting against BC's in minor versions. We certainly need to always be hyper aware of the impact BC's have on the community. First, I'll address your concerns you raised on GitHub here: https://github.com/php/php-src/pull/3649#issuecomment-435684017 > Well, throwing an exception instead of returning false is a huge BC break in a minor version. Indeed and it is. And it's an important and very much needed one since we're dealing with cryptographic contexts. As infosec people say, "Bad cryptography is not backwards compatible with good cryptography." If we don't make the CSPRNG fail closed, we're allowing a potential attack vector in every single PHP script that doesn't have a fail check. > This type of stuff is what makes people (and hosting providers) to not upgrade to newer minor versions. I think it's important to point out, we're talking about two very distinct BC's here. 1) Failing closed which is a security-related BC (a must break IMO). 2) A confusing API clean-up. I'm not sure there's a 100% right answer to fixing the second BC; whether we keep the second param or deprecate it, both options have tradeoffs. Perhaps this second deprecation BC should wait until 8.0, but that's why I made it a separate vote (and at this point the deprecation BC looks like it will fail anyway). It's really tough to get this stuff right - everything is a tradeoff. :) > "should rarely occur" Can you provide some metrics for this statement? I don't have stats on how often `RAND_bytes()` returns `0`. But even if it failed often, that's even more reason to have `openssl_random_pseudo_bytes()` fail closed and shut down that potential attack vector. > the "Backward Incompatible Changes" section fails to mention that this > function will start throwing an exception in next MINOR PHP version (7.4) Failing closed is an inherent BC break and the RFC explicitly targets PHP 7.4. The entire RFC after all is a big "Backward Incompatible Change". :) So I'm not really following how that is unclear. But again - thanks for fighting against BC's! I totally agree with you that we gotta keep those BC's to a minimum; even in major versions. <3 Thanks, Sammy Kaye Powers sammyk.me --- ## #103449 — Sammy Kaye Powers — 2018-11-16T18:34:01+00:00 URL: https://externals.io/message/103415#103449 · In reply to: #103415 Hey internals folks! I'm happy to announce that the vote to make `openssl_random_pseudo_bytes()` fail closed in the "Improve `openssl_random_pseudo_bytes()`" RFC passed unanimously with 30 votes. The other vote to deprecate the second parameter failed with a straight 50/50 split. I still have to make some small tweaks to the implementation before it can be merged to master, but yay for better security! :D https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes Thanks, Sammy Kaye Powers sammyk.me