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

Potentially conflicting information in Password Storage: Pre-Hashing Passwords with bcrypt #1532

Closed
regunakyle opened this issue Nov 6, 2024 · 6 comments

Comments

@regunakyle
Copy link
Contributor

So the recent Okta username-longer-than-52 security advisory got me interested in best practice for handling longer-than-72 password length with bcrypt.

Someone in X/Twitter suggests we can SHA256 hash the password before applying bcrypt. The OWASP cheatsheet explicitly discourage this:

An alternative approach is to pre-hash the user-supplied password with a fast algorithm such as SHA-256, and then to hash the resulting hash with bcrypt (i.e., bcrypt(base64(hmac-sha256(data:$password, key:$pepper)), $salt, $cost)). This is a dangerous (but common) practice that should be avoided due to password shucking and other issues when combining bcrypt with other hash functions.

This blog post cited in the above excerpt explained that null bytes might show up after hashing if we don't encode the raw output with base64 or hex.
Quoting the blog post:

You are 100% safe if you do one of the following:

  • Use straight bcrypt (don’t pre-hash)
  • Use hex output from the pre-hash
  • Base64 encode the raw output of a pre-hash

However, the OWASP excerpt above uses bcrypt(base64(hmac-sha256(data:$password, key:$pepper)), $salt, $cost) as an example of unsafe prehashing practice (this is how I interpret it anyway). To me, they feel like conflicting information.

Could someone clarify this?

Note: I created a relevant question on the security stack exchange.

@jmanico
Copy link
Member

jmanico commented Nov 6, 2024

I really think "just dont use bcrypt" since pre-hashing can cause problems and NOT pre-hashing can cause problems. We are at an era where Argon2id is the right call and bcrypt is not. I want developers to just use the right algorithm and using bcrypt "right" is to challenging IMO. I see the Okta situation as bcrypt's end. Finally.

@regunakyle
Copy link
Contributor Author

I really think "just dont use bcrypt" since pre-hashing can cause problems and NOT pre-hashing can cause problems. We are at an era where Argon2id is the right call and bcrypt is not. I want developers to just use the right algorithm and using bcrypt "right" is to challenging IMO. I see the Okta situation as bcrypt's end. Finally.

Thanks for your answer!
I think the main source of my confusion comes from the cited blog post and the example bcrypt function in OWASP cheatsheet (they seem to contradict each other). A colleague of mine is also confused by this.

Maybe add something like this at the end of the paragraph?

... with other hash functions (Note: "The Fix" in this blog post only protect you from null bytes, but it does NOT protect you from password shucking!)

@ja2048
Copy link

ja2048 commented Nov 6, 2024

@jmanico I agree that people should switch to more modern algorithms like Argon2 whenever possible. I still wonder why the article recommends against SHA-256 hashing (which is indeed vulnerable to password shucking) but then shows Base64-encoded HMAC in the code (which should neither suffer from password shucking nor the null-byte problem). Originally, the code fragment contained a SHA-384 example, but you changed this to HMAC in commit 658ba04, pointing to “advice from twitter.com/AaronToponce”. Do you happen to remember why you thought HMAC was problematic as well? Because if it isn't, this might be an acceptable solution for legacy applications which cannot switch to Argon2 (for whatever reason).

@jmanico
Copy link
Member

jmanico commented Nov 6, 2024

The comments from the last two posters only leads me to wanting to drop bcrypt from the cheatsheet and only focus on Argon2id and PBKDF2 only. I am eager to get PR's to fix bcrypt, go for it, but the days of this cheatsheet supporting bcrypt are coming to an end.

@ja2048
Copy link

ja2048 commented Nov 6, 2024

@jmanico: PBKDF2 also suffers from password shucking (if the typical construction with HMAC as the PRF is used). When the password exceeds the block size of the underlying hash function (e.g., 64 bytes in the case of SHA-256), then it's pre-hashed and only used in this pre-hashed form, allowing an attacker to reuse leaked hashes. So this isn't a bcrypt-specific problem. Recommending against all algorithms except Argon2 might be an option, but this would leave developers working on legacy systems with essentially nothing.

How about the following three options:?

  • If possible, switch to Argon2
  • If you have to use bcrypt/PBKDF2 and can live with the input limitations, then restrict the password (maximum size and no null bytes)
  • If you have to use bcrypt/PBKDF2 and want to accept arbitrary input, then pre-processing with HMAC using a randomly generated key should be an acceptable choice

If you believe this is going in the right direction, then I'll happily open a PR.

@jmanico
Copy link
Member

jmanico commented Nov 6, 2024

Go for it. I think this is a fair direction even though I cant wait for bcrypt to die in a fire.

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

No branches or pull requests

3 participants