# [RFC] Improve openssl_random_pseudo_bytes() - Thread: https://externals.io/message/103345 - Original archive: https://news-web.php.net/php.internals/103345 - Started by Sammy Kaye Powers on 2018-10-19T20:46:52+00:00 - Messages: 8 - RFC: Improve openssl_random_pseudo_bytes() (Implemented (PHP 7.4)) — https://externals.io/rfc/improve-openssl-random-pseudo-bytes.md - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · RFCs at https://externals.io/rfc.md · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #103345 — Sammy Kaye Powers — 2018-10-19T20:46:52+00:00 URL: https://externals.io/message/103345#103345 Hi internals friends! I'd like to start a discussion on the "Improve `openssl_random_pseudo_bytes()`" RFC: https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes TL;DR: CSPRNG implementations should always fail closed so this change would make `openssl_random_pseudo_bytes()` fail closed. The second `$crypto_strong` parameter doesn't do anything despite the docs stating otherwise. This unnecessarily confusing parameter would be deprecated. Thanks, Sammy Kaye Powers sammyk.me --- ## #103346 — Sara Golemon — 2018-10-20T02:20:39+00:00 URL: https://externals.io/message/103345#103346 · In reply to: #103345 > I'd like to start a discussion on the "Improve > `openssl_random_pseudo_bytes()`" RFC: > https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes > lgtm; though a small nit: s/RETURN_BOOL(pzv, 1)/RETURN_TRUE(pzv)/g Slightly more readable that way. --- ## #103347 — Sara Golemon — 2018-10-20T02:21:25+00:00 URL: https://externals.io/message/103345#103347 · In reply to: #103346 > > > […] > lgtm; though a small nit: s/RETURN_BOOL(pzv, 1)/RETURN_TRUE(pzv)/g > Slightly more readable that way. > Or rather: ZVAL_BOOL(pzv, 1) => ZVAL_TRUE(pzv) --- ## #103349 — Sammy Kaye Powers — 2018-10-20T13:01:13+00:00 URL: https://externals.io/message/103345#103349 · In reply to: #103347 > > > > […] > Or rather: ZVAL_BOOL(pzv, 1) => ZVAL_TRUE(pzv) Good call - done! :) --- ## #103355 — Tom Worster — 2018-10-22T00:09:39+00:00 URL: https://externals.io/message/103345#103355 · In reply to: #103345 > I'd like to start a discussion on the "Improve > `openssl_random_pseudo_bytes()`" RFC: > https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes > > TL;DR: > > CSPRNG implementations should always fail closed so this change would > make `openssl_random_pseudo_bytes()` fail closed. > > The second `$crypto_strong` parameter doesn't do anything despite the > docs stating otherwise. This unnecessarily confusing parameter would > be deprecated. 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. 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 and A Bad Choice™ in all versions of PHP except when OS==Windows AND 5.4.0 <= PHP < 7.0. The only reason to keep this function is BC but removing the second param breaks BC for ALL conscientious and safe uses, 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. 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? Tom --- ## #103369 — Sammy Kaye Powers — 2018-10-22T13:46:50+00:00 URL: https://externals.io/message/103345#103369 · In reply to: #103355 Hey Tom! Thanks for the feedback! :) > > 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 --- ## #103372 — Tom Worster — 2018-10-22T19:26:01+00:00 URL: https://externals.io/message/103345#103372 · In reply to: #103369 Hi Sammy, > What makes the function obsolete? The addition of the `random_bytes()` Yes. > What makes the function obsolete? The addition of the `random_bytes()` > CSPRNG (which uses the kernel's CSPRNG) doesn't invalidate OpenSSL's > CSPRNG. According to one argument that has a lot of currency, it does. From the point of view of a consumer of unpredictable randoms comparing two APIs: a CSPRNG implemented in user-space memory vs. the system call to the kernel's non-blocking CSPRNG, the user-space CSPRNG 1) cannot do anything you need that the system call cannot, and 2) relies on the kernel for its entropy and periodic reseeding. Hence the user-space CSPRNG adds potential failure modes and adds to the attack surface and is therefore less trustworthy. The same thing stated from a different point of view: Nobody knows how to verify CSPRNG code so staking CSPRNGs is a bad idea. This argument has perhaps most famously been made by Thomas Ptacek https://sockpuppet.org/blog/2014/02/25/safely-generate-random-numbers/ The argument seems to win a lot including on this list where I've never seen it fail. Personally, I find the argument generally convincing and I wouldn't dare argue with Thomas Ptacek. While it's a broad argument with sweeping consequences, I think it is sound general advice for most consumers and probably all PHP coders. > > […] > > 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) Such a list supports concern over potential failure modes and attack surface. It's good to fix bugs but it's even better to avoid them. The only thing you can loose by avoiding openssl_random_pseudo_bytes is hazard. Personal story: sometime early in the 7.0 epoc, I tried to argue that random_compat should not use openssl_random_pseudo_bytes except on Windows. I failed and, iirc, item 2 in your list was a consequence. > > […] > > I'm not sure of the reasons for why this would be the case. :) It's because anyone authoring new code should use the safer option. > `RAND_bytes()` is a proper CSPRNG and isn't set up to be used as a > deterministic PRNG. Yes. (Btw, "a proper CSPRNG" might be misinterpreted as a **bold** claim. Some CSPRNG implementations have a relatively good record (so far) but that's about as much as you can confidently say. I saw some interesting research on formal verification at 33c3 but I believe it's still true that code review is the state of the art. https://fahrplan.events.ccc.de/congress/2016/Fahrplan/events/8099.html https://formal.iti.kit.edu/klebanov/software/entroposcope/ from which I also learned to my shame that statistical tests are useless as they only test the output bit mixing function.) > 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. :) I missed those remarks. I think differently that it is a good thing if PHP offers only one CSRNG. > Thanks again for the feedback! Maybe the vote should be split up into > two parts: 1) Fail closed & 2) deprecate the second parameter. :) Idk. It's your RFC and I kinda hijacked the thread. Tom --- ## #103414 — Sammy Kaye Powers — 2018-10-31T17:55:03+00:00 URL: https://externals.io/message/103345#103414 · In reply to: #103372 Hey Tom! > According to one argument that has a lot of currency, it does. You have great points that I totally agree with; after all, my original proposal was to alias to `random_bytes()`. But this RFC just targets the implementation problems, not the OpenSSL CSPRNG itself. We already discussed in a separate thread the possibility of removing/aliasing it, but have decided we're gonna keep it. :) > (Btw, "a proper CSPRNG" might be misinterpreted as a **bold** claim. Good point - I'm not an infosec expert by any means so I try to avoid making any hard claims regarding cryptography. :) > Idk. It's your RFC and I kinda hijacked the thread. Hehe - thanks for hijacking! I always love the feedback. :) Thanks, Sammy Kaye Powers sammyk.me