Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:107300
User-Agent: Cyrus-JMAP/3.1.7-238-g170a812-fmstable-20190913v1
Mime-Version: 1.0
Message-ID: <696dc114-c2df-40aa-aad6-5b87d4373c0e@www.fastmail.com>
In-Reply-To: 
 <CAF+90c_75x0ZoD63CoLXY3kUKNQQuR_HhaO998osHuzV7ZGXuw@mail.gmail.com>
References: <A91E718E-B023-42DC-AC8D-BC989F5A80E4@cschneid.com>
 <CAF+90c_75x0ZoD63CoLXY3kUKNQQuR_HhaO998osHuzV7ZGXuw@mail.gmail.com>
Date: Mon, 23 Sep 2019 08:16:49 -0700
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] PHP 7.4 BC break with openssl_random_pseudo_bytes()
From: larry@garfieldtech.com ("Larry Garfield")

On Mon, Sep 23, 2019, at 6:01 AM, Nikita Popov wrote:
> On Mon, Sep 23, 2019 at 2:52 PM Christian Schneider <cschneid@cschneid.com>
> wrote:
> 
> > Hi,
> > I just noted (too late in the process, I know) that
> > openssl_random_pseudo_bytes(0) now throws an exception.
> >
> > This breaks code like
> >         $ivsize = openssl_cipher_iv_length($method);
> >         $iv = openssl_random_pseudo_bytes($ivsize);
> >         $data = openssl_encrypt($string, $method, $key, OPENSSL_RAW_DATA,
> > $iv);
> > if $method is 'aes-256-ecb' because $ivsize is 0.
> >
> > I do realize that ECB mode ciphers are deprecated but having them throw an
> > exception indirectly via openssl_random_pseudo_bytes() seems a bit strange,
> > even in the context of security.
> >
> > I checked the RFC
> > https://wiki.php.net/rfc/improve-openssl-random-pseudo-bytes and it
> > doesn't mention this BC break:
> > "False-checks on the return value of openssl_random_pseudo_bytes() will do
> > nothing since the function fails closed. Usage of $crypto_strongwill
> > generate errors."
> >
> > While I would have preferred the exception to be thrown only when $ivsize
> > is not an integer or less than 0 but I guess this cannot be changed at the
> > RC stage.
> >
> > I would recommend though that we aim to keep BC breaks to what's mentioned
> > in RFCs.
> >
> 
> This was noted during the PR review in:
> https://github.com/php/php-src/pull/3649#discussion_r230598754 Especially
> in conjunction with your example, I think we should revert this part an
> make openssl_random_pseudo_bytes(0) return "" without exception or warning.
> Ideally we'd adjust random_bytes() to do the same.
> 
> Nikita

I cannot speak for OpenSSL,  but random_bytes() and random_int() were changed very late in the 7.0 cycle to throw exceptions so that they "fail closed".  Otherwise if you expect a random value back but get a constant value (false or empty string), if you don't remember to check it yourself every time then you now have a security hole because you're using a constant seed for random-dependent behavior.

That was a good change, and it should be kept that way, IMO.

--Larry Garfield