Re: PHP 7.4 BC break with openssl_random_pseudo_bytes()

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-bytesI@“·J > > ñåY•ŽZï$" 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