Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:61038
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.42 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAF+90c_e6Lzd_TZXW-2J_fJjNH=3Up9W744=KH5nbwTE_eUDzw@mail.gmail.com>
References: <CAF+90c_e6Lzd_TZXW-2J_fJjNH=3Up9W744=KH5nbwTE_eUDzw@mail.gmail.com>
Date: Fri, 29 Jun 2012 08:56:59 -0400
Message-ID: <CAAyV7nGc2fw9QmedxifQPhaUCWugY+gydZM2ZX9BJ1RJ49NuKA@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset=ISO-8859-1
Subject: Re: [PHP-DEV] Asking for a review of crypt() allocation changes
From: ircmaxell@gmail.com (Anthony Ferrara)

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
>