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);
We can do the same for sha256. But in that case, we'll be
overallocating the buffer as well. Although not nearly as bad as we
are now.
Thoughts?
Anthony
On Fri, Jun 29, 2012 at 8:43 AM, Nikita Popov <nikita....@gmail.com> 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
> 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
>
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
