[VOTE] Add simplified password hashing API

php.internals

Anthony Ferrara

13 years ago
Hello all, I'm opening the vote for the simplified password hashing API indicated here: https://wiki.php.net/rfc/password_hash Please vote, Thanks, Anthony

Hannes Magnusson

13 years ago
On Tue, Sep 4, 2012 at 3:16 PM, Anthony Ferrara <ircmaxell@gmail.com> wrote:
> Hello all, > > I'm opening the vote for the simplified password hashing API indicated here: > > https://wiki.php.net/rfc/password_hash >
I like the idea, but I don't understand why this isn't developed as an extension first and then brought into core when it has proven to work and actually simplify things for the user? Especially considering the patch is unfinished. -Hannes

Anthony Ferrara

13 years ago
Hannes, On Sun, Sep 9, 2012 at 12:23 PM, Hannes Magnusson < hannes.magnusson@gmail.com> wrote:
> On Tue, Sep 4, 2012 at 3:16 PM, Anthony Ferrara <ircmaxell@gmail.com> > wrote: > > Hello all, > > > > I'm opening the vote for the simplified password hashing API indicated > here: > > > > https://wiki.php.net/rfc/password_hash > > > > > I like the idea, but I don't understand why this isn't developed as an > extension first and then brought into core when it has proven to work > and actually simplify things for the user? >
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? 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 main reason for putting it in core is so that it's available to everyone, including people who have no idea to use a library. By putting notes in the hash, md5, sha1 and crypt documentation pages pointing to this alternative, hopefully it will make it far easier for novice and people who don't know any better to securely hash passwords. If you know enough to understand this problem, you're likely solving it already. But as recent attacks show, even experienced developers don't understand the problem. So by putting it in core, we're making a point and making it trivially easy to do it right. So trivial that it's actually just as hard (if not harder) to do it wrong. To that effect, the only way it can be done is to do it in core... 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... Thanks, Anthony

Hannes Magnusson

13 years ago
On Mon, Sep 10, 2012 at 3:31 PM, Anthony Ferrara <ircmaxell@gmail.com> wrote:
> Hannes, > > On Sun, Sep 9, 2012 at 12:23 PM, Hannes Magnusson > <hannes.magnusson@gmail.com> wrote: >> >> On Tue, Sep 4, 2012 at 3:16 PM, Anthony Ferrara <ircmaxell@gmail.com> >> wrote: >> > Hello all, >> > >> > I'm opening the vote for the simplified password hashing API indicated >> > here: >> > >> > https://wiki.php.net/rfc/password_hash >> > >> >> >> I like the idea, but I don't understand why this isn't developed as an >> extension first and then brought into core when it has proven to work >> and actually simplify things for the user? > > > 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?
> 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. This is not the sort of thing you want to get security bug reports the day after its released in core. 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. 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. Like I said, I really like the idea, just don't see why it isn't tested out as an pecl extension first.
>> 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. -Hannes

Stas Malyshev

13 years ago
Hi!
> The benefit is that it can be tested properly and bugs discovered and > ironed out first. > This is not the sort of thing you want to get security bug reports the > day after its released in core. > 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. > > 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.
I see you point, and indeed, if we recommend something as a one-stop shop for security-sensitive area, we better be sure we do it right. But: are we really going to get enough testing if we put it out as PECL module? Also, we're not releasing 5.5 tomorrow, so we can have a lot of testing before. But if it's a non-core module, adoption would be much lower since apps could not rely on it being there even in 5.5. OTOH, PECL module that can be built in 5.3/5.4 too might be nice. Not everybody is going to upgrade to 5.5 soon, so having them participate would be good too. Maybe we could do it as a module and have it workable as PECL too for those who are not on 5.5? PHP solution is not really the same - if we have two separate codebases, nobody can be sure they actually do the same thing.
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227

Pierre Joye

13 years ago
hi Stas, On Tue, Sep 11, 2012 at 12:23 AM, Stas Malyshev <smalyshev@sugarcrm.com> wrote:
> OTOH, PECL module that can be built in 5.3/5.4 too might be nice. Not > everybody is going to upgrade to 5.5 soon, so having them participate > would be good too. Maybe we could do it as a module and have it workable > as PECL too for those who are not on 5.5? PHP solution is not really the > same - if we have two separate codebases, nobody can be sure they > actually do the same thing.
Yes, that's actually what I wanted to ask as well for this ext. But I'm not sure it is easy as it relies on PHP APIs which were no exposed in 5.3 nor 5.4. Maybe we could expose them in the next releases (to check which). However maintaining both core and pecl can be sometimes time consuming (hours matter, not days :), but it is definitively a great way to provide updates more frequently or to provide more tests releases (beta). Anthony, is it something you would consider? It could also help to speed up the adoption. Cheers,
-- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org

Lester Caine

13 years ago
Pierre Joye wrote:
> On Tue, Sep 11, 2012 at 12:23 AM, Stas Malyshev<smalyshev@sugarcrm.com> wrote: >> >OTOH, PECL module that can be built in 5.3/5.4 too might be nice. Not >> >everybody is going to upgrade to 5.5 soon, so having them participate >> >would be good too. Maybe we could do it as a module and have it workable >> >as PECL too for those who are not on 5.5? PHP solution is not really the >> >same - if we have two separate codebases, nobody can be sure they >> >actually do the same thing. > Yes, that's actually what I wanted to ask as well for this ext. But > I'm not sure it is easy as it relies on PHP APIs which were no exposed > in 5.3 nor 5.4. Maybe we could expose them in the next releases (to > check which). > > However maintaining both core and pecl can be sometimes time consuming > (hours matter, not days:), but it is definitively a great way to > provide updates more frequently or to provide more tests releases > (beta).
I seem to recall asking in the past asking for a bit more of a 'modular' approach as we have in other areas of the system. Why shouldn't things be tidied up so that single modules can be rebuilt and new ideas distributed without affecting the rest. The only thing that really makes this difficult is that the 'distributed' core is all bundled in one repo and pecl in another. Some more flexible way of working with all code would help reduce the time taken.
> Anthony, is it something you would consider? It could also help to > speed up the adoption.
And logging some of the actions required to produce a 'non-core' module that will work with a stock distribution would be helpful.
-- Lester Caine - G8HFL ----------------------------- Contact - http://lsces.co.uk/wiki/?page=contact L.S.Caine Electronic Services - http://lsces.co.uk EnquirySolve - http://enquirysolve.com/ Model Engineers Digital Workshop - http://medw.co.uk Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

Anthony Ferrara

13 years ago
Pierre, On Tue, Sep 11, 2012 at 2:14 AM, Pierre Joye <pierre.php@gmail.com> wrote:
> hi Stas, > > On Tue, Sep 11, 2012 at 12:23 AM, Stas Malyshev <smalyshev@sugarcrm.com> > wrote: > > OTOH, PECL module that can be built in 5.3/5.4 too might be nice. Not > > everybody is going to upgrade to 5.5 soon, so having them participate > > would be good too. Maybe we could do it as a module and have it workable > > as PECL too for those who are not on 5.5? PHP solution is not really the > > same - if we have two separate codebases, nobody can be sure they > > actually do the same thing. > > Yes, that's actually what I wanted to ask as well for this ext. But > I'm not sure it is easy as it relies on PHP APIs which were no exposed > in 5.3 nor 5.4. Maybe we could expose them in the next releases (to > check which). >
That is correct. It involved refactoring crypt() internally to expose an internal API for php_crypt: https://github.com/ircmaxell/php-src/blob/hash_password/ext/standard/crypt.c#L148 It's not the end of the world, because we can copy/paste that function into the PECL extension, and just conditionally include it. There would be duplication between the two, but it wouldn't be too bad... However maintaining both core and pecl can be sometimes time consuming
> (hours matter, not days :), but it is definitively a great way to > provide updates more frequently or to provide more tests releases > (beta). > > Anthony, is it something you would consider? It could also help to > speed up the adoption. >
Consider? Sure. But I'm not sure it's worth while. Stas brought up the adoption point, which I think is the kicker. The target audience for this API is not the type that usually has access to install PECL extensions. So while some may use it as an extension, the majority who would significantly benefit from it wouldn't be able to. So the benefit to releasing it as a PECL extension would basically defeat the point... And to Stas's point about the PHP solution not being the same, I fail to understand why. It's built using identical algorithms (translated from C to PHP as best as possible). It's tested using the same tests with the only difference surrounding error messages. Plus it's portable (can be used on shared hosts). The only real difference is testing the PHP version doesn't say anything to the *security* of the C version. But it does test the API and the concept... Anthony

Pierre Joye

13 years ago
hi, On Tue, Sep 11, 2012 at 1:41 PM, Anthony Ferrara <ircmaxell@gmail.com> wrote:
> It's not the end of the world, because we can copy/paste that function into > the PECL extension, and just conditionally include it. There would be > duplication between the two, but it wouldn't be too bad...
agreed.
> Consider? Sure. But I'm not sure it's worth while. Stas brought up the > adoption point, which I think is the kicker. The target audience for this > API is not the type that usually has access to install PECL extensions. So > while some may use it as an extension, the majority who would significantly > benefit from it wouldn't be able to. So the benefit to releasing it as a > PECL extension would basically defeat the point... > > And to Stas's point about the PHP solution not being the same, I fail to > understand why. It's built using identical algorithms (translated from C to > PHP as best as possible). It's tested using the same tests with the only > difference surrounding error messages. Plus it's portable (can be used on > shared hosts). The only real difference is testing the PHP version doesn't > say anything to the *security* of the C version. But it does test the API > and the concept...
distros will provide it and that's the kicker. Believe, in the beginning of the new ext/zip, having it pecl drastically improve the adoption through distros. Less now as we have frequent PHP releases (also why I do much more less pecl releases ;). Cheers,
-- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org

Anthony Ferrara

13 years ago
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

Anthony Ferrara

13 years ago
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:

Anthony Ferrara

13 years ago
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:

Scott MacVicar

13 years ago
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:

Adam Jon Richardson

13 years ago
On Wed, Sep 12, 2012 at 12:57 PM, Scott MacVicar <scott@macvicar.net> wrote:
> "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?
A salt (similar to a nonce), only needs to be unique within the system (see "Practical Cryptography" by Ferguson and Schneier)
> "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.
Again, the salt is only weak IFF it is not unique within that particular system (app, website, etc.) Making the easier option be the one that ensures the uniqueness seems reasonable here, as most developers will use the provided functionality, whilst maintaining the developers more comfortable with the security requirements involved with customization to do so with some modest extra work. Adam

Anthony Ferrara

13 years ago
Scott, On Wed, Sep 12, 2012 at 12:57 PM, Scott MacVicar <scott@macvicar.net> wrote:
> 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? >
What stops it? Nothing. You can use a CS random string for a salt. That's fine. The point was that salts don't need to be CS to work. There's no requirement that they be CS (which is what I said). In fact, the only requirement is that they be unique. Not purely globally unique, but unique to the point that there's a very insignificant chance that anyone else is using the same settings with that same salt. Even if it's on a different application... Now, given that the only good CS source on most machines is /dev/random, and that it is blocking if it runs out of entropy, there's good reason to not use them unless you need them. So my approach is to only use CS randomness when I need it (key generation mainly). Cases when the quality of randomness significantly impacts the security. So the line is in there because the generated random salt is generated by /dev/urandom. I just wanted to make that distinction clear.
> "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. >
It's there because some use-cases may dictate using a CS random salt. In those cases, you can read from /dev/random (or use mcrypt, etc) to generate the salt. The vast majority should not generate that salt parameter though. And that will be reflected in the documentation.
> 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. >
Absolutely. That's why that function exists. I did not try to make the RFC into end-user documentation. In fact, I explicitly tried to avoid doing that. I wanted the RFC to be the technical justification and explanation. The end-user documentation will be made later, and it will include information like that (including examples)...
> 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.
I partially agree. I think a FATAL is a bit much (especially if it's a recoverable error, such as an invalid param). And exceptions are out (not allowed in core functions). I think the warning is the best approach, since it will always return a value conducive with the error (NULL or FALSE, depending on the function). If it returns a value, it's because no warning was thrown... Anthony

Stas Malyshev

13 years ago
Hi!
> "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.
This assumes that we have the best salt generation algorithm that ever existed or could exist, and never would have any bugs or vulnerabilities in it. If it's not the case, it makes sense to let people provide their own salts in case they can generate better ones.
> 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.
This means the function will be behaving differently than other PHP functions. Because it's crypto and very important, I understand. But then comes somebody else and says "my file functions are super-important too, whole application depends on them, why can't I have them work the same as crypto ones?", etc. Core should be consistent in its responses. Also, since the function returns false on failure, you can easily know something went wrong and throw exception/produce fatal error there if you wanted. Just like other functions - e.g. fopen() - do.
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227