Den 2019-11-06 kl. 20:44, skrev Jakub Zelenka:
On Wed, Oct 30, 2019 at 6:33 PM Jakub Zelenka <bu...@php.net> wrote:
On Wed, 30 Oct 2019, 18:32 Jakub Zelenka, <bu...@php.net> wrote:
On Mon, 23 Sep 2019, 14:02 Nikita Popov, <nikita....@gmail.com> wrote:
On Mon, Sep 23, 2019 at 2:52 PM Christian Schneider <
cschn...@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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
