Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:61043
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain lerdorf.com from 209.85.212.42 cause and error)
Message-ID: <4FEDC721.7050202@lerdorf.com>
Date: Fri, 29 Jun 2012 08:17:53 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120615 Thunderbird/13.0.1
MIME-Version: 1.0
To: Anthony Ferrara <ircmaxell@gmail.com>
CC: Nikita Popov <nikita.ppv@gmail.com>, 
 PHP internals <internals@lists.php.net>
References: <CAF+90c_e6Lzd_TZXW-2J_fJjNH=3Up9W744=KH5nbwTE_eUDzw@mail.gmail.com> <CAAyV7nGc2fw9QmedxifQPhaUCWugY+gydZM2ZX9BJ1RJ49NuKA@mail.gmail.com>
In-Reply-To: <CAAyV7nGc2fw9QmedxifQPhaUCWugY+gydZM2ZX9BJ1RJ49NuKA@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] Asking for a review of crypt() allocation changes
From: rasmus@lerdorf.com (Rasmus Lerdorf)

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