579510 - Make sure multi-byte passwords are stored and sent correctly

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Ed Lee :Mardak]

We might be dropping high-bytes similar to the passphrase issue in bug 558963. Not sure if this is happening at btoa or some other place. We should probably just encodeUTF8 the password after getting it from the user?

Comment 1

[Ed Lee :Mardak]

So potentially we're correctly creating the account in that we're sending all the bytes to the server. (I tested with  characters.) But logging in (basic auth) fails because btoa doesn't work with those characters.

Comment 2

[Ed Lee :Mardak]

Ignore what I said in comment 1 about not being able to create an account and log in/use it. That is working "correctly"/consistently.

I'm able to log in to my account with..



I'm also able to log in with characters that have the LSB == 255 ( == 63743):
㓿㗿㛿㟿㣿㧿㫿㯿
ÿǿ˿Ͽӿᓿᗿỿ
⇿⋿⓿◿⣿⧿⫿㓿

Firefox seems to be correctly saving the full bytes of passwords. Account creation uses a PUT, and I'm guessing necko is dropping off high bytes per http spec (?). Somehow the high bytes are getting dropped before we do btoa as well for auth.

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to comment #0)
> We might be dropping high-bytes similar to the passphrase issue in bug 558963.
> Not sure if this is happening at btoa or some other place.

ID.get('WeaveID').password stores unicode strings correctly. The high byte gets dropped in btoa().

> We should probably just encodeUTF8 the password after getting it from the user?

Not really, I think it's ok to keep it as unicode characters in ID.get('WeaveID').password. After all, we just extended the ID object to keep unicode *and* UTF-8 representations of passwords. Instead we should probably make sure that what goes over the wire, or rather, what is fed into btoa() is UTF-8. So BasicAuthenticator should use id.passwordUTF8 instead of id.password.

(In reply to comment #1)
> So potentially we're correctly creating the account in that we're sending all
> the bytes to the server.

We are indeed sending the whole unicode string to the server since we're simply putting it inside a JSON object. JSON will always do the right thing in terms of unicode. Now, that doesn't mean the PHP on the server side knows what to do with it and I have no idea what it ends up writing into the password record.

We would be safe if we stuck to UTF-8 encoding everything, both the password we send upon sign-up as well as the password we send in basic auth. This will fix things for new users. For existing users we could simply do a transparent password change if their password contains non-ASCII characters (IOW characters in the 128-255 range). ASCII-only passwords will continue to work unchanged.

Comment 4

[Ed Lee :Mardak]

(In reply to comment #3)
> We are indeed sending the whole unicode string to the server since we're simply
> putting it inside a JSON object.
This might not be so per comment 2. I was able to create the account and log in with all those different characters for the same account. That seems to indicate that the server is only getting the low byte and is accepting all the requests I made that go through btoa.

So I believe in one place, btoa is dropping high bytes and another place is necko on the PUT.

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to comment #4)
> So I believe in one place, btoa is dropping high bytes and another place is
> necko on the PUT.

Oh I see. Shrug. Our JSON.stringify() doesn't do the \u1234 escaping that I know from other implementations. So there's no guarantee that the return value of JSON.stringify() is ASCII only, so yeah, probably Necko is dropping the high bytes here.

Either way, that doesn't change the way my proposal on how to fix this:
1. Send passwordUTF8 in createAccount
2. Use passwordUTF8 in BasicAuthenticator
3. Do a transparent password change for existing users who have a non-ASCII password (not unlike the way we dealt with the passphrase situation).

Ed, are you working on this? If not, I can tackle it.

Comment 6

[Ed Lee :Mardak]

We'll probably want for 1.4.2 perhaps?

Comment 7

[Philipp von Weitershausen [:philikon]]

Attachment: v1

This changes Weave.Service to UTF8-encode the password when
a) creating a new account
b) changing the password
c) authenticating on every resource

For c) a new BasicUTF8Authenticator object was implemented. The old BasicAuthenticator implementation was kept around so that authentication using low-bytes only was still possible. This is needed when migrating users to UTF8-based passwords. Migration happens when Weave.Service.verifyLogin() encounters a 401 *and* the password is non-ASCII.

Note that the server currently special-cases Latin-1 passwords in that it converts them to UTF8. So passwords with e.g. umlauts won't be migrated as they won't cause a 401.

I couldn't test this with the iPhone client yet as it actually gets the encoding wrong (it accidentally applies the UTF8 encoder transformation twice.)

Comment 8

[Mike Connor [:mconnor]]

Comment on attachment 458437 [details] [diff] [review]
v1

r=me, with renaming of BasicAuthenticator to BrokenBasicAuthenticator and BasicUTF8 to Basic.  Ambiguity of which authenticator to use is a ticking timebomb. :)

Comment 9

[Philipp von Weitershausen [:philikon]]

Attachment: v1.1

Authenticators renamed as requested.

Comment 10

[Ragavan S [:rags]]

Targeting 1.4.2

Comment 11

[Ed Lee :Mardak]

http://hg.mozilla.org/services/fx-sync/rev/97e3c78c3144
UTF8-encode passwords when creating accounts, changing passwords, and when authenticating. Detect old low-byte only passwords on the server and reupload them as UTF8. 

http://hg.mozilla.org/services/fx-sync/rev/11367abc4295
1.4.x

Comment 12

[Tony Chung [:tchung]]

Verified fix on 1.4.2b1