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

Several issues with custom salt #42

Open
Jacques1 opened this issue Dec 14, 2013 · 1 comment
Open

Several issues with custom salt #42

Jacques1 opened this issue Dec 14, 2013 · 1 comment

Comments

@Jacques1
Copy link
Contributor

If the custom salt contains a character which isn't a valid bcrypt Base64 digit, the library will silently encode the salt. This is a huge problem. If the salt is just a faulty Base64 string (e. g., it uses + instead of .), it will be double-encoded, causing it to effectively lose 33 bits.

The library also requires the custom salt to always contain 22 bytes, even if a raw salt would only have to be 16 bytes long. This forces us to either generate 6 useless extra bytes or pad the string with garbage data.

Unfortunately, there's no simple fix, because it's not always possible to distinguish between a raw salt and a pre-encoded one. If the string happens to contain only Base64 digits, it could be both.

I suggest to either ask for raw salts or for pre-encoded salts, not both. Using raw salts is probably the better solution:

  • Why would anybody want to do the weird Base64 encoding herself?
  • This is also very error-prone. Many people aren't aware that bcrypt doesn't use standard Base64.
  • Base64 encoded salts require complicated validation (the length, the digits and a special check for the last digit). With raw salts, we only have to check the length.

Either way, it should be only one possible format, and the manual should clearly state what the function expects.

@TerjeBr
Copy link

TerjeBr commented Jul 20, 2014

The main bug as I see it, is that is not possible to pass a custom salt that is 16 bytes long.

If you want to use your own salt, in the current implementation you needlessly have to pad it to 22 bytes.

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