compatibility with sha512

[fredericve]

These changes add support for sha512 hashing algorithm. This is often required for compatibility with applications that don't support bcrypt.

I am far from an expert on this level, but these changes work fine on our setup. What are your thoughts?

[kd35a]

Shouldn't this be done in the same way as on line 232, for consistency? Or the other way around.

$rounds = isset($options['rounds']) ? $options['rounds'] : 5000;

[kd35a]

I now read the code for bcrypt above, and you're consistent with that implementation. Maybe both should be rewritten? But that should probably go into another issue.

[fredericve]

I don't think either of them should be rewritten. In the password_hash() function we're just setting a default value but afterwards checking the value provided in the $options array for validity. In password_needs_rehash() we're effectively checking if the rounds (or cost) in the hash needs to be changed.

[sarciszewski]

My thoughts are as follows:

http://php.net/manual/en/function.crypt.php#refsect1-function.crypt-changelog

First and foremost, CRYPT_BLOWFISH was added in PHP 5.3.0. Since the minimum requirement for this library is 5.3.7, we can assume it's supported.

But even if that were not the case:

http://php.net/manual/en/function.password-hash.php - only supports PASSWORD_BCRYPT (and PASSWORD_DEFAULT)

Since this library is a compatibility layer, it should not seek to introduce functionality that the target library does not, itself, support.

Therefore, if this patch is going to be merged, then PHP's password_hash() and password_verify() also need to accept a PASSWORD_SHA512 parameter as well. Which would only be acceptable to patch in for a 5.7 release and/or 7.0.

[fredericve]

@sarciszewski
We can indeed assume compatibility in php 5.3.0+, however, I am talking about compatibility with other applications that do not support bcrypt hashes yet (e.g. debian wheezy does not support bcrypt hashes in the shadow file out of the box).

I do agree that ideally the change should go into php as well. I was already looking at patching the source but I am no skilled C developer and time is short right now unfortunately. I will continue with that when I have more time on my hands though. In any case, I'd first like to see how this plays out to see if it would even be acceptable for upstream php.

[sarciszewski]

I see. I would leave that decision with @ircmaxell and @rlerdorf -- I can
see how it would be useful.

Does password_verify() validate non-bcrypt hashes? I'm on my phone and
cannot test immediately.

@sarciszewski https://github.com/sarciszewski
We can indeed assume compatibility in php 5.3.0+, however, I am talking
about compatibility with other applications that do not support bcrypt
hashes yet (e.g. debian wheezy does not support bcrypt hashes in the shadow
file out of the box).

I do agree that ideally the change should go into php as well. I was
already looking at patching the source but I am no skilled C developer and
time is short right now unfortunately. I will continue with that when I
have more time on my hands though. In any case, I'd first like to see how
this plays out to see if it would even be acceptable for upstream php.

—
Reply to this email directly or view it on GitHub
#71 (comment)
.

[ircmaxell]

I am a strong -1 on this.

Why would we want to add support for a demonstrably weaker alternative?

If you have compatibility needs (with other applications/systems that don't support bcrypt), then you don't have the same problem that this API was designed for.

I'd rather not add support for a weaker alternative to this API, and rather would have people with those needs use a different library (like my PasswordLib).

I am talking about compatibility with other applications that do not support bcrypt hashes yet (e.g. debian wheezy does not support bcrypt hashes in the shadow file out of the box).

I would argue that the request should be to make those stronger, not make us weaker.

[ircmaxell]

@sarciszewski

Does password_verify() validate non-bcrypt hashes? I'm on my phone and
cannot test immediately.

Yes. However that fact should not be relied upon and is undocumented. This may change in a future version.

[fredericve]

@ircmaxell
I am obviously not going to argue that sha512 is weaker for password hashing than bcrypt. I still believe compatibility would be a plus though. Recent linux distributions still mostly use sha512 password hashing.

password_verify() relies on crypt(). And password_hash() is documented to be compatible with crypt(). crypt() in turn is documented to be compatible with a number of hashing algorithms, including sha512.

[GrahamCampbell]

I have to agree with @ircmaxell here. Sorry. ;)

[ircmaxell]

Recent linux distributions still mostly use sha512 password hashing.

Again, then I would suggest having them improve their systems rather than reducing the strength of ours.

password_verify() relies on crypt()

Today. As an implementation detail. This is not documented, and nor should be relied upon.

And password_hash() is documented to be compatible with crypt()

In that the results of password_hash() will be verifyable with crypt(). Not implying the other way around.

crypt() in turn is documented to be compatible with a number of hashing algorithms, including sha512.

Correct. But password_hash() never claimed compatibility with crypt() other than that the generated hash would be work with crypt(). It's a one-way relationship.

[sarciszewski]

Again, then I would suggest having them improve their systems rather than reducing the strength of ours.

👍