Re: com php-src: Fix possible overflow in openssl_pbkdf2: ext/openssl/openssl.c

Hi Jakub, ----- Original Message ----- From: "Jakub Zelenka" Sent: Tuesday, August 18, 2015

On Tue, Aug 18, 2015 at 9:48 PM, Matt Wilmas <php_lists@realplain.com> wrote:

Hi Anatol, Jakub, [...] 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.

If any compiler can't do such a simple optimization, then it's going to be missing many other possible optimizations as well. Read: Who cares about them. :-)

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?

sizeof(INT_literal_or_var) should be equal (not <) to sizeof(size_t_var) on 32-bit (if ints are 32-bit). So the condition becomes (4 < 4 && _max < _var) and is optimized out. The signedness doesn't matter, and we don't care in this case, but the compiler can't otherwise know that... Of course on 64-bit, only (_max < _var) remains -- (4 < 8) is removed.

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... ;)

Yeah, I know, I was just sayin'. :-D Since I assumed EVP_sha1()'s return value didn't need to be checked, that's all (I don't know anything about it!).

Cheers Jakub

- Matt