Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:54269
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:54:29 +0400
To: Stas Malyshev <smalyshev@sugarcrm.com>,
	Pierre Joye <pierre.php@gmail.com>
Cc: PHP Internals List <internals@lists.php.net>
Message-ID: <20110731225429.GA23508@openwall.com>
References: <20110719234406.GB28946@openwall.com> <4E35CC70.1050203@sugarcrm.com> <20110731223327.GA23361@openwall.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20110731223327.GA23361@openwall.com>
User-Agent: Mutt/1.4.2.3i
Subject: Re: [PHP-DEV] CRYPT_SHA256 fails tests in trunk
From: solar@openwall.com (Solar Designer)

On Mon, Aug 01, 2011 at 02:33:27AM +0400, Solar Designer wrote:
> On Sun, Jul 31, 2011 at 02:43:12PM -0700, Stas Malyshev wrote:
> > 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).

Upon a closer look, the NUL termination might not be needed - neither
for the key nor for the salt.  Ulrich Drepper's reference implementation
similarly does not NUL-terminate these, and does not appear to rely on
NUL termination in further code.  Has the code in PHP somehow deviated
from this convention?

Pierre - what was the reason for your commit?

...Oh, I think I see the problem: __php_stpncpy() is not sufficiently
correct.  It starts by calling strlen(src).  Maybe fixing it to more
closely match the real thing would be a cleaner fix?

Either way, crypt_sha256.c and crypt_sha512.c need to be made similar in
this respect.  Right now, they are not.

> 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.)

This problem appears to be present in Ulrich Drepper's reference
implementation as well.

> We also need to check the MD5-crypt code for this.

No alloca() in the code in ext/standard/php_crypt_r.c.  Instead, it
assumes that the MD5 implementation itself is able to handle unaligned
inputs, which it is.

Alexander