Developing PHP

Nikita Popov | 29 Jun 2012 14:43

[PHP-DEV] Asking for a review of crypt() allocation changes

Hi internals!

Anthony and me have been looking a lot at the crypt() code recently
and noticed that there are some strange things going on in the buffer
allocations for the sha algorithms.

We did two commits to fix them up a bit:

http://git.php.net/?p=php-src.git;a=commitdiff;h=7e8276ca68fc622124d51d18e4f7b5cde3536de4
http://git.php.net/?p=php-src.git;a=commitdiff;h=e6cf7d774519300c08399cae5bfba90e33749727

The complete code is here:
http://lxr.php.net/xref/PHP_TRUNK/ext/standard/crypt.c#150

To prevent another crypt() mess, I'd really appreciate if some people
familiar with the code could look over it.

Nikita

--

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

headers

Anthony Ferrara | 29 Jun 2012 14:56

Re: [PHP-DEV] Asking for a review of crypt() allocation changes

Additionally, it appears that SHA256/512 are way overallocating the buffer.

For SHA512:

int needed = (sizeof(sha512_salt_prefix) - 1
                        + sizeof(sha512_rounds_prefix) + 9 + 1
                        + salt_in_len + 1 + 86 + 1);
output = emalloc(needed);
salt[salt_in_len] = '\0';
crypt_res = php_sha512_crypt_r(str, salt, output, needed);
// snip
memset(output, 0, needed);
efree(output);

The salt_in_len includes the salt_prefix and the rounds_prefix as well.

Since salt_in_len (thanks to the allocations before hand) can be up to
PHP_MAX_SALT_LEN
http://lxr.php.net/xref/PHP_TRUNK/ext/standard/crypt.c#88 which is 123
characters, the output of the function can be no bigger than
PHP_MAX_SALT_LEN. Therefore the "needed" variable can be replaced by
PHP_MAX_SALT_LEN everywhere...

output = emalloc(MAX_SALT_LEN);
salt[salt_in_len] = '\0';
crypt_res = php_sha512_crypt_r(str, salt, output, MAX_SALT_LEN);
// snip
memset(output, 0, MAX_SALT_LEN);
efree(output);

(Continue reading)

headers

Rasmus Lerdorf | 29 Jun 2012 17:17

Re: [PHP-DEV] Asking for a review of crypt() allocation changes

On 06/29/2012 05:56 AM, Anthony Ferrara wrote:
> Additionally, it appears that SHA256/512 are way overallocating the buffer.
> 
> For SHA512:
> 
> int needed = (sizeof(sha512_salt_prefix) - 1
>                         + sizeof(sha512_rounds_prefix) + 9 + 1
>                         + salt_in_len + 1 + 86 + 1);
> output = emalloc(needed);
> salt[salt_in_len] = '\0';
> crypt_res = php_sha512_crypt_r(str, salt, output, needed);
> // snip
> memset(output, 0, needed);
> efree(output);
> 
> The salt_in_len includes the salt_prefix and the rounds_prefix as well.
> 
> Since salt_in_len (thanks to the allocations before hand) can be up to
> PHP_MAX_SALT_LEN
> http://lxr.php.net/xref/PHP_TRUNK/ext/standard/crypt.c#88 which is 123
> characters, the output of the function can be no bigger than
> PHP_MAX_SALT_LEN. Therefore the "needed" variable can be replaced by
> PHP_MAX_SALT_LEN everywhere...
> 
> output = emalloc(MAX_SALT_LEN);
> salt[salt_in_len] = '\0';
> crypt_res = php_sha512_crypt_r(str, salt, output, MAX_SALT_LEN);
> // snip
> memset(output, 0, MAX_SALT_LEN);
> efree(output);

(Continue reading)

headers

Ángel González | 30 Jun 2012 09:38

Re: [PHP-DEV] Asking for a review of crypt() allocation changes

On 29/06/12 14:43, Nikita Popov wrote:
> Hi internals!
>
> Anthony and me have been looking a lot at the crypt() code recently
> and noticed that there are some strange things going on in the buffer
> allocations for the sha algorithms.
>
> We did two commits to fix them up a bit:
>
> http://git.php.net/?p=php-src.git;a=commitdiff;h=7e8276ca68fc622124d51d18e4f7b5cde3536de4
It took me a while to realise the problem being fixed. The bug is not
the memset (as reported in bug 62443),
which is using needed (got fixed in e6cf7d), or php_sha{256,512}_crypt_r
(uses a null-terminated string), but the salt[salt_in_len] = '\0';
after allocating only strlen(salt).
So that you would be accessing the position PHP_MAX_SALT_LEN of the
array but have reserved only a few bytes.
Just*sizeof*(sha512_rounds_prefix
<http://lxr.php.net/xref/PHP_TRUNK/ext/standard/crypt.c#sha512_rounds_prefix>)
+ 9 + 1 seem enough for not making bug62443.phpt segfault.
I have been able to crash it with var_dump( crypt("foo", '$6$'.chr(0).
str_pad('', 500, '*') . '$abc') );
but only if it's the first call.