Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:91979
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, 28 Mar 2016 17:21:47 +0300
To: Pierre Joye <pierre.php@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Message-ID: <20160328142147.GA4936@openwall.com>
References: <20160327151603.GA31135@openwall.com> <CAEZPtU6=cUx4q2p+VOwNZv6=vYTOwUqTN-+T+NOEZshW6294kQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAEZPtU6=cUx4q2p+VOwNZv6=vYTOwUqTN-+T+NOEZshW6294kQ@mail.gmail.com>
User-Agent: Mutt/1.4.2.3i
Subject: Re: [PHP-DEV] crypt_blowfish salt padding
From: solar@openwall.com (Solar Designer)

Hi Pierre,

On Mon, Mar 28, 2016 at 02:22:13PM +0700, Pierre Joye wrote:
> On Sun, Mar 27, 2016 at 10:16 PM, Solar Designer <solar@openwall.com> wrote:
> > $ git show 03315d9625dc87515f1dfbf1cc7d53c4451b5ec9 | fgrep -i hack
> > +       if (tmp == '$') break; /* PHP hack */ \
> > +       while (dptr < end) /* PHP hack */
> >
> > I vaguely recall a PHP developer (Pierre?) mentioning this at the time,
> > and I guess I didn't object strongly enough - perhaps because the hack
> > itself was in there before.
> 
> I cannot remember why I added this comment there but as far as I can
> tell now this code was present before, actually since the very first
> version I use when I first make blowfish always available for PHP
> using your implementation. See:
> 
> https://github.com/php/php-src/commit/1e820eca02dcf322b41fd2fe4ed2a6b8309f8ab5#diff-243b169f3d5e753b5314e8eb994e36bc
> 
> I cannot say why it is specific to PHP. Is it something that may have
> been present in your implementation earlier and then removed but kept
> in PHP for some BC reasons?

No.  As far as I'm aware, this always was a PHP-only hack.

IIRC, there might have been a wrong example with too-short bcrypt salt
on the PHP crypt() function documentation page back then (2008?), and
maybe the code was adapted to match that.  As to how that example worked
before, I suspect it might have worked through exploiting *BSD
implementations' lack of strict checking on salt decoding, when PHP was
running on such systems.  If so, the padding wasn't necessarily zeroes -
it was uninitialized stack data - issue fixed in OpenBSD 2 years ago,
rejecting such invalid salts:

http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libc/crypt/bcrypt.c.diff?r1=1.37&r2=1.38

-	decode_base64(csalt, BCRYPT_MAXSALT, salt);
+	if (decode_base64(csalt, BCRYPT_MAXSALT, salt))
+		return -1;

However, PHP isn't actually better in that respect, with its '.' vs. '$'
padding discrepancy across versions/builds.  I still don't know where
exactly this discrepancy comes from.

> > Well, this PHP-specific behavior confuses people in several ways.  It's
> > especially weird that the behavior may be seen or may be gone on the
> > same PHP version depending on whether it uses PHP's bundled (and hacked
> > as above) copy of crypt_blowfish or an implementation of bcrypt provided
> > by the underlying system.  Maybe PHP should not use the system's crypt()
> > for whatever hash types it has bundled code for - and not only because
> > of this issue.

> > In practical terms, padding with dots (zero bits) is an unpleasant
> > surprise for people who choose to store their invalid salts and hashes
> > into separate database fields (and then authentication stops working
> > after some upgrade or migration, where the new system provides its
> > non-hacked bcrypt), and padding with '$' signs is also a similar
> > unpleasant surprise for people who store the full hash strings as
> > returned by crypt().  While those strings might work in the same way
> > with some bcrypt implementations, they may also be rejected (ideally,
> > like upstream crypt_blowfish does) or processed differently.
> >
> > I'm not sure whether/how PHP should recover from this now.

Alexander