# Alias openssl_random_pseudo_bytes() to php_random_bytes_throw()

- Thread: https://externals.io/message/103331
- Original archive: https://news-web.php.net/php.internals/103331
- Started by Sammy Kaye Powers on 2018-10-19T00:38:19+00:00
- Messages: 5
- 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

---

## #103331 — Sammy Kaye Powers — 2018-10-19T00:38:19+00:00

URL: https://externals.io/message/103331#103331

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

---

## #103332 — Kalle Sommer Nielsen — 2018-10-19T05:29:19+00:00

URL: https://externals.io/message/103331#103332 · In reply to: #103331

Hi Sammy

Den fre. 19. okt. 2018 kl. 02.38 skrev Sammy Kaye Powers <me@sammyk.me>:
> 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.

I would prefer this, any instead of making it an alias or something,
then just straight down deprecate it instead seems like a more
flawless option to me.

> 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?

It would.



-- 
regards,

Kalle Sommer Nielsen
kalle@php.net

---

## #103334 — Jakub Zelenka — 2018-10-19T09:27:57+00:00

URL: https://externals.io/message/103331#103334 · In reply to: #103331

> 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 has been already fixed in openssl ext as we now seed it with time as
well. The issue was already fixed in 1.1.0 so it applies to 1.0.2- only.
For more info see:

https://github.com/php/php-src/pull/1857

There also is a link to the bug where you can find even more info.

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.
>
>
When do you think PHP 7.4 will land in LTS distros..? :) I guess it will be
much later as it's gonna get released 1.5 year later. ;)


> 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.
>
>
Considering that the fail is really unlikely, it could be changed to
exception but that's completely unrelated to the underlying
implementations.


> 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);
> }
>
>
That parameter is actually always true for all supported platform by PHP so
it probably makes sense to deprecate it. Again it's unrelated to the
underlying implementations.

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.
>
>
Well CSPRNG in OpenSSL 1.1.1 got really improved so I think there is a
value to keep it. I don't really see much reason to drop it.


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

-1 as there is no valid reason to do that.


> 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.
>
>
It is already true but I'm fine with deprecating it.

Cheers

Jakub

---

## #103336 — Sara Golemon — 2018-10-19T14:00:59+00:00

URL: https://externals.io/message/103331#103336 · In reply to: #103334

> 
> > […]
>
Sorry, I'm with Jakub here, and for the largely the same reasons, but I'll add:

1) Magic.  Having something say: "I'm going to call OpenSSL for a
security related reason", then proceed to not call OpenSSL at all is
false advertising, for good or bad.
2) The point about the openssl function's poor return value stands,
but I would say we can trivially make that function throwing without
having to change its happy-path behavior.
3) I don't actually think making all sources of randomness the same is
good.  There's value in having `random_bytes()` *and* OpenSSL *and*
Sodium to hedge against weaknesses being discovered in any one of
them.

> > […]
-1 for reasons above and what Jakub's already said.
Per #2 above however, I support having openssl_random_psuedo_bytes()
throw on failure rather than merely return false.  A failure there
**should** break any code which isn't explicitly dealing with it.

> > […]
>
+1

-Sara

---

## #103342 — Sammy Kaye Powers — 2018-10-19T16:39:29+00:00

URL: https://externals.io/message/103331#103342 · In reply to: #103336

Thanks for the feedback! That's a good point of having multiple CSPRNG sources.

I'll start drafting an RFC to make `openssl_random_pseudo_bytes()` fail
closed and deprecate the second parameter. :)

Thanks,
Sammy Kaye Powers
sammyk.me