Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:107778 Return-Path: Delivered-To: mailing list internals@lists.php.net Received: (qmail 97902 invoked from network); 7 Nov 2019 12:20:06 -0000 Received: from unknown (HELO php-smtp3.php.net) (208.43.231.12) by pb1.pair.com with SMTP; 7 Nov 2019 12:20:06 -0000 Received: from php-smtp3.php.net (localhost [127.0.0.1]) by php-smtp3.php.net (Postfix) with ESMTP id D6B962C0541 for ; Thu, 7 Nov 2019 02:09:58 -0800 (PST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on php-smtp3.php.net X-Spam-Level: X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_LOW, SPF_HELO_NONE autolearn=no autolearn_force=no version=3.4.2 X-Spam-ASN: AS3301 81.224.0.0/12 X-Spam-Virus: Error (Cannot connect to unix socket '/var/run/clamav/clamd.ctl': connect: Connection refused) Received: from v-smtpout3.han.skanova.net (v-smtpout3.han.skanova.net [81.236.60.156]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by php-smtp3.php.net (Postfix) with ESMTPS for ; Thu, 7 Nov 2019 02:09:57 -0800 (PST) Received: from [192.168.7.5] ([213.64.245.126]) by cmsmtp with ESMTPA id SejiiqUXKXLfmSejiiIxhY; Thu, 07 Nov 2019 11:09:55 +0100 To: Jakub Zelenka , Nikita Popov , "Christoph M. Becker" Cc: Christian Schneider , PHP internals list References: Message-ID: <3379d788-0244-de70-21f1-c03d6214e453@telia.com> Date: Thu, 7 Nov 2019 11:09:55 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-CMAE-Envelope: MS4wfEafZSyV92zVYY0vaVpnSKNGD2dohkkHLooQfhWVwT7fIEtbSqvaiNWjApVdVDyC2YdUfz8q/QPcTfr5ssg4qQykedRtl4QO2l758WdptYxdqcHWgt00 r6u9mbxE5pAEzgzU3umLx0Hm8mdk+ag78MUcAaYY3UllPtcHkkwAzYx62LTNJ7DS/II76FLmcUfndUtq88OUg0OHj+wfiOJE6MWNIQA4WuhluOhwE8pX0rDI d3eFLMKxnqz3cdqOnlZE8WwJXe6+BdIVcDUgtIvAwRD38PzwEnyp3w5AAmWNIzLM X-Envelope-From: Subject: Re: [PHP-DEV] PHP 7.4 BC break with openssl_random_pseudo_bytes() From: 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 wrote: > >> >> On Wed, 30 Oct 2019, 18:32 Jakub Zelenka, wrote: >> >>> >>> On Mon, 23 Sep 2019, 14:02 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. >>>> >>> 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