PHP 7.4 BC break with openssl_random_pseudo_bytes()

  107294
September 23, 2019 12:51 cschneid@cschneid.com (Christian Schneider)
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.

- Chris
  107295
September 23, 2019 13:01 nikita.ppv@gmail.com (Nikita Popov)
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
  107300
September 23, 2019 15:16 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
  107301
September 23, 2019 16:34 cschneid@cschneid.com (Christian Schneider)
Am 23.09.2019 um 17:16 schrieb Larry Garfield <larry@garfieldtech.com>:
> 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.
I see your point but I'm still not convinced that it is worth the BC. But whatever is decided for this specific change, I'm more interested in handling this properly for future RFCs, i.e. people should get the full picture concerning BC before voting. A little side-node: random_int(0, 0) does not throw an exception which makes random_bytes and random_int inconsistent by your logic ;-) - Chris
  107303
September 23, 2019 18:37 mo.mu.wss@gmail.com ("M. W. Moe")
Hello,

"A little side-node: random_int(0, 0) does not throw an exception which
makes random_bytes and random_int inconsistent by your logic ;-)"

not really; there are still different functions; hence they can differ in
their behavior; + that's not a matter of individual logic but an api
choice; everything can be argued *; however, I don't see any BC break here
but a `addon` instead of failing silently, like it was before; hiding a
very wrong state.


Regards.

* the smiley doesn't help.

On Mon, Sep 23, 2019 at 9:34 AM Christian Schneider <cschneid@cschneid.com>
wrote:

> Am 23.09.2019 um 17:16 schrieb Larry Garfield <larry@garfieldtech.com>: > > 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. > > I see your point but I'm still not convinced that it is worth the BC. > But whatever is decided for this specific change, I'm more interested in > handling this properly for future RFCs, i.e. people should get the full > picture concerning BC before voting. > > A little side-node: random_int(0, 0) does not throw an exception which > makes random_bytes and random_int inconsistent by your logic ;-) > > - Chris > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > >
  107311
September 24, 2019 14:26 larry@garfieldtech.com ("Larry Garfield")
On Mon, Sep 23, 2019, at 11:34 AM, Christian Schneider wrote:
> Am 23.09.2019 um 17:16 schrieb Larry Garfield <larry@garfieldtech.com>: > > 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. > > I see your point but I'm still not convinced that it is worth the BC. > But whatever is decided for this specific change, I'm more interested > in handling this properly for future RFCs, i.e. people should get the > full picture concerning BC before voting. > > A little side-node: random_int(0, 0) does not throw an exception which > makes random_bytes and random_int inconsistent by your logic ;-) > > - Chris
Er. Leaving random_bytes() as is has no BC break, kinda by definition. I was arguing that changing it to return false would be a Very Bad Thing(tm). And no, random_int(0,0) does what it says on the tin: return a random int between 0 and 0. If you call it that way, well, it's your own PEBCAK. But it throws an exception if the underlying sources of entropy are not working for some reason, rather than returning something that can easily be mistaken for a valid integer. random_*() are Doing It Right(tm). Don't change them. :-) --Larry Garfield
  107312
September 24, 2019 15:49 rowan.collins@gmail.com (Rowan Tommins)
On Tue, 24 Sep 2019 at 15:26, Larry Garfield <larry@garfieldtech.com> wrote:

> And no, random_int(0,0) does what it says on the tin: return a random int > between 0 and 0. If you call it that way, well, it's your own PEBCAK. But > it throws an exception if the underlying sources of entropy are not working > for some reason, rather than returning something that can easily be > mistaken for a valid integer. >
I think the argument was that the consistent behaviour would be for random_bytes(0) and openssl_random_pseudo_bytes(0) to return '' (i.e. a random string which was zero bytes long). The result is just as logical, and just as meaningless, as "a number between 0 and 0" - in both cases, there is exactly one valid value, so every random choice returns that value. The BC break is a separate discussion - the RFC listed some changes to openssl_random_pseudo_bytes but not this one. Regards, -- Rowan Tommins [IMSoP]
  107306
September 24, 2019 04:18 pierre.php@gmail.com (Pierre Joye)
On Mon, Sep 23, 2019 at 10:17 PM Larry Garfield <larry@garfieldtech.com> wrote:

> 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.
Fully agree. This is actually pretty the only way to handle errors with these functions. Anything else creates a risk that we could have easily prevented. Best, -- Pierre @pierrejoye | http://www.libgd.org
  107309
September 24, 2019 08:11 cschneid@cschneid.com (Christian Schneider)
Am 24.09.2019 um 06:18 schrieb Pierre Joye php@gmail.com>:
> On Mon, Sep 23, 2019 at 10:17 PM Larry Garfield <larry@garfieldtech.com> wrote: > >> 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. > > Fully agree. This is actually pretty the only way to handle errors > with these functions. Anything else creates a risk that we could have > easily prevented.
The main point of my original mail was stripped so I changed the subject to emphasise what I really care about. So here is my question: Am I the only one who thinks BC breaks should be fully covered in an RFC before voting? Regards, - Chris
  107318
September 25, 2019 01:47 pierre.php@gmail.com (Pierre Joye)
On Tue, Sep 24, 2019, 3:11 PM Christian Schneider <cschneid@cschneid.com>
wrote:

> > So here is my question: Am I the only one who thinks BC breaks should be > fully covered in an RFC before voting? >
If I am not mistaken this is the rule yes. A specific section should exist to list BC breaks. Also a BC break is not allowed in minor versions. The question is also about what is a BC break, f.e is changing error level a BC break? or the return value on error? I am in favour of more strict BC rules for minor versions, but this is a tricky topic. Best,
  107319
September 25, 2019 10:14 cschneid@cschneid.com (Christian Schneider)
Am 25.09.2019 um 03:47 schrieb Pierre Joye php@gmail.com>:
> The question is also about what is a BC break, f.e is changing error level a BC break? or the return value on error?
This seems to be a complicated question but I think if we boil it down to a guideline instead of a hard rule it is not that hard after all. My personal view is this: Changing error levels is certainly an edge case: No, the code does not have to be changed to get the same result. Yes, they can output additional error messages (hopefully to a log file, not stdout) which may need to be filtered. But I'd still think that's a very minor change and is less important. When the control flow of the program for well-defined behaviour changes, that's when previously working code breaks. - Changing a notice/warning to an exception is a BC break as it needs different code to handle it. - Return values on error is a BC break as code trying to properly handle errors has to be changed (e.g. strpos === false breaks if the error return value is changed to null). This doesn't mean such changes cannot be done but there should be a consensus that it is worth it IMHO. An broad agreement on what is and what isn't a BC break would be useful. It doesn't have to cover everything but getting on the same page for stuff like the above might help reduce friction when discussing changes. - Chris
  107734
October 30, 2019 18:32 bukka@php.net (Jakub Zelenka)
On Mon, 23 Sep 2019, 14:02 Nikita Popov, ppv@gmail.com> 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. >
I agree this should be reverted for 7.4 at least as it's a BC and wasn't really in RFC. It really doesn't matter if it's a good thing or not as it's a BC that we shouldn't do in minor release. Jakub
>
  107735
October 30, 2019 18:33 bukka@php.net (Jakub Zelenka)
On Wed, 30 Oct 2019, 18:32 Jakub Zelenka, <bukka@php.net> wrote:

> > > On Mon, 23 Sep 2019, 14:02 Nikita Popov, ppv@gmail.com> 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. >> > > I agree this should be reverted for 7.4 at least as it's a BC and wasn't > really in RFC. It really doesn't matter if it's a good thing or not as it's > a BC that we shouldn't do in minor release. >
I mean BC break ofc... :)
>
  107776
November 6, 2019 19:44 bukka@php.net (Jakub Zelenka)
On Wed, Oct 30, 2019 at 6:33 PM Jakub Zelenka <bukka@php.net> wrote:

> > > On Wed, 30 Oct 2019, 18:32 Jakub Zelenka, <bukka@php.net> wrote: > >> >> >> On Mon, 23 Sep 2019, 14:02 Nikita Popov, ppv@gmail.com> 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. >>> >> >> I agree this should be reverted for 7.4 at least as it's a BC and wasn't >> really in RFC. It really doesn't matter if it's a good thing or not as it's >> a BC that we shouldn't do in minor release. >> > > I mean BC break ofc... :) >
I was thinking about the revert but after reading the RFC again, I don't think it would be right thing to do. Although it's a bit generic and doesn't mention when it can fail, it states that the exception is thrown on fail. We always considered this case as a fail because it returned false so it's kind of covered. It would be also quite late to do such changes in 7.4 and I guess RM wouldn't allow it anyway. It's basically BC break that went through the RFC which is probably acceptable. I have to say that the RFC wasn't really well done as the implementation followed which caused this omission. We should really look properly to the implementation when creating RFC so it's more detailed and doesn't cause omission like this. Cheers Jakub
  107777
November 7, 2019 10:00 rowan.collins@gmail.com (Rowan Tommins)
On Wed, 6 Nov 2019 at 19:44, Jakub Zelenka <bukka@php.net> wrote:

> I have to say that the RFC wasn't really well done as the implementation > followed which caused this omission. We should really look properly to the > implementation when creating RFC so it's more detailed and doesn't cause > omission like this. >
There's a bit of a Catch-22 there: voters want to know all the edge-cases before they approve an RFC; but developers want to know the RFC will be approved before they spend hours refining a patch. In some ways, it would be good to have two votes: one on the principle, and one on the detailed implementation; but that could make the process feel quite long and bureaucratic. Regards, -- Rowan Tommins [IMSoP]
  107778
November 7, 2019 10:09 bjorn.x.larsson@telia.com (=?UTF-8?Q?Bj=c3=b6rn_Larsson?=)
Den 2019-11-06 kl. 20:44, skrev Jakub Zelenka:
> On Wed, Oct 30, 2019 at 6:33 PM Jakub Zelenka <bukka@php.net> wrote: > >> >> On Wed, 30 Oct 2019, 18:32 Jakub Zelenka, <bukka@php.net> wrote: >> >>> >>> On Mon, 23 Sep 2019, 14:02 Nikita Popov, ppv@gmail.com> 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. >>>> >>> I agree this should be reverted for 7.4 at least as it's a BC and wasn't >>> really in RFC. It really doesn't matter if it's a good thing or not as it's >>> a BC that we shouldn't do in minor release. >>> >> I mean BC break ofc... :) >> > I was thinking about the revert but after reading the RFC again, I don't > think it would be right thing to do. Although it's a bit generic and > doesn't mention when it can fail, it states that the exception is thrown on > fail. We always considered this case as a fail because it returned false so > it's kind of covered. It would be also quite late to do such changes in 7.4 > and I guess RM wouldn't allow it anyway. It's basically BC break that went > through the RFC which is probably acceptable. > > I have to say that the RFC wasn't really well done as the implementation > followed which caused this omission. We should really look properly to the > implementation when creating RFC so it's more detailed and doesn't cause > omission like this. > > Cheers > > Jakub
Hi, So, what's the RM view on this? I think one should take into account the total amount of deprecations & BC breakages in 7.4. If they are many, it's a pity to add things making code break even more. Another thing to consider is that when upgrading and there are breakages one doesn't upgrade, instead staying on an old PHP version. So providing an easier upgrade path also has value! Also, is there an estimate on how this BC breakage would affect libraries? Or to phrase it differently is it a common code pattern that now will generate exceptions instead? r//Björn L