Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:75674
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.220.177 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CADajX4C9sip=57QGaacZVq9cmAFhH6CJWpvSSjB7c921TGsJMQ@mail.gmail.com>
References: <CAGa2bXYP4L-EVjqiTvrOjbiRvzQJ7zyTQyu78O1AEEkk7nwEhQ@mail.gmail.com>
	<CAESVnVqBxo0VCYM_4V4_KzrD1+-F-0ipumrr_a=mAwaYje10Og@mail.gmail.com>
	<CAGa2bXa48gEEcE_DWE27B3UHD0Yi12bFVZsx-9t_anHunkj5Dg@mail.gmail.com>
	<CAHMUw2FLW06ABT=1RoH2gMuM7HbGDdSnLzxqfv8wNVp0rzfw+Q@mail.gmail.com>
	<CAGa2bXZF5sznggoYRFTtngO32b60NVG=6KqUBDB3guGb7_8oMA@mail.gmail.com>
	<CAHMUw2GyQOevKfZXuyd7S-PPUnShVVeoRvZNHK3q9w1yEe1wow@mail.gmail.com>
	<CADajX4C9sip=57QGaacZVq9cmAFhH6CJWpvSSjB7c921TGsJMQ@mail.gmail.com>
Date: Thu, 17 Jul 2014 15:38:23 -0400
Message-ID: <CAAyV7nFva4u-pW0Ago-VpkJ=-RuBvE_fjCg27Wza8_T6M0oYHA@mail.gmail.com>
To: Adam Harvey <aharvey@php.net>
Cc: Tjerk Meesters <tjerk.meesters@gmail.com>, Yasuo Ohgaki <yohgaki@ohgaki.net>, 
	Sara Golemon <pollita@php.net>, "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] crypt() BC issue
From: ircmaxell@gmail.com (Anthony Ferrara)

All,

Look at the issue, there's a line in there that is the crux of the issue:

> So problem isn't only in ROUNDS_MIN.

In fact, the overall algorithm changed.

Look at a quick example: http://3v4l.org/Eov3o

From 5.3.2+ (when we pulled in our own implementation of crypt-sha256
and crypt-sha512, and crypt-blowfish):

using salt:
string(31) "$6$rounds=3D1000$abcdefghijklmnop"
gives hash:
string(118) "$6$rounds=3D1000$abcdefghijklmnop$SgL/Atu7DClkX0qBUuG6FS2bRf2X=
mLFWY9b8pRttPEj9ZSh4MKE5bKlz4WAKomLuWI.YQ5oIPLO2L.0OioAeW/"

great.
But in 4.3.0 - 5.2.17:
the same salt gives:
string(99) "$6$rounds=3D10$EdPD9pXG2vAIeeyG2E/9Mzey2mA/qZgIBAbccXWtrNiymhFY=
XNK4r2gphMi4KOksXRubHnBWGVh/p2pP2D3R.."

Which is shorter. Also note that the rounds went from the defined 1000, to =
10.

Things get far weirder: http://3v4l.org/KZoIv

$h1=3D'$6$rounds=3D1$abcdefghijklmnop';
$h2=3D'$6$abcdefghijklmnop';
gives us (on 4.3.0 - 5.2.17):
string(102) "$6$rounds=3D1000$$6TFP.7u1vZP5A9fccvmUPteI8f29BLhgfqL1XEQqcrvq=
TSKKw5SBa2qw1sOwLEQ41Dhl1u/Jbi2hRHRYCdxuv0"
string(99) "$6$abcdefghi$4Dv/xQG4euniWsyrRHGUrNqM9CFl5Pg9EI88OgO4tIzuV952Jn=
T09wVxrzUP9l/dr0wP3YSs1Ufz1qJpifLAA0"

Note that these are different all around from the other results from
the same version range.

Basically, the version of crypt being linked to is either buggy (not
likely) or our documentation of it is (likely). And our implementation
(since 5.3.2) conforms to our documentation instead of the old
behavior.

Considering 5.3.2 was released 4 years ago, and 5.2 has been EOL for
3.5 years, I don't think there's anything that can be done about this.
If it was a simple limit that needed changing, a polyfill would be
easy (took me 10 minutes:
https://gist.github.com/ircmaxell/ac0710ce044504f65cf0 ). But seeing
the entire algorithm is different, much harder.

I think this should be documented, and then called a day. I don't see
anything else that could reasonably be done (changing crypt() at this
point is out of the question IMHO).

>  - Add bool crypt_migration INI that is default to OFF for PHP 5.4 and up=
.
>  - master will not have this INI.

Please don't. As I showed above, it's already long past that point.
And we don't need new ini settings.

And what that ini setting would control is not the minimum iteration
flag (as that's not the only problem), but which version of libcrypt
we're binding to (our internal one, or an external one). Which means
that the overall behavior *will* change. And unless you do it
perfectly, it could effect all of crypts algorithms (potentially
impacting password_hash, etc).

We internalized the algorithms in 5.3.2 at least partially because the
system provided libraries were inconsistent at best (hence why many
but not all 5.2 systems don't support bcrypt, it depended on the build
of libcrypt you linked against).

Please don't make us re-live this inconsistency... Especially when it
won't really solve the problem.






Regarding password_verify() accepting crypt(), I consider it an
implementation detail that it works. I know the RFC specifies it, but
it specifies it not as a conceptual fact (that it will always be no
more than a reason to be there), but more as an explanation for what
it's currently doing. I would not rely on that fact. It may work
today. It may work tomorrow, but it shouldn't be documented as such
(as it's the complement of password_hash(), not the complement of
crypt()).

> As far as I'm aware, the only reason for not marking crypt()
> E_DEPRECATED right now is for compatibility with external systems, and
> as far as those go, changing a default won't effect anything.

I want to reinforce that point, because it's spot on the money:

I think crypt should live on. password_hash should be the way new
systems are built, sure. But as you mention external systems, crypt
should be a standard way of interacting with them (heck, that's what
the lib was designed for). It shouldn't be a "if you're not using
password_hash(), you're doing it wrong". It's "password_hash() should
solve the majority of use-cases, but if you have a different need,
there are other options".

My $0.02

Anthony

On Thu, Jul 17, 2014 at 1:50 PM, Adam Harvey <aharvey@php.net> wrote:
> On 16 July 2014 23:16, Tjerk Meesters <tjerk.meesters@gmail.com> wrote:
>> On Thu, Jul 17, 2014 at 10:25 AM, Yasuo Ohgaki <yohgaki@ohgaki.net> wrot=
e:
>>
>>> Hi Tjerk,
>>>
>>> On Thu, Jul 17, 2014 at 11:09 AM, Tjerk Meesters <tjerk.meesters@gmail.=
com
>>> > wrote:
>>>
>>>> Why should `password_verify()` work on a hash that wasn't generated wi=
th
>>>> `password_hash()`? The fact that it uses `crypt()` internally should n=
ot
>>>> leak outside of its API, imho.
>>>
>>>
>>> password_*() is designed as crypt() wrapper and this fact is documented
>>> since it was released.
>>>
>>> Obsolete password hash is easy to verify with password_needs_rehash().
>>> Developers can check password database easily with password_needs_rehas=
h().
>>>
>>
>> The documentation states that the `hash` argument to both
>> `password_needs_rehash()` and `password_verify()` is:
>>
>>     hash - A hash created by password_hash().
>
> Whoa. Don't put too much stock in that =E2=80=94 I think I made that up o=
ut of
> whole cloth while trying to get _something_ into the manual for one of
> the early alpha releases of 5.5, and it wasn't intended as a
> restrictive statement.
>
>> Passing a value from your own crypt() implementation may work, but that
>> shouldn't be relied upon. I certainly wouldn't classify it as a problem
>> that should be fixed in the password api.
>
> The original RFC specified that both crypt() and password_hash()
> hashes were accepted here, and that's probably what the documentation
> should say too.
>
> Adam
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>