RE: [PHP-CVS] com php-src: Fix possible overflow in openssl_pbkdf2: ext/openssl/openssl.c

Hi Matt, > -----Original Message----- > From: Matt Wilmas [mailto:php_lists@realplain.com] > Sent: Tuesday, August 18, 2015 10:48 PM > To: Jakub Zelenka <bukka@php.net>; Anatol Belski <anatol.php@belski.net> > Cc: php-cvs@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.php@belski.net> > > wrote: > > > >> Hi Jakub, > >> > >> > -----Original Message----- > >> > From: Jakub Zelenka [mailto:bukka@php.net] > >> > Sent: Tuesday, August 18, 2015 8:47 PM > >> > To: php-cvs@lists.php.net > >> > Subject: [PHP-CVS] com php-src: Fix possible overflow in > >> > openssl_pbkdf2: > >> > ext/openssl/openssl.c > >> > > >> > Commit: 618c327a56b03449324cdaa0d630ea710aea22fd > >> > Author: Jakub Zelenka <bukka@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. > 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: > Are there platforms where sizeof(size_t) <= sizeof(zend_long) ? By the current info, size_t is same as zend_ulong, so on any given platform sizeof(size_t) > sizeof(zend_long). And almost I were not aware of platforms SIZEOF_SIZE_T < 4 (some 16-bit?) but they should go by the same scheme, actually. Regards Anatol