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

[techhead]

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]

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.

[techhead]

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.

[recrsn]

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.

[recrsn]

@techhead Can you rebase this off master?

[techhead]

@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.