search

ircmaxell / password_compat

Compatibility with the password_* functions that ship with PHP 5.5
MIT License
2.15k stars 421 forks source link

compatibility with sha512 #71

Closed fredericve closed 9 years ago

fredericve commented 9 years ago

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?

sarciszewski commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

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 https://github.com/ircmaxell/password_compat/pull/71#issuecomment-63799452 .

ircmaxell commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

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

ircmaxell commented 9 years ago

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 commented 9 years ago

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

:+1: