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

Add footnote warning to hashing a hash #411

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

FWDekker
Copy link

It's generally not recommended to use bcrypt on top of another hashing algorithm. While doing it over sha256 is obviously not as bad as doing it over md5, I think it's important to note in the README that OWASP recommends against the practice.

Copy link

@charmander charmander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While doing it over sha256 is obviously not as bad as doing it over md5

Sure it is. “Hash shucking” remains an issue, and the differences in collision resistance and output size aren’t relevant to this application.

How about recommending pre-hashing with HMAC as a keyed hash instead? Or just bcrypt.kdf?

README.rst Outdated Show resolved Hide resolved
@FWDekker
Copy link
Author

Sure it is. “Hash shucking” remains an issue, and the differences in collision resistance and output size aren’t relevant to this application.

You're absolutely right!

How about recommending pre-hashing with HMAC as a keyed hash instead? Or just bcrypt.kdf?

I've removed the mention of the DDoS attack, replaced sha256 with bcrypt_pbkdf, and adjusted the explanation of why using an unkeyed hash function is not a good idea. What do you think?

I don't have a lot of experience with bcrypt_pbkdf, so I just copied the parameters from the kdf part of the README. Do you think those parameters are readable?

@reaperhulk
Copy link
Member

https://flak.tedunangst.com/post/new-openssh-key-format-and-bcrypt-pbkdf has some information about choosing parameters for bcrypt_pbkdf. I've fixed CircleCI so if you push a new commit here CI should pass now and we can get it reviewed 😄

README.rst Outdated
>>> password = b"an incredibly long password" * 10
>>> hashed = bcrypt.hashpw(
... base64.b64encode(hashlib.sha256(password).digest()),
... base64.b64encode(bcrypt.kdf(password=password,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I meant using bcrypt.kdf directly instead of bcrypt.hashpw. This works, but it… feels unusual? If people want to keep the descriptive hash format that hashpw creates, HMAC seems simpler (although I guess the only concrete difference is that you don’t have to make an arbitrary choice for the number of output bytes and rounds).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. I just literally placed one in the other, but there's more intuitive ways of doing this.

Do you mean base64.b64encode(bcrypt.kdf(password=password, salt=bcrypt.gensalt(), <whatever>))? Would password verification simply be to repeat the procedure and compare strings, or is there an analogue to hashpw?

And with the HMAC one, do you mean bcrypt.hashpw(base64.b64encode(hmac.digest(pepper, password, "sha256")), bcrypt.gensalt())?

If so, I suppose another important difference is that the latter requires storing a pepper (or per-hash salt) separately.

Copy link
Author

@FWDekker FWDekker Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had forgotten about this PR, but it's now almost a year old. Luckily(?) the relevant section in the README has not been updated, so the changes here are still relevant.

Do you perhaps have time to take a look at my question to see if I understood your suggestion correctly?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean base64.b64encode(bcrypt.kdf(password=password, salt=bcrypt.gensalt(), <whatever>))? Would password verification simply be to repeat the procedure and compare strings, or is there an analogue to hashpw?

And with the HMAC one, do you mean bcrypt.hashpw(base64.b64encode(hmac.digest(pepper, password, "sha256")), bcrypt.gensalt())?

Yes and yes.

On second thought, though… maybe the best answer is replacing the code with your original footnote, and pointing back to “but you should really use argon2id” at the top of the README?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, good point. I've made sure to emphasise that not using bcrypt is probably the best solution. However, I'm sure there will be people who will somehow be forced to use bcrypt, either because of legacy software, weird interoperability, legal requirements, incompetent management, you name it. So I think it is still worthwhile to explain how to work around the length limitation. I've also chosen to use the HMAC variant because if a reader doesn't already know about hash shucking and peppers, I think it's unlikely they'll manage to choose reasonable numbers of rounds and bytes.

Let me know what you think of the rewritten section.

@reaperhulk
Copy link
Member

closing/reopening to trigger CI

@reaperhulk reaperhulk closed this Sep 23, 2022
@reaperhulk reaperhulk reopened this Sep 23, 2022
README.rst Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants