Is using sha1() as hash good enough?

[m-ober]

For passwords, using md5 or any of the sha* hashing algorithms is a bad idea, as they can be brute forced (even with salt) with little effort. Thus, for hashing passwords, algorithms like bcrypt are state of the art. I stumbled upon this text from the FAQ:

An attacker has obtained the database of login tokens from the server. The stored tokens are hashed so he can't use them without computational effort (rainbow tables or brute force).

So let's say an attacker gained access to my db. He "can't" crack the passwords because they are hashed with bcrypt - but it would be rather easy to bake a stolen rememberme cookie. Of course, the legitimate user would get a notice - but only as soon as he visits the page again. And even gaining full access to a site for a short period of time can be very harmful.

Is there anything I am missing?

[gbirke]

Thank you for pointing this out! When I started to work on this, SHA1 was still considered secure enough, but now in 2020 it should really be password_hash.

Feel free to supply a patch, I'm not sure when I will get around to this.

[m-ober]

I have further looked into this issue and to be honest, I'm not 100% sure what to do.

Given this line, which retrieves a token given the username and the persistent token:

$query->execute(array($credential, sha1($persistentToken), date("Y-m-d H:i:s")));

One cannot easily use password_hash() here: We would have to retrieve all rows with the given username and then use password_verify() on each row. Password hashing is supposed to be slow (~100 ms), thus with a lots of entries this may sum up to seconds, which is not acceptable.

We would need to use no or a static salt, so our hashing function produces the same output for the same input (password_hash() will produce a different output every time, because a random salt is chosen - simply replacing sha1() with password_hash() will NOT work).

But there is something else that catched my eye: The hash function limits the entropy. SHA-1 has a digest size of 160 bits (20 bytes). Using tokens with more than 20 bytes will not improve security, because the entropy is limited by the digest size. Thus, it might still be a very good idea to use SHA-256 or even SHA-512. Also, contrary to SHA-1, SHA-2 is not completely broken.

I opened a PR that uses the hash() function and makes the algorithm a configuration option of the Storage class. I think it might also be possible to use crypt(), which will further harden the hashes (for example by using 10.000 rounds of SHA-256). On the other hand, it is harder to configure. That is, the algorithm is a string like $5$rounds=10000$verysecretsalt$. So using hash() might be a good compromise, accepting something like sha512 or ripemd160 as algorithm.

Edit: hash_pbkdf2() might be a viable alternative with a simple API to configure algorithm, salt and iteration count.