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.
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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
