PHP RFC: Improve openssl_random_pseudo_bytes()

• Version: 0.1
• Date: 2018-10-19
• Author: Sammy Kaye Powers sammyk@php.net
• Implementation: https://github.com/php/php-src/pull/3649
• Status: Implemented (in PHP 7.4)

The openssl_random_pseudo_bytes() function is a wrapper for OpenSSL's 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 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.

There is also a confusing pass-by-reference param $crypto_strong. According to the docs:

It also indicates if a cryptographically strong algorithm was used to produce the pseudo-random bytes, and does this via the optional crypto_strong parameter.

This forces yet another check in userland to determine if the bytes are strong enough for crypto. The usage of this parameter is unnecessary since openssl_random_pseudo_bytes() already returns false on failure and the implementation doesn't allow returning a string of bytes while also setting $crypto_strong to false.

The API is unnecessarily confusing making it easy to get it wrong. The above userland example isn't even correct according to the docs. 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);
}

This redundant check is confusing for developers and the documentation does not properly describe the behavior of the implementation.

To fix the fail-open problem, we simply throw an \Exception (just like ''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.

PHP Fatal error: Uncaught ArgumentCountError: openssl_random_pseudo_bytes() expects exactly 1 parameter, 2 given

False-checks on the return value of openssl_random_pseudo_bytes() will do nothing since the function fails closed. Usage of $crypto_strong will generate errors.

PHP 7.4

The openssl_random_pseudo_bytes() function will continue to use OpenSSL's RAND_bytes CSPRNG.

Requires a 2/3 majority

Voting started 2018-11-02 @ 19:30 UTC and will close sometime around 2018-11-16 @ 19:30 UTC

1. The patch & tests

After the project is implemented, this section should contain

1. the version(s) it was merged into
2. a link to the git commit(s)
3. a link to the PHP manual entry for the feature
4. a link to the language specification section (if any)

1. Initial discussion
2. Under-discussion announcement

1. 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.