silverstripe / cwp-core

CWP basic compatibility module
BSD 3-Clause "New" or "Revised" License
3 stars 12 forks source link

Use SHA-512 rather than blowfish for password hashing (NZISM compliance) #51

Closed chillu closed 5 years ago

chillu commented 5 years ago

Blowfish is the default hashing algo for SilverStripe. It is still considered a safe hashing algo (https://en.wikipedia.org/wiki/Blowfish_(cipher)), but is not on the list of approved cryptographic algorithms in the NZISM: https://www.nzism.gcsb.govt.nz/ism-document/#2106. We should set SHA-512 as a new default. Since the system tracks the algo per user, I believe we can do this without password resets. It kicks in either when an existing user changes their password, or when new users create a password. It should retain the cost parameter used in the blowfish logic.

Note that blowfish is a crypto algo which can be used for encryption, but can also form the basis for the bcrypt hashing algorithm - which is how we're using it. Meaning we're hashing passwords, not encrypting them - even though the APIs in SilverStripe are named misleadingly (Security::encrypt_password()).

To be clear, this is not a security vulnerability, but rather a compliance issue.

/cc @robbieaverill @Firesphere

ScopeyNZ commented 5 years ago

I disagree. We should stick with bcrypt and upgrade to argon2 where available. As I understand SHA-512 is not as appropriate for bcrypt when it comes to passwords as it's designed to be efficient. It doesn't appear that the NZISM recommends any password hashing algo?

chillu commented 5 years ago

This isn't a matter of agreeing or not, it's compliance with a standard we're bound to in a CWP context. SHA-512 has a cost function. Do you have any references which discount SHA-512 for password hashing use?

Firesphere commented 5 years ago

It's the speed at which SHA512 works compared to proper password hashing algorithms. The slow hashing algo helps prevent the creation of rainbow tables, for example

chillu commented 5 years ago

So you can't get it to work in a similar way through the cost function?

CRYPT_SHA512 - SHA-512 hash with a sixteen character salt prefixed with $6$. If the salt string starts with 'rounds=$', the numeric value of N is used to indicate how many times the hashing loop should be executed, much like the cost parameter on Blowfish. The default number of rounds is 5000, there is a minimum of 1000 and a maximum of 999,999,999. Any selection of N outside this range will be truncated to the nearest limit.

http://php.net/crypt

madmatt commented 5 years ago

In my uneducated opinion this is a flaw in the NZISM where it assumes that all 'hashing' is the same.

Hashing of a significant amount of content should be done via SHA-512 as NZISM states, however a significant feature of the SHA2 family is that they are fast and relatively inexpensive to compute. This is good when you're comparing a large amount of text (e.g. two binary blobs or git commits) but bad when you're checking if a password matches, because you can brute-force the password offline much more easily (as each hash is easier to calculate, even with a high number of rounds).

I'd probably prefer to follow OWASP's guidance on this instead of NZISM, noting that IMO this is far better than the NZISM guidance: https://www.owasp.org/index.php/Password_Storage_Cheat_Sheet#Leverage_an_adaptive_one-way_function

Mozilla blogged back in 2011 that SHA-512 with per-user salts isn't enough: https://blog.mozilla.org/security/2011/05/10/sha-512-w-per-user-salts-is-not-enough/

madmatt commented 5 years ago

Another point raised internally - SHA-512 on its own is not suitable due to the speed factor, however using PBKDF2 with SHA-512 as the hashing algorithm could be used.

This would be via https://secure.php.net/hash-pbkdf2, however sha512 is only viable from PHP7 onwards (see https://secure.php.net/manual/en/function.hash-algos.php for when each algorithm is added).

ScopeyNZ commented 5 years ago

This would be via secure.php.net/hash-pbkdf2, however sha512 is only viable from PHP7 onwards (see secure.php.net/manual/en/function.hash-algos.php for when each algorithm is added).

I love how on the documentation for that function it advises against using hash_pbkdf2 for password storage.

chillu commented 5 years ago

This would be via https://secure.php.net/hash-pbkdf2, however sha512 is only viable from PHP7 onwards

After a bit more research, we figured out that sha512 is actually available on PHP 5.6 already. The https://secure.php.net/manual/en/function.hash-algos.php mentioned more specific sha512/* algos as being added in PHP 7.1. See section under "As of PHP 5.6.0, hash_algos() will return the following list of algorithm names". Which means we can use built-in features in PHP to support this hashing without increasing our requirements around PHP extensions or language versions.

ScopeyNZ commented 5 years ago

I've raised an issue in cwp/cwp for altering the hash method for backup codes in the MFA module:

silverstripe/cwp#199

robbieaverill commented 5 years ago

PR at https://github.com/silverstripe/cwp-core/pull/69

robbieaverill commented 5 years ago

This is now configured by default for CWP 2.4.x onwards.