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