kelektiv / node.bcrypt.js

bcrypt for NodeJs
MIT License
7.47k stars 515 forks source link

Consider adding warning for long strings #325

Closed thanpolas closed 9 years ago

thanpolas commented 9 years ago

I've stumbled upon a case where long strings (over 72 bytes long) would always return true in matching even if the last characters where different...

Digging deeper i discovered that indeed bcrypt has a hard limit of 72 characters beyond which it will ignore any other characters. http://security.stackexchange.com/questions/39849/does-bcrypt-have-a-maximum-password-length

Would you consider adding a waning when this situation occurs to prevent security issues?

I used bcrypt to produce tokens for server to server communications, the strings used where 80 chars long, a combination of dates, ids, etc... when in a test i added + 'lol' to the string to test for security, to my amazement the string passed when it shouldn't.

This can go unnoticed easily.

defunctzombie commented 9 years ago

A note on the readme would be good.

thanpolas commented 9 years ago

After a day of having this issue sip in, i believe an error should be thrown to prevent security issues with unforeseen consequences. Remember, password hashing is not the only usage for hashing algorithms and the natural stance is that a hash algo will happily hash any size of bytes.

In my particular case where bcrypt was used to sign token for authentication between servers, the string to hash was a concatenation of ids and dates which was over 120 chars long. In effect, this algo was not working leaving us vulnerable to a variety of attacks.

I've wrapped bcrypt in a library that throws an error if length is 72 chars or beyond. I encourage you to protect your users too.

jimlei commented 9 years ago

I think it's generally well known that Bcrypt has a hard limit, but the docs could/should definitely mention it for those who don't know. I'm not sure about throwing errors/exceptions, other implementations/libs doesn't seem to, so I'd worry it would throw people off ^^

I'd consider using something else entirely for HMAC.