Re: 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. 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. BTW, in openssl_pbkdf2(), it looks like the if (!digest) check can be moved up after EVP_get_digestbyname() as well...?

Cheers

- Matt