Everything

php

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

[RFC] [VOTE] Improve openssl_random_pseudo_bytes()

php.internals

Sammy Kaye Powers

7 years ago

Hey internals!

Given there has been at least two weeks of discussion around the RFC,
"Improve openssl_random_pseudo_bytes()" and there doesn't seem to be
any outstanding issues, I have moved the RFC into voting phase.

The voting period lasts two weeks; from now until 2018-11-16.

The RFC can be found here:
https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes

Happy voting! :)

Thanks,
Sammy Kaye Powers
sammyk.me

Niklas Keller

7 years ago

Hey Sammy,

could you please turn your patch into a PR, so it's easier to comment?

Thanks,
Niklas
Am Fr., 2. Nov. 2018 um 20:30 Uhr schrieb Sammy Kaye Powers <me@sammyk.me>:

Nikita Popov

7 years ago

On Fri, Nov 2, 2018 at 8:30 PM Sammy Kaye Powers <me@sammyk.me> wrote:

> Hey internals!
>
> Given there has been at least two weeks of discussion around the RFC,
> "Improve openssl_random_pseudo_bytes()" and there doesn't seem to be
> any outstanding issues, I have moved the RFC into voting phase.
>
> The voting period lasts two weeks; from now until 2018-11-16.
>
> The RFC can be found here:
> https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes
>
> Happy voting! :)

I've voted against the deprecation of the $crypto_strong param. IMHO we
should just document that it is guaranteed to always be true if the call
succeeds and checking it is not necessary. But we should not penalize
existing code that is checking the flag -- it may no longer necessary
nowadays, but previously that was the correct use of the function.

Nikita

Sammy Kaye Powers

7 years ago

@Niklas: Ooops! Thanks for pointing that out. Done! :)
https://github.com/php/php-src/pull/3649

@Nikita

> IMHO we should just document that it is guaranteed to always be true if the call succeeds and checking it is not necessary. But we should not penalize existing code that is checking the flag -- it may no longer necessary nowadays, but previously that was the correct use of the function.

That's fair. I'm just thinking of devs that will encounter the second
param outside of the docs (i.e. via their IDE hints). And since
ext/openssl seems to be here for the long haul, the goal would be to
have a non-confusing API going forward. But I can totally see your
point as well which is the main reason I went for two separate votes.
:)

Thanks,
Sammy Kaye Powers
sammyk.me

Jakub Zelenka

7 years ago

On Sat, Nov 3, 2018 at 10:27 AM Nikita Popov <nikita.ppv@gmail.com> wrote:

> On Fri, Nov 2, 2018 at 8:30 PM Sammy Kaye Powers <me@sammyk.me> wrote:
>
> > Hey internals!
> >
> > Given there has been at least two weeks of discussion around the RFC,
> > "Improve openssl_random_pseudo_bytes()" and there doesn't seem to be
> > any outstanding issues, I have moved the RFC into voting phase.
> >
> > The voting period lasts two weeks; from now until 2018-11-16.
> >
> > The RFC can be found here:
> > https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes
> >
> > Happy voting! :)
>
>
> I've voted against the deprecation of the $crypto_strong param. IMHO we
> should just document that it is guaranteed to always be true if the call
> succeeds and checking it is not necessary. But we should not penalize
> existing code that is checking the flag -- it may no longer necessary
> nowadays, but previously that was the correct use of the function.
>
>

I think this is a good point so voted against it too.

Michael Moravec

7 years ago

On Fri, 2 Nov 2018 at 20:30, Sammy Kaye Powers <me@sammyk.me> wrote:

>
> Hey internals!
>
> Given there has been at least two weeks of discussion around the RFC,
> "Improve openssl_random_pseudo_bytes()" and there doesn't seem to be
> any outstanding issues, I have moved the RFC into voting phase.
>
> The voting period lasts two weeks; from now until 2018-11-16.
>
> The RFC can be found here:
> https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes
>
> Happy voting! :)

Hi,

the "Backward Incompatible Changes" section fails to mention that this
function will start throwing an exception in next MINOR PHP version (7.4)
in all existing code. As such this is quite heavy BC break.

I think this should be clarified in the "Backward Incompatible Changes" section,
the RFC postponed to 8.0 (as majors are for BC breaks) and restarted.

Thanks,
Michael

Sammy Kaye Powers

7 years ago

Hey Michael!

I really appreciate you fighting against BC's in minor versions. We
certainly need to always be hyper aware of the impact BC's have on the
community.

First, I'll address your concerns you raised on GitHub here:
https://github.com/php/php-src/pull/3649#issuecomment-435684017

> Well, throwing an exception instead of returning false is a huge BC break in a minor version.

Indeed and it is. And it's an important and very much needed one since
we're dealing with cryptographic contexts. As infosec people say, "Bad
cryptography is not backwards compatible with good cryptography." If
we don't make the CSPRNG fail closed, we're allowing a potential
attack vector in every single PHP script that doesn't have a fail
check.

> This type of stuff is what makes people (and hosting providers) to not upgrade to newer minor versions.

I think it's important to point out, we're talking about two very
distinct BC's here. 1) Failing closed which is a security-related BC
(a must break IMO). 2) A confusing API clean-up. I'm not sure there's
a 100% right answer to fixing the second BC; whether we keep the
second param or deprecate it, both options have tradeoffs. Perhaps
this second deprecation BC should wait until 8.0, but that's why I
made it a separate vote (and at this point the deprecation BC looks
like it will fail anyway). It's really tough to get this stuff right -
everything is a tradeoff. :)

> "should rarely occur" Can you provide some metrics for this statement?

I don't have stats on how often `RAND_bytes()` returns `0`. But even
if it failed often, that's even more reason to have
`openssl_random_pseudo_bytes()` fail closed and shut down that
potential attack vector.

> the "Backward Incompatible Changes" section fails to mention that this
> function will start throwing an exception in next MINOR PHP version (7.4)

Failing closed is an inherent BC break and the RFC explicitly targets
PHP 7.4. The entire RFC after all is a big "Backward Incompatible
Change". :) So I'm not really following how that is unclear.

But again - thanks for fighting against BC's! I totally agree with you
that we gotta keep those BC's to a minimum; even in major versions. <3

Thanks,
Sammy Kaye Powers
sammyk.me

Sammy Kaye Powers

7 years ago

Hey internals folks!

I'm happy to announce that the vote to make
openssl_random_pseudo_bytes() fail closed in the "Improve
openssl_random_pseudo_bytes()" RFC passed unanimously with 30 votes.

The other vote to deprecate the second parameter failed with a
straight 50/50 split.

I still have to make some small tweaks to the implementation before it
can be merged to master, but yay for better security! :D

https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes

Thanks,
Sammy Kaye Powers
sammyk.me