Next revision | Previous revisionNext revisionBoth sides next revision |
rfc:improve-openssl-random-pseudo-bytes [2018/10/19 17:55] – created sammyk | rfc:improve-openssl-random-pseudo-bytes [2018/11/02 19:29] – Fix heading size sammyk |
---|
* Date: 2018-10-19 | * Date: 2018-10-19 |
* Author: Sammy Kaye Powers <sammyk@php.net> | * Author: Sammy Kaye Powers <sammyk@php.net> |
* Status: Draft | * Status: Voting |
| |
===== Introduction ===== | ===== Introduction ===== |
The ''openssl_random_pseudo_bytes()'' function is a wrapper for OpenSSL's [[https://www.openssl.org/docs/man1.0.2/crypto/RAND_bytes.html|''RAND_bytes'' CSPRNG]]. CSPRNG implementations should always fail closed, but ''openssl_random_pseudo_bytes()'' fails open pushing critical fail checks into userland. It also has an unnecessary second parameter that confuses the usage of the API. | The ''openssl_random_pseudo_bytes()'' function is a wrapper for OpenSSL's [[https://www.openssl.org/docs/man1.0.2/crypto/RAND_bytes.html|RAND_bytes CSPRNG]]. CSPRNG implementations should always fail closed, but ''openssl_random_pseudo_bytes()'' fails open pushing critical fail checks into userland. It also has an unnecessary second parameter that confuses the usage of the API. |
| |
===== The Fail-Open Problem ===== | ===== The Fail-Open Problem ===== |
| |
===== Proposal ===== | ===== Proposal ===== |
To fix the fail-open problem, we simply throw an ''\Error'' exception (just like [[https://github.com/php/php-src/blob/8d3f8ca12a0b00f2a74a27424790222536235502/ext/standard/random.c#L179|''random_bytes()'' does]]). This is the Correct Behavior™️ for any CSPRNG implementation. | To fix the fail-open problem, we simply throw an ''\Exception'' (just like [[https://github.com/php/php-src/blob/8d3f8ca12a0b00f2a74a27424790222536235502/ext/standard/random.c#L179|''random_bytes()'' does]]). This is the Correct Behavior™️ for any CSPRNG implementation. |
| |
To fix the the confusing-api problem, we should deprecate the usage of the second ''$crypto_strong'' parameter and just make it always set the value to true. In PHP 8.0 we'd completely remove the second parameter and upgrade the function's ZPP to ''ZEND_PARSE_PARAMS_THROW'' causing the following fatal error when attempting to send in the second argument. | To fix the the confusing-api problem, we should deprecate the usage of the second ''$crypto_strong'' parameter and just make it always set the value to true. In PHP 8.0 we'd completely remove the second parameter and upgrade the function's ZPP to ''ZEND_PARSE_PARAMS_THROW'' causing the following fatal error when attempting to send in the second argument. |
Requires a 2/3 majority | Requires a 2/3 majority |
| |
===== Patches and Tests ===== | Voting started **2018-11-02 @ 19:30 UTC** and will close sometime around **2018-11-16 @ 19:30 UTC** |
Links to any external patches and tests go here. | |
| |
If there is no patch, make it clear who will create a patch, or whether a volunteer to help with implementation is needed. | ==== Vote #1: Make openssl_random_pseudo_bytes() fail closed ==== |
| |
Make it clear if the patch is intended to be the final patch, or is just a prototype. | <doodle title="Make openssl_random_pseudo_bytes() fail closed" auth="sammyk" voteType="single" closed="false"> |
| * Yes |
| * No |
| </doodle> |
| |
For changes affecting the core language, you should also provide a patch for the language specification. | ==== Vote #2: Deprecate the usage of the $crypto_strong parameter ==== |
| |
| <doodle title="Deprecate the usage of the $crypto_strong parameter" auth="sammyk" voteType="single" closed="false"> |
| * Yes |
| * No |
| </doodle> |
| |
| ===== Patches and Tests ===== |
| - [[https://github.com/php/php-src/compare/master...SammyK:rfc-improve-openssl-random-pseudo-bytes|The patch & tests]] |
| |
===== Implementation ===== | ===== Implementation ===== |
===== References ===== | ===== References ===== |
- [[https://externals.io/message/103331|Initial discussion]] | - [[https://externals.io/message/103331|Initial discussion]] |
| - [[https://externals.io/message/103345|Under-discussion announcement]] |
| |
===== Rejected Features ===== | ===== Rejected Features ===== |
The original ping to @internals suggested aliasing ''openssl_random_pseudo_bytes()'' to ''random_bytes()'', but this was not received well so that idea got put in the bin. | - The original ping to @internals suggested aliasing ''openssl_random_pseudo_bytes()'' to ''random_bytes()'', but this was not received well so that idea got put in the bin. |