Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
chillu opened this issue Oct 30, 2018 · 11 comments · Fixed by #69
Closed

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

chillu opened this issue Oct 30, 2018 · 11 comments · Fixed by #69

Comments

@chillu
Copy link
Member

chillu commented Oct 30, 2018

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
Copy link
Contributor

ScopeyNZ commented Oct 30, 2018

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
Copy link
Member Author

chillu commented Oct 30, 2018

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
Copy link

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
Copy link
Member Author

chillu commented Oct 30, 2018

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
Copy link
Member

madmatt commented Oct 31, 2018

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
Copy link
Member

madmatt commented Oct 31, 2018

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
Copy link
Contributor

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
Copy link
Member Author

chillu commented Nov 2, 2018

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
Copy link
Contributor

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

silverstripe/cwp#199

@robbieaverill
Copy link
Contributor

PR at #69

@robbieaverill
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants