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

Password hashing helper functions generate salt insecurely #160

Closed
mammothbane opened this issue Jul 15, 2020 · 4 comments
Closed

Password hashing helper functions generate salt insecurely #160

mammothbane opened this issue Jul 15, 2020 · 4 comments
Milestone

Comments

@mammothbane
Copy link

The password hashing functions depend on math/rand to generate the salt, but that package isn't suitable on its own as a source of cryptographic randomness (rabbit-hole doesn't even seed the default source). Salt generation should depend on crypto/rand instead.

Additionally, restricting the permissible set of values for the salt to just ASCII upper-/lowercase means that the salt only has ~24 bits of effective entropy out of the possible 32.

@michaelklishin
Copy link
Owner

@mammothbane you are welcome to submit a pull request that addresses both.

@michaelklishin
Copy link
Owner

I am not a security expert and not sure I correctly understood the comment on ~ 24 bits vs. 32 but I have extended the source range to include digits. If there is a better way to produce a salt value, please submit a PR. I hope this is "secure enough" for most use cases now that we use crypto/rand. And for all others, they can generate a value they need since these functions are just that, convenience helpers.

@michaelklishin
Copy link
Owner

f34a861 introduced a test failure that seemed legit and therefore was reverted.

@michaelklishin michaelklishin changed the title password hashing functions generate salt insecurely Password hashing helper functions generate salt insecurely Jul 16, 2020
@michaelklishin
Copy link
Owner

My conclusion is this: the salt source characters in the first attempt used some of the characters that differ between URL-safe and standard base64 encoding. This client was using the former while RabbitMQ expects the latter (these values are not passed as part of the URI). The mismatch resulted in 400 Bad Request responses. Since there was a probabilistic component to this (the salt value), sometimes the suite would pass and sometimes it would fail.

@michaelklishin michaelklishin added this to the 2.3.1 milestone Jul 16, 2020
@michaelklishin michaelklishin modified the milestones: 2.3.1, 2.4.0 Aug 7, 2020
michaelklishin added a commit that referenced this issue Oct 5, 2020
The error in the example seems to have been there from the moment
when the password hash computation functions were exposed in the API.

References #160.
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