[DRAFT RFC] Adding Simplified Password Hashing API

13 years ago by Simon Schick — view source

unread

Hi, Anthony

Some questions coming up in my mind by reading this RFC:

Will the value of the constant PASSWORD_DEFAULT remain unchanged
forever? Otherwise this lib, in my opinion, can cause big problems when
trying to port an existing system to a newer PHP-version.
Is this a native version of phpass? http://www.openwall.com/phpass/

Sorry if those things have already been discussed somewhere. I was not
actively following this list in the last weeks ;)

Bye
Simon

On Tue, Jun 26, 2012 at 5:25 PM, Anthony Ferrara ircmaxell@gmail.comwrote:

Hello All,

I've taken the conversation of the previous simplified password
hashing API, and generated a patch and draft RFC for it. The patch
isn't ready yet (needs review, cleanup and testing), but it's a start.

https://wiki.php.net/rfc/password_hash

Please have a look and comment away!

Thanks,

Anthony

13 years ago by Pierre Joye — view source

unread

hi!

Hi, Anthony

Some questions coming up in my mind by reading this RFC:

Will the value of the constant PASSWORD_DEFAULT remain unchanged
forever? Otherwise this lib, in my opinion, can cause big problems when
trying to port an existing system to a newer PHP-version.

I won't set allow any default. This can't stay unchanged.

Is this a native version of phpass? http://www.openwall.com/phpass/

Cheers,

Pierre

@pierrejoye

13 years ago by Anthony Ferrara — view source

unread

Simon,

Will the value of the constant PASSWORD_DEFAULT remain unchanged forever?
Otherwise this lib, in my opinion, can cause big problems when trying to
port an existing system to a newer PHP-version.

No. That's why it's a separate constant. As newer, stronger hashing
options become available, the default is designed to change over time.
I'll update the RFC to indicate such.

Is this a native version of phpass? http://www.openwall.com/phpass/

In a sense, yes. It's designed to have a dirt-simple API (similar to
yours) built in to the core.

Thanks,

Anthony

13 years ago by Pierre Joye — view source

unread

hi,

Simon,

Will the value of the constant PASSWORD_DEFAULT remain unchanged forever?
Otherwise this lib, in my opinion, can cause big problems when trying to
port an existing system to a newer PHP-version.

the default is designed to change over time.

That's exactly what I meant, having a changing default in this may
force code change during php updates. I'm not in favour of having such
default.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Gustavo Lopes — view source

unread

Em Wed, 27 Jun 2012 13:37:50 +0200, Pierre Joye pierre.php@gmail.com
escreveu:

That's exactly what I meant, having a changing default in this may
force code change during php updates. I'm not in favour of having such
default.

This would not require any code changes after updates.

As I understand, hashes computed with the old default method could still
be checked without any modification as the hash itself stores information
about the method.

--
Gustavo Lopes

13 years ago by Pierre Joye — view source

unread

hi,

Em Wed, 27 Jun 2012 13:37:50 +0200, Pierre Joye pierre.php@gmail.com
escreveu:

That's exactly what I meant, having a changing default in this may
force code change during php updates. I'm not in favour of having such
default.

This would not require any code changes after updates.

As I understand, hashes computed with the old default method could still be
checked without any modification as the hash itself stores information about
the method.

That's only about one relatively simple use case where only PHP would
be involved or crypt-like implemenation. For any other and rather
common cases, it won't. I do not think a default should be implemented
and actually let the user knows what he uses and what he is doing.
That's one argument after all and clears all possible caveats.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Anthony Ferrara — view source

unread

Pierre,

As I understand, hashes computed with the old default method could still be
checked without any modification as the hash itself stores information about
the method.

That's only about one relatively simple use case where only PHP would
be involved or crypt-like implemenation. For any other and rather
common cases, it won't. I do not think a default should be implemented
and actually let the user knows what he uses and what he is doing.
That's one argument after all and clears all possible caveats.

Well, the argument could be made that if you need portability in that
respect, that you should be using crypt() or the other library
directly. This API is designed for the common use-case that impacts
99.9% of developers, where their applications will be the only ones
accessing the passwords. And even if in the future they need to access
it from a different system entirely (python for example), we're only
implementing standard algorithms. So there should be a python binding
available to verify (and hash) those passwords.

This won't take the place of crypt() or other libraries. It's only
there to solve the lowest common denominator in a dirt simple way. I
think removing the default (and forcing an algorithm specification)
kind-of defeats that point a little bit. It's not the end of the
world, and I could possibly be convinced, but I'm skeptical.

As for the issue you raised, I think that could be handled in the
documentation. Perhaps something to the effect of:

PASSWORD_DEFAULT - This is the default algorithm that password_hash will use if none is specified. Note that this is designed to change over time as newer and stronger algorithms are implemented. If you need to stay with a single algorithm (for portability or other reasons), it's recommended to always specify the algorithm in the function call.

Actually, now that I'm talking that out, perhaps the way to do it
would be to specify the default algorithm in a php.ini parameter
instead of the constant? That way the API can stay the same, but gives
people more control over the default creation... Then again, maybe
not.

Thoughts?

Anthony

13 years ago by Gustavo Lopes — view source

unread

Em Wed, 27 Jun 2012 14:24:39 +0200, Anthony Ferrara ircmaxell@gmail.com
escreveu:

Actually, now that I'm talking that out, perhaps the way to do it
would be to specify the default algorithm in a php.ini parameter
instead of the constant? That way the API can stay the same, but gives
people more control over the default creation... Then again, maybe
not.

Thoughts?

I don't see any advantage in adding complexity through another level of
indirection. If people want control over the default their application
uses, they can just use a constant they define.

That said, I think the default algorithm should provide sufficient
guarantees to enable it to be used in a forward compatible fashion. For
instance, if the default hash at one point consumes n bytes, then it may
be backwards incompatible to change to use more than n bytes as at that
point you may need a larger database field. So it should be documented
with future changes in mind.

--
Gustavo Lopes

13 years ago by Pierre Joye — view source

unread

hi,

Em Wed, 27 Jun 2012 14:24:39 +0200, Anthony Ferrara ircmaxell@gmail.com
escreveu:

Actually, now that I'm talking that out, perhaps the way to do it
would be to specify the default algorithm in a php.ini parameter
instead of the constant? That way the API can stay the same, but gives
people more control over the default creation... Then again, maybe
not.

Thoughts?

I don't see any advantage in adding complexity through another level of
indirection. If people want control over the default their application uses,
they can just use a constant they define.

And people will have to, as I described it earlier, and see below.

That said, I think the default algorithm should provide sufficient
guarantees to enable it to be used in a forward compatible fashion.

Back then MD5 alone was all nice and shiny. So no, it is not possible
to be forward compatible.

For
instance, if the default hash at one point consumes n bytes, then it may be
backwards incompatible to change to use more than n bytes as at that point
you may need a larger database field. So it should be documented with future

It is not about size but ability to use the password across many
applications. The days were only PHP were involved are behind us. yes,
crypt may (in some extend) allows that, but this RFC purpose is to
replace it, for a more developer friendly API.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Gustavo Lopes — view source

unread

Em Wed, 27 Jun 2012 14:43:35 +0200, Pierre Joye pierre.php@gmail.com
escreveu:

On Wed, Jun 27, 2012 at 2:32 PM, Gustavo Lopes glopes@nebm.ist.utl.pt
wrote:

Em Wed, 27 Jun 2012 14:24:39 +0200, Anthony Ferrara
ircmaxell@gmail.com escreveu:

I don't see any advantage in adding complexity through another level of
indirection. If people want control over the default their application
uses, they can just use a constant they define.

And people will have to, as I described it earlier, and see below.

You described why people may have to, depending on the circumstances --
for instance, when interoperability in mixed environments is required. No
one is saying that relying on a default value is appropriate in those
circumstances, so this argument misses the mark.

That said, I think the default algorithm should provide sufficient
guarantees to enable it to be used in a forward compatible fashion.

Back then MD5 alone was all nice and shiny. So no, it is not possible
to be forward compatible.

If this API existed 10 or more years ago and used MD5 as a default, I
don't see how it could not be used in a forward compatible manner back
then -- seen from the outside there's nothing different about MD5 or other
digest method except for different parameters (which can be stored
together with the salt and the method in the result of password_hash())
and digest size. And, unsurprisingly, you have no justification on why it
could not be made forward compatible.

--
Gustavo Lopes

13 years ago by Pierre Joye — view source

unread

hi,

You described why people may have to, depending on the circumstances --
for instance, when interoperability in mixed environments is required. No
one is saying that relying on a default value is appropriate in those
circumstances, so this argument misses the mark.

No, it is exactly one example out of many where changing values are a
real pain to deal with over the years. We should not have one.

If this API existed 10 or more years ago and used MD5 as a default, I don't
see how it could not be used in a forward compatible manner back then --
seen from the outside there's nothing different about MD5 or other digest
method except for different parameters (which can be stored together with
the salt and the method in the result of password_hash()) and digest size.
And, unsurprisingly, you have no justification on why it could not be made
forward compatible.

Changing default value forces code change if you have to keep a given
hash, for one obvious side effect.

If you disagree or does not like the idea, that's all fine, but you
can't really say that it is not an argument (nothing to justify, this
is a draft and it is being discussed).

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by keisial@gmail.com — view source

unread

Changing default value forces code change if you have to keep a given
hash, for one obvious side effect.

If you disagree or does not like the idea, that's all fine, but you
can't really say that it is not an argument (nothing to justify, this
is a draft and it is being discussed).

Cheers,
Precisely the point of such constant is to allow the applications to
magically
change to use more secure hashes, without needing to parform a recursive
sed in the codebase to change HASH_SHA2015 with HASH_SHA2048.
If you want to be in precise control of the actual hash used in a newer
version
(such as an hereogeneous deployment), you could set it to the lower
denominator
in php.ini.

Obviously, any such bump -which I would expect to happen on major releases-
would hold an entry in the NEWS file explaining that PASSWORD_DEFAULT_HASH
is now md5 by default instead of crc32 (still, I would expect a new hash
to have
been available on several releases -they could easily be added on minor
ones-
before becoming the PASSWORD_DEFAULT_HASH).

Remember that the goal was to make the next-generation password hasing api.
An (almost) foolproof way to make the applications secure. If you expect
them to
timely realise the problems of md5() and go back to change all their
functions,
you will replace the current function with password_hash('password',
SILLY_HASH, ...).

Developers with higher security knowledge (few of them, you'd almost
need to be a
cryptographer yourself) can use the advanced parameters to tweak it to
their needs.

Regards

13 years ago by Pierre Joye — view source

unread

hi,

Precisely the point of such constant is to allow the applications to
magically

Right, but not a default argument, which is bad in this case, for the
reasons explained earlier.

Obviously, any such bump -which I would expect to happen on major releases-
would hold an entry in the NEWS file explaining that PASSWORD_DEFAULT_HASH

I have no problem with such constant and let the user uses it instead
of a given algo. But then he will do it on purpose and being well
informed about the implications.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Anthony Ferrara — view source

unread

Pierre,

No, it is exactly one example out of many where changing values are a
real pain to deal with over the years. We should not have one.

While I completely see your point (and don't disagree with it in
isolation), I also see the counter point of making it easy for people
to use. Knowing anything about algorithms to force the common
developer to make a choice between bcrypt and scrypt is unreasonable
IMHO. It's an implementation detail that they should know, but most
won't. Knowing the intricacies of the different algorithms is
something even most senior devs (who are not active in security at
least) don't understand. I'd rather present them best-case defaults,
and let them make the decision to diverge from them.

With that said, what about a compromise. What if we made the API:

password_hash($password, $algo, array $options = array())

And instead of just making the users choose which algorithm to use, we
provide a "moving" constant called

PASSWORD_MOST_SECURE

Which will be updated every major release (assuming there's an update
to apply) or in extreme circumstances (a serious flaw is found in the
current most secure algorithm, justifying a concern).

That way, people don't have to worry about moving targets, because the
core moves it for them as needed. But the choice has to be made. They
aren't just relying upon the default. And the documentation
surrounding it must indicate that if cross-platform interoperability
is a concern, pick a standard algorithm such as bcrypt and use just
that.

So then, a basic call would be $hash = password_hash($password,
PASSWORD_MOST_SECURE);

It solves both problems, while still being reasonably easy...

Thoughts?

Anthony

13 years ago by Anthony Ferrara — view source

unread

Pierre,

Back then MD5 alone was all nice and shiny. So no, it is not possible
to be forward compatible.

By forward compatible, if you mean able to support any new algo, I
think this is forward compatible. The options array allows for new
implementations to implement whatever options they need. If you mean
100% compatibility, then no. That's not possible (due to storage
requirements, etc). But the API would stay the same...

For
instance, if the default hash at one point consumes n bytes, then it may be
backwards incompatible to change to use more than n bytes as at that point
you may need a larger database field. So it should be documented with future

It is not about size but ability to use the password across many
applications. The days were only PHP were involved are behind us. yes,
crypt may (in some extend) allows that, but this RFC purpose is to
replace it, for a more developer friendly API.

This RFC does not intend to replace crypt(). It is intended to be a
reasonably thin wrapper around it to make it easier to use for the
average use case. Crypt() will still be there, and will still be
encouraged for the use-cases that it makes sense for (portability,
etc). This just attempts to solve the problem for the vast majority of
users.

In fact, the exposed password_make_salt() should make it easier for
developers to use crypt:

$hash = crypt($pass, "$6$" . password_make_salt(16));

Thanks,

Anthony

13 years ago by Arvids Godjuks — view source

unread

Hello.

I personally think that using PASSWORD_DEFAULT for algorythm by default is
a bad idea. This should be defined by user in the code. Even worse if it is
defined by .ini setting - deploy to a remote server and realize that there
is a different .ini default that messes up everything. Lessons learned in
the past are forgetten fast?

And the thing I don't get is how do I verify a salted password? I have read
throught the RFC and what I know about the salts makes me wonder - how da
hell I will verify my salted hash if I can't pass the salt to
password_verify?

If there is some trick behind, it should be explained in the RFC (and in
the docs later, because otherwise it makes people WTF?! who are not into
cryptography).

Arvids.

13 years ago by Anthony Ferrara — view source

unread

Arvids,

On Wed, Jun 27, 2012 at 9:23 AM, Arvids Godjuks
arvids.godjuks@gmail.com wrote:

Hello.

I personally think that using PASSWORD_DEFAULT for algorythm by default is a
bad idea. This should be defined by user in the code. Even worse if it is
defined by .ini setting - deploy to a remote server and realize that there
is a different .ini default that messes up everything. Lessons learned in
the past are forgetten fast?

It wouldn't mess up anything. All it would do is change the algorithm
used by the library when creating new passwords. Existing ones will
still validate. The new ones will validate on the old server as long
as that algorithm is supported (could be an issue in a mixed
environment where there are servers using an older version without
support for the new method in crypt())...

And the thing I don't get is how do I verify a salted password? I have read
throught the RFC and what I know about the salts makes me wonder - how da
hell I will verify my salted hash if I can't pass the salt to
password_verify?

Ah, I think I see the disconnect. crypt() returns the full salt
information along with everything necessary to hash it (all settings).
So the generated hash includes the salt, the method, and the cost
parameter. For example:

var_dump(crypt("rasmuslerdorf", "$2a$07$usesomesillystringfor"));

string(60) "$2a$07$usesomesillystringfore2uDLvp1Ii2e./U9C8sBjqp8I90dH6hi"

So just storing the hash is enough...

If there is some trick behind, it should be explained in the RFC (and in the
docs later, because otherwise it makes people WTF?! who are not into
cryptography).

That's completely fair. I'll add a section to the RFC about that...

Thanks,

Anthony

13 years ago by Arvids Godjuks — view source

unread

On that note I have only one request - please point me to the good article
that describes how this thing works (I would prefer one that at least tries
to explain in simple words) because at the moment i do not understand how
salt stored in the hash itself makes hash more secure than an unsalted one.

Thank you :-)
27.06.2012 14:16 пользователь "Anthony Ferrara" ircmaxell@gmail.com
написал:

Arvids,

On Wed, Jun 27, 2012 at 9:23 AM, Arvids Godjuks
arvids.godjuks@gmail.com wrote:

Hello.

I personally think that using PASSWORD_DEFAULT for algorythm by default
is a
bad idea. This should be defined by user in the code. Even worse if it is
defined by .ini setting - deploy to a remote server and realize that
there
is a different .ini default that messes up everything. Lessons learned in
the past are forgetten fast?

It wouldn't mess up anything. All it would do is change the algorithm
used by the library when creating new passwords. Existing ones will
still validate. The new ones will validate on the old server as long
as that algorithm is supported (could be an issue in a mixed
environment where there are servers using an older version without
support for the new method in crypt())...

And the thing I don't get is how do I verify a salted password? I have
read
throught the RFC and what I know about the salts makes me wonder - how da
hell I will verify my salted hash if I can't pass the salt to
password_verify?

Ah, I think I see the disconnect. crypt() returns the full salt
information along with everything necessary to hash it (all settings).
So the generated hash includes the salt, the method, and the cost
parameter. For example:

var_dump(crypt("rasmuslerdorf", "$2a$07$usesomesillystringfor"));

string(60) "$2a$07$usesomesillystringfore2uDLvp1Ii2e./U9C8sBjqp8I90dH6hi"

So just storing the hash is enough...

If there is some trick behind, it should be explained in the RFC (and in
the
docs later, because otherwise it makes people WTF?! who are not into
cryptography).

That's completely fair. I'll add a section to the RFC about that...

Thanks,

Anthony

13 years ago by Anthony Ferrara — view source

unread

Arvids,

On Wed, Jun 27, 2012 at 12:32 PM, Arvids Godjuks
arvids.godjuks@gmail.com wrote:

On that note I have only one request - please point me to the good article
that describes how this thing works (I would prefer one that at least tries
to explain in simple words) because at the moment i do not understand how
salt stored in the hash itself makes hash more secure than an unsalted one.

Here are some articles that are worth while:

http://php.net/manual/en/function.crypt.php
http://www.devshed.com/c/a/PHP/Using-the-PHP-Crypt-Function/
http://stackoverflow.com/a/4808616/338665

If more explanation is needed, I can try to expand the RFC a bit more.
But give the new sections a read and let me know what you think.

Thanks,

Anthony

12 years ago by Jonathan Bond-Caron — view source

unread

because at the moment i do not understand how salt stored in the hash itself makes hash more
secure than an unsalted one.

a) In terms of 'effort' to break many passwords, there's a benefit to the salt stored in the hash itself.
It's not 'more secure' but 'better/recommended' since the attacker would need to create a 'rainbow table' for each password it's trying to crack
Overall, the technique offers better protection.

b) In terms of 'effort' to break a single password, there's no benefit to the salt stored in the hash itself.

If you want a single password to be really secure, don't let the attacker know the salt and keep it long:

// no benefit of short salt, ~ same effort required by the attacker
$password = '234';
md5($password);

$salt = '1';
$password = '234';
md5($salt . $password);

c) The best of both worlds: long private salt (b) + different for every user (a)
$saltInpassword = $password[0]; // could be random bytes, stored in password like crypt() does
$salt = 'my-long-private-value-use-all-bytes'. $saltInPassword;
$password = '234';
$hash = md5($salt . $password);

This one requires more effort by the attacker since the long salt forces more 'bits/guesses' to pass into md5()

// require even more effort, iterate
for($i = 0; $i < 1000; $i++)
$hash = md5($i . $hash);

12 years ago by Pierre Joye — view source

unread

hi,

a) In terms of 'effort' to break many passwords, there's a benefit to the salt stored in the hash itself.
It's not 'more secure' but 'better/recommended' since the attacker would need to create a 'rainbow table' for each password it's trying to crack
Overall, the technique offers better protection.

b) In terms of 'effort' to break a single password, there's no benefit to the salt stored in the hash itself.

If you want a single password to be really secure, don't let the attacker know the salt and keep it long:

// no benefit of short salt, ~ same effort required by the attacker
$password = '234';
md5($password);

$salt = '1';
$password = '234';
md5($salt . $password);

c) The best of both worlds: long private salt (b) + different for every user (a)
$saltInpassword = $password[0]; // could be random bytes, stored in password like crypt() does
$salt = 'my-long-private-value-use-all-bytes'. $saltInPassword;
$password = '234';
$hash = md5($salt . $password);

This one requires more effort by the attacker since the long salt forces more 'bits/guesses' to pass into md5()

// require even more effort, iterate
for($i = 0; $i < 1000; $i++)
$hash = md5($i . $hash);

This is somehow the 1st implementation (part of) of crypt. See
ext/standard for the full code. And md5 is now known to do not be
secure enough. IIRC.

I would not, but really totally not, begin to try to implement our own
little algorithm but rely on standard well tested implementations.
Crypt support blowfish, for example. Anthony also works on supporting
more algos afair.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Christopher Jones — view source

unread

Arvids,

On Wed, Jun 27, 2012 at 9:23 AM, Arvids Godjuks
arvids.godjuks@gmail.com wrote:

Hello.

I personally think that using PASSWORD_DEFAULT for algorythm by default is a
bad idea. This should be defined by user in the code. Even worse if it is
defined by .ini setting - deploy to a remote server and realize that there
is a different .ini default that messes up everything. Lessons learned in
the past are forgetten fast?

It wouldn't mess up anything. All it would do is change the algorithm
used by the library when creating new passwords. Existing ones will
still validate. The new ones will validate on the old server as long
as that algorithm is supported (could be an issue in a mixed
environment where there are servers using an older version without
support for the new method in crypt())...

Hi Anthony,

Can you update the RFC (aka future documentation) and make this obvious
to an end user?

Chris

--
christopher.jones@oracle.com
http://twitter.com/#!/ghrd

13 years ago by Anthony Ferrara — view source

unread

Pierre,

Getting back to the PASSWORD_DEFAULT discussion...

I know you didn't like PASSWORD_MOST_SECURE. So what about keeping
PASSWORD_DEFAULT as a moving target, documented, and just making the
second parameter (algo) to password_hash required? That way users
could choose between PASSWORD_BCRYPT and PASSWORD_DEFAULT.

That way, over time, PASSWORD_DEFAULT could be updated, and it would
be documented that it would change. But it would require them to
understand that it could change...

Would that satisfy your issues?

Thanks,

Anthony

hi,

Em Wed, 27 Jun 2012 13:37:50 +0200, Pierre Joye pierre.php@gmail.com
escreveu:

That's exactly what I meant, having a changing default in this may
force code change during php updates. I'm not in favour of having such
default.

This would not require any code changes after updates.

As I understand, hashes computed with the old default method could still be
checked without any modification as the hash itself stores information about
the method.

That's only about one relatively simple use case where only PHP would
be involved or crypt-like implemenation. For any other and rather
common cases, it won't. I do not think a default should be implemented
and actually let the user knows what he uses and what he is doing.
That's one argument after all and clears all possible caveats.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Pierre Joye — view source

unread

hi Anthony,

Pierre,

Getting back to the PASSWORD_DEFAULT discussion...

I know you didn't like PASSWORD_MOST_SECURE. So what about keeping
PASSWORD_DEFAULT as a moving target, documented, and just making the
second parameter (algo) to password_hash required? That way users
could choose between PASSWORD_BCRYPT and PASSWORD_DEFAULT.

That way, over time, PASSWORD_DEFAULT could be updated, and it would
be documented that it would change. But it would require them to
understand that it could change...

Would that satisfy your issues?

Yes.

Using this constant name and clearly document its changing nature is
fine. The argument being required fully solves my worry about optional
argument with changing default value.

Thanks for your efforts and work!

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Anthony Ferrara — view source

unread

Pierre,

I know you didn't like PASSWORD_MOST_SECURE. So what about keeping
PASSWORD_DEFAULT as a moving target, documented, and just making the
second parameter (algo) to password_hash required? That way users
could choose between PASSWORD_BCRYPT and PASSWORD_DEFAULT.

That way, over time, PASSWORD_DEFAULT could be updated, and it would
be documented that it would change. But it would require them to
understand that it could change...

Would that satisfy your issues?

Yes.

Using this constant name and clearly document its changing nature is
fine. The argument being required fully solves my worry about optional
argument with changing default value.

I've implemented this in my branch. I've also updated the RFC to
indicate such (we may want to expand it a little bit, but it should
suffice for now).

I've also added a bit to the RFC about a policy for updating the
default constant over time. (Indicating for a non-security release,
changing the default must be done via an RFC, how long the algorithm
must be available before being eligible for default, etc).

Thanks for your efforts and work!

Absolutely!

Thanks,

Anthony

13 years ago by johannes@schlueters.de — view source

unread

Hi,

https://wiki.php.net/rfc/password_hash

Some comments on the "error behavior" part:

`E_WARNING` - When CRYPT is not included in core (was disabled
compile-time, or is listed in disabled_functions declaration)

Disabling a different function should have no effect. This is not
intuitive. If crypt is a dependency and is not available this function
shouldn't be available either.

`E_WARNING` - When supplied an incorrect number of arguments.
`E_WARNING` - When supplied a non-string first parameter (password)

This should follow common semantics of zend_parse_parameters(... "s").
i.e. it has to support objects with __toString(). Also other scalars are
fine. (if they can be casted to string)

`E_WARNING` - If a non-string salt option is provided

As above.

If any error is raise, false is returned by the function.

In http://de.php.net/functions.internal it is documented that internal
functions return NULL on error during parameter parsing. New exceptions
for that should have a good reason.

These things are all minor and you might consider them bad, but then
change it globally, not by adding new inconsistencies.

johannes

13 years ago by Anthony Ferrara — view source

unread

Johannes,

Some comments on the "error behavior" part:

E_WARNING - When CRYPT is not included in core (was disabled
compile-time, or is listed in disabled_functions declaration)

Disabling a different function should have no effect. This is not
intuitive. If crypt is a dependency and is not available this function
shouldn't be available either.

Hrm. Well, then I guess I could re-implement against crypt internally.
That would take either a slight re-implementation of the crypt()
internals, or slight refactoring of the PHP_FUNCTION(crypt) function
to enable c calls to it (even if it's disabled).

I don't like the concept of core functions disappearing if they are
not implemented. I think that does nothing but make it harder on the
developers (now having to inject a function_exists(), etc).
Additionally, since this is a security issue, I think that always
defining the function is the better approach. Otherwise, you can wind
up in a situation where someone else comes in and implements function
password_verify($password, $hash) { return true; }, which would be all
sorts of bad... I can see the static linking to the function (instead
of the dynamic call that's there), So in this case, I personally think
the warning is appropriate.

E_WARNING - When supplied an incorrect number of arguments.
E_WARNING - When supplied a non-string first parameter (password)

This should follow common semantics of zend_parse_parameters(... "s").
i.e. it has to support objects with __toString(). Also other scalars are
fine. (if they can be casted to string)

E_WARNING - If a non-string salt option is provided

As above.

Yeah, I guess that's fair. Is there a macro or function like
Z_REPRESENTABLE_AS_STRING_P() or something? To make it easier to
check?

If any error is raise, false is returned by the function.

In http://de.php.net/functions.internal it is documented that internal
functions return NULL on error during parameter parsing. New exceptions
for that should have a good reason.

The good reason is consistency. Otherwise there would be three return
values, null, false and true for password_verify(). Therefore, a
check of false === password_verify($foo) would actually fail
inadvertantly.

My opinion is that it should do what's appropriate. The other 2 I can
live with returning null (although I disagree with it significantly),
but password_verify I think really should only ever return a
boolean...

These things are all minor and you might consider them bad, but then
change it globally, not by adding new inconsistencies.

Fair enough...

Thanks,

Anthony

13 years ago by Pierre Joye — view source

unread

Hi Anthony!

Hrm. Well, then I guess I could re-implement against crypt internally.
That would take either a slight re-implementation of the crypt()
internals, or slight refactoring of the PHP_FUNCTION(crypt) function
to enable c calls to it (even if it's disabled).

We would have to expose the crypt implementation, it is not the case
right now. But you could still use it using func call.

Let me know if you need to expose it and which parts, we will need to
refactor it a bit (which should be a good thing to do anyway).

I don't like the concept of core functions disappearing if they are
not implemented.

Well, that's the case for extensions altogether so... :)

However if I remember correctly, I made crypt always available from
php 5.3 and later, for all algorithms.

E_WARNING - When supplied an incorrect number of arguments.
E_WARNING - When supplied a non-string first parameter (password)

I would not use warnings at all but return false on error. Maybe add
an argument or a function to fetch the error messages.

In case you call other PHP functions, you can push/pop the errors
easily as well.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by johannes@schlueters.de — view source

unread

Johannes,

Some comments on the "error behavior" part:

E_WARNING - When CRYPT is not included in core (was disabled
compile-time, or is listed in disabled_functions declaration)

Disabling a different function should have no effect. This is not
intuitive. If crypt is a dependency and is not available this function
shouldn't be available either.

Hrm. Well, then I guess I could re-implement against crypt internally.
That would take either a slight re-implementation of the crypt()
internals, or slight refactoring of the PHP_FUNCTION(crypt) function
to enable c calls to it (even if it's disabled).

I haven't looked at your patch. But if it has to call another
PHP_FuNCTION then it's not good. crypt implementation should be
accessible via C.

I don't like the concept of core functions disappearing if they are
not implemented. I think that does nothing but make it harder on the
developers (now having to inject a function_exists(), etc).

I think it's rather the opposite. In that case the user has no way to
check whether the function is there without calling it and reacting to
errors. If the function "disappears" there is a possibility to check.

Additionally, since this is a security issue, I think that always
defining the function is the better approach. Otherwise, you can wind
up in a situation where someone else comes in and implements function
password_verify($password, $hash) { return true; }, which would be all
sorts of bad... I can see the static linking to the function (instead
of the dynamic call that's there), So in this case, I personally think
the warning is appropriate.

Who should be the bad person in that scenario? The developer himself? I
think even the security goes the other way round - deelopers don't do
error checking, so they store an empty password in case of error. If the
function does not exist and is being called the script temrinates and
nothing further ad happens.

E_WARNING - When supplied an incorrect number of arguments.
E_WARNING - When supplied a non-string first parameter (password)

This should follow common semantics of zend_parse_parameters(... "s").
i.e. it has to support objects with __toString(). Also other scalars are
fine. (if they can be casted to string)

E_WARNING - If a non-string salt option is provided

As above.

Yeah, I guess that's fair. Is there a macro or function like
Z_REPRESENTABLE_AS_STRING_P() or something? To make it easier to
check?

No, but a simple zend_parse_parameters with "s" modifier should be
enough?

If any error is raise, false is returned by the function.

In http://de.php.net/functions.internal it is documented that internal
functions return NULL on error during parameter parsing. New exceptions
for that should have a good reason.

The good reason is consistency. Otherwise there would be three return
values, null, false and true for password_verify(). Therefore, a
check of false === password_verify($foo) would actually fail
inadvertantly.

My opinion is that it should do what's appropriate. The other 2 I can
live with returning null (although I disagree with it significantly),
but password_verify I think really should only ever return a
boolean...

I don't see what makes password_verify special. If one wants a typesafe
check one can go for true === password_verify.

johannes

13 years ago by Anthony Ferrara — view source

unread

Johannes,

I haven't looked at your patch. But if it has to call another
PHP_FuNCTION then it's not good. crypt implementation should be
accessible via C.

I've refactored crypt() slightly to expose a PHP_API crypt_execute()
function that does just about everything except the argument parsing /
default randomizing.
https://github.com/ircmaxell/php-src/blob/hash_password/ext/standard/crypt.c

Now that I did that, I adjusted the implementation to call that instead...

I don't like the concept of core functions disappearing if they are
not implemented. I think that does nothing but make it harder on the
developers (now having to inject a function_exists(), etc).

I think it's rather the opposite. In that case the user has no way to
check whether the function is there without calling it and reacting to
errors. If the function "disappears" there is a possibility to check.

I've now based this implementation on HAVE_CRYPT. If that's not
defined, neither are these functions...

Additionally, since this is a security issue, I think that always
defining the function is the better approach. Otherwise, you can wind
up in a situation where someone else comes in and implements function
password_verify($password, $hash) { return true; }, which would be all
sorts of bad... I can see the static linking to the function (instead
of the dynamic call that's there), So in this case, I personally think
the warning is appropriate.

No, but a simple zend_parse_parameters with "s" modifier should be
enough?

Well, that was enough for the main parsing. But I had to do a little
bit of copy/paste for the argument array parsing (not comfortable with
it): https://github.com/ircmaxell/php-src/blob/hash_password/ext/standard/password.c#L262

I don't see what makes password_verify special. If one wants a typesafe
check one can go for true === password_verify.

I've changed all other parameter based error returns to NULL (even the
out of range checks). But I left password_verify for now as returning
false on any error. I still think this one case is significant enough
to always return false/true instead of a third parameter. But we can
talk about that more...

Thanks,

Anthony

13 years ago by Pierre Joye — view source

unread

hi Anthony,

I haven't looked at your patch. But if it has to call another
PHP_FuNCTION then it's not good. crypt implementation should be
accessible via C.

I've refactored crypt() slightly to expose a PHP_API crypt_execute()
function that does just about everything except the argument parsing /
default randomizing.
https://github.com/ircmaxell/php-src/blob/hash_password/ext/standard/crypt.c

It looks good. I would name it php_crypt instead. _execute means
there is a prepare (well, there is but it is called on init :).

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Christopher Jones — view source

unread

Hello All,

I've taken the conversation of the previous simplified password
hashing API, and generated a patch and draft RFC for it. The patch
isn't ready yet (needs review, cleanup and testing), but it's a start.

https://wiki.php.net/rfc/password_hash

Please have a look and comment away!

Thanks,

Anthony

Hi Anthony,

I think PASSWORD_BCRYPT should be an ordinal value, which the new
library maps to "2y" when bcrypt is called.

The API of password_make_salt() seems restrictive. What if other
options are needed in future?

Chris

--
christopher.jones@oracle.com
http://twitter.com/#!/ghrd

13 years ago by Anthony Ferrara — view source

unread

Chris,

Can you update the RFC (aka future documentation) and make this obvious
to an end user?

I just made an update (in the behavior sections). Let me know if
additional clarification is needed.

I think PASSWORD_BCRYPT should be an ordinal value, which the new
library maps to "2y" when bcrypt is called.

That would be fine. The initial goal for mapping the prefix to the
constant was to provide the ability to map hash prefixes to the
argument. That way, we could add user-supplied algorithms and base
everything off the prefix with no additional mapping needed. But now
that's off the table, I think switching back to an ordinal would be
fine (and would pretty up the code a bit)...

The API of password_make_salt() seems restrictive. What if other
options are needed in future?

Can you give any examples of what options would be needed in the
future, or how you would like to see the API?

Thanks for the feedback!

Anthony

13 years ago by Christopher Jones — view source

unread

Chris,

Can you update the RFC (aka future documentation) and make this obvious
to an end user?

I just made an update (in the behavior sections). Let me know if
additional clarification is needed.

To be honest, a note next to PASSWORD_DEFAULT would be good too.

The API of password_make_salt() seems restrictive. What if other
options are needed in future?

Can you give any examples of what options would be needed in the
future, or how you would like to see the API?

I only have brainstorm thoughts on this, since I don't have a crystal
ball. What if characters other than a-zA-Z0-9./ should/can be used
for some PASSWORD_xxx algorithms? What if some seed is needed? What
if the salt creation algorithm should be swappable due to resource
usage reasons, etc?

Also, do you really need a php.ini parameter? It's yet another
potential way to attack a system.

Chris

--
christopher.jones@oracle.com
http://twitter.com/#!/ghrd

13 years ago by Anthony Ferrara — view source

unread

Chris,

To be honest, a note next to PASSWORD_DEFAULT would be good too.

Ok, I'll add that in shortly.

The API of password_make_salt() seems restrictive. What if other
options are needed in future?

Can you give any examples of what options would be needed in the
future, or how you would like to see the API?

I only have brainstorm thoughts on this, since I don't have a crystal
ball. What if characters other than a-zA-Z0-9./ should/can be used
for some PASSWORD_xxx algorithms? What if some seed is needed? What
if the salt creation algorithm should be swappable due to resource
usage reasons, etc?

Actually... What about making the raw_output parameter a bitmask.
Then provide:

PASSWORD_SALT_CRYPT = 1
PASSWORD_SALT_RAW = 2

Then, in the future you could add a bunch of others PASSWORD_SALT_SOMETHINGELSE.

And you could combine some: PASSWORD_SALT_DEV_RANDOM, PASSWORD_SALT_WAHTEVER...

Also, do you really need a php.ini parameter? It's yet another
potential way to attack a system.

Well, if not for an ini parameter, what way would you suggest to alter
the default bcrypt cost? (seriously, I'm open to suggestions)...

Thanks,

Anthony

13 years ago by Pierre Joye — view source

unread

hi!

Well, if not for an ini parameter, what way would you suggest to alter
the default bcrypt cost? (seriously, I'm open to suggestions)...

Simply by not allowing to change it. If one does not like it, it can
pass the option value as he wishes.

An ini setting for that sounds wrong to me.

Cheers,

Pierre

@pierrejoye | http://blog.thepimp.net | http://www.libgd.org

13 years ago by Nikita Popov — view source

unread

hi!

Well, if not for an ini parameter, what way would you suggest to alter
the default bcrypt cost? (seriously, I'm open to suggestions)...

Simply by not allowing to change it. If one does not like it, it can
pass the option value as he wishes.

An ini setting for that sounds wrong to me.
Agree on this. We don't need any more ini settings. Having an
ini-setting would just force everyone to explicitly specify the load
parameter.

Nikita

13 years ago by Anthony Ferrara — view source

unread

Pierre,

Simply by not allowing to change it. If one does not like it, it can
pass the option value as he wishes.

An ini setting for that sounds wrong to me.

Alright. I've pulled the ini option from the fork, and have updated
the RFC to the same...

Anthony