Second preimage vulnerability in `verify` function

[tarcieri]

Per #86 and the following link, only the first 72 bytes of the input are considered when generating a hash, leading to hash collisions in the case that inputs are 73 bytes or longer:

https://github.com/Keats/rust-bcrypt/blob/9c9e138/src/lib.rs#L117-L119

(Sidebar: the comment seems to suggest a potential panic when computing a hash for an input longer than 72 bytes, which is a potential DoS vulnerability)

I would suggest returning a Result-based error from both functions in the event the input is longer than 72 bytes.

I'm aware there's some "precedent" with other implementations of bcrypt silently truncating the input to 72 bytes, but I would argue those implementations are similarly vulnerable to second preimage attacks. #86 is a real-world example that this behavior is unexpected.

[Keats]

I'm aware there's some "precedent" with other implementations of bcrypt silently truncating the input to 72 bytes, but I would argue those implementations are similarly vulnerable to second preimage attacks. #86 is a real-world example that this behavior is unexpected.

It's pretty much every implementation that truncates. Sometimes they have an argument to error on len > 72 but it's not the default.
Making it an error would mean this crate would error on passwords that are valid in pretty much all other programming languages so that's not going to happen. We could add another function that errors when len > 72 if needed but that wouldn't be the default.

[tarcieri]

Perhaps consider supporting both options: one that errors if the input is too long, and a compatibility API which truncates for people who have legitimate iterop reasons for that behavior?

This class of vulnerability was implicated in a real-world login bypass bug in Okta: https://www.theverge.com/2024/11/1/24285874/okta-52-character-login-password-authentication-bypass

[Keats]

Optional error would be fine, but with documentation that enabling it means you lose compatibility with most languages.

[FeldrinH]

Making it an error would mean this crate would error on passwords that are valid in pretty much all other programming languages so that's not going to happen. We could add another function that errors when len > 72 if needed but that wouldn't be the default.

If a password is valid but causes truncation then wouldn't that mean that the last characters (or possibly even the entire password) is ignored when computing the hash? Supporting such passwords seems like a huge footgun and an easy way to accidentally cause major security issues.

I strongly support getting rid of the functions that truncate by default or at least adding a prominent warning that they may truncate inputs. If a user wants the truncation then they can do it in their own code.

[Keats]

I strongly support getting rid of the functions that truncate by default or at least adding a prominent warning that they may truncate inputs. If a user wants the truncation then they can do it in their own code.

Not going to happen. As mentioned pretty much every other implementation do truncate so it needs to be consistent

[FeldrinH]

Why does it need to be consistent? As far as i know the truncation is not required by any bcrypt specification, it is just a common convention.

For example the Go standard library implementation errors instead of truncating and as far as I know no one has complained.

[tarcieri]

I now see this post suggesting erroring out as a more conservative choice after doing a survey of implementations (also referencing this issue): https://n0rdy.foo/posts/20250121/okta-bcrypt-lessons-for-better-apis/

[Keats]

How about swapping hash/verify and have truncating_hash/truncating_verify? I'd like to keep the truncating versions around since not many people know about the 72 chars limit and it's nice to have a function they can call if they need to read hashed text from python/JS

[tarcieri]

@Keats sure, that sounds fine

[k98kurz]

Theoretically, this problem can be solved by using a recursive feature that breaks the input into 72-byte max chunks, concatenates the 24-byte digests to form a new preimage, and then repeats until a single digest is returned. All digest operations would use the same salt and cost, so only the digests would need to be used recursively. It's a novel approach, but it would work in all scenarios, and it is a sane convention that would not limit password length while maintaining the security of the algorithm. Just my 2 cents.

Edit: actually, if input lengths are never checked, and some huge value is used, it could lead to a DoS, so maybe the solution is just for people to use bcrypt correctly 🤷