Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:62960
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain macvicar.net from 209.85.160.42 cause and error)
Content-Type: text/plain; charset=iso-8859-1
Mime-Version: 1.0 (Mac OS X Mail 6.0 \(1486\))
In-Reply-To: <CAAyV7nG7kJrWeAtphNManq9+ZnDuEchniN-PqOiRvRjx6zqkPw@mail.gmail.com>
Date: Wed, 12 Sep 2012 09:57:00 -0700
Cc: internals@lists.php.net
Content-Transfer-Encoding: quoted-printable
Message-ID: <E85A63F7-42A4-4F9A-BFE8-0321ACFD4104@macvicar.net>
References: <CAAyV7nHZ6Kr7yKF0B-UgauN6eKKxRsqCztP9w9hUzOdiPLHYAw@mail.gmail.com> <CADNQb0USW3j7BWEjvDwOdCKoLy3xVJ8eCRjZ610ekKmTL7R9VQ@mail.gmail.com> <CAAyV7nGA=1pWLj1G+X1EvoNB0jb4ji6oJ2tGZNwXH=CCae_9wA@mail.gmail.com> <CADNQb0XRrh21QX1qmqvHpvONRSOARXQpC8XroCFiw2z6F2Px5Q@mail.gmail.com> <CAAyV7nGSfZPbt23+0Yy2HGdc-gh2=Hf83=y2_e6V=pXi9Tg2XA@mail.gmail.com> <CAAyV7nFe0LUE4tObPaz3Mp1M657tmjxvNJo_fG-PeNSCJW01pw@mail.gmail.com> <CAAyV7nG7kJrWeAtphNManq9+ZnDuEchniN-PqOiRvRjx6zqkPw@mail.gmail.com>
To: Anthony Ferrara <ircmaxell@gmail.com>
Subject: Re: [PHP-DEV] [VOTE] Add simplified password hashing API
From: scott@macvicar.net (Scott MacVicar)

Concerns about the RFC after talking with someone (Alok) on our security =
team at work.

"There is no requirement for them to be cryptographically secure. "
What stops the salt from being cryptographically secure? I think it =
should be a goal or we should state what parts aren't cryptographically =
secure, is it the random data source?

"The salt parameter, if provided, will be used in place of an =
auto-generated salt."
This is setting someone up for failure by letting them put in something =
weak, you should be forced to get an auto-generated salt. If this is for =
unit testing then it should be explicitly stated.

The documentation also needs improved around password_needs_rehash. Tell =
the developer that at login time (or any other password validation =
time), if the options have changed, the password can be rehashed.

E_WARNING in a crypto function is also bad. Throw an exception or fatal. =
There's no reason to just raise a warning and continue execution, if =
something went wrong in crypto then its a bad time for everyone.

- S

On 12 Sep 2012, at 09:02, Anthony Ferrara <ircmaxell@gmail.com> wrote:

> All,
>=20
> I have added the tests and ensured that everything seems pretty clean. =
I
> have opened a Pull Request for this item as I would like to get more =
eyes
> on it (especially since it touches crypt()). Please review the PR and
> comment away.
>=20
> https://github.com/php/php-src/pull/191/files
>=20
> Once it looks good, I'll merge it in. I just wanted to get more eyes =
on the
> diff first...
>=20
> Thanks
>=20
> Anthony
>=20
> On Wed, Sep 12, 2012 at 10:02 AM, Anthony Ferrara =
<ircmaxell@gmail.com>wrote:
>=20
>> Hello all,
>>=20
>> I've closed the vote and it's been accepted with a vote total of =
19:0,
>> unanimous. I've moved the RFC into Accepted.
>>=20
>> I'm going to add the remaining tests, and then move it into master =
later.
>>=20
>> As for the PECL extension route, I'll work on splitting it into a =
PECl
>> extension for 5.3/5.4 at the same time.
>>=20
>> Thanks all,
>>=20
>> Anthony
>>=20
>> On Tue, Sep 11, 2012 at 7:58 AM, Anthony Ferrara =
<ircmaxell@gmail.com>wrote:
>>=20
>>> Hannes,
>>>=20
>>>> First off, this has been discussed on the list for literally =
months.
>>>> Why
>>>>> wait until the day before voting can end before bringing this up?
>>>>=20
>>>> So commenting is strictly forbidden during votes?
>>>=20
>>>=20
>>> Not in the least. Just pointing out that this discussion could have =
been
>>> better if it was started much earlier (prior to the time and effort =
being
>>> put into it). Just pointing it out. There's nothing "wrong" =
(procedurally)
>>> about bringing it up now, just that it would have been nice to hear =
this
>>> point earlier in the discussion...
>>>=20
>>>=20
>>>>> Secondly, the main reason for not developing this as an extension =
is
>>>> that
>>>>> there's really no benefit to it. There are little to no =
performance
>>>> gains to
>>>>> be had by the C implementation. It can live quite as easily as a =
PHP
>>>>> library.
>>>>=20
>>>> The benefit is that it can be tested properly and bugs discovered =
and
>>>> ironed out first.
>>>>=20
>>>=20
>>> I'm not so sure about that. If it was widely adopted as an =
extension,
>>> sure. But I would predict a very limited adoption. Almost purely =
academic.
>>> The reason is that there's no advantage to doing it in C other than =
to
>>> provide a working API for those who don't understand how to build it =
(or
>>> realize the benefits thereto). Those are the same people who =
wouldn't be
>>> able to install a PECL extension (know-how or access). And projects =
who
>>> realize this is needed are likely already using one of the many =
libraries
>>> available.
>>>=20
>>> So I'm not sure the install rate would be anywhere high enough to =
work
>>> any significant issues out. Additionally, there are testing phases =
for the
>>> release that would hopefully catch any major issues prior to 5.5 =
going
>>> gold...
>>>=20
>>>=20
>>>> This is not the sort of thing you want to get security bug reports =
the
>>>> day after its released in core.
>>>>=20
>>>=20
>>> Sure. Which is why I've been going around looking for security =
reviews
>>> (posted to the crypt-dev list on openwall, and to
>>> security.stackexchange.com).
>>>=20
>>>=20
>>>> If your ego is big enough you can guarantee you have tested this
>>>> thoroughly and want it to become the recommended way.. You have to =
be
>>>> damn sure you don't fuck it up.
>>>>=20
>>>=20
>>> I want to say that this is really unfair. By saying it this way =
you've
>>> pushed me into a corner. I disagree that your suggestion would have =
a
>>> result of any significant gain in security. Yet I cannot disagree =
with you
>>> without coming across as an egotist now that you said it that way. =
Please
>>> try to keep things civil without setting up traps to try to prove =
your
>>> point. I don't appreciate this remark at all...
>>>=20
>>>=20
>>>> This is exactly the sort of thing that doesn't need to be developed =
in
>>>> the core tree, but can later be merged in once proven successful.
>>>>=20
>>>=20
>>> As indicated before, if that's the case, then it would never be =
merged.
>>> Because this would never be successful as a stand alone PECL module. =
The
>>> primary reason for adding it is so that people who don't know any =
better
>>> would be able to use a secure API. That is the antithesis of a PECL
>>> extension.
>>>=20
>>>=20
>>>>>> Especially considering the patch is unfinished.
>>>>>=20
>>>>>=20
>>>>> Aside from adding a few more tests, what's unfinished? If you're
>>>> referring
>>>>> to the line in the RFC, I just haven't updated it. The patch has =
been
>>>> worked
>>>>> on and is in a place where I'd be comfortable submitting it...
>>>>=20
>>>> The test suite seems very limited, and the code seems to be waiting
>>>> for more algorithms to be implemented.
>>>>=20
>>>=20
>>> Yes, the test suite needs to be expanded. I tested it additionally =
with
>>> the PHPUnit tests that I wrote for the PHP fallback, so it's been =
tested
>>> fairly well... I just need to expand the phpt test suite included in =
core...
>>>=20
>>> As far as more algorithms, right now there's no reason to add more
>>> algorithms. The whole point was to never add anything but the =
strongest
>>> algorithm available. Which today is bcrypt. But as time goes on, and =
better
>>> algorithms become available, the API is designed so that they can be
>>> implemented. But today, there's no other algorithm that should be
>>> implemented. (SCrypt is a candidate, but since it lacks crypt(3) =
bindings,
>>> and is still very young, it's inclusion isn't prudent at this =
point).
>>>=20
>>> There's a section to the RFC for adding new algorithms as time goes =
on:
>>> https://wiki.php.net/rfc/password_hash#updating_password_default
>>>=20
>>> Anthony
>>>=20
>>=20
>>=20