sha256 password before passing to bcrypt to avoid issues with 72 bytes truncation for passwords

[le0pard]

More info: bcrypt-ruby/bcrypt-ruby#283

Reproduction:

BCrypt::Password.new(BCrypt::Password.create('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1')) == 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa2'
BCrypt::Password.new(BCrypt::Password.create('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1')) == 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa222333'
BCrypt::Password.new(BCrypt::Password.create('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1')) == 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa222333234234324'

All return true, so

Password 1: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1
Password 2: aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa2

These two users can login to each other's accounts because brcypt caps hashing to the first 72 bytes.

> hash = Devise::Encryptor.digest(Devise, 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1')
=> "$2a$13$XxwZStO7/NHTDjJsnGQhIOSb8ZO12PTL1/.Lze6OIT.qOAfBrqBHS"
> Devise::Encryptor.compare(Devise, hash, 'password')
=> false
> Devise::Encryptor.compare(Devise, hash, 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1')
=> true
> Devise::Encryptor.compare(Devise, hash, 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa2')
=> true

As solution - hash password to sha256, so it always will be smaller, than 72 bytes. Added fallback for old passwords.

In this case we can reject #5806

[kossy0701]

Excuse me for interrupting, I thought a half space was needed.

suggest

old_hashed_password = ::BCrypt::Password.create(password_with_pepper, cost: Devise.stretches)

[le0pard]

thanks, fixed @kossy0701

[le0pard]

ping @carlosantoniodasilva

[carlosantoniodasilva]

I think we should ensure passwords are not longer than the max BCrypt supports, rather than trying to work around it?

I can't think of any immediate issue running through SHA would cause off the top of my head, but that doesn't mean it won't open a can of worms to bite us in the future.

[le0pard]

You can read about this here - https://security.stackexchange.com/questions/92175/what-are-the-pros-and-cons-of-using-sha256-to-hash-a-password-before-passing-it

TLDR: SHA-256 allows for avoiding length constraints where entropy would otherwise be lost

Passlib in python do the same - https://passlib.readthedocs.io/en/stable/lib/passlib.hash.bcrypt.html#bcrypt-password-truncation
Django framework have such implementation (BCryptSHA256PasswordHasher)

Possible alternative, move to Argon2, which not have such limitations, but this will require to add another dependency and leave bcrypt for backward compatibility

[le0pard]

https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html - right now owasp for usage Argon2, bcrypt is way to hash passwords for legacy systems.

But yes, we can limit to 72 bytes (it will be less characters, because of Unicode) passwords and raise error

Or I can create Argon2 usage PR, with fallback to bcrypt, but this will require major release version for devise

[carlosantoniodasilva]

@le0pard thanks for the links, I'll read up to understand the implications a bit better, but this called my attention at a first glance: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#pre-hashing-passwords-with-bcrypt

Just using pure SHA-512, ( i.e. bcrypt(base64(sha512($password))), $salt, $cost)) is a dangerous practice and is as secure as just using pure SHA-512.
...
To summarize if bcrypt has to be used and the password should to be pre-hashed you should do bcrypt(base64(hmac-sha384(data:$password, key:$pepper)), $salt, $cost) and store the pepper not in the database.

Re Argon2: something I'd like to support at some point, yes. Whether it'd become the default or not, not sure, but probably yes, eventually. (it doesn't need to be the default from the get go)

[le0pard]

@carlosantoniodasilva that is why I am using .hexdigest method. Hexadecimal strings only contain 0-9 and a-f. They do not contain null bytes. Therefore, this implememtation is not falling into the specific "dangerous" trap OWASP mentioned regarding raw hashes.

Ok, so what is preferable for now - such fix or maybe better prepare PR for Argon2 (which is not default and can be activated by option, which may be will be default in future)?

[carlosantoniodasilva]

Makes sense, but I still want to catch up a bit on that reading before applying anything, as it is a more complex implementation to keep track of under the hood.

I think for now I'd rather enforce the 72 bytes limitation if using BCrypt (I imagine for the majority of users this won't be a big deal), and then we can start working towards offering Argon2 as an opt-in to swap with BCrypt. (in case people need more than 72 bytes) (with a potential "rolling" option for people to migrate over time, e.g.: https://github.com/heartcombo/devise-encryptable/pull/19/changes which I haven't merged yet.)

[le0pard]

ok, so we can have something like this validation:

https://github.com/rails/rails/blob/ffcbf6f205363f8c2fb3e9834bc86690dd59f1cb/activemodel/lib/active_model/secure_password.rb#L138-L140

But again, not sure it will be not broken changes (maybe not critical, but still)

[carlosantoniodasilva]

It will probably be potentially breaking/annoying, but not one that warrants a major imo (perhaps / probably minor). And we should probably warn if the password length validation is larger or something, to give people a hint. (I understand that length validation != bytes validation, and we'll have to enforce bytes validation, but it's something...)

I can also add a warning when installing that new version of Devise if necessary (via the gemspec), plus the changelog, so not too worried about that.

[le0pard]

ok, so should I prepare this changes is different PR for validation or you will do this @carlosantoniodasilva ? I can handle good amount, except maybe "add a warning when installing that new version of Devise" - not sure about this, because it maybe busy with turbo already :D

[carlosantoniodasilva]

I removed the turbo post install message with Devise 5: 47e8716

If you're up for it and have the availability, feel free to take a stab. :) (you can probably build on #5806)

[le0pard]

ok, thanks. closing this one