451155 - Password manager does not work correctly on IDN site whose name contains any character over U+0100

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Atsushi Sakai]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.1a2pre) Gecko/20080818113442 Minefield/3.1a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

Password manager does not work correctly on IDN site (for example, Japanese domain name) whose name contains any character over U+0100.

Reproducible: Always

Steps to Reproduce:
1. Start Fx2.0.0.16.
2. Go to the URL (and accept security error).
3. Enter username and password.
4. Click ログイン button to submit.
5. Click Remember button in Confirm dialog.
6. Upgrade to Fx3.0.1.
7. Go to the URL again (and add exeption).

Actual Results:  
Username and password are NOT filled in automatically.

Expected Results:  
Username and password SHOULD BE filled in automatically.

The Site is garbled in
Tools -> Options -> Security -> Saved Passwords
after upgrade to Fx3.

Comment 1

[Justin Dolske [:Dolske]]

Ugh. I looked into IDN issues back in bug 396316, and they seemed to work fine. I suspect the testcase I used was insufficient -- the UCS2 representation of the non-ascii characters was probably still U+00xx.

The storage-Legacy.js module probably needs to use nsIScriptableUnicodeConverter (as it already does in _encrypt()/_decrypt() for usernames and passwords) for every string that might be written to disk. This would mean hostname, formSubmitURL, httpRealm, and usernameField/passwordField. Oh, and the disabled-hosts list too.

One nice thing: this bug is already fixed on 3.1, as a result of switching to mozStorage. Yay!

Also, for the hostname/formSubmitURL, it would probably be best to store as punycode version (which should just be plain ascii), and convert to IDN in the UI. This keeps the stored value the same, even when the IDN whitelist changes -- see bug 400097. Would still need to support extensions using arbitrary UCS2 strings, though. Changing the format could break existing stored IDN logs (when they are not already broken by this bug), so this might incur a storage file version bump or DB scheme version bump. Probably best to spin these issues off into a separate bug...

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Passes existing tests, need to add tests to see if it actually fixes this problem. :)

Comment 3

[Atsushi Sakai]

Requesting blocking because this should be fixed before major update. Otherwise, stored password for some site will be corrupted.

(In reply to comment #2)
> Created an attachment (id=334422) [details]
> Patch v.1

This patch works fine for me with form and also with HTTP authentication.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Same fix, added tests.

I also modified the mozStorage module with the move of nsIScriptableUnicodeConverter into a getter... It's ok without the move, but I think this will be a performance win -- we're not creating an instance of it every time we decrypt a string.

Comment 5

[Atsushi Sakai]

Adding "dataloss" keyword because stored data is garbled and can not be recovered, even if this bug is fixed the after data is garbled.

Comment 6

[Samuel Sidler (old account; do not CC)]

We need to block on this. Gavin, can you review this patch asap?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 334548 [details] [diff] [review]
Patch v.2

>+ * UCS2 characteres above U+00FF.

comentarios en inglés, por favor (dos casos)

Comment 8

[Justin Dolske [:Dolske]]

I'm not sure this is reaaaaally a blocker, just because it's been broken since the new password manager landed back in May 2007, and this is the first report. But in any case, I'm happy to fix it ASAP. :)

Comment 9

[Justin Dolske [:Dolske]]

Pushed changeset 442ac72e88f1.

Comment 10

[Samuel Sidler (old account; do not CC)]

Comment on attachment 334548 [details] [diff] [review]
Patch v.2

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch v.2 (FF3.0 branch)

Checked in on FF3.0 branch:

Checking in toolkit/components/passwordmgr/src/storage-Legacy.js;
  new revision: 1.28; previous revision: 1.27
Checking in toolkit/components/passwordmgr/test/unit/test_storage_legacy_3.js;
  new revision: 1.7; previous revision: 1.6

Comment 13

[Al Billings [:abillings - ex-MoCo]]

We need a username and password to verify this fix or the reporter needs to do so for 3.02.

Comment 14

[Atsushi Sakai]

(In reply to comment #13)
> We need a username and password to verify this fix or the reporter needs to do
> so for 3.02.

Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.2) Gecko/2008090213 Firefox/3.0.2.

I don't know username and password either.
But password can be stored to password manager even if it's wrong.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Ah, that makes sense. Marking it as verified.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

This caused a regression in Firefox 3.0.2 - see bug 454708

Comment 17

[Tracy Walker [:tracy]]

relnote: passwords entries of this type created in the broken builds (3.0, 3.0.1) will not be corrected in 3.0.3, they will have to be created and saved again by the user. The invalid entry will be there in PWM and should probably also just be removed by the user.