Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:103435
MIME-Version: 1.0
References: <CALYT1YpSw7NyGugVgmuppx9c46nBzXDz8Ovpfadyz944ZqV-4Q@mail.gmail.com>
 <CAO7eTqO2P=w5VPeZg1NEEQqS0CFRERn711tsR+i6SxUJOOEKaQ@mail.gmail.com>
In-Reply-To: <CAO7eTqO2P=w5VPeZg1NEEQqS0CFRERn711tsR+i6SxUJOOEKaQ@mail.gmail.com>
Date: Fri, 9 Nov 2018 23:26:09 -0500
Message-ID: <CALYT1YpnSbfsW0q9bZSjrqgcXzuOq=vtm=j8err7RDFqPJxUzQ@mail.gmail.com>
To: php.net@majkl578.cz
Cc: PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [PHP-DEV] [RFC] [VOTE] Improve openssl_random_pseudo_bytes()
From: me@sammyk.me (Sammy Kaye Powers)

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