Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:61032
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.49 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <1340875005.2516.9.camel@guybrush>
References: <CAAyV7nH6FCZUqrirJ0uOApuwR64Cky3cVnRfbrcjkZrpOKiN6w@mail.gmail.com>
	<1340815507.2802.9.camel@guybrush>
	<CAAyV7nG8fRMbqe2syW_7xbDKHbOPvqG5F3hO3HMyR=JY0RELyw@mail.gmail.com>
	<1340875005.2516.9.camel@guybrush>
Date: Thu, 28 Jun 2012 15:36:19 -0400
Message-ID: <CAAyV7nFkOHpjKsBwH5JMCx-4P5A33Qfg24UGdZAi0U4iFqaRZg@mail.gmail.com>
To: =?ISO-8859-1?Q?Johannes_Schl=FCter?= <johannes@schlueters.de>
Cc: internals@lists.php.net
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] [DRAFT RFC] Adding Simplified Password Hashing API
From: ircmaxell@gmail.com (Anthony Ferrara)

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... =A0I 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/p=
assword.c#L262

> I don't see what makes password_verify special. If one wants a typesafe
> check one can go for true =3D=3D=3D 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