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

Pass key_len to bcrypt(). Fix for issues kelektiv#774, kelektiv#776 #807

Merged
merged 3 commits into from Jun 1, 2020
Merged

Pass key_len to bcrypt(). Fix for issues kelektiv#774, kelektiv#776 #807

merged 3 commits into from Jun 1, 2020

Conversation

@techhead
Copy link
Contributor

@techhead techhead commented Jun 1, 2020

Fixes #774 #776

Bcrypt already includes the final NUL termination character when calculating the hash. I believe that also Including those caught in the middle is a reasonable default that will help to prevent a possible class of errors. And it's a simple fix.

This patch also fixes #776, though I previously released a stand-alone patch for it. This one is preferred (by me).

@recrsn
Copy link
Collaborator

@recrsn recrsn commented Jun 1, 2020

The current bcrypt implementations are fragmented. Some accept nulls and some dont. Given we use OpenBSD bcrypt as the source implementation, and UTF-8 as the canonical character encoding (which wont contain nulls) this change is controversial, OpenBSD bcrypt doesn't accept nulls in password chars.

Loading

@techhead
Copy link
Contributor Author

@techhead techhead commented Jun 1, 2020

The NUL character is valid in UTF-8. That's actually kind of part of the problem. This is a javascript library, and the string inputs are coming from javascript strings. If this was a C library, the controversy would be bigger, I imagine. But many (most?) javascript programmers know nothing about NUL-terminated strings.

I think that the expected behavior should be the default behavior as a general rule. It's safer. The programmer who uses this library is generally going to expect that the full input provided is going to be hashed, at least up to the well-documented 72-byte limitation.

But of course, in real-world usage, passwords are not going to contain NUL characters unless someone made a mistake or the password has been pre-processed or pre-hashed in some way. This is the use case that I'm interested in.

https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#pre-hashing-passwords

As you can see from the OWASP recommendation, implementations that do not hash NULs have been the unexpected cause of unsafe passwords in some cases.

As such, the preferred option should generally be to limit the maximum password length. Pre-hashing of passwords should only be performed where there is a specific requirement to do so, and appropriate steps have been taking to mitigate the issues discussed above.

Well, I have a "specific requirement." I'm a big proponent of pass-phrases over passwords. And a 72-byte limitation really works against that.

Pre-hashing is a pretty common means of overcoming the limitation. Dropbox does it. (Or did it, at least. https://dropbox.tech/security/how-dropbox-securely-stores-your-passwords) The easier a library makes to do it safely, the better IMO.

I already have a backwards-compatible branch of this lib that takes either a string or Buffer as an input to further help to do this easily and properly. I'd love to have it merged eventually so I don't have to maintain a separate fork. This is a step towards that.

Anyway, that's my goal, use case, and argument. Thank you for your consideration.

Loading

@recrsn
Copy link
Collaborator

@recrsn recrsn commented Jun 1, 2020

The NUL character is valid in UTF-8. That's actually kind of part of the problem.

Only as NUL byte. UTF-8 is designed in such a way to avoid NUL anywhere resulting from combination of any multi-byte character.

This is a javascript library, and the string inputs are coming from javascript strings. If this was a C library, the controversy would be bigger, I imagine. But many (most?) javascript programmers know nothing about NUL-terminated strings.

This I will agree with.

I already have a backwards-compatible branch of this lib that takes either a string or Buffer as an input to further help to do this easily and properly. I'd love to have it merged eventually so I don't have to maintain a separate fork. This is a step towards that.

I would actually like that. I have been working on a Buffer input for bcrypt for some time now but it's nowhere complete.

Anyway, that's my goal, use case, and argument. Thank you for your consideration.

Loading

@recrsn
Copy link
Collaborator

@recrsn recrsn commented Jun 1, 2020

@techhead Can you rebase this off master?

Loading

@recrsn recrsn merged commit 5916a46 into kelektiv:master Jun 1, 2020
3 of 4 checks passed
Loading
@techhead
Copy link
Contributor Author

@techhead techhead commented Jun 1, 2020

@techhead Can you rebase this off master?

Looks like you already got it done and merged. Thanks for this! I'll send a pull request for using Buffers when I have a version that I feel is ready.

Loading

@techhead techhead deleted the known_length branch Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants