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
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
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=e6cf7d774519300c08399cae5bfba90e33749727The complete code is here:
http://lxr.php.net/xref/PHP_TRUNK/ext/standard/crypt.c#150To prevent another
crypt()mess, I'd really appreciate if some people
familiar with the code could look over it.Nikita
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?
We need to get into the habit of using safe_emalloc() on any emalloc()
that takes its size from a user-provided argument.
-Rasmus
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.
Justsizeof(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.