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

3.1.13 possibly resulting in different hash values for some users in JRuby version #200

Closed
mohamedhafez opened this issue Jun 4, 2019 · 4 comments

Comments

@mohamedhafez
Copy link

mohamedhafez commented Jun 4, 2019

It really could just be coincidence, but I've had a handful of users over the last couple days since running 3.1.13 in production swearing up and down they use only one password and they definitely haven't forgotten it. In all the cases I've tested 3.1.13 produces the same results as 3.1.11 that I was running before on my JRuby server, but in #182 @fonica says

updated lib/bcrypt/engine.rb to pass the secret as java bytes; it seems jruby messes up the encoding for certain bytes if the secret is passed as a string.

Could this result in the same password resulting in different hashes in 3.1.12 and 3.1.13? If so, is it just in extreme edge cases or is this something that will happen for sure?

@mohamedhafez
Copy link
Author

mohamedhafez commented Feb 10, 2020

@headius @enebo any idea when JRuby would "mess up the encoding for certain bytes"? the couple times I try to update to this version of BCrypt, i get unusually high reports of passwords not working (which, granted could be coincidence), but the description of @fonica's change above makes it sound like it would result in different bytes being sent to BCrypt in the situations where jruby would have messed up the encoding. I'm just trying to figure out how common that situation is to estimate how many users it will impact, and if there is any way for me to detect the situation and handle it better

@enebo
Copy link

enebo commented Feb 11, 2020

@mohamedhafez I can see some substantial changes between 3.1.12 and 3.1.13 (none made for 3.1.11). Are all passwords in this required to be UTF-8? The older version would just assume the converted String was decodable as that. If it wasn't then it would clearly return broken bytes but pretend they were UTF-8.

The new version will get a byte array from a RubyString bytelist and then rewrap it as a Java consumable byte[]. This will be whatever the bytelist internally is encoded as so a EUC-JP string will return EUC-JP bytes (as opposed to old one which will decode EUC-JP as UTF-8).

So I can say the new one will treat non-ascii differently than the old one. I don't know what passwords would break from this. I can only guess if someone pasted in smart quotes from a windows CP-1252 encoding the old version would make those something. This new version would definitely encode those bytes differently. I would argue the new one would be correct and the old one would be wrong. If you happened to save the old hash from the old version you would get a mismatch? Perhaps you are seeing something like that?

As an aside if BCrypt used the native extension API for defining hashpw as a @JRubyMethod they could eliminate this extra wrapping to pass in byte[] and just pass in the RubyString instance directly. This is unrelated to whatever problem you are seeing but it might speed up JRuby somewhat by not forcing RubyString -> Java consuable wrapper of byte[]. It certainly would wind through a lot less code to make the same thing.

@mohamedhafez
Copy link
Author

First off I really appreciate the detailed response @enebo! It sounds to me like as long as the string hashed with version 3.1.12 was UTF-8, there shouldn't be any difference moving to 3.1.13 if I'm understanding correctly. I'm running a little experiment in production to see if versions 3.1.12 and 3.1.13 ever produce different hashes, and will update the ticket with what I find. But since everything is UTF8 these days, my guess is that it was just coincidence/power of suggestion that I saw more complaints from customers, it was just scary when I saw that comment in the pull request and couldn't get an explanation. Thanks again @enebo!

@mohamedhafez
Copy link
Author

I've been comparing the hashes from 3.12 and 3.13 in production for a couple weeks now and haven't found a single difference, sorry all for the false alarm, closing.

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