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

When using hashpass with own salt, the salt is not truncated to 128 bits #111

Closed
mbaeuerle opened this issue Jul 5, 2017 · 2 comments
Closed

Comments

@mbaeuerle
Copy link

When using own salt, the salt is not truncated to 128 bits. This leads to different hashes between this implementation and say PHP (if you leave aside the $2y$ at the beginning).
It's best shown by example:

iex(2)> Comeonin.Bcrypt.hashpass("test", "$2b$10$1234567899123456789012")
"$2b$10$1234567899123456789012.OtL1A1eGK5wmvBKUDYKvuVKI7h2XBu"

Notice how in the resulting hash the salt ends with a 2:
"$2b$10$1234567899123456789012.OtL1A1eGK5wmvBKUDYKvuVKI7h2XBu"

Doing the same in PHP:

>>> crypt("test", "$2y$10$1234567899123456789012")
=> "$2y$10$123456789912345678901u.OtL1A1eGK5wmvBKUDYKvuVKI7h2XBu"

"$2y$10$123456789912345678901u.OtL1A1eGK5wmvBKUDYKvuVKI7h2XBu"
Here the last character is truncated to an u because of the 128 bit length of the salt.
See this Question on Security.Stackexchange.

I think there is no problem with the open bsd bcrypt module itself because both versions are valid:

iex(5)> Comeonin.Bcrypt.checkpw("test", "$2b$10$1234567899123456789012.OtL1A1eGK5wmvBKUDYKvuVKI7h2XBu")
true
iex(6)> Comeonin.Bcrypt.checkpw("test", "$2b$10$123456789912345678901u.OtL1A1eGK5wmvBKUDYKvuVKI7h2XBu")
true

It's rather a problem with the return value from hashpass/2: https://github.com/riverrun/comeonin/blob/master/lib/comeonin/bcrypt.ex#L197
Here the salt is just returned as given by the user, and I think this should be truncated to 128 bits.

@riverrun
Copy link
Owner

riverrun commented Jul 6, 2017

Thanks for raising the issue. I'll look into it further and get back to you as soon as possible.

riverrun added a commit that referenced this issue Jul 7, 2017
@mbaeuerle
Copy link
Author

Thanks for the fast fix! 🎉

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

2 participants