Re: 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); 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.ppv@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 >