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
Hello all,
I'm opening the vote for the simplified password hashing API indicated here:
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
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: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,
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: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
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
hi Stas,
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
Pierre Joye 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
Pierre,
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
hi,
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
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
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.comwrote:
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_defaultAnthony
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.comwrote:
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.comwrote:
Hannes,
First off, this has been discussed on the list for literally months.
Whywait 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_defaultAnthony
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
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 touchescrypt()). 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.comwrote:
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.comwrote:
Hannes,
First off, this has been discussed on the list for literally months.
Whywait 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_defaultAnthony
"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
Scott,
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_WARNINGin 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
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_WARNINGin 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