Hi all,
crypt() has BC issue with older systems.
https://bugs.php.net/bug.php?id=62372&edit=1
The reason rounds became 1000 from 10 is hardcoded lower limit for newer
PHPs.
Generally speaking, developer should never use less than 1000 rounds and
better to have
at least few thousands rounds or more, tens of thousands or more is
recommended.
I would like to make this bug report 'wont fix', since migration is
possible.
- Developer may use larger rounds and store updated hash when
user is authenticated with old PHP. - Developer may ask users to reset password if password hash has
to fewer rounds than 1000 (i.e. outdated hash) with new PHP.
Any comments?
--
Yasuo Ohgaki
yohgaki@ohgaki.net
- Developer may use larger rounds and store updated hash when
user is authenticated with old PHP.- Developer may ask users to reset password if password hash has
to fewer rounds than 1000 (i.e. outdated hash) with new PHP.
Wait, doesn’t that mean you’re unable to verify passwords now?
Andrea Faulds
http://ajf.me/
Hi Andrea,
- Developer may use larger rounds and store updated hash when
user is authenticated with old PHP.- Developer may ask users to reset password if password hash has
to fewer rounds than 1000 (i.e. outdated hash) with new PHP.Wait, doesn’t that mean you’re unable to verify passwords now?
It means old PHP users may need preparation for their apps to
migrate newer PHP.
If developer upgrades PHP blindly, they may see a lots of failed logins.
This change was done while ago, so it would not worth the effort to relax
the requirement now. IMHO.
We may add optional flag to relax the limitation, though.
I don't mind modifying crypt() or adding migration INI setting.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
This change was done while ago, so it would not worth the effort to relax
the requirement now. IMHO.We may add optional flag to relax the limitation, though.
I don't mind modifyingcrypt()or adding migration INI setting.
Yeah, that’s my thoughts as well. We changed it for a reason, but to avoid breaking things, perhaps a migratory INI setting (which we’d eventually remove) is for the best.
Andrea Faulds
http://ajf.me/
Hi Andrea,
This change was done while ago, so it would not worth the effort to relax
the requirement now. IMHO.We may add optional flag to relax the limitation, though.
I don't mind modifyingcrypt()or adding migration INI setting.Yeah, that’s my thoughts as well. We changed it for a reason, but to avoid
breaking things, perhaps a migratory INI setting (which we’d eventually
remove) is for the best.
OK. I'll write a new RFC to add migration INI for this unless
there aren't any more comments.
- Add bool crypt_migration INI that is default to OFF for PHP 5.4 and up.
- master will not have this INI.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
crypt()has BC issue with older systems.https://bugs.php.net/bug.php?id=62372&edit=1
The reason rounds became 1000 from 10 is hardcoded lower limit for newer
PHPs.
Generally speaking, developer should never use less than 1000 rounds and
better to have
at least few thousands rounds or more, tens of thousands or more is
recommended.I would like to make this bug report 'wont fix', since migration is
possible.
- Developer may use larger rounds and store updated hash when
user is authenticated with old PHP.- Developer may ask users to reset password if password hash has
to fewer rounds than 1000 (i.e. outdated hash) with new PHP.
At the risk of perhaps missing the point, wouldn't it be more useful
to encourage users in some way (perhaps through documentation only) to
use password_hash()/password_verify() instead? It was designed with
migration paths in mind.
Apps which are currently using crypt() for their own password systems
(the ones you would have migrate to crypt() + 1000 rounds) should be
pointed at the right solution, not placated with an "okay for now, but
may need to be migrated again later" route.
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.
-Sara
Hi Sara,
At the risk of perhaps missing the point, wouldn't it be more useful
to encourage users in some way (perhaps through documentation only) to
usepassword_hash()/password_verify() instead? It was designed with
migration paths in mind.
I'll add them.
Apps which are currently using
crypt()for their own password systems
(the ones you would have migrate tocrypt()+ 1000 rounds) should be
pointed at the right solution, not placated with an "okay for now, but
may need to be migrated again later" route.As far as I'm aware, the only reason for not marking
crypt()
E_DEPRECATEDright now is for compatibility with external systems, and
as far as those go, changing a default won't effect anything.
Instead of relaxing crypt(), how about relax password_verify()?
<?php
$h='$6$rounds=10$qNElXs2yMnL2.GNS3kiM7DqmGbFLdQfIwu2691aJgT3xgJazPLtw7RPKz3Dp8RIc4b5fmJ7qvlq/mPN8a.rE40';
$p='salasana';
$c=crypt($p,$h);
echo "HASH: $h\n";
echo "CRYPT: $c\n";
if ($c == $h) {
echo "MATCH OK\n";
} else {
echo "NO MATCH\n";
}
var_dump(password_verify($p, $h)); // Fails since password_verify() is
crypt() wrapper
$h2='$6$rounds=1000$qNElXs2yMnL2.GNS$/q7trYkbKkoJernsumbObt2IysdXGRx/ytFaG0HBC97rHHhYRQvUcyEuRHP6h5yj8V.fH7XKEw5hjofVmYONw1';
var_dump(password_verify($p, $h2)); // Success since it has 1000 rounds
?>
Current password_verify() is using the same hard coded 1000 rounds
limitation, but
it could be relaxed. This would be the best solution.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
Hi Sara,
At the risk of perhaps missing the point, wouldn't it be more useful
to encourage users in some way (perhaps through documentation only) to
usepassword_hash()/password_verify() instead? It was designed with
migration paths in mind.I'll add them.
Apps which are currently using
crypt()for their own password systems
(the ones you would have migrate tocrypt()+ 1000 rounds) should be
pointed at the right solution, not placated with an "okay for now, but
may need to be migrated again later" route.As far as I'm aware, the only reason for not marking
crypt()
E_DEPRECATEDright now is for compatibility with external systems, and
as far as those go, changing a default won't effect anything.Instead of relaxing
crypt(), how about relaxpassword_verify()?<?php
$h='$6$rounds=10$qNElXs2yMnL2.GNS3kiM7DqmGbFLdQfIwu2691aJgT3xgJazPLtw7RPKz3Dp8RIc4b5fmJ7qvlq/mPN8a.rE40';
$p='salasana';
$c=crypt($p,$h);
echo "HASH: $h\n";
echo "CRYPT: $c\n";
if ($c == $h) {
echo "MATCH OK\n";
} else {
echo "NO MATCH\n";
}var_dump(password_verify($p, $h)); // Fails since
password_verify()is
crypt()wrapper$h2='$6$rounds=1000$qNElXs2yMnL2.GNS$/q7trYkbKkoJernsumbObt2IysdXGRx/ytFaG0HBC97rHHhYRQvUcyEuRHP6h5yj8V.fH7XKEw5hjofVmYONw1';
var_dump(password_verify($p, $h2)); // Success since it has 1000 rounds
?>Current
password_verify()is using the same hard coded 1000 rounds
limitation, but
it could be relaxed. This would be the best solution.
Why should password_verify() work on a hash that wasn't generated with
password_hash()? The fact that it uses crypt() internally should not
leak outside of its API, imho.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
--
Tjerk
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 with
password_hash()? The fact that it usescrypt()internally should not
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_rehash().
i.e. They don't have to parse password hash to detect obsolete hash.
Therefore, using password_*() for crypt() generated passwords makes sense.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
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 with
password_hash()? The fact that it usescrypt()internally should not
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 withpassword_needs_rehash().
The documentation states that the hash argument to both
password_needs_rehash() and password_verify() is:
hash - A hash created by `password_hash()`.
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.
i.e. They don't have to parse password hash to detect obsolete hash.
Therefore, using password_*() for
crypt()generated passwords makes sense.Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
--
Tjerk
Hi Tjerk,
On Thu, Jul 17, 2014 at 3:16 PM, Tjerk Meesters tjerk.meesters@gmail.com
wrote:
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 with
password_hash()? The fact that it usescrypt()internally should not
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_rehash().The documentation states that the
hashargument to both
password_needs_rehash()andpassword_verify()is:hash - A hash created by `password_hash()`.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
It's easier to change crypt() behavior, since password_() is crypt()
wrapper.
Are we going to relax the crypt() restriction permanently? It's ok for me.
Users
are better to use password_() anyway. We must remove 72 bytes restriction
in password_*() ASAP, though. i.e. blowfish truncates password longer than
72
bytes.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
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 with
password_hash()? The fact that it usescrypt()internally should not
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 withpassword_needs_rehash().The documentation states that the
hashargument to both
password_needs_rehash()andpassword_verify()is:hash - A hash created by `password_hash()`.
Whoa. Don't put too much stock in that — I think I made that up out 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
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=1000$abcdefghijklmnop"
gives hash:
string(118) "$6$rounds=1000$abcdefghijklmnop$SgL/Atu7DClkX0qBUuG6FS2bRf2XmLFWY9b8pRttPEj9ZSh4MKE5bKlz4WAKomLuWI.YQ5oIPLO2L.0OioAeW/"
great.
But in 4.3.0 - 5.2.17:
the same salt gives:
string(99) "$6$rounds=10$EdPD9pXG2vAIeeyG2E/9Mzey2mA/qZgIBAbccXWtrNiymhFYXNK4r2gphMi4KOksXRubHnBWGVh/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='$6$rounds=1$abcdefghijklmnop';
$h2='$6$abcdefghijklmnop';
gives us (on 4.3.0 - 5.2.17):
string(102) "$6$rounds=1000$$6TFP.7u1vZP5A9fccvmUPteI8f29BLhgfqL1XEQqcrvqTSKKw5SBa2qw1sOwLEQ41Dhl1u/Jbi2hRHRYCdxuv0"
string(99) "$6$abcdefghi$4Dv/xQG4euniWsyrRHGUrNqM9CFl5Pg9EI88OgO4tIzuV952JnT09wVxrzUP9l/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_DEPRECATEDright 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
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 with
password_hash()? The fact that it usescrypt()internally should not
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 withpassword_needs_rehash().The documentation states that the
hashargument to both
password_needs_rehash()andpassword_verify()is:hash - A hash created by `password_hash()`.Whoa. Don't put too much stock in that — I think I made that up out 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()andpassword_hash()
hashes were accepted here, and that's probably what the documentation
should say too.Adam
Hi all,
On Fri, Jul 18, 2014 at 4:38 AM, Anthony Ferrara ircmaxell@gmail.com
wrote:
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.
OK for me. I suggested to close the bug as 'wont fix' in first place.
Regarding
password_verify()acceptingcrypt(), 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 ofpassword_hash(), not the complement of
crypt()).As far as I'm aware, the only reason for not marking
crypt()
E_DEPRECATEDright 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".
I agree. crypt() should be available as normal function.
Anthony, do you have suggestion for removing 72 char restriction of
PASSWORD_BCRYPT?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Yasuo
Anthony, do you have suggestion for removing 72 char restriction of
PASSWORD_BCRYPT?
My suggestion is to leave it there (it's no longer bcrypt if you do
something to remove it). Here's a deeper explanation:
http://stackoverflow.com/a/16597402/338665
Once scrypt (or yescrypt, or whatever comes out of PHC) gets crypt(3)
bindings, then we can implement that and pull it into the password
API.
Until then, implementing anything else is a step backwards
(crypt-sha256/512 is weaker than bcrypt, as is PBKDF2+sha512). So
since the 72 character restrict has little if any practical effect
(see my answer above), it's not worth making a practical weakening
(measurably harming everyone) of the overall algorithm for no reason.
My assertion is that, for the average developer, they are far more
likely to screw something up than actually improve security. The
documentation should be updated (and was, but instead of fixing a
misleading line, someone simply removed it, making the overall
statement more misleading).
Could we pre-hash internally ourselves? Sure. But at that point it's
no longer bcrypt (but our own hybrid) which was not what we were
after.
My stance here is that password_hash() should use standard algorithms
and formats only. The last thing we should be doing is inventing
crypto ourselves. Even if it "seems safe". Even if it "seems better".
Let's stick with the current implementation, make the "72 character
warning" a lot less scary, and move on. Once there are crypt(3)
bindings to a more secure algorithm (again, scrypt or yescrypt, or
whatever), then move to it....
Anthony
Hi Anthony,
On Fri, Jul 18, 2014 at 6:56 AM, Anthony Ferrara ircmaxell@gmail.com
wrote:
Anthony, do you have suggestion for removing 72 char restriction of
PASSWORD_BCRYPT?My suggestion is to leave it there (it's no longer bcrypt if you do
something to remove it). Here's a deeper explanation:
http://stackoverflow.com/a/16597402/338665Once scrypt (or yescrypt, or whatever comes out of PHC) gets crypt(3)
bindings, then we can implement that and pull it into the password
API.
Sounds good to me.
Until then, implementing anything else is a step backwards
(crypt-sha256/512 is weaker than bcrypt, as is PBKDF2+sha512). So
since the 72 character restrict has little if any practical effect
(see my answer above), it's not worth making a practical weakening
(measurably harming everyone) of the overall algorithm for no reason.My assertion is that, for the average developer, they are far more
likely to screw something up than actually improve security. The
documentation should be updated (and was, but instead of fixing a
misleading line, someone simply removed it, making the overall
statement more misleading).Could we pre-hash internally ourselves? Sure. But at that point it's
no longer bcrypt (but our own hybrid) which was not what we were
after.
I'll suggest users to use SHA512 raw output as password to
remove 72 chars limitation if it is needed.
<?php
// Use sha512's raw(binary) output
$h = password_hash(hash('sha512', 'my long password', true),
PASSWORD_DEFAULT);
// Verify
$r = password_verify(hash('sha512', 'my long password', true), $h);
var_dump($r); // TRUE
?>
My stance here is that
password_hash()should use standard algorithms
and formats only. The last thing we should be doing is inventing
crypto ourselves. Even if it "seems safe". Even if it "seems better".
I agree.
Let's stick with the current implementation, make the "72 character
warning" a lot less scary, and move on. Once there are crypt(3)
bindings to a more secure algorithm (again, scrypt or yescrypt, or
whatever), then move to it....
Raising E_NOTICE for too long password for PASSWORD_BCRYPT
makes sense. I'll add it later.
https://bugs.php.net/bug.php?id=67653
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Yasuo
I'll suggest users to use SHA512 raw output as password to
remove 72 chars limitation if it is needed.
Then you either misunderstood what I was saying, or completely ignored it.
Raising
E_NOTICEfor too long password forPASSWORD_BCRYPT
makes sense. I'll add it later.
Please, don't change anything without first proposing an RFC so that
sufficient discussion can occur. Literally every single person in this
thread has disagreed with you, yet you want to go ahead and make
changes. It's not saying that you are wrong, but that discussion needs
to happen before any changes to security sensitive code and
documentation.
Also, Deprecating crypt() without first discussing it (and having an
RFC to vote on) is not cool (and has been reverted):
http://svn.php.net/viewvc/phpdoc/en/trunk/reference/strings/functions/crypt.xml?r1=333973&r2=334296
Generally speaking, it would not be serious issue. 72 bytes constant prefix
would
not be used most likely.
Correct. So if it's not a serious issue, why are we talking about
adding serious warnings to documentation pages (big red boxes), and
notices and pre-hashing?
However, bug like this in "authentication" code must be detected and
fixed.
It's not a bug. It's by design. You could argue if it's a good design
or not, but it's very much by design.
If password should be truncated, it should be truncated by app developers
explicitly and
notified users that their password had been truncated. IMHO.
We disagree. So rather than just committing changes, why not draft up
an RFC with your suggestions, which can be discussed and voted upon?
There's already a notice about this in the
password_hash()docs, one that
almost looks like is designed to scare users, which is bad. Throwing an
E_NOTICEwill cause more problems than it would supposedly solve.
I agree 100%. It's on my list of things to do to "clean up" that
warning, to word it to be less scary. And I agree 100% on the notice
side as well.
Application developers should just state this limitation on their
registration/password change pages, anything else is pointless.
I don't think so. If the end user enters a 72 character+ password, the
chances of a collision because of the truncation are so small that
it's not worth worrying about.
However, it's worth documenting, as what you don't want is people
inventing their own crypto by prefixing their own secret:
password_hash(hash('sha512', SECRET_KEY) . $password, PASSWORD_DEFAULT)
Which would then always produce the same hash (since the sha512 hash
is 128 bytes long, truncation would happen too early).
We should be educating people to avoid their own crypto. That was the
entire point of the password_hash APIs. Make it so simple that it's
difficult to screw up. Yet people will always find a way. The only way
to solve that problem is education. And adding big red boxes isn't
education. It's fear.
Anthony
Hi Anthony,
On Sun, Jul 20, 2014 at 12:27 AM, Anthony Ferrara ircmaxell@gmail.com
wrote:
I'll suggest users to use SHA512 raw output as password to
remove 72 chars limitation if it is needed.Then you either misunderstood what I was saying, or completely ignored it.
SHA512 raw output may truncate bytes from 72 to 64 in return of limitless
password
length. Do you suggest developers not to workaround 72 bytes limitation by
themselves
like this?
SHA512 raw results could weaken password length longer than 64. However,
allowed
chars for passwords are limited almost always. It would not be an issue to
worry much.
Raising
E_NOTICEfor too long password forPASSWORD_BCRYPTmakes sense. I'll add it later.
Please, don't change anything without first proposing an RFC so that
sufficient discussion can occur. Literally every single person in this
thread has disagreed with you, yet you want to go ahead and make
changes. It's not saying that you are wrong, but that discussion needs
to happen before any changes to security sensitive code and
documentation.Also, Deprecating
crypt()without first discussing it (and having an
RFC to vote on) is not cool (and has been reverted):http://svn.php.net/viewvc/phpdoc/en/trunk/reference/strings/functions/crypt.xml?r1=333973&r2=334296
Generally speaking, it would not be serious issue. 72 bytes constant
prefix
would
not be used most likely.Correct. So if it's not a serious issue, why are we talking about
adding serious warnings to documentation pages (big red boxes), and
notices and pre-hashing?However, bug like this in "authentication" code must be detected and
fixed.It's not a bug. It's by design. You could argue if it's a good design
or not, but it's very much by design.
I'm not saying that 72 bytes length limit is a blowfish bug.
I'm saying it's a user code bug that blindly accepts password exceeds its
limit.
If password should be truncated, it should be truncated by app developers
explicitly and
notified users that their password had been truncated. IMHO.We disagree. So rather than just committing changes, why not draft up
an RFC with your suggestions, which can be discussed and voted upon?
I think you misunderstood me.
What I'm going to change is adding E_NOTICE for password longer than 72
bytes.
It's just a simple input validation.
There's already a notice about this in the
password_hash()docs, one
that
almost looks like is designed to scare users, which is bad. Throwing an
E_NOTICEwill cause more problems than it would supposedly solve.I agree 100%. It's on my list of things to do to "clean up" that
warning, to word it to be less scary. And I agree 100% on the notice
side as well.
Application developers should just state this limitation on their
registration/password change pages, anything else is pointless.I don't think so. If the end user enters a 72 character+ password, the
chances of a collision because of the truncation are so small that
it's not worth worrying about.
I agree it would not be an serious issue.
However, it's worth documenting, as what you don't want is people
inventing their own crypto by prefixing their own secret:
password_hash(hash('sha512', SECRET_KEY) . $password, PASSWORD_DEFAULT)Which would then always produce the same hash (since the sha512 hash
is 128 bytes long, truncation would happen too early).
Good example. People do this kind of things.
E_NOTICE error should help to prevent such code.
We should be educating people to avoid their own crypto. That was the
entire point of the password_hash APIs. Make it so simple that it's
difficult to screw up. Yet people will always find a way. The only way
to solve that problem is education. And adding big red boxes isn't
education. It's fear.
I'm not closely following doc commit. It seems my commit was modified.
http://jp2.php.net/manual/en/function.password-hash.php
"Caution Using the PASSWORD_BCRYPT for the algo parameter, will result in
the password parameter being truncated to a maximum length of 72
characters."
This may be too much. I've added truncation spec in PASSWORD_BCRYPT
constant explanation.
I suppose it was better than this. (Besides grammar)
Anyway, there are 2 things to resolve.
- If we have
E_NOTICEforpassword_hash()withPASSWORD_BCRYPTor not - How we document password_*()
Your example
password_hash(hash('sha512', SECRET_KEY) . $password, PASSWORD_DEFAULT)
shows that we are better to have E_NOTICE for too long password.
I'm OK with any documentation as long as it's precise.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Anthony,
I want to finish and close this issue.
Also, Deprecating
crypt()without first discussing it (and having anRFC to vote on) is not cool (and has been reverted):
http://svn.php.net/viewvc/phpdoc/en/trunk/reference/strings/functions/crypt.xml?r1=333973&r2=334296
I wrote
"Use of <function>crypt</function> is deprecated."
not
"crypt() is deprecated". I didn't mean crypt() function deprecation. If
it's confusing, I don't mind at all to rewrite it.
BTW, what part is wrong?
It seems we have misunderstanding each other, but the basis is the same, I
suppose.
-
crypt()will remain - User/developer should use password_*()
- It's not good idea to have PHP own
crypt()
The differences are E_NOTICE and workaround, it seems.
IMO, password_hash() must raise E_NOTICE for too long password. Truncation
without error is
not an option for me. People write stupid code without internal knowledge.
Adding fixed salt was
common since crypt() was not good enough used to be. In addition, maximum
password length
is not decided by us, but decided by app developers. Therefore, we are
better to provide/explain
workaround for password_hash() limitation.
Prehash in PHP is not an option as we don't want PHP only crypt(). Prehash
by developer is
acceptable workaround for me. As you know, chars that are used in password
is limited. Some
developers even allow UTF-8 for password, structured encoding could reduce
total number of bits
in password hash with limited password length. Prehash with raw SHA512 will
give us adequate
data for PASSWORD_BCRYPT hashing regardless of password length.
Prehash ruins password_*() flexibility for sure, but there is workaround
also. Developer may use
timestamp to check new hash (e.g. 1024 bits hash, etc) should be used or
not.
If we don't want such workarounds, we may enable SHA512 hash for
password_hash() for those
who don't want password length limit.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Anthony,
I want to finish and close this issue.
Also, Deprecating
crypt()without first discussing it (and having anRFC to vote on) is not cool (and has been reverted):
http://svn.php.net/viewvc/phpdoc/en/trunk/reference/strings/functions/crypt.xml?r1=333973&r2=334296
I wrote"Use of <function>crypt</function> is deprecated."
not
"crypt() is deprecated". I didn't mean
crypt()function deprecation. If
it's confusing, I don't mind at all to rewrite it.
BTW, what part is wrong?It seems we have misunderstanding each other, but the basis is the same, I
suppose.
crypt()will remain- User/developer should use password_*()
- It's not good idea to have PHP own
crypt()The differences are
E_NOTICEand workaround, it seems.
IMO,password_hash()must raiseE_NOTICEfor too long password. Truncation
without error is
not an option for me. People write stupid code without internal knowledge.
Adding fixed salt was
common sincecrypt()was not good enough used to be. In addition, maximum
password length
is not decided by us, but decided by app developers. Therefore, we are
better to provide/explain
workaround forpassword_hash()limitation.Prehash in PHP is not an option as we don't want PHP only
crypt(). Prehash
by developer is
acceptable workaround for me. As you know, chars that are used in password
is limited. Some
developers even allow UTF-8 for password, structured encoding could reduce
total number of bits
in password hash with limited password length. Prehash with raw SHA512 will
give us adequate
data forPASSWORD_BCRYPThashing regardless of password length.Prehash ruins password_*() flexibility for sure, but there is workaround
also. Developer may use
timestamp to check new hash (e.g. 1024 bits hash, etc) should be used or
not.If we don't want such workarounds, we may enable SHA512 hash for
password_hash()for those
who don't want password length limit.Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Yasuo,
Prehashing with sha512 means it is no longer blowfish. It is now a non-vetted DIY algorithm. The whole point of password_hash is to avoid this type of thing, and should be clearly discouraged in the documentation.
It's a classic example of what not to do.
David
Hi David,
Prehashing with sha512 means it is no longer blowfish. It is now a
non-vetted DIY algorithm. The whole point of password_hash is to avoid this
type of thing, and should be clearly discouraged in the documentation.
I agree. It's far better if it could handle limitless password length.
The problem is "there is no way to achieve this with current
implementation".
It's a classic example of what not to do.
I agree here, too. We are better to have algorithm that does not enforce
user/developer to certain password and recommend "Just use it".
However, Using multiple hashes for better security is common technique.
An example is SSL. So I would not say one should not. Especially when there
is a limitation.
In old days, crypt() was unusable securely. There are many users/developers
that
are used to have static slat. Code like below disables authentication
completely.
password_hash(hash('sha512', SOME_SECRET_SALT).$password, DEFAULT);
This should be prevented. (I would like to prevent it by raising E_NOTICE
error)
If we would like to recommend "Just use it", we may consider adding SHA512
to password_hash().
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
In old days,
crypt()was unusable securely. There are many
users/developers that
are used to have static slat. Code like below disables authentication
completely.password_hash(hash('sha512', SOME_SECRET_SALT).$password, DEFAULT);
This should be prevented. (I would like to prevent it by raising
E_NOTICE
error)
E_NOTICE for password larger than 72 is mandatory. Current password_hash()
works without any sign of problem even if it may not be working as
authentication.
I'll add E_NOTICE as bug fix if there aren't any more comments.
If we would like to recommend "Just use it", we may consider adding SHA512
to
password_hash().
This one needs RFC, but I'm OK with prehashing in userland.
Please write RFC and implement it if you are willing to have this.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Yasuo,
E_NOTICEfor password larger than 72 is mandatory. Currentpassword_hash()
works without any sign of problem even if it may not be working as
authentication.
I'll addE_NOTICEas bug fix if there aren't any more comments.
Could you please not.
I have asked you to draft an RFC to justify what you intend to do
(documentation, errors, etc) so that we may discuss it better. There
is not a single person in this thread who has said "I think a notice
is a good idea" except you. Yet you insist on just adding it as a "bug
fix". Could you please just slow down, and write out the explanations
so that we can have a meangingful discussion instead of just rushing
through to commit ignoring what everyone is saying?
However, Using multiple hashes for better security is common technique.
An example is SSL. So I would not say one should not. Especially when there
is a limitation.
TLS 1.0 (SSL) used a very specific construction of PRF<MD5>(secret1,
label + seed) XOR PRF<SHA1>(secret2, label + seed). This is very much
not "using multiple hashes", but using 2 different PRF (pseudo-random
functions) and xoring them together. The end result should be
resistent to cryptoanalysis or attacks that only target one of the
hash functions.
However, TLS 1.2 removed that construction and uses the PRF based on
SHA-256 alone. Why?
As it turns out, it's not based in science at all, and can be proven
faulty. Here's a quick proof:
HASH_FUNC_A - A secure cryptographic hash function (resistent to
collisions, pre-image attacks and 2nd pre-image attacks, ex: SHA-256)
HASH_FUNC_B - Defined as HASH_FUNC_A XOR C (for some arbitrary constant C).
Since HASH_FUNC_A is secure, HASH_FUNC_B is secure as well (since
xoring with an arbitrary constant changes none of its properties).
However, HASH_FUNC_A XOR HASH_FUNC_B will always produce C. Proving
that XOR of two arbitrary hash functions isn't necessarily secure.
More information: http://tuprints.ulb.tu-darmstadt.de/2094/1/thesis.lehmann.pdf
Now, if all you care about is pre-image resistance, then H1(H2(x))
appears to be secure. And if we assume that H1() and H2() are secure
functions, it is.
However, that's a tricky assumption to make. For example, any
collisions in H2() automatically become collisions in H1()
(incidentally, this was the reason PBKDF2 replaced PBKDF1). So that
effectively means that the collision resistance of H1(H2(x)) is at
least as weak as the weakest hash function (potentially weaker). But
we said that all we care about is pre-image resistance, so why does
that matter? Because in the end, we don't just care about pre-image
resistance. We also do care about collision resistance (though to a
lesser degree for password storage). Because with the hash, if an
attacker can generate a collision, they can use that to login to your
system. Yes, that didn't leak the original password (and hence other
systems may be safe), but it did let him compromise you.
In practice, this isn't very likely (as SHA-512 is a strong function
with no known collisions, nor decent attack against it). But in terms
of recommending crypto to the general public, "isn't very likely" is
the wrong way to make recommendations.
In old days,
crypt()was unusable securely. There are many users/developers
that
No, crypt() was perfectly able to be used securely. The password_hash
apis prove that. Many didn't use it securely, but that didn't mean it
isn't secure.
are used to have static slat. Code like below disables authentication
completely.password_hash(hash('sha512', SOME_SECRET_SALT).$password, DEFAULT);
This should be prevented. (I would like to prevent it by raising
E_NOTICE
error)
And a E_NOTICE will prevent nothing. If you really want to prevent
it, raise a warning and return false from the hash function. But
that's not possible without a MASSIVE BC break. Hence why I'm
recommending that you open an RFC on it.
If we would like to recommend "Just use it", we may consider adding SHA512
topassword_hash().
As I pointed out above, no.
I wrote
"Use of <function>crypt</function> is deprecated."
not
"crypt() is deprecated". I didn't mean
crypt()function deprecation. If it's
confusing, I don't mind at all to rewrite it.
BTW, what part is wrong?
"use of crypt is deprecated" is the same as saying "crypt() is
deprecated". That word has very specific and special meaning.
Adding fixed salt was
common sincecrypt()was not good enough used to be. In addition, maximum
password length
is not decided by us, but decided by app developers. Therefore, we are
better to provide/explain
workaround forpassword_hash()limitation.
And as I've said before, the way to combat that is with education.
I'm not closely following doc commit. It seems my commit was modified.
It wasn't modified. It was reverted. That warning that you cite has
existed for 6 months: https://bugs.php.net/bug.php?id=66564
Anthony
Hi Anthony,
On Mon, Jul 21, 2014 at 11:32 PM, Anthony Ferrara ircmaxell@gmail.com
wrote:
E_NOTICEfor password larger than 72 is mandatory. Current
password_hash()
works without any sign of problem even if it may not be working as
authentication.
I'll addE_NOTICEas bug fix if there aren't any more comments.Could you please not.
I have asked you to draft an RFC to justify what you intend to do
(documentation, errors, etc) so that we may discuss it better. There
is not a single person in this thread who has said "I think a notice
is a good idea" except you. Yet you insist on just adding it as a "bug
fix". Could you please just slow down, and write out the explanations
so that we can have a meangingful discussion instead of just rushing
through to commit ignoring what everyone is saying?
I see you and Andrey against to have E_NOTICE for password_hash().
There are only 2 persons to be correct. I don't know about IRC since I
don't it at all.
However, I don't mind at all to write RFC raising E_NOTICE for
password_hash() with
PASSWORD_BCRYPT.
However, Using multiple hashes for better security is common technique.
An example is SSL. So I would not say one should not. Especially when
there
is a limitation.TLS 1.0 (SSL) used a very specific construction of PRF<MD5>(secret1,
label + seed) XOR PRF<SHA1>(secret2, label + seed). This is very much
not "using multiple hashes", but using 2 different PRF (pseudo-random
functions) and xoring them together. The end result should be
resistent to cryptoanalysis or attacks that only target one of the
hash functions.However, TLS 1.2 removed that construction and uses the PRF based on
SHA-256 alone. Why?As it turns out, it's not based in science at all, and can be proven
faulty. Here's a quick proof:HASH_FUNC_A - A secure cryptographic hash function (resistent to
collisions, pre-image attacks and 2nd pre-image attacks, ex: SHA-256)
HASH_FUNC_B - Defined as HASH_FUNC_A XOR C (for some arbitrary constant C).Since HASH_FUNC_A is secure, HASH_FUNC_B is secure as well (since
xoring with an arbitrary constant changes none of its properties).However, HASH_FUNC_A XOR HASH_FUNC_B will always produce C. Proving
that XOR of two arbitrary hash functions isn't necessarily secure.More information:
http://tuprints.ulb.tu-darmstadt.de/2094/1/thesis.lehmann.pdf
Although cryptgraphic hash functions are supposed work cryptgraphic way, but
many of them are failing. It was not in real world and I aware of the risk.
Now, if all you care about is pre-image resistance, then H1(H2(x))
appears to be secure. And if we assume that H1() and H2() are secure
functions, it is.However, that's a tricky assumption to make. For example, any
collisions in H2() automatically become collisions in H1()
(incidentally, this was the reason PBKDF2 replaced PBKDF1). So that
effectively means that the collision resistance of H1(H2(x)) is at
least as weak as the weakest hash function (potentially weaker). But
we said that all we care about is pre-image resistance, so why does
that matter? Because in the end, we don't just care about pre-image
resistance. We also do care about collision resistance (though to a
lesser degree for password storage). Because with the hash, if an
attacker can generate a collision, they can use that to login to your
system. Yes, that didn't leak the original password (and hence other
systems may be safe), but it did let him compromise you.In practice, this isn't very likely (as SHA-512 is a strong function
with no known collisions, nor decent attack against it). But in terms
of recommending crypto to the general public, "isn't very likely" is
the wrong way to make recommendations.
I agree with you discussion and have no objection. I understand it's
correct in theory
and real world.
Even if I understand the risk, it's acceptable for me to use SHA512 to
workaround
password length limit as long as SHA512 and/or blowfish is considered safe.
As I
wrote, password length limit is decided by app developers and we are not
the app
developer.
In old days,
crypt()was unusable securely. There are many
users/developersthat
No,
crypt()was perfectly able to be used securely. The password_hash
apis prove that. Many didn't use it securely, but that didn't mean it
isn't secure.
I agree that recent crypt() can be used securely(reliably). I'm pointing it
out that it was
not, especially with pre 5.3 as you know. We know that there are many users
out
there still using pre 5.3.
http://w3techs.com/technologies/details/pl-php/5/all
are used to have static slat. Code like below disables authentication
completely.
password_hash(hash('sha512', SOME_SECRET_SALT).$password, DEFAULT);
This should be prevented. (I would like to prevent it by raising
E_NOTICE
error)And a
E_NOTICEwill prevent nothing. If you really want to prevent
it, raise a warning and return false from the hash function. But
that's not possible without a MASSIVE BC break. Hence why I'm
recommending that you open an RFC on it.
I'll strongly object returning FALSE for faulty usage. IMHO, it is as bad
as using too long
prefix for PASSWORD_BCRYPT. It could disable authentication completely.
If we would like to recommend "Just use it", we may consider adding SHA512
to
password_hash().As I pointed out above, no.
I'm lost here. Are you going to discuss use of SHA512 for password hashing
is
insecure? Blowfish is suitable for password hashing because of its
"slowness".
Isn't it matter of rounds?
SHA is targeted for attack and hardware cracking became common. Is this the
reason?
I wrote
"Use of <function>crypt</function> is deprecated."
not
"crypt() is deprecated". I didn't mean
crypt()function deprecation. If
it's
confusing, I don't mind at all to rewrite it.
BTW, what part is wrong?"use of crypt is deprecated" is the same as saying "crypt() is
deprecated". That word has very specific and special meaning.
OK. This was too much. I'll use "recommend" or like.
Adding fixed salt was
common sincecrypt()was not good enough used to be. In addition, maximum
password length
is not decided by us, but decided by app developers. Therefore, we are
better to provide/explain
workaround forpassword_hash()limitation.And as I've said before, the way to combat that is with education.
I wish I can educate users/developers "Just use password_*()".
The problems are password length limit and truncation without any
noticeable error.
password_hash() is named "hash" and average users expect that "hash" will
compute hash value from given parameter, not part of it.
I agree that leaving crypt() as it is now. I've closed the bug report as
"wont fix" already.
It was my original intention, too.
I'm not sure your opinions for
-
password_hash()truncation behavior - What to do to accept password longer than 72 bytes
Could you give your opinion for these?
My opinions are
- Raise
E_NOTICEfor too long password. (Error only, returns result with
the truncated password) - Make a documentation issue. Recommend prehash. Alternatively, add SHA512
hash with large enough rounds.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Yasuo,
Hi Anthony,
On Mon, Jul 21, 2014 at 11:32 PM, Anthony Ferrara ircmaxell@gmail.com
wrote:
E_NOTICEfor password larger than 72 is mandatory. Current
password_hash()
works without any sign of problem even if it may not be working as
authentication.
I'll addE_NOTICEas bug fix if there aren't any more comments.Could you please not.
I have asked you to draft an RFC to justify what you intend to do
(documentation, errors, etc) so that we may discuss it better. There
is not a single person in this thread who has said "I think a notice
is a good idea" except you. Yet you insist on just adding it as a "bug
fix". Could you please just slow down, and write out the explanations
so that we can have a meangingful discussion instead of just rushing
through to commit ignoring what everyone is saying?I see you and Andrey against to have
E_NOTICEforpassword_hash().
There are only 2 persons to be correct. I don't know about IRC since I
don't it at all.
I do not see any discussion about that on IRC, but I would rather not
add it either. It brings little but more confusions.
I would suggest to do what Anthony suggested and we will see the outcome.
Cheers,
Hi Anthony,
I noticed I've made silly mistakes in previous reply. I've added little
more.
Even if I remove text formatting, gmail insists strange formatting. It may
be hard to read...
Please disregard old one and read this.
On Mon, Jul 21, 2014 at 11:32 PM, Anthony Ferrara ircmaxell@gmail.com
wrote:
E_NOTICEfor password larger than 72 is mandatory. Current
password_hash()
works without any sign of problem even if it may not be working as
authentication.
I'll addE_NOTICEas bug fix if there aren't any more comments.Could you please not.
I have asked you to draft an RFC to justify what you intend to do
(documentation, errors, etc) so that we may discuss it better. There
is not a single person in this thread who has said "I think a notice
is a good idea" except you. Yet you insist on just adding it as a "bug
fix". Could you please just slow down, and write out the explanations
so that we can have a meangingful discussion instead of just rushing
through to commit ignoring what everyone is saying?
I see you and Andrey against to have E_NOTICE for password_hash().
There are only 2 persons to be correct in this thread. I don't know about
IRC
since I don't use it at all.
However, I don't mind at all to write RFC raising E_NOTICE for
password_hash()
with PASSWORD_BCRYPT.
However, Using multiple hashes for better security is common technique.
An example is SSL. So I would not say one should not. Especially when
there
is a limitation.TLS 1.0 (SSL) used a very specific construction of PRF<MD5>(secret1,
label + seed) XOR PRF<SHA1>(secret2, label + seed). This is very much
not "using multiple hashes", but using 2 different PRF (pseudo-random
functions) and xoring them together. The end result should be
resistent to cryptoanalysis or attacks that only target one of the
hash functions.However, TLS 1.2 removed that construction and uses the PRF based on
SHA-256 alone. Why?As it turns out, it's not based in science at all, and can be proven
faulty. Here's a quick proof:HASH_FUNC_A - A secure cryptographic hash function (resistent to
collisions, pre-image attacks and 2nd pre-image attacks, ex: SHA-256)
HASH_FUNC_B - Defined as HASH_FUNC_A XOR C (for some arbitrary constant
C).Since HASH_FUNC_A is secure, HASH_FUNC_B is secure as well (since
xoring with an arbitrary constant changes none of its properties).However, HASH_FUNC_A XOR HASH_FUNC_B will always produce C. Proving
that XOR of two arbitrary hash functions isn't necessarily secure.More information:
http://tuprints.ulb.tu-darmstadt.de/2094/1/thesis.lehmann.pdf
Although cryptgraphic hash functions are supposed work cryptgraphic way, but
many of them are failing. This was in real world and I aware of the risk.
Now, if all you care about is pre-image resistance, then H1(H2(x))
appears to be secure. And if we assume that H1() and H2() are secure
functions, it is.However, that's a tricky assumption to make. For example, any
collisions in H2() automatically become collisions in H1()
(incidentally, this was the reason PBKDF2 replaced PBKDF1). So that
effectively means that the collision resistance of H1(H2(x)) is at
least as weak as the weakest hash function (potentially weaker). But
we said that all we care about is pre-image resistance, so why does
that matter? Because in the end, we don't just care about pre-image
resistance. We also do care about collision resistance (though to a
lesser degree for password storage). Because with the hash, if an
attacker can generate a collision, they can use that to login to your
system. Yes, that didn't leak the original password (and hence other
systems may be safe), but it did let him compromise you.In practice, this isn't very likely (as SHA-512 is a strong function
with no known collisions, nor decent attack against it). But in terms
of recommending crypto to the general public, "isn't very likely" is
the wrong way to make recommendations.
I agree with your discussion and have no objection. I understand that it's
correct in
theory and real world.
Even if I understand the risk, it's acceptable for me to use SHA512 to
workaround
password length limit as long as SHA512 and/or blowfish is considered safe
by
specialists. As I wrote, password length limit is decided by app developers
and we
are not the app developer.
(BTW, I do code audit/web security check/education for living, but I'm not
a cryptographer)
In old days,
crypt()was unusable securely. There are many
users/developers
thatNo,
crypt()was perfectly able to be used securely. The password_hash
apis prove that. Many didn't use it securely, but that didn't mean it
isn't secure.
I agree that recent crypt() can be used securely (and reliably). I'm
pointing it out that
it was not, especially with pre 5.3 as you know. We know that there are
many users
out there still using pre 5.3.
http://w3techs.com/technologies/details/pl-php/5/all
are used to have static slat. Code like below disables authentication
completely.password_hash(hash('sha512', SOME_SECRET_SALT).$password, DEFAULT);
This should be prevented. (I would like to prevent it by raising
E_NOTICE
error)And a
E_NOTICEwill prevent nothing. If you really want to prevent
it, raise a warning and return false from the hash function. But
that's not possible without a MASSIVE BC break. Hence why I'm
recommending that you open an RFC on it.
I'll strongly object returning FALSE for faulty usage. IMHO, it is as bad
as using too long
prefix for PASSWORD_BCRYPT. It could disable authentication completely.
If we would like to recommend "Just use it", we may consider adding
SHA512
topassword_hash().As I pointed out above, no.
I'm lost here. Are you going to discuss use of SHA512 for password hashing
is
insecure? Blowfish is suitable for password hashing because of its
"slowness".
Isn't it matter of rounds?
SHA is targeted by attackers and hardware cracking became common. Is this
the
reason?
We need 72 bytes limit workaround/solution some how, but it seems you are
objecting possible choices. Most developers need adequate level of security
to
build applications, not perfect security. Not all developers have right to
decide
maximum password length and how password is hashed. (e.g. Adding secret
salt is
still valid protection against stolen password database via SQL
injection,etc.
Some organizations require to add custom static slat to user defined
password.)
I wrote
"Use of <function>crypt</function> is deprecated."
not
"crypt() is deprecated". I didn't mean
crypt()function deprecation. If
it's
confusing, I don't mind at all to rewrite it.
BTW, what part is wrong?"use of crypt is deprecated" is the same as saying "crypt() is
deprecated". That word has very specific and special meaning.
OK. This was too much. I'll use "recommend" or like.
Adding fixed salt was
common sincecrypt()was not good enough used to be. In addition,
maximum
password length
is not decided by us, but decided by app developers. Therefore, we are
better to provide/explain
workaround forpassword_hash()limitation.And as I've said before, the way to combat that is with education.
I wish I could educate users/developers "Just use password_*()".
Problems are password length limit and truncation without any noticeable
error.
password_hash() is named "hash" and average users expect that "hash" will
compute hash value from given parameter, not part of it.
I agreed that leaving crypt() as it is now. I've closed the bug report as
"wont fix" already.
It was my original intention, in fact.
I'm not sure your opinions for
-
password_hash()truncation behavior - What to do to accept password longer than 72 bytes
Could you give your opinion for these?
My opinions are
- Raise
E_NOTICEfor too long password. (Error only, returns result with
the truncated password) - Make a documentation issue. Recommend prehash. Alternatively, add SHA512
hash with large enough rounds.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Yasuo,
However, I don't mind at all to write RFC raising
E_NOTICEfor
password_hash()
with PASSWORD_BCRYPT.
Awesome, thanks!
Although cryptgraphic hash functions are supposed work cryptgraphic way, but
many of them are failing. This was in real world and I aware of the risk.
It's less about being aware of the risk, and more showing that the
example you cited of combining hash functions (to show that's it's OK
to do) was actually changed to stop doing that because it's known to
be insecure. So if anything, by citing that example you're actually
demonstrating what shouldn't be done, rather than giving an example of
"it's ok to do in the real world".
I agree with your discussion and have no objection. I understand that it's
correct in
theory and real world.Even if I understand the risk, it's acceptable for me to use SHA512 to
workaround
password length limit as long as SHA512 and/or blowfish is considered safe
by
specialists. As I wrote, password length limit is decided by app developers
and we
are not the app developer.(BTW, I do code audit/web security check/education for living, but I'm not a
cryptographer)
For your code, that's fine. For you personally to advocate for it,
that's fine. Write blog posts, answer stack-overflow questions, etc,
that's absolutely fine!
To have a programming language and associated community (which is seen
as a significant authority) to advocate for it is not. To document it
in an official manner in an official context (PHP's documentation),
"understanding the risk" isn't good enough. We shouldn't be
recommending anything that "probably is fine", instead we should be
documenting known good method. Methods that have proofs.
And a
E_NOTICEwill prevent nothing. If you really want to prevent
it, raise a warning and return false from the hash function. But
that's not possible without a MASSIVE BC break. Hence why I'm
recommending that you open an RFC on it.I'll strongly object returning
FALSEfor faulty usage. IMHO, it is as bad as
using too long
prefix for PASSWORD_BCRYPT. It could disable authentication completely.
And that's why this discussion needs to happen. A notice isn't going
to change anything. A significant number of developers turn them off.
Especially in production (which is where long passwords are likely to
occur). It may help a few people, but the majority it wouldn't. And it
would make some people start calling @password_hash() to suppress
the notice (because why bother fixing bugs, when we can ignore them!).
Which is really bad.
If you truly want to prevent people doing silly things like
password_hash(SHA512($secret) . $password), then returning false and
raising a warning is the only way that will actually do so.
How to do so is tricky, as it's a pretty significant BC break, and
will break some running software. But it would prevent the issue that
you've stated you want to prevent.
So that leaves 2 practical options (IMHO):
- Document the behavior, and don't change code functionality.
- Add warning, returning false on overlong password (with documentation).
I don't like 2 (for a similar reason as you), so that leaves 1. Which
is where we are today, so nothing left to do.
If we would like to recommend "Just use it", we may consider adding
SHA512
topassword_hash().As I pointed out above, no.
I'm lost here. Are you going to discuss use of SHA512 for password hashing
is
insecure? Blowfish is suitable for password hashing because of its
"slowness".
Isn't it matter of rounds?
I assumed that you meant bcrypt(sha512(password)). If you mean
crypt-sha512 (aka $6$), why would we want to add a demonstrably less
secure algorithm?
If anything, I'd add PBKDF2-SHA512, but considering there are no
public crypt bindings to PBKDF2 (aka a crypt format), I'm really
hesitant to do that.
Yes, Python has one (a crypt format):
https://www.dlitz.net/software/python-pbkdf2/
Yes, PERL has one:
http://search.cpan.org/~arodland/Crypt-PBKDF2-0.101170/lib/Crypt/PBKDF2.pm
Yes, Ruby has one: https://github.com/nomoon/pbkdf2-crypt
But note none of those are in core distributions. And all of them are
different (incompatible with one another).
We need 72 bytes limit workaround/solution some how, but it seems you are
objecting possible choices. Most developers need adequate level of security
to
build applications, not perfect security. Not all developers have right to
decide
maximum password length and how password is hashed. (e.g. Adding secret salt
is
still valid protection against stolen password database via SQL
injection,etc.
Some organizations require to add custom static slat to user defined
password.)
I am objecting to the possible choices, because none of them are good
enough to offset the risk. The risk associated with overlong passwords
is trivially small. The risk to "inventing crypto" (such as with
crypt(sha512(password)) ) or weakening the case for the rest of
passwords (such as with crypt-sha512) is significantly greater IMHO
then the case where a user's password exceeds 72 characters. Yes, we
could add a notice, but it wouldn't really help the class of users
you're intending it to. And it may wind up hurting some by masking
more serious errors (when the suppress with @).
So I stick with the only good option that IMHO doesn't hurt users,
which is documentation.
But again, this is why I asked for the RFC. My opinion isn't the only
one that matters. I care about it significantly, and I don't want to
see users hurt. But ultimately there are far more people involved than
just you and me. Putting together an RFC will let the discussion
happen, each of us make our case, and let the community decide.
I'm not sure your opinions for
password_hash()truncation behavior- What to do to accept password longer than 72 bytes
Could you give your opinion for these?
My opinions (which I've stated above, but will summarize here):
Truncation behavior: document it. That should be all that's needed.
What to do to accept passwords longer than 72 bytes: Either put an
upper cap on password length in the application, or don't worry about
it. The chances the first 72 bytes can be guessed, but a further one
is different is EXTREMELY small (you're more likely to win a lottery's
jackpot 5 times, than to do so, based on a quick back-of-envelope
estimation).
Yes, ideally a hash function would allow for infinitely long
(practically, 2^32-1 would be enough, 2^64-1 would be better)
passwords. But it's not worth weakening a system to defend against it
(by moving to crypt-sha512). And it's not worth suggesting invented
crypto to defend against it (recommending crypt(sha512(password))).
My recommendation: deal with it today. When scrypt (or the winner of
the current PHC) gets crypt(3) bindings, then we'll pull it in and
make it available to password_hash(). Until then, it's not a
significant enough risk to worry about...
Anthony
Hi Anthony,
On Tue, Jul 22, 2014 at 11:27 PM, Anthony Ferrara ircmaxell@gmail.com
wrote:
However, I don't mind at all to write RFC raising
E_NOTICEfor
password_hash()
with PASSWORD_BCRYPT.Awesome, thanks!
I'll write it later.
Although cryptgraphic hash functions are supposed work cryptgraphic way,
butmany of them are failing. This was in real world and I aware of the risk.
It's less about being aware of the risk, and more showing that the
example you cited of combining hash functions (to show that's it's OK
to do) was actually changed to stop doing that because it's known to
be insecure. So if anything, by citing that example you're actually
demonstrating what shouldn't be done, rather than giving an example of
"it's ok to do in the real world".
I agree that the SSL implementation was bad for the reason you described it.
In fact, it was awful because it reduced hash value(key) space effectively.
In practical security (i.e. not science), it would be acceptable as you
described
"It's ok for stackoverflow, etc". Users choose weak passwords anyway. Even
some specialists suggest to use weak passwords.
I agree with your discussion and have no objection. I understand that
it's
correct in
theory and real world.Even if I understand the risk, it's acceptable for me to use SHA512 to
workaround
password length limit as long as SHA512 and/or blowfish is considered
safe
by
specialists. As I wrote, password length limit is decided by app
developers
and we
are not the app developer.(BTW, I do code audit/web security check/education for living, but I'm
not a
cryptographer)For your code, that's fine. For you personally to advocate for it,
that's fine. Write blog posts, answer stack-overflow questions, etc,
that's absolutely fine!To have a programming language and associated community (which is seen
as a significant authority) to advocate for it is not. To document it
in an official manner in an official context (PHP's documentation),
"understanding the risk" isn't good enough. We shouldn't be
recommending anything that "probably is fine", instead we should be
documenting known good method. Methods that have proofs.
I understand point of your view.
Re-invention of crypt feature has history of failures and re-invention/
tweak of crypt feature should not be done in general. I agree.
And a
E_NOTICEwill prevent nothing. If you really want to prevent
it, raise a warning and return false from the hash function. But
that's not possible without a MASSIVE BC break. Hence why I'm
recommending that you open an RFC on it.I'll strongly object returning
FALSEfor faulty usage. IMHO, it is as
bad as
using too long
prefix for PASSWORD_BCRYPT. It could disable authentication completely.And that's why this discussion needs to happen. A notice isn't going
to change anything. A significant number of developers turn them off.
Especially in production (which is where long passwords are likely to
occur). It may help a few people, but the majority it wouldn't. And it
would make some people start calling@password_hash()to suppress
the notice (because why bother fixing bugs, when we can ignore them!).
Which is really bad.If you truly want to prevent people doing silly things like
password_hash(SHA512($secret) . $password), then returning false and
raising a warning is the only way that will actually do so.How to do so is tricky, as it's a pretty significant BC break, and
will break some running software. But it would prevent the issue that
you've stated you want to prevent.So that leaves 2 practical options (IMHO):
- Document the behavior, and don't change code functionality.
- Add warning, returning false on overlong password (with documentation).
I don't like 2 (for a similar reason as you), so that leaves 1. Which
is where we are today, so nothing left to do.
OK. I'll add option keep current behavior. (No errors)
If we would like to recommend "Just use it", we may consider adding
SHA512
topassword_hash().As I pointed out above, no.
I'm lost here. Are you going to discuss use of SHA512 for password
hashing
is
insecure? Blowfish is suitable for password hashing because of its
"slowness".
Isn't it matter of rounds?I assumed that you meant bcrypt(sha512(password)). If you mean
crypt-sha512 (aka $6$), why would we want to add a demonstrably less
secure algorithm?
I meant "crypt-sha512 (aka $6$)". Sorry that it wasn't clear.
It's less secure for sure. However, it's considered good enough for
practical
security. Since it is less secure, it would not be PASSWORD_DEFAULT and
should be used only when users require it to satisfy application
requirement.
The reason why I would like to have "crypt-sha512" is to educate average
PHP users "Just use password_*()". "If your password is less than 72 bytes,
use PASSWORD_BCRYPT. If it is longer than 72 bytes for reasons, use
PASSWORD_SHA512 until next standard crypt() algorithm that removes
length limit."
Although, you would not like to have it, if we have this what would be the
suggested
rounds? (for now. It would be changed as time goes by)
If anything, I'd add PBKDF2-SHA512, but considering there are no
public crypt bindings to PBKDF2 (aka a crypt format), I'm really
hesitant to do that.Yes, Python has one (a crypt format):
https://www.dlitz.net/software/python-pbkdf2/
Yes, PERL has one:
http://search.cpan.org/~arodland/Crypt-PBKDF2-0.101170/lib/Crypt/PBKDF2.pm
Yes, Ruby has one: https://github.com/nomoon/pbkdf2-cryptBut note none of those are in core distributions. And all of them are
different (incompatible with one another).
Adding PBKDF2-SHA512 to crypt()/password_*() wouldn't be a good idea
for reasons you describe. We may recommend(document) alternative, when
it is needed by users/developers.
http://jp1.php.net/manual/en/function.openssl-pbkdf2.php
http://jp1.php.net/manual/en/function.hash-pbkdf2.php
Problem is it's not "password_*()", but recommending possible choice would
be acceptable.
Suggesting users to use strongest hash algo and proper iterations with
these function would
be enough. It seems current document is missing "User should use hash
stronger than SHA-2
to be considered secure", "$iterations must be at least 2000" (IIRC, it was
2000 in NIST
document. A few years have passed since I read it and my memory is vague.
Is this valid
still in their document? 2000 would be too small now a days.)
1password is using PBKDF2 SHA512 with 10 thousands rounds.
http://blog.agilebits.com/2014/03/10/crackers-report-great-news-for-1password-4/
LastPass is using PBKDF2 SHA-1 with 5 thousands rounds. (Weak to GPU
crack...)
https://helpdesk.lastpass.com/security-options/password-iterations-pbkdf2/
Even if user uses them, they may migrate their password database to
password_*() or other
new standards with timestamping and few lines of additional codes in the
future.
Baseline for recommendation would be SHA-512 with at least 10,000 rounds.
This might be
too slow for embedded environment, though. Do you have suggestion?
We need 72 bytes limit workaround/solution some how, but it seems you are
objecting possible choices. Most developers need adequate level of
security
to
build applications, not perfect security. Not all developers have right
to
decide
maximum password length and how password is hashed. (e.g. Adding secret
salt
is
still valid protection against stolen password database via SQL
injection,etc.
Some organizations require to add custom static slat to user defined
password.)I am objecting to the possible choices, because none of them are good
enough to offset the risk. The risk associated with overlong passwords
is trivially small. The risk to "inventing crypto" (such as with
crypt(sha512(password))) or weakening the case for the rest of
passwords (such as with crypt-sha512) is significantly greater IMHO
then the case where a user's password exceeds 72 characters. Yes, we
could add a notice, but it wouldn't really help the class of users
you're intending it to. And it may wind up hurting some by masking
more serious errors (when the suppress with @).So I stick with the only good option that IMHO doesn't hurt users,
which is documentation.
But again, this is why I asked for the RFC. My opinion isn't the only
one that matters. I care about it significantly, and I don't want to
see users hurt. But ultimately there are far more people involved than
just you and me. Putting together an RFC will let the discussion
happen, each of us make our case, and let the community decide.
Even if I prefer to have E_NOTICE error (since I think it's our
responsibility
for PHP users), resolving this issue as documentation problem is
acceptable choice for me also as long as it is precise and users
could find out what they should do to adopt their application requirements.
I'm not sure your opinions for
password_hash()truncation behavior- What to do to accept password longer than 72 bytes
Could you give your opinion for these?
My opinions (which I've stated above, but will summarize here):
Truncation behavior: document it. That should be all that's needed.
What to do to accept passwords longer than 72 bytes: Either put an
upper cap on password length in the application, or don't worry about
it. The chances the first 72 bytes can be guessed, but a further one
is different is EXTREMELY small (you're more likely to win a lottery's
jackpot 5 times, than to do so, based on a quick back-of-envelope
estimation).Yes, ideally a hash function would allow for infinitely long
(practically, 2^32-1 would be enough, 2^64-1 would be better)
passwords. But it's not worth weakening a system to defend against it
(by moving to crypt-sha512). And it's not worth suggesting invented
crypto to defend against it (recommending crypt(sha512(password))).My recommendation: deal with it today. When scrypt (or the winner of
the current PHC) gets crypt(3) bindings, then we'll pull it in and
make it available topassword_hash(). Until then, it's not a
significant enough risk to worry about...
I understood your point of view. I'll write it in upcoming RFC. Please feel
free
to edit it when it is available.
My point of view is "Crypt related document/feature should not be cryptic.
It should be easy to find out what is happening(i.e. truncation), what
users
should do for their requirements."
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
crypt()has BC issue with older systems.https://bugs.php.net/bug.php?id=62372&edit=1
The reason rounds became 1000 from 10 is hardcoded lower limit for newer
PHPs.
Generally speaking, developer should never use less than 1000 rounds and
better to have
at least few thousands rounds or more, tens of thousands or more is
recommended.I would like to make this bug report 'wont fix', since migration is
possible.
- Developer may use larger rounds and store updated hash when
user is authenticated with old PHP.- Developer may ask users to reset password if password hash has
to fewer rounds than 1000 (i.e. outdated hash) with new PHP.Any comments?
I'm not going to touch crypt(), but password_hash() behavior that truncates
byte larger than 72
silently is not good.
I made a bug report for this to add E_NOTICE.
https://bugs.php.net/bug.php?id=67653
Any comments? If not, I'll add E_NOTICE in a few days.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
crypt()has BC issue with older systems.https://bugs.php.net/bug.php?id=62372&edit=1
The reason rounds became 1000 from 10 is hardcoded lower limit for newer
PHPs.
Generally speaking, developer should never use less than 1000 rounds and
better to have
at least few thousands rounds or more, tens of thousands or more is
recommended.I would like to make this bug report 'wont fix', since migration is
possible.
- Developer may use larger rounds and store updated hash when
user is authenticated with old PHP.- Developer may ask users to reset password if password hash has
to fewer rounds than 1000 (i.e. outdated hash) with new PHP.Any comments?
I'm not going to touch
crypt(), butpassword_hash()behavior that truncates
byte larger than 72
silently is not good.I made a bug report for this to add E_NOTICE.
https://bugs.php.net/bug.php?id=67653Any comments? If not, I'll add
E_NOTICEin a few days.
I'm against adding this notice to password_hash. This will require all
applications to ensure that passwords are shorter than 72 chars. I don't
think that's a good idea.
Nikita
Hi Nikita,
I'm against adding this notice to password_hash. This will require all
applications to ensure that passwords are shorter than 72 chars. I don't
think that's a good idea.
Generally speaking, it would not be serious issue. 72 bytes constant prefix
would
not be used most likely.
However, bug like this in "authentication" code must be detected and
fixed.
If password should be truncated, it should be truncated by app developers
explicitly and
notified users that their password had been truncated. IMHO.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Nikita,
On Sat, Jul 19, 2014 at 2:46 PM, Nikita Popov nikita.ppv@gmail.com
wrote:I'm against adding this notice to password_hash. This will require all
applications to ensure that passwords are shorter than 72 chars. I don't
think that's a good idea.Generally speaking, it would not be serious issue. 72 bytes constant
prefix
would
not be used most likely.However, bug like this in "authentication" code must be detected and
fixed.
If password should be truncated, it should be truncated by app developers
explicitly and
notified users that their password had been truncated. IMHO.Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
There's already a notice about this in the password_hash() docs, one that
almost looks like is designed to scare users, which is bad. Throwing an
E_NOTICE will cause more problems than it would supposedly solve.
Application developers should just state this limitation on their
registration/password change pages, anything else is pointless.
Cheers,
Andrey.