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

Interoperability issue with PHP implementation (and others) when truncating long password #22

Closed
Indigo744 opened this issue Oct 17, 2019 · 10 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@Indigo744
Copy link

Hello,

Interoperability is quite important for us, since we have 4 different software developed in different languages (Java, C#, C++, PHP) using the same database.

We have detected a very serious interoperability issue with this library when dealing with long password (and truncating strategy).

If we generate a hash like this:

String hash = BCrypt.with(BCrypt.Version.VERSION_2Y, new SecureRandom(), LongPasswordStrategies.truncate()).hashToString(12, password.toCharArray());

With password =

password_longer_than_72_bytes_abcdefghijklmnopqrstuvwxyz0123456789_abcdefghijklmnopqrstuvwxyz0123456789

We obtain hash = $2y$12$BhmM4lJ91dMTHQoh3XgxY.QZg2j1EfH6DpiwmgufAAHImqCSvA/b.

If we take this hash and try to verify it using PHP (using password_verify()), the check will fail.

You can quickly try it using this handy online tool: https://bcrypt-generator.com
image


After looking into the code, we though that the issue could come from the truncating strategy.
We tried overriding the MAX_PW_LENGTH_BYTE:

public static final int MAX_PW_LENGTH_BYTE = 72;

The new hash generated was $2y$12$wHUr.PTUQPb7.CsK/0jHq.G4m6uIiHinBDBsWf2/cnwwwANZceQYm

And this one works:
image

@patrickfav
Copy link
Owner

Hmm interessting, the 71 bytes are because the last byte needs to be a null-terminator byte. Are you sure this isn't just an issue with this specific bcrypt hasher? I

@Indigo744
Copy link
Author

Indigo744 commented Oct 17, 2019

Well, all other libraries works, except this one, so I'm inclined to think the issue is here.

For reference, tested libraries are:
PHP https://www.php.net/manual/en/ref.password.php
C# .Net https://github.com/BcryptNet/bcrypt.net
C++ https://github.com/trusch/libbcrypt


Edit: I hope my comment doesn't sound too harsh. I know bcrypt is kind of a mess implementation-wise so I don't blame you 😉. I just want to go to the bottom of this issue, and hopefully fix any issues.

@patrickfav
Copy link
Owner

No offense taken :) I will look into it, Im a bit out of the topic so I have to reread the issue, AFAIK since version 2a a null terminator must be present as the last characther, but maybe there are different interpretations on how to behave with 72+ long passwords.

@patrickfav patrickfav added the bug Something isn't working label Oct 17, 2019
@patrickfav patrickfav self-assigned this Oct 17, 2019
@patrickfav patrickfav added this to the v0.9.0 milestone Oct 17, 2019
@Indigo744
Copy link
Author

Alright 😺

Don't hesitate to ask for help, I'll assist if I can 😉

@Indigo744
Copy link
Author

I was thinking about it, and for when looking at other implementation, I noticed that no truncation on input is ever performed.

Your lib is the only one really truncating before running the bcrypt algorithm.

I don't know if it's linked to the way the algorithm is implemented, or the platform limitation.

Maybe if we simply use a "pass-through" and let the algorithm naturally truncate the input, the issue won't be seen anymore?

Just a thought.

@patrickfav
Copy link
Owner

patrickfav commented Oct 20, 2019

I wanted to simulate the way, I understood, other bcrypt implementations to work - i.e. I derived the raw hashing algorithm from jBcrypt which does not truncate or have an opinion on hash length, therefore I wanted to "correctly" truncate the input to 72 byte (ie. 71+null terminator) - but let's see if this is correct. My idea ist to provide a simple "hack" for v0.9.0 and do a more elegant fix in v1.0.0 in combination with #27

@patrickfav
Copy link
Owner

patrickfav commented Oct 21, 2019

After revisiting the issue, I'm still quite sure my implementation is correct:

Bcrypt uses blowfish internally which uses 18x32bit words, i.e. the absolute maximum possible byte length that the hash can process is 72 bytes. According to this since version $2a$ a null terminator is mandatory.

That being said, to be able to be compatible, I altered the implementation to allow a maximumAllowedPw property, which is, for backwards compatibility, now set to 71. I added an VERSION_2Y_NO_NULL_TERMINATOR version that supports 72 byte long passwords. (you can implement your own version btw.). I hope now interoperability with non-standard bcrypt routines are possible.

See PR #28

Reference:

@patrickfav
Copy link
Owner

@Indigo744 I, again, appreciate your input for #28

@Indigo744
Copy link
Author

Thank you! I'll look into it.

If you can wait a bit, we have some more tests coming in tomorrow.

@patrickfav
Copy link
Owner

#28 is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants