Everything

php

Home » Project » Newsgroups » php.internals » [RFC] Improve openssl_random_pseudo_bytes() »

[RFC] Improve openssl_random_pseudo_bytes()

php.internals

Sammy Kaye Powers

7 years ago

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

Sara Golemon

7 years ago

On Fri, Oct 19, 2018 at 3:47 PM Sammy Kaye Powers <me@sammyk.me> wrote:

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

Sara Golemon

7 years ago

On Fri, Oct 19, 2018 at 9:20 PM Sara Golemon <pollita@php.net> wrote:

> On Fri, Oct 19, 2018 at 3:47 PM Sammy Kaye Powers <me@sammyk.me> wrote:
> > 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.
>

Or rather:   ZVAL_BOOL(pzv, 1)  => ZVAL_TRUE(pzv)

Sammy Kaye Powers

7 years ago

On Fri, Oct 19, 2018 at 10:21 PM Sara Golemon <pollita@php.net> wrote:

>
> On Fri, Oct 19, 2018 at 9:20 PM Sara Golemon <pollita@php.net> wrote:
> > On Fri, Oct 19, 2018 at 3:47 PM Sammy Kaye Powers <me@sammyk.me> wrote:
> > > 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.
> >
> Or rather:   ZVAL_BOOL(pzv, 1)  => ZVAL_TRUE(pzv)

Good call - done! :)

Tom Worster

7 years ago

On 19 Oct 2018, at 16:46, Sammy Kaye Powers wrote:

> 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

Sammy Kaye Powers

7 years ago

Hey Tom! Thanks for the feedback! :)

On Sun, Oct 21, 2018 at 8:09 PM Tom Worster <fsb@thefsb.org> wrote:

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

Tom Worster

7 years ago

Hi Sammy,

On 22 Oct 2018, at 9:46, Sammy Kaye Powers wrote:

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

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

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.

>> The only reason to keep this function is BC
>
> 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

Sammy Kaye Powers

7 years ago

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