Discussion:
Simplified PHP Password Hashing
Anthony Ferrara
2012-06-26 15:30:21 UTC
Permalink
Hello all,

I've recently been working on an RFC to add a simplified PHP password
hashing API to PHP's core. I was hoping for some feedback, and a code
review of the implementation.

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

https://github.com/ircmaxell/php-src/blob/hash_password/ext/standard/password.c

It's still a work in progress, but I wanted to reach out for input and
review prior to moving too far forward.

Thanks,

Anthony
Solar Designer
2012-06-27 11:00:55 UTC
Permalink
Hi Anthony, all -
Post by Anthony Ferrara
I've recently been working on an RFC to add a simplified PHP password
hashing API to PHP's core. I was hoping for some feedback, and a code
review of the implementation.
https://wiki.php.net/rfc/password_hash
https://github.com/ircmaxell/php-src/blob/hash_password/ext/standard/password.c
It's still a work in progress, but I wanted to reach out for input and
review prior to moving too far forward.
Thanks. I've just provided a relevant reply here:

http://news.php.net/php.internals/60977

Unfortunately, I don't expect to have much time for this during the
summer. If you're not in much of a hurry to include this in PHP, then
I'd be happy to review and discuss it with you later.

On a related note, I think that your PHP-PasswordLib has too much stuff
in it:

https://github.com/ircmaxell/PHP-PasswordLib

What immediately caught my attention is this:

"Secure Random Number/String Generation
[...]
The mixing function is also dependent upon the strength required. For
non-cryptographic numbers, a simple XOR mixing function is used (for
speed). As strength requirements increase, it will use a SHA512 based
mixing function, then a DES based mixing function and finally an AES-128
based mixing function at "High" strength."

I see no reason to support these four modes instead of just one, e.g.
based on SHA-512. Just how is DES or AES any better for this purpose?
Perhaps leave the mode based on SHA-512 only. (I haven't looked at the
code yet, though.)

I do realize that this is a separate project, but I am concerned that
you might similarly provide too much stuff via your new API right away.
The API should be generic enough that future stuff would be likely to
fit it well, but there shouldn't be unneeded stuff available via it now
just to provide more examples.

Thanks,

Alexander
Anthony Ferrara
2012-06-27 12:02:46 UTC
Permalink
Alex,
Post by Solar Designer
http://news.php.net/php.internals/60977
Thank you very much!
Post by Solar Designer
Unfortunately, I don't expect to have much time for this during the
summer.  If you're not in much of a hurry to include this in PHP, then
I'd be happy to review and discuss it with you later.
Not too much, but I'd like to ensure that it makes it in php 5.5...
IIRC the first beta release isn't for several months, so I think we
have some time...
Post by Solar Designer
On a related note, I think that your PHP-PasswordLib has too much stuff
https://github.com/ircmaxell/PHP-PasswordLib
I've hear that before too. :-D
Post by Solar Designer
"Secure Random Number/String Generation
[...]
The mixing function is also dependent upon the strength required. For
non-cryptographic numbers, a simple XOR mixing function is used (for
speed). As strength requirements increase, it will use a SHA512 based
mixing function, then a DES based mixing function and finally an AES-128
based mixing function at "High" strength."
I see no reason to support these four modes instead of just one, e.g.
based on SHA-512.  Just how is DES or AES any better for this purpose?
Perhaps leave the mode based on SHA-512 only.  (I haven't looked at the
code yet, though.)
The interesting thing there is that it's a holdover from another
project of mine that I forked into PasswordLib: CryptLib
https://github.com/ircmaxell/PHP-CryptLib

In PasswordLib, I actually pulled all but the hash implementation
(SHA-512): https://github.com/ircmaxell/PHP-PasswordLib/tree/master/lib/PasswordLib/Random/Mixer
That documentation is out-dated. I'll update it.
Post by Solar Designer
I do realize that this is a separate project, but I am concerned that
you might similarly provide too much stuff via your new API right away.
The API should be generic enough that future stuff would be likely to
fit it well, but there shouldn't be unneeded stuff available via it now
just to provide more examples.
That's a very valid concern. My goal was flexibility, portability and
extend-ability. But it did wind up somewhat, well, big...

Thanks for the feedback,

Anthony
Anthony Ferrara
2012-08-28 15:33:49 UTC
Permalink
Alex
Post by Anthony Ferrara
I've recently been working on an RFC to add a simplified PHP password
Post by Anthony Ferrara
hashing API to PHP's core. I was hoping for some feedback, and a code
review of the implementation.
https://wiki.php.net/rfc/password_hash
https://github.com/ircmaxell/php-src/blob/hash_password/ext/standard/password.c
Post by Anthony Ferrara
It's still a work in progress, but I wanted to reach out for input and
review prior to moving too far forward.
http://news.php.net/php.internals/60977
Unfortunately, I don't expect to have much time for this during the
summer. If you're not in much of a hurry to include this in PHP, then
I'd be happy to review and discuss it with you later.
Would now be a better time for some feedback? I'd like to propose this soon
(so it doesn't drop completely off the radar)...

I did post this to security.stackexchange:
http://security.stackexchange.com/q/16506/1148 and got some good feedback.
The size_t changes are on my list to implement shortly.

Thanks for the help,

Anthony

Loading...