On Tue, Aug 18, 2015 at 9:48 PM, Matt Wilmas <php_li...@realplain.com> wrote:
> Hi Anatol, Jakub, > > > ----- Original Message ----- > From: "Jakub Zelenka" > Sent: Tuesday, August 18, 2015 > > On Tue, Aug 18, 2015 at 8:23 PM, Anatol Belski <anatol....@belski.net> >> wrote: >> >> Hi Jakub, >>> >>> > -----Original Message----- >>> > From: Jakub Zelenka [mailto:bu...@php.net] >>> > Sent: Tuesday, August 18, 2015 8:47 PM >>> > To: php-...@lists.php.net >>> > Subject: [PHP-CVS] com php-src: Fix possible overflow in > >>> openssl_pbkdf2: >>> > ext/openssl/openssl.c >>> > >>> > Commit: 618c327a56b03449324cdaa0d630ea710aea22fd >>> > Author: Jakub Zelenka <bu...@php.net> Tue, 18 Aug 2015 >>> 19:46:59 +0100 >>> > Parents: 000be61fb8a034b3ce2dc3d7bbfbc912295b86e9 >>> > Branches: master >>> > >>> > Link: http://git.php.net/?p=php- >>> > src.git;a=commitdiff;h=618c327a56b03449324cdaa0d630ea710aea22fd >>> > >>> > Log: >>> > Fix possible overflow in openssl_pbkdf2 >>> > >>> > Especially key_length would lead to the crash if it overflowed to the >>> negative >>> > value. >>> > >>> > Changed paths: >>> > M ext/openssl/openssl.c >>> > >>> > >>> > Diff: >>> > diff --git a/ext/openssl/openssl.c b/ext/openssl/openssl.c index >>> > 1608e5d..1e03ce7 100644 >>> > --- a/ext/openssl/openssl.c >>> > +++ b/ext/openssl/openssl.c >>> > @@ -4011,6 +4011,22 @@ PHP_FUNCTION(openssl_pbkdf2) >>> > if (key_length <= 0) { >>> > RETURN_FALSE; >>> > } >>> > + if (INT_MAX < key_length) { >>> > + php_error_docref(NULL, E_WARNING, "key_length is too >>> > long"); >>> > + RETURN_FALSE; >>> > + } >>> Key_length is probably not going to be bigger than INT_MAX on 32-bit. I >>> guess there are other similar cases. IMHO it'd make sense to #ifdef them >>> to >>> avoid overheads (even if small) on 64-bit. >>> >>> >> >> Yeah these are just for 64bit where it can be bigger. I just wrapped all >> checks with macros so I can split them for size_t -> int and long -> int >> and make the letter noop on 32-bit. However it's really really small >> overhead compare to other bits in the function. :) But good point anyway >> ;) >> > > The checks with zend_long vars like key_length and iterations are > impossible when ZEND_LONG_MAX == INT_MAX (most if not all 32-bit, I > guess). So those checks should already be removed by the compiler. > > I was thinking that compiler should be able to optimize it out but not sure about all compilers that we support. It's quite cheap so can do that anyway but not sure if it's worthy it. > But for the size_t ones, would need to check SIZEOF_SIZE_T > 4 around the > macros or such. Or you could just change in the definition: > > if (_max < _var) > > to > > if (sizeof(_max) < sizeof(_var) && _max < _var) > > Which should work fine and allow the compiler to remove it easily, without > any extra clutter. > we are talking about size_t (unsigned) and int (signed) so not sure how could compiler optimize it out on 32bit? Did I miss anything? > > BTW, in openssl_pbkdf2(), it looks like the if (!digest) check can be > moved up after EVP_get_digestbyname() as well...? I'm not really sure if there is any point to optimize this at all. PKCS5_PBKDF2_HMAC is quite expensive function ( it is a kdf creating hmac in iterations rounds ) and any such optimization in PHP will be just too small compare to that overhead... But I can of course move it and I might do that.. :) Just think that there is not much point to spend time on optimizing these functions. I think that OpenSSL overhead (also for most other functions) will be just too big compare to these micro optimizations... ;) Cheers Jakub
