Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:54268
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain openwall.com designates 195.42.179.200 as permitted sender)
Date: Mon, 1 Aug 2011 02:33:27 +0400
To: Stas Malyshev <smalyshev@sugarcrm.com>
Cc: PHP Internals List <internals@lists.php.net>
Message-ID: <20110731223327.GA23361@openwall.com>
References: <20110719234406.GB28946@openwall.com> <4E35CC70.1050203@sugarcrm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4E35CC70.1050203@sugarcrm.com>
User-Agent: Mutt/1.4.2.3i
Subject: Re: [PHP-DEV] CRYPT_SHA256 fails tests in trunk
From: solar@openwall.com (Solar Designer)

Hi Stas, Pierre -

On Sun, Jul 31, 2011 at 02:43:12PM -0700, Stas Malyshev wrote:
> On 7/19/11 4:44 PM, Solar Designer wrote:
> >That is, the salts are truncated.  There's a relevant recent change in
> >crypt.c involving the line:
> >
> >		salt_in_len = MIN(PHP_MAX_SALT_LEN, salt_in_len);
> 
> Thanks for the report.
> This problem seems to be unrelated to this change, but in fact looks 
> like it's related to this code in

You appear to be correct.

> 	if ((salt - (char *) 0) % __alignof__(uint32_t) != 0) {
> 		char *tmp = (char *) alloca(salt_len + 1 + 
> 		__alignof__(uint32_t));
> 		salt = copied_salt =
> 		memcpy(tmp + __alignof__(uint32_t) - (tmp - (char *) 0) % 
> 		__alignof__ (uint32_t), salt, salt_len);
> 		tmp[salt_len] = 0;
> 	}
> 
> As you can see, the last line cuts the string relative to tmp, but salt 
> is copied to tmp with offest, which leads to salt truncation. Changing 
> it from tmp to copied_salt seems to fix the problem. I'll apply the fix 
> in a minute.

Right.

> The change that introduced this problem is:
> http://svn.php.net/viewvc/php/php-src/branches/PHP_5_3/ext/standard/crypt_sha256.c?r1=300427&r2=312952

Now that I look at this, I think there are more problems around this
place in the code:

1. Pierre's commit that inadvertently introduced the truncation was a
bugfix, adding the previously missed NUL termination of the copied salt.
However, the similar piece of code copying the key appears to still lack
the NUL termination (it works by pure luck).

2. alloca() of potentially user-controlled size is unsafe - it may
result in the stack pointer being moved outside of allowable range and
onto other areas, such as the heap.  With a large enough allocation,
it'd jump over the few guard pages there might be.  In the worst case,
this will ultimately allow for arbitrary machine code execution via an
overly long password.  (BTW, we need to check glibc for similar issues.)

You could move away from using alloca() there or introduce some sane
length limits (less than one page).  But then you need to decide on the
proper action on failure.  Another approach would be to avoid the
copying, although it may have a performance impact (having to deal with
potentially unaligned data inside loops), would involve more invasive
changes, and thus would require testing and benchmarking - so probably
not an option for the upcoming releases now (and maybe beyond scope of
PHP development at all).

Oh, there are two more alloca() calls further down the function - one of
the salt (not an issue if we limit the salt length above), and one of
the key (problematic).  This needs to be dealt with as well.

So perhaps you need to have the function return NULL right after the
first strlen(key) if the length turns out to be excessive (more than
2048 bytes? which is half a page on x86).  Then the wrapper code in
crypt.c will translate this into "*0".

crypt_sha512.c has similar issues - both with NUL termination and with
unlimited alloca().

We also need to check the MD5-crypt code for this.

I hope this helps...

Alexander