Hi On 18 Aug 2015 22:21, "Anatol Belski" <anatol....@belski.net> wrote: > > Hi Matt, > > > -----Original Message----- > > From: Matt Wilmas [mailto:php_li...@realplain.com] > > Sent: Tuesday, August 18, 2015 10:48 PM > > To: Jakub Zelenka <bu...@php.net>; Anatol Belski <anatol....@belski.net> > > Cc: php-...@lists.php.net; internals@lists.php.net > > Subject: Re: [PHP-CVS] com php-src: Fix possible overflow in openssl_pbkdf2: > > ext/openssl/openssl.c > > > > 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. > > > It's unlikely to be optimized away, key_length is a variable, not a constant. It is "INT_MAX < variable", unlikely a compiler can predict what is in the variable, disregarding bitness
Yeah but a compiler knows the range of the variable and if it cannot be more than int_max (e.g. long on 32bit) then it could be possible to optimize it out. We would probably have to try it to see... :)
