Upgrade hashes to avoid SHA1

[Synchro]

The PHP sha1 function is used in lots of places in Laravel. SHA-1 has been deprecated for security uses for some time, not least because:

As of 2020, attacks against SHA-1 are as practical as against MD5

The biggest impact of this has been in TLS certificates, which banned the use of SHA-1 signatures some time ago. They still remain in use in git as integrity checks, but are not really much of a security risk there.

However, sha1() is used in quite a few places in Laravel, mainly for temp file naming, which is exactly the kind of thing that attackers might want to try to create hash collisions for. Many of the uses of SHA-1 may be entirely benign from a security perspective, but figuring out whether they are or not is far more work than simply avoiding it altogether.

Security issues notwithstanding, SHA-1 is also relatively slow compared with modern hash functions. The choice of modern hash functions available in PHP is somewhat limited, with one nice exception: Blake2b in libsodium. Libsodium has been in the default base installation of PHP since 7.2, which is also the minimum version Laravel has required since 6.x.

Libsodium's "generic hash" function implements Blake2b and is available in PHP as sodium_crypto_generichash(). It returns its hash value in binary, so it needs converting to hex to be a drop-in replacement for sha1(), and libsodium provides a function to that as well, sodium_bin2hex, though PHP's bin2hex function does the same thing. Despite this, it's still around 60% faster than sha1() in PHP. hash('sha256') takes about three times as long as sha1() (it's constrained by a 32-bit algorithm), and hash('sha512/256') about twice as long, so Blake2b is significantly faster than all of them, while also being more secure. Blake2b uses less memory than sha1, and is also at the heart of the Argon2 hash function used for passwords.

I noticed this old Laravel 5.2 PR which changed everything from md5 to sha1, and I'm wondering what people think of doing the same thing, but from sha1() to sodium_bin2hex(sodium_crypto_generichash()). The other thing that struck me here is that it looks like we should be doing this via a consistent helper, just as we do for Hash::make for passwords.

[GrahamCampbell]

I don't think this is an actual risk? Many of the places sha1 is used is not processing user input or not in a way where collisions would matter. Did you have any specific place in mind where you think there would be a vulnerability. There might be a case for changing specific instances, but probably not a blanket change. Nobody has complained about performance issues due to sha1 in their apps.

[Synchro]

Well that was rather my point – figuring out whether every use of SHA-1 represents a risk or not is a much bigger job than changing all uses of it anyway. The basis for the change is exactly the same as for the earlier change from md5 to sha1, which was done (as far as I can see) for security reasons, even though the way the functions were used was not changed at all. The performance difference is indeed probably negligible at low load, but it is something you get "for free" while removing any possible risk due to SHA-1. Put it another way – if you were writing new code, you would not use SHA-1 for anything; there is nothing that it's better at than other hash functions.

[benjamindoe]

If there's a pull request that "fixes" this issue, would it be accepted? There are times where a fast hash makes sense and a slower more secure has makes sense.

Briefly looking at the sha1 uses in the framework, you can see that sha1 is used for uniqueness. As this is the case, there will be some collisions. As @GrahamCampbell said, collisions may not matter but I've seen the framework is using sha1 for fingerprints for requests which may cause problems if there are colliding fingerprints.

I think if there are security advisories advising against the use of sha1, a pull request that would change this should be considered.

It would be better to also use a consistent helper so it will be easier to hot-swap an algorithm when it eventually becomes obsolete. We may need two, one to produce a crypto secure hash and another that is faster that will almost guarantee uniqueness?

[Synchro]

We already have a separate call for password hashes: Hash::make. It might perhaps be good to put the fingerprint hash in the same class as Hash::fast, Hash::generic (to use libsodium's terminology) or something similar.

One other practical concern for blake2 / sha256 is that the hashes produced are longer, just as sha1 hashes are longer than md5 ones. Given that most of these are used as filenames, that's not an issue, but there may be cases where hashes are stored in fixed-width DB columns, and that would cause a problem, though that's not a framework issue as such.

It looks like these hashes may be used as session identifiers, which is definitely a security-sensitive issue, and some of the discussion about the previous change noted that changing the hash function would result in everybody getting logged out. It might be possible to provide a fallback with auto-upgrading of session identifiers (a bit like rehashing passwords on login) to avoid that problem, though it was not done last time (I think there may have been some other coincidental change that caused the same thing anyway, so it was considered unimportant).

The short version of this discussion is really that if we thought that changing from md5 to sha1 was appropriate for security reasons (as it apparently was), then we should move away from sha1 on the same basis.

Another concern is the possible absence of ext-sodium. While it's meant to be compiled in by default, I have seen instances where it has actively been removed by packagers. I've seen the same thing done for libargon2, which can break password_hash. Fortunately there is at least sodium_compat to deal with that.

[vlakoff]

Just to point out that if an algorithm produces too long hashes, they can simply be truncated.
From a cryptographic point of view, it is perfectly valid; doing as so doesn't "weaken" the algorithm, it only reduces entropy.

[vlakoff]

Actually, for non-security use cases (where we only care about hash randomness), MD5 is largery sufficient and even SHA-1 is overkill.

So, there are two cases:

• When there is no security involvement, MD5 is sufficient (we could even truncate a bit the hash)
  • (though, SHA-1 hashes aren't hugely long, so let's say the current ones are acceptable)
• When there is security involvement, SHA-1 is a no-no, and the code has to be upgraded to a better algo.

As OP pointed out, the actual work is to determine where there are security risks.

[Synchro]

My point was more that the effort involved in changing everything to Blake2 is far smaller than the effort in figuring out whether a hash usage has security concerns or not. The performance improvement is largely academic, but overall there is no benefit in holding on to MD5 or SHA1 for any purpose. The only other thing I can think of is if you are ever relying on the database having the same hash functions available, but I've never seen that done in Laravel.

One other thought: if temp files are used as caches, then changing the hash algorithm would result in mismatches, resulting in lots of cache misses after an upgrade, though I'd expect that most caches are cleared during upgrades as their content may be wrong otherwise.