457358 - need to reset UTF8 converter after it encounters invalid input (pwmgr problems in FF3.0.3)

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[LB.RudeWolf]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en; rv:1.9.0.2) Gecko/20080528 Epiphany/2.22 Firefox/3.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr-FR; rv:1.9.0.3) Gecko/2008092416 Firefox/3.0.3

Forewords:
Tested with ft.mozilla.org's tar.bz2 686 localized for french, on Ubuntu 8.04.
Got redirected here by mzz from support chat. 
(mzz: the patch does not seem to do what I thought it did (it's not actually skipping over corrupted entries, it's effectively treating them as latin-1) 

Description:
In short: Doesn't recover passwords even with 3.0.3 build. (both ubuntu packaged and tar.bz2). Editing and saving signons3.txt with UTF8 (used gedit) solve this.

The longer story: I about:config signon.debug to true, so I catch ouput launching 3.0.3 from terminal. With the faulty signons3.txt, the output, after fresh start is very verbose: I stored it in /tmp/logfile via 
$ firefox 2>/tmp/logfile
here is the head of /tmp/logfile (seems to be a latin-1 file, I cannot gedit it in utf8; my locale is UTF8)

FoxyProxy settingsDir = /home/rudewolf/firefox/profileFF3/ib3ctest.default
Login Manager: onStateChange accepted: req = http://start.ubuntu.com/8.04/, flags = 0x30004
Login Manager: onStateChange accepted: req = http://www.google.fr/firefox, flags = 0x30004
Login Manager: domEventListener: got event DOMContentLoaded
Login Manager: Counting logins matching host: http://www.google.fr, formSubmitURL: , httpRealm: null
Login Manager: No alternate nsILoginManagerStorage registered
PwMgr Storage: Checking file signons3.txt (SignonFileName3)
PwMgr Storage: Reading passwords from /home/rudewolf/firefox/profileFF3/ib3ctest.default/signons3.txt
PwMgr Storage: Bad UTF8 conversion: http://tolkien.originet.eu (Accès reservé)
PwMgr Storage: Bad UTF8 conversion: MDoEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECLvOciU8I0vfBBCg86MSNa7rj8nIFcNBdk5N

the following is Bad UTF8 complaining for every line of my signons3.txt from tolkien above till the end. It ends with the irrelevent

Login Manager: domEventListener: got event DOMContentLoaded
Login Manager: Counting logins matching host: http://start.ubuntu.com, formSubmitURL: , httpRealm: null

The problem is the only faulty line is 
http://tolkien.originet.eu (Accès reservé)
which od signons3.txt -c | less describe as 
0031600  \r  \n   -   -   -  \r  \n   .  \r  \n   h   t   t   p   :   /
0031620   /   t   o   l   k   i   e   n   .   o   r   i   g   i   n   e
0031640   t   .   e   u       (   A   c   c 350   s       r   e   s   e
0031660   r   v 351   )  \r  \n  \r  \n   M   D   o   E   E   P   g   A
I utf8ed this file via a gedit open/save as UTF8. The latter has only two more bytes
0031600  \r  \n   -   -   -  \r  \n   .  \r  \n   h   t   t   p   :   /
0031620   /   t   o   l   k   i   e   n   .   o   r   i   g   i   n   e
0031640   t   .   e   u       (   A   c   c 303 250   s       r   e   s
0031660   e   r   v 303 251   )  \r  \n  \r  \n   M   D   o   E   E   P
and doesn't produce Bad UTF8 output (and the latter let one access to one's passwords).

Furthermore, when accessing with faulty profile, via options menu, the (empty) list of passwords, I get this kind of output:
PwMgr Storage: Failed to decrypt string: MDoEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECNqUZoEO0ob5BBC4Slog/40TiGK+QRtrVo8N (NS_ERROR_UNEXPECTED)
These errors seems to show for every entry/section in signons3.txt, from the very begining, even before the faulty line.
I've not tested for decrypt complainings with utf8ed file.


Reproducible: Always

Steps to Reproduce:
1. signon.debug set to true
2. start any(?) 3.0.3 'firefox' script from terminal
3. choose the 'test' profile with faulty signons3.txt



I got mzz to reproduce it, uploading him only the faulty signons3.txt file.

Comment 1

[Marien Zwart]

storage-Legacy.js is creating an nsIScriptableUnicodeConverter once, feeding it one line at a time, logging the exception if it raises. It tries to reuse the nsIScriptableUnicodeConverter for the next line. According to a quick test and some inspection of the code that does not actually work:

js> uconv = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"]. createInstance(Components.interfaces.nsIScriptableUnicodeConverter);
[xpconnect wrapped nsIScriptableUnicodeConverter]
js> uconv.charset = 'UTF-8'
UTF-8
js> uconv.ConvertToUnicode('blarf');
blarf
js> uconv.ConvertToUnicode(String.fromCharCode(200))

js> uconv.ConvertToUnicode('blarf');
uncaught exception: [Exception... snipped]
js> uconv.ConvertToUnicode('blarf');
uncaught exception: [Exception... snipped]

And so on. I think this is (apart from the spew with signon.debug enabled) a bug: if there are any valid utf-8 entries after a broken latin-1 entry they will not be decoded correctly. I'm not sure how likely encountering such a signons3.txt is in practice though (as far as I can tell purely ascii entries are handled correctly (but noisily)). Using a fresh encoder after triggering an exception would fix this problem.

I am also not sure if it is intentional that when decoding fails the line is treated as latin-1. Is this for backwards compatibility with previous 3.0 releases writing the entry that way?

I do not understand where the decrypt errors come from.

Comment 2

[LB.RudeWolf]

This got a mid-air collision:
With fixed signons output (firefox 2>/tmp/logfile-fixed) reduce to

FoxyProxy settingsDir = /home/rudewolf/firefox/profileFF3/ib3ctest.default
Login Manager: onStateChange accepted: req = http://start.ubuntu.com/8.04/,
flags = 0x30004
Login Manager: onStateChange accepted: req = http://www.google.fr/firefox,
flags = 0x30004
Login Manager: domEventListener: got event DOMContentLoaded
Login Manager: Counting logins matching host: http://www.google.fr,
formSubmitURL: , httpRealm: null
Login Manager: No alternate nsILoginManagerStorage registered
PwMgr Storage: Checking file signons3.txt (SignonFileName3)
PwMgr Storage: Reading passwords from
/home/rudewolf/firefox/profileFF3/ib3ctest.default/signons3.txt
Login Manager: domEventListener: got event DOMContentLoaded
Login Manager: Counting logins matching host: http://start.ubuntu.com,
formSubmitURL: , httpRealm: null

after fresh start, with 

Login Manager: Getting a list of all logins

more upon accessing password's list (nonempty this time)

Comment 3

[Marien Zwart]

Two more problems:

The first one is probably not a problem in practice, but still: If the line
ends with a non-utf-8 character uconv.ConvertToUnicode remembers it and
(probably) throws on the next call. As far as I can tell ReadLine strips
newlines, so it might be possible to hit this. The logged error would be for
the wrong line, the next line would similarly be misinterpreted if it is not
ascii, and if it happens to start with a byte that is a valid utf-8 character
when combined with the last one from the previous line weirdness happens.

The bigger problem: _decrypt reuses _utfConverter too. This causes all calls to
_decrypt to fail if signons3.txt is not valid utf-8, meaning none of your saved
passwords work. I think this can be worked around by adding any password and
restarting Firefox (the file will be rewritten as valid utf-8 and work from
that point onwards) but that deserves at least a relnote, I think.

Comment 4

[Justin Dolske [:Dolske]]

(In reply to comment #0)

> PwMgr Storage: Bad UTF8 conversion: http://tolkien.originet.eu (Accès reservé)
> PwMgr Storage: Bad UTF8 conversion: MDoEEPgAAAAAAAAAAAAAAAAAAAE...

Interesting. This doesn't seem to happen for me (OS X, trunk). I'm only getting the "Bad UTF8" errors for the lines where it's expected. I'll look again in the morning, but could be a Linux-specific issue, fixed on trunk, or something wrong with my testing.

Comment 5

[Marien Zwart]

Attachment: Manually modified signons3.txt demonstrating this bug

This is the result of starting Firefox 3.0.3 with a fresh profile, setting signon.debug to true in about:config, then adding three bogus logins (gmail, bugzilla.mozilla.org, wiki.mozilla.org) to produce a boring ascii-only working signons3.txt. I then shut down Firefox, manually added " (Accès reservé)" to the second login, and restarted it. Log output follows...

Comment 6

[Marien Zwart]

Attachment: Output from Firefox 3.0.3 (Linux)

This is the dump output generated by going to edit -> preferences -> security -> saved passwords with the broken signons3.txt in the profile (getting no passwords). This is with:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.3) Gecko/2008092416 Firefox/3.0.3

The file was loaded on startup, triggering the "Bad UTF8 conversion" warnings. Trying to view saved passwords triggered the "Failed to decrypt string" warnings. Minefield log coming up...

Comment 7

[Marien Zwart]

Attachment: Output from Minefield (Linux)

The same thing using Minefield:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20080926021032 Minefield/3.1b1pre

Comment 8

[LB.RudeWolf]

Apparently, analoguous symptoms (no password retrieval with 3.0.3) appears on Windows build too:
https://bugzilla.mozilla.org/show_bug.cgi?id=457403

Comment 9

[Justin Dolske [:Dolske]]

Ok, I can reproduce this here now. The analysis in comment 0 and comment 1 appears to basically be correct -- thanks! It seems that not all invalid inputs cause the charset decoder to get into a bad state. I'd like to look at that a bit more to understand why, and make sure we nail this problem dead, dead, dead.

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

This should fix it.

I'm planning to do a little more investigating and add some more sanity-check testcases before calling this final, though.

Comment 11

[Marien Zwart]

(In reply to comment #9)
> Ok, I can reproduce this here now. The analysis in comment 0 and comment 1
> appears to basically be correct -- thanks! It seems that not all invalid inputs
> cause the charset decoder to get into a bad state. I'd like to look at that a
> bit more to understand why, and make sure we nail this problem dead, dead,
> dead.

See nsUTF8ToUnicode::Convert. If you pass it a byte that could be the start of a multibyte sequence (è is 0xe8 aka 11101000 in latin-1, which is the start of a 3 byte multibyte character) it remembers that byte and keeps throwing exceptions for later bytes until they match:

js> uc.ConvertToUnicode(String.fromCharCode(0xe8))
(empty string returned)
js> uc.ConvertToUnicode('a')                
uncaught exception: [Exception snipped]
js> uc.ConvertToUnicode(String.fromCharCode(162))
(another empty string returned)
js> uc.ConvertToUnicode('a')                
uncaught exception: [Exception snipped]
js> uc.ConvertToUnicode(String.fromCharCode(184))
(single unicode character returned, snipped in case bugzilla/mail hates it)

And after that decoding other things works again.

Not all non-ascii characters are valid for starting a multi-byte sequence though:

js> uc.ConvertToUnicode(String.fromCharCode(255))
uncaught exception: [Exception snipped]

This raises immediately, and passing valid utf-8 after that works.

What is somewhat annoying is there is no nice way to check if the input given to ConvertToUnicode is incomplete:

js> uc.ConvertToUnicode('test' + String.fromCharCode(0xe8))
test

At this point the converter is waiting for the rest of the multi-byte sequence, but I know no way other than feeding it an ascii character to check if it completely converted the input (an ascii character can never be part of an utf-8 multibyte sequence). So if that was the final line of input you might never notice decoding it failed, and you might incorrectly think the next line was broken if there was one. Nor does there seem to be a way to reset the ScriptableUnicodeConverter without simply creating a new one.

I think the incomplete input problem cannot actually be triggered (newlines are stripped, if they were not they would guard against this, but I think all non-ascii lines in signons3.txt end in either a ")" character or a toplevel dns domain name?)

Comment 12

[Justin Dolske [:Dolske]]

(In reply to comment #11)
> (In reply to comment #9)
> > Ok, I can reproduce this here now. The analysis in comment 0 and comment 1
> > appears to basically be correct -- thanks! It seems that not all invalid inputs
> > cause the charset decoder to get into a bad state. I'd like to look at that a
> > bit more to understand why, and make sure we nail this problem dead, dead,
> > dead.
> 
> See nsUTF8ToUnicode::Convert. If you pass it a byte that could be the start of
> a multibyte sequence (è is 0xe8 aka 11101000 in latin-1, which is the start of
> a 3 byte multibyte character) it remembers that byte and keeps throwing
> exceptions for later bytes until they match:

Right. It just seems like maybe the converter should reset itself to the initial state whenever an invalid sequence is found, but maybe there's a reason for it. In any case, having the caller reset the converter on errors fixes both cases, even if the one case isn't a persistent failure.

> I think the incomplete input problem cannot actually be triggered

They can in one case -- form field names. This ends up being ok, though. We don't use the field name (even though we store it). The exception is thrown on the next line (the encrypted username/password), but that's ok too with the fix to just ignore the failed conversion... The encrypted value is always ASCII, so the conversion wasn't needed anyway.

I've added tests for these cases.

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Also added a converter reset in case someone encrypts an invalid UTF8 string (username/password). This shouldn't ever come from us, but I suppose it's possible that broken 3rd party code which directly encrypts and modifies the signons file could cause this. We'll ignore such a corrupt login, but at least it won't cause other operations to fail.

Comment 15

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

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

We should probably have some test coverage for characters outside of Latin1 apart from the 0x0163 (though that test *is* testing both read and write, correct?). I also failed to mention in the other review that you can use "\uXXXX" notation instead of fromCharCode in all those tests, but I guess fromCharCode is probably clearer anyways and it doesn't really matter.

Comment 16

[Justin Dolske [:Dolske]]

Landed changeset 791bd7de18f7 on trunk

Branch patch for Firefox 3.0.4 coming up soon.

(In reply to comment #15)
> (From update of attachment 341014 [details] [diff] [review])
> We should probably have some test coverage for characters outside of Latin1
> apart from the 0x0163 (though that test *is* testing both read and write,
> correct?).

Right. That's what the reloadStorage() calls do. [The legacy storage immediately writes out a file when initWithFile() is called with an output file specified.]

> I also failed to mention in the other review that you can use
> "\uXXXX" notation instead of fromCharCode in all those tests, but I guess
> fromCharCode is probably clearer anyways and it doesn't really matter.

Yeah. It's 1/2 making sure it's obvious that something special is being done there, and 1/2 jsut lazy cut'n'paste from how it was done in previous tests.

Comment 17

[Justin Dolske [:Dolske]]

Attachment: Patch v.2 (for branch)

Comment 18

[Justin Dolske [:Dolske]]

Requesting approval304...

This is a followup to bug 454708. That bug fixed the way pwmgr was completely broken for many users, but we found is was still mostly-broken for some users. The UTF8 converter gets into a bad state, and we can't decrypt any logins.

That issue is well understood, patch is small and safe, and is tested extensively.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 341674 [details] [diff] [review]
Patch v.2 (for branch)

Approved for 1.9.0.4, a=dveditz for release-drivers

Comment 20

[Justin Dolske [:Dolske]]

Checked in on branch:

Checking in toolkit/components/passwordmgr/src/storage-Legacy.js;
  new revision: 1.30; previous revision: 1.29
Checking in toolkit/components/passwordmgr/test/unit/test_storage_legacy_3.js;
  new revision: 1.9; previous revision: 1.8
Checking in toolkit/components/passwordmgr/test/unit/data/signons-457358-1.txt;
  initial revision: 1.1
Checking in toolkit/components/passwordmgr/test/unit/data/signons-457358-2.txt;
  initial revision: 1.1
Checking in toolkit/components/passwordmgr/test/unit/data/signons-457358-3.txt;
  initial revision: 1.1
Checking in toolkit/components/passwordmgr/test/unit/data/signons-457358-4.txt;
  initial revision: 1.1
Checking in toolkit/components/passwordmgr/test/unit/data/signons-457358-5.txt;
  initial revision: 1.1

Comment 21

[Douglas]

Is the bug described here:
<https://bugzilla.mozilla.org/show_bug.cgi?id=457403>
a duplicate of this bug?

Comment 26

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

[e-mail], can you verify that this is fixed with a nightly 1.9.0.4 build from ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla1.9.0/?

Comment 27

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

In a 1.9.0.3 build, I see the following output:

Login Manager: onStateChange accepted: req = http://start.ubuntu.com/8.04/, flags = 0x30004
Login Manager: domEventListener: got event DOMContentLoaded
Login Manager: Counting logins matching host: http://start.ubuntu.com, formSubmitURL: , httpRealm: null
Login Manager: No alternate nsILoginManagerStorage registered
PwMgr Storage: Checking file signons3.txt (SignonFileName3)
PwMgr Storage: Reading passwords from /home/mozilla/.mozilla/firefox/k8mpm7jm.signon/signons3.txt
PwMgr Storage: Bad UTF8 conversion: https://bugzilla.mozilla.org (Acc�s reserv
PwMgr Storage: Bad UTF8 conversion: Bugzilla_login
PwMgr Storage: Bad UTF8 conversion: MDIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECDLRFkzajtidBAgX+A7nnUw+OA==
PwMgr Storage: Bad UTF8 conversion: *Bugzilla_password
PwMgr Storage: Bad UTF8 conversion: MDIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECAACzrmL9iGaBAiA9UnmkpC7kA==
PwMgr Storage: Bad UTF8 conversion: https://bugzilla.mozilla.org
PwMgr Storage: Bad UTF8 conversion: ---
PwMgr Storage: Bad UTF8 conversion: .
PwMgr Storage: Bad UTF8 conversion: https://wiki.mozilla.org
PwMgr Storage: Bad UTF8 conversion: wpName
PwMgr Storage: Bad UTF8 conversion: MDIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECHLD3G8iCZIBBAhhdqmXkBCLIw==
PwMgr Storage: Bad UTF8 conversion: *wpPassword
PwMgr Storage: Bad UTF8 conversion: MDIEEPgAAAAAAAAAAAAAAAAAAAEwFAYIKoZIhvcNAwcECKrN4t1gBkWNBAhwY3qrNB2FWA==
PwMgr Storage: Bad UTF8 conversion: https://wiki.mozilla.org
PwMgr Storage: Bad UTF8 conversion: ---
PwMgr Storage: Bad UTF8 conversion: .

In a 1.9.0.4 build with the fix (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.4pre) Gecko/2008102704 GranParadiso/3.0.4pre), I see the following output:

Login Manager: onStateChange accepted: req = http://www.mozilla.org/projects/granparadiso/, flags = 0x30004
Login Manager: domEventListener: got event DOMContentLoaded
Login Manager: Counting logins matching host: http://www.mozilla.org, formSubmitURL: , httpRealm: null
Login Manager: No alternate nsILoginManagerStorage registered
PwMgr Storage: Checking file signons3.txt (SignonFileName3)
PwMgr Storage: Reading passwords from /home/mozilla/.mozilla/firefox/k8mpm7jm.signon/signons3.txt
PwMgr Storage: Bad UTF8 conversion: https://bugzilla.mozilla.org (Acc�s reserv

Is this the correct expected output with the fix?

Comment 28

[Justin Dolske [:Dolske]]

Yes. It logs the line that's actually bad UTF8, and has successfully reset the converter so that other lines are not also logged as bad.

Comment 29

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

Marking as verified then.

Comment 30

[Douglas]

Is this bug a duplicate?
<https://bugzilla.mozilla.org/show_bug.cgi?id=451679>

Comment 31

[Justin Dolske [:Dolske]]

No, that bug was reported agains 3.0.1. The UTF8 issues were only in 3.0.2/3.0.3.

Comment 32

[LB.RudeWolf]

In reponse to comment 26:
« [e-mail], can you verify that this is fixed with a nightly 1.9.0.4
build from ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla1.9.0/? »

Tested with
Mozilla/5.0 (X11; U; Linux i686; fr-FR; rv:1.9.0.4pre) Gecko/2008103104 GranParadiso/3.0.4pre
and the test profile from last month, with the faulty (two missing bytes) signons3.txt.

I checked the profile to be faulty (no PMgr usage) with the same 3.0.3 as before.

I recovered the usage of the passwords manager with this new build. Here is the terminal output:
~$ Archives/firefox304pre/firefox -P --no-remote
FoxyProxy settingsDir = /home/rudewolf/firefox/profileFF3/ib3ctest.default
Login Manager: onStateChange accepted: req = http://start.ubuntu.com/8.04/, flags = 0x30004
Login Manager: onStateChange accepted: req = http://www.google.fr/firefox, flags = 0x30004
Login Manager: onStateChange accepted: req = http://www.mozilla.org/projects/firefox/3.0.4pre/whatsnew/, flags = 0x30004
Login Manager: domEventListener: got event DOMContentLoaded
Login Manager: Counting logins matching host: http://www.google.fr, formSubmitURL: , httpRealm: null
Login Manager: No alternate nsILoginManagerStorage registered
PwMgr Storage: Checking file signons3.txt (SignonFileName3)
PwMgr Storage: Reading passwords from /home/rudewolf/firefox/profileFF3/ib3ctest.default/signons3.txt
PwMgr Storage: Bad UTF8 conversion: http://tolkien.originet.eu (Acc�s reserv
Login Manager: domEventListener: got event DOMContentLoaded
Login Manager: Counting logins matching host: http://www.mozilla.org, formSubmitURL: , httpRealm: null
Login Manager: domEventListener: got event DOMContentLoaded
Login Manager: Counting logins matching host: http://start.ubuntu.com, formSubmitURL: , httpRealm: null
Login Manager: Getting a list of all logins
Login Manager: Getting a list of all logins
Login Manager: onStateChange accepted: req = about:blank, flags = 0x30004
Login Manager: domEventListener: got event DOMContentLoaded

I remarked the signons3.txt remained unmodified. As a consequence it keeps producing undesirable behaviour when used in conjunction with 3.0.3, but this shouldn't be a problem for a normal usage.

Comment 33

[LB.RudeWolf]

And I forgot the obvious: gratz, and thanks, for the good work.