Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:80308
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.215.46 as permitted sender)
MIME-Version: 1.0
Date: Fri, 9 Jan 2015 11:45:04 -0500
Message-ID: <CAAyV7nHCi3hhmQ3h9AgKj4OOSx+DCRy7ZPGUA8+=aUG5dB782A@mail.gmail.com>
To: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Crypt Warnings (was PHP 5.5.21RC1 is ready for testing)
From: ircmaxell@gmail.com (Anthony Ferrara)

All,

As identified in the previous thread, there is an issue when providing
invalid salts to crypt() where it will silently fall back to STD_DES.
This was evidenced by a fix to crypt() when upgrading blowfish to 1.3.
Causing a failing test for Horde_Auth:

$ php -r 'var_dump(crypt("foobar", "*0OayF9ttbxIs"));'
string(13) "*0OayF9ttbxIs"

With 5.4.36 / 5.5.21RC1 (with)

$ php55 -r 'var_dump(crypt("foobar", "*0OayF9ttbxIs"));'
string(2) "*1"

The new behavior is absolutely correct due to *0 and *1 being error
conditions, so any salt with *0 should immediately fail with *1 (to
prevent one equaling the other). Previously it was used as a salt for
STD_DES.

This raises another issue though. Currently, an invalid bcrypt salt
such as $2$10$... would actually result in a DES hash:

string(13) "$2rcByx51ejoM"

Which is completely incorrect.

This not only hides errors, but it falls back to an absurdly insecure
format while hiding them. It's worth noting that this case will cause
HHVM to fail with *0.

Changing this fallback behavior to the correct error should happen.
However, this will likely break a number of live systems which are
currently relying on the incorrect behavior (likely without knowing
it).

It's worth nothing that failing is the currently documented behavior:
http://php.net/crypt

Therefore, I'm suggesting we add an E_DEPRECATED error when we detect
an invalid STD_DES salt but still execute the fallback:
https://github.com/php/php-src/pull/989

Then in a future version (7.1, 8, whatever) remove the fallback and
keep the error along with returning a failure indication (*0).

I'm open to tweaking the error message, and possibly changing to
E_WARNING if people think it's worth it.

Thoughts?

Anthony