[PHP-DEV] [DRAFT RFC] Adding Simplified Password Hashing API

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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 <[email protected]>wrote:

> 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
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>

hi!

On Wed, Jun 27, 2012 at 12:13 PM, Simon Schick <[email protected]> wrote:
> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

hi,

On Wed, Jun 27, 2012 at 1:24 PM, Anthony Ferrara <[email protected]> wrote:
> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Em Wed, 27 Jun 2012 13:37:50 +0200, Pierre Joye <[email protected]>
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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

hi,

On Wed, Jun 27, 2012 at 1:49 PM, Gustavo Lopes <[email protected]> wrote:
> Em Wed, 27 Jun 2012 13:37:50 +0200, Pierre Joye <[email protected]>
> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Em Wed, 27 Jun 2012 14:24:39 +0200, Anthony Ferrara <[email protected]>
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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

hi,

On Wed, Jun 27, 2012 at 2:32 PM, Gustavo Lopes <[email protected]> wrote:
> Em Wed, 27 Jun 2012 14:24:39 +0200, Anthony Ferrara <[email protected]>
> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Em Wed, 27 Jun 2012 14:43:35 +0200, Pierre Joye <[email protected]>
escreveu:

> On Wed, Jun 27, 2012 at 2:32 PM, Gustavo Lopes <[email protected]>
> wrote:
>> Em Wed, 27 Jun 2012 14:24:39 +0200, Anthony Ferrara
>> <[email protected]> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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.

Arvids,

On Wed, Jun 27, 2012 at 9:23 AM, Arvids Godjuks
<[email protected]> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

hi,

On Wed, Jun 27, 2012 at 2:59 PM, Gustavo Lopes <[email protected]> wrote:

> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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" <[email protected]>
написал:

> Arvids,
>
> On Wed, Jun 27, 2012 at 9:23 AM, Arvids Godjuks
> <[email protected]> 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
>

Hi,

On Tue, 2012-06-26 at 11:25 -0400, Anthony Ferrara wrote:
> 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


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On 27/06/12 18:13, Pierre Joye wrote:
> 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


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Arvids,

On Wed, Jun 27, 2012 at 12:32 PM, Arvids Godjuks
<[email protected]> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

hi,

On Thu, Jun 28, 2012 at 12:03 AM, Ángel González <[email protected]> wrote:

> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

Hi Anthony!

On Thu, Jun 28, 2012 at 4:00 AM, Anthony Ferrara <[email protected]> wrote:

> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On Wed, 2012-06-27 at 22:00 -0400, Anthony Ferrara wrote:
> 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



--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

hi Anthony,

On Thu, Jun 28, 2012 at 9:36 PM, Anthony Ferrara <[email protected]> wrote:

>> 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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On 06/27/2012 07:16 AM, Anthony Ferrara wrote:
> Arvids,
>
> On Wed, Jun 27, 2012 at 9:23 AM, Arvids Godjuks
> <[email protected]> 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



--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On 06/26/2012 08:25 AM, Anthony Ferrara wrote:
> 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



--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

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

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

On 07/02/2012 01:55 PM, Anthony Ferrara wrote:
> 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



--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php