-
Notifications
You must be signed in to change notification settings - Fork 12
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
Comments
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? |
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? |
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 |
So you can't get it to work in a similar way through the cost function?
|
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/ |
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 |
I love how on the documentation for that function it advises against using |
After a bit more research, we figured out that |
I've raised an issue in cwp/cwp for altering the hash method for backup codes in the MFA module: silverstripe/cwp#199 |
PR at #69 |
This is now configured by default for CWP 2.4.x onwards. |
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
The text was updated successfully, but these errors were encountered: