INGInious icon indicating copy to clipboard operation
INGInious copied to clipboard

Weak password hashing / salting

Open bartwojcik opened this issue 6 years ago • 2 comments

passwd_hash = hashlib.sha512(data["passwd"].encode("utf-8")).hexdigest()

How about using something like bcrypt, scrypt, Argon2 or PBKDF2 for user password hashing? Any of these should provide an orders of magnitude better security in case of database leak.

bartwojcik avatar Sep 30 '18 16:09 bartwojcik

Interesting suggestion. Actually there was no prior thinking about such a case.

Changing the hash method will require an automatic migration mechanism for older accounts and ideally make this completely transparent or very easy to users and maintainers.

Maybe we can simply store the hash method in the user document (as this is not a secret) to automatically check if the password is OK with the appropriate hash function and update it at the same moment to the new one if it is not the current one used.

We should also think of an automated procedure in case of a known leak. Users should at least be informed to reset their passwords. Ideally we can set the passwords as compromised and require the password reset before accepting the credentials.

@GuillaumeDerval What do you think ?

anthonygego avatar Oct 01 '18 06:10 anthonygego

Maybe we can simply store the hash method in the user document (as this is not a secret) to automatically check if the password is OK with the appropriate hash function and update it at the same moment to the new one if it is not the current one used.

A simple method is to prepend the method to the hash. Something like "bcrypt-thehash" or "sha512-thehash". We don't even need to update the database: we can simply say that if it doesn't begin by "bcrypt-", it should be updated (not risk of collision as - is not an hexadecimal character).

We should also think of an automated procedure in case of a known leak. Users should at least be informed to reset their passwords. Ideally we can set the passwords as compromised and require the password reset before accepting the credentials.

This should be another issue, but I agree this is an issue we should tackle.

GuillaumeDerval avatar Oct 01 '18 06:10 GuillaumeDerval

We could use Argon2id as it is suggested by OWASP here. We could at first keep the old algorithm to ease the transition. Both algorithms could be available for authentication but users would be encouraged to change their password with the new algorithm.

A simple method is to prepend the method to the hash.

Argon2 hashes contain the different parameters used, including the variant. We can easily detect if a hash uses the new algorithm thanks to that. For example : '$argon2id$v=19$m=65536,t=3,p=4$MIIRqgvgQbgj220jfp0MPA$YfwJSVjtjSU0zzV/P3S9nnQ/USre2wvJMjfCIjrTQbg'

AlexandreDoneux avatar Oct 19 '23 12:10 AlexandreDoneux

We can easily detect if a hash uses the new algorithm thanks to that.

We may have to change the algorithm again in the future. Prepending takes this case into account.

anthonygego avatar Oct 20 '23 06:10 anthonygego