Re: [VOTE] Add simplified password hashing API

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, > > 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. > > https://github.com/php/php-src/pull/191/files > > Once it looks good, I'll merge it in. I just wanted to get more eyes on the > diff first... > > Thanks > > Anthony > > On Wed, Sep 12, 2012 at 10:02 AM, Anthony Ferrara <ircmaxell@gmail.com>wrote: > >> Hello all, >> >> 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. >> >> I'm going to add the remaining tests, and then move it into master later. >> >> 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. >> >> Thanks all, >> >> Anthony >> >> On Tue, Sep 11, 2012 at 7:58 AM, Anthony Ferrara <ircmaxell@gmail.com>wrote: >> >>> Hannes, >>> >>>> 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? >>>> >>>> So commenting is strictly forbidden during votes? >>> >>> >>> 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... >>> >>> >>>>> 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. >>>> >>>> The benefit is that it can be tested properly and bugs discovered and >>>> ironed out first. >>>> >>> >>> 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. >>> >>> 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... >>> >>> >>>> This is not the sort of thing you want to get security bug reports the >>>> day after its released in core. >>>> >>> >>> 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). >>> >>> >>>> 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. >>>> >>> >>> 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... >>> >>> >>>> 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. >>>> >>> >>> 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. >>> >>> >>>>>> Especially considering the patch is unfinished. >>>>> >>>>> >>>>> 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... >>>> >>>> The test suite seems very limited, and the code seems to be waiting >>>> for more algorithms to be implemented. >>>> >>> >>> 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... >>> >>> 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). >>> >>> 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 >>> >>> Anthony >>> >> >>