Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:103331
MIME-Version: 1.0
Date: Thu, 18 Oct 2018 17:38:19 -0700
Message-ID: <CALYT1YoC4Us2Hwh+MGfBaUrx7CQ32krVBQ2M2xs414WTMC8veQ@mail.gmail.com>
To: PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Subject: Alias openssl_random_pseudo_bytes() to php_random_bytes_throw()
From: me@sammyk.me (Sammy Kaye Powers)

Hello internals friends!

I wanted to propose aliasing openssl_random_pseudo_bytes() to
php_random_bytes_throw() in PHP 7.4 for the following reasons:

1) OpenSSL's CSPRNG isn't fork safe:
https://wiki.openssl.org/index.php/Random_fork-safety

This did get fixed last month with the release of v1.1.1 with a
complete rewrite of their CSPRNG, but my guess is it'll be a while
before we see that version landing in a LTS distro.

2) 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.

3) It gets worse: there's that confusing pass-by-reference param
zstrong_result_returned that forces yet another check in userland to
determine if the bytes are strong enough for crypto. To be honest, in
checking the implementation, I can't even tell how the function would
ever return a string of bytes and also set the zstrong_result_returned
param to false.

The API is unnecessarily confusing making it easy to get it wrong. The
above userland example isn't even correct, 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);
}

4) We get to consolidate our CSPRNG code in one place. This would make
it nice to be able to upgrade all the CSPRNG code to libsodium's
CSPRNG if we choose to in the future for example.

The change I'm proposing would be to:

1) Make openssl_random_pseudo_bytes() return bytes from
php_random_bytes_throw() causing the function to fail closed and never
returning false.
2) Deprecate the usage of the second pass-by-reference parameter and
remove in PHP 8.0. Until then, it always sets the value to true.

Do you think this kind of change would warrant an RFC?

Thanks,
Sammy Kaye Powers
sammyk.me