Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:91976
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.218.53 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <20160327151603.GA31135@openwall.com>
References: <20160327151603.GA31135@openwall.com>
Date: Mon, 28 Mar 2016 14:22:13 +0700
Message-ID: <CAEZPtU6=cUx4q2p+VOwNZv6=vYTOwUqTN-+T+NOEZshW6294kQ@mail.gmail.com>
To: Solar Designer <solar@openwall.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] crypt_blowfish salt padding
From: pierre.php@gmail.com (Pierre Joye)

Hi Alexander,

On Sun, Mar 27, 2016 at 10:16 PM, Solar Designer <solar@openwall.com> wrote:
> Hi,
>
> This commit:
>
> commit 03315d9625dc87515f1dfbf1cc7d53c4451b5ec9
> Author: Pierre Joye <pajoye@php.net>
> Date:   Mon Jul 18 21:26:29 2011 +0000
>
>     - update blowfish to 1.2 (Solar Designer)
>
> documented this hack:
>
> $ 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?


> 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.
>
> As to the hack above, I didn't review it closely (I disapprove of it
> anyhow), but maybe it doesn't even work reliably when the code is
> compiled in: some people are reporting that salts are padded with dots
> (which correspond to all zero bits) and some that salts are padded with
> '$' signs.  I've just reproduced the latter on CentOS 7.1:
>
> $ php -r 'echo crypt("", "\$2a\$05\$0123456789"), "\n";'
> $2a$05$0123456789$$$$$$$$$$$.UQ9ZZoltOie8zhLfF5KWLrJBeoos.S6
> $ php -v
> PHP 5.4.16 (cli) (built: Jun 23 2015 21:17:27)
>
> Maybe there's an extra hack somewhere causing this, but I can't find it
> easily.  I'm not looking at the exact branch/package for CentOS, though.
> Maybe it was a temporary PHP 5.4 thing.
>
> While padding with dots (zero bits) produces valid bcrypt hash strings
> (it's just that the input salt strings were invalid), padding with '$'
> signs produces invalid bcrypt hash strings.
>
> 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.



-- 
Pierre

@pierrejoye | http://www.libgd.org