316084 - Migrated base64 suite passwords not encrypted when master PW added in Firefox

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[eznie0ba]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7

I have always wanted to be able to 'export' my saved userid/password as standard text so I could encrypt/archive/use elsewhere. So, I started poking around the Firefox source code. 

I found some code in the Javascript module mozilla\extensions\wallet\signonviewer\resources\content\SignonViewer.js.

I took parts of that code and embedded into a local HTML file, which I load in the browser to generate an HTML document which contains a table of web sites, userids, and passwords.

I have never used a Master Password, so the HTML document is created automatically whenever I load the local HTML file.

As a test, I set a Master Password on my profile and loaded this local HTML file to output the userid/password information. I was prompted for the Master Password in order to continue. 

If I continuously to hit the Cancel button to dismiss the prompts for the master password, some records are still returned and displayed by the javascript, but not all of the userid/passwords are listed. In my setup, I have 62 userid/passwords, and it appears that around 14 userid/password records are returned if I continuously hit the Cancel button.

I am not sure, but I believe somewhere in the enumeration/query sequences, the password manager is prompting for the master password, but still manages to return some information if the master password popup is dismissed.

Maybe the javascript I have used is flawed, but, still some of the master password protected information is returned to the caller.

I am running using Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7 on a Windows 2000 Professional SP4 system.

Reproducible: Always

Steps to Reproduce:
1. Set a Master Password on a profile with saved userid/password information.
   (The more the better).
2. Load the sample HTML file from a local Windows directory.
3. When prompted for a Master Password, continuously click
   the Cancel button.
Actual Results:  
I would expect no data to be returned from the enumeration calls in the sample javascript, if the user cancels the master password popup.

Expected Results:  
Some of the password information was returned even though the master password prompt was being dismissed/cancelled without specifying a valid master password.

Using default Firefox theme.

I will attach my sample HTML document which demonstrates the problem.

Comment 1

[eznie0ba]

Attachment: Simple HTML file with embedded javascript to display userid/password records

Sample HTML file with embedded javascript to display saved userid/password records.

Comment 2

[Daniel Veditz [:dveditz]]

I cannot reproduce this using either 1.0.7 or 1.5rc2 on WinXP. If I grant UniversalXPConnect and cancel on the Master PW dialog (once per saved ID it looks like) nothing is displayed except the table header. If I enter the Master PW then I get all the records displayed as I expect.

If you do this multiple times is there a pattern to the data that is leaked? Is it the same every time? is it a mixture of ID's and Passwords, or all of one type? Are any completely ID/PW pairs retrieved? Only complete pairs?

Comment 3

[eznie0ba]

There is a pattern or regularity in the data returned.

I have 62 password records (host/user/password) saved in my default profile. If I have no master password set, all 62 records are returned/displayed in the same order every time, as expected.

If I set a master password, and run the embedded HTML script *and* provide the master password when prompted the first time, the same 62 records are returned in the same order.

If a master password is set and I load the HTML file with the embedded script, I will be prompted to enter the master password. I have the mouse pointer positioned over the Cancel button and just repeatedly hit the left mouse button.  I normally click rapidly, but as a test, I clicked the cancel button slow enough to be able to count the clicks. Curiously, I seem to count 48 clicks each time.

I always see the same 14 records returned every time, and these 14 records are complete and valid host/user/password records.

When I compare this partially leaked list to my full list of host/userid/password records, it appears that the records returned are always: 1, 3, 10, 11, 16, 19, 21, 29, 33, 34, 38, 40, 45, 61. These records are completely valid sets of host/userid/password data.

My system is an older Dell Dimension 4100 (900MHz PIII 256MB) running Windows 2000 SP4 with FF 1.0.7 with a typical (if there is such a thing) set of extensions and preferences.

I have access to a new Dell Dimension 4700 XP SP2 system (2.8GHz P4 512MB). I installed a clean FF 1.0.7 setup. I copied my master password protected files (cert8.db, key3.db, and signons.txt) into the new profile directory and tried to run the same HTML embedded script. The results were the same, the same 14 records were returned.

Comment 4

[eznie0ba]

I have done some additional testing and now suspect that my signons.txt and/or key3.db file(s) may be the source of the problem.

In all my testing, I have copied those two files to different profile directories on my own computer and two other computers. Each time I ran the script on the master password protected files and dismissed the password prompt, I always see the same 14 records 'leaked' when using those files.

I created a new user profile and proceeded to save passwords to a test FTP server I have available. After creating 62 entries and setting a master password, I tried the script and nothing was leaked.

My password information was originally imported from the Mozilla suite by FF 0.9 when I started using FF way back when. Over time, I have added/removed passwords, installed new versions of FF and always kept the same profile. I guess it is possible that over this time, something has gone out of whack which causes me to see the problem on my specific files. Although, the information stored is valid, as user/password prompts for these sites is valid.

Comment 5

[timeless]

the master password only protects new records because of a fear of datalose/leakage when you change your password. as such, anything from *before* you set/changed your password has the old guard (i.e. none). you should find that the leaked itesm are the oldest remembered items.

Comment 6

[eznie0ba]

I am not sure about that.

I have not added any new userid/password data since I started testing. I just simply tested the script by assigning a master password to my existing profile without adding/removing any passwords.

But, I guess there may be something specific to my password file(s).

Comment 7

[eznie0ba]

I believe I have found the source of my problem. For some reason, those records that were being 'leaked' by the embedded javascript appear to have only Base64 encoding that was used in earlier mozilla suite installations. Those user/password pairs in signons.txt was preceeded with a '~' and terminated with an '=' signon.

I am not sure why those were not encoded like the other user/password entries, but that explains the problem. I have removed them and then visited those sites to save the user/password information, and now those entries are no longer 'leaked'.

Comment 8

[Daniel Veditz [:dveditz]]

I was just composing a comment that guessed that might be the cause (since we are able to replay the data, after all), but I had doubts since they were scattered through the list. Timeless was guessing this too, based on comment 5.

Although this could be a privacy issue it's not one that can be induced by an attacker so I'm clearing the confidential flag.

Comment 9

[Justin Dolske [:Dolske]]

This was partially fixed in bug 381164 (although the code was wrong, so bug 400751 addressed that).

Old base64 entries are re-encrypted when they're used, since that's when the user would be entering the master password anyway (but we're don't flush to disk right then, so this is more of a opportunistic thing).

We should probably just move this code into the _upgrade_entry_to_2E() function, flush to disk, and be done with it.

Comment 10

[Serge Gautherie (:sgautherie)]

<http://mxr.mozilla.org/mozilla/search?string=upgrade_entry_to_2E&case=on&find=%2Ftoolkit%2F&filter=%5E%5B%5E%5C0%5D*%24&tree=mozilla>

It seems SM and TB need this.

Comment 11

[Justin Dolske [:Dolske]]

Bug 288040 will fix this -- when existing logins are imported into mozStorage, they'll be reencrypted.

Comment 12

[Justin Dolske [:Dolske]]

Now fixed by the landing of 288040.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

So, please tell me, is it now the case that passwords are no longer 
"opportunistically" (read: unreliably) encrypted?  Or is it still the
case that when the user sets a Master Password, he has no assurances 
that all his passwords have been encrypted, and may never have that 
assurance?

Comment 14

[Justin Dolske [:Dolske]]

The landing of bug 288040 caused all logins to be reencrypted when they're imported into the new sqlite backend. So this is currently fixed.

Unfortunately, doing that caused bug 451267, so we'll be reopening this (when 451267 lands) to fix it a different way. The tricky part of this problem is that we can't reencrypt entries without potentially prompting for a master password. 

What probably needs to happen is something along the lines of:

(1) pwmgr starts up, and opens signons.sqlite as normal
(2) determine if any logins are base64 encoded (flag in DB? make a query?)
(3) wait
(4) when pwmgr call is made that would require a MP entry anyway (addLogin, findLogins, etc), reencrypt any base64 encoded logins.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Is it really so unacceptable to tell the user "you have unencrypted passwords
and we're going to encrypt them now, so enter your master password so that 
we may do so" ?

Comment 16

[Justin Dolske [:Dolske]]

Reopening, since bug 451267 has landed and we lost the free fix (see previous comments).

Comment 17

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

I whipped up this patch (not working, just a proof-of-concept) as an approach to fix this problem explicitly. It's on top of bug 467463, which adds DB schema upgrade/downgrade code that makes this easier.

The basic idea: the first time the storage module successfully encrypts or decrypts something, it will check to see if there are any base64 encoded entries in the DB, and will reencrypt all of them. This avoids prompting for a master password at mysterious times.

Comment 18

[Dave Townsend [:mossop]]

Seems to be a serious enough privacy concern that we should fix this for 3.1

Comment 19

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 355265 [details] [diff] [review]
Patch v.1 (WIP)

>+    /*
>+     * _reencryptBase64Logins
>+     *
>+     * Checks the signons DB for any logins using the old wallet-style base64
>+     * obscuring of the username/password, instead of proper encryption. We're
>+     * called once per session, after the user has successfully encrypted or
>+     * decrypted some login (this helps ensure the user doesn't get mysterious
>+     * prompts for a master password, when set).
>+     */
>+    _reencryptBase64Logins : function () {
>+        this.base64checked = true;
>+        // Ignore failures, will try again next session...
>+
>+        try {
>+            // SELECT * FROM moz_logins WHERE encType = 0
>+            // TODO: extend_queryLogins() to do this? Or add new func?
>+            let [logins, ids] =
>+               this._queryLogins("", "", "", 0);

I think implementing bug 479894 before this will make this easier, and take care of that TODO item. That way we can query without having to add a new function just for this or modifying _queryLogins

Comment 20

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2 (Needs Tests)

Mostly just refinement of Dolske's patch, still need to write tests for it (but I'm sure it's working, I did my own console output testing).

Unit testing becomes a little difficult because we don't have any public methods that perform searches on all fields, so we'll have to create a storage instance to use (which is pretty heavy for a single thing to test). I still think that it would be worth implementing bug 479894 first so that we can do that.

Dolske - thoughts on that?

Comment 21

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.3

Now with tests & a little bit of cleanup.

Comment 22

[Justin Dolske [:Dolske]]

Comment on attachment 365105 [details] [diff] [review]
Patch v0.3

>+        // Determine encryption type
>+        let encType = (encUsername.charAt(0) == '~' ? 0 : 1);

Hmm, so, in theory one could have a username that's encrypted but a password that's just base64 encoded...

I'm not sure this can actually ever happen, but the old password manager code (*shudder*) did attempt to only reencrypt the password when changing the password for an existing entry. FF2 only reencrypts with SecretDecoderRing, but if some earlier version selectively used base64...

Probably just best to set encType to 0 if one or the other is base64, to ensure we're catch it. The code archeology to prove it either way isn't worth the effort.

Also, maybe have the code in addLogin() only compute encType when isEncrypted is true, and force it to 1 for normal usage? Not sure if that helps clarify when this can happen, or just makes things messy.

>+    _dbMigrateToVersion3 : function () {
...
>+        query = "SELECT id, encryptedUsername FROM moz_logins WHERE encType isnull";

Same issue.

>+            stmt = this._dbCreateStatement(query);
>+            while (stmt.step())
>+                logins.push([stmt.row.id, stmt.row.encryptedUsername]);

It would be cleaner to just compute the encType here, instead of carrying around encryptedUsername (and now encryptedPassword too).

>     _dbAreExpectedColumnsPresent : function () {

Can you add a comment, up in _dbSchema at the end of moz_logins, pointing out this function needs changed? That way it's less likely future column additions might forget to add the check here.


>diff --git a/toolkit/components/passwordmgr/test/unit/data/signons-v999.sqlite b/toolkit/components/passwordmgr/test/unit/data/signons-v999.sqlite
>index e96a9910d3a46278ac97754b15af8c3e8b196099..1a125d39533d17097943531babb424ee78e2105c

Why is signons-v999 changing? Oh, I guess to just add the encType column.


>+++ b/toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_3.js
...
>+/* ========== 16 ========== */

Add another flavor of this test that imports from signons-380961-3.txt, checks for the base64 logins, calls countLogins, and then checks that the base64 logins still exist. [ie, confirm we don't reencrypt for countLogins]. Just paranoia, but easy cut'n'paste.


>+++ b/toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_migrate.js

Need migration tests here:

* signons-v2.sqlite
  Containing logins with one/both/neither of user+pass using base64. Import it, check that encType is set to expected value for each row.

* signons-v2v3.sqlite
  Containing a v3 DB that's been downgraded to v2, with 1 login added (which has encType == null). Import it, check that that row gets expected encType value.

Comment 23

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.4

Addresses previous comments

Comment 24

[Justin Dolske [:Dolske]]

Comment on attachment 367086 [details] [diff] [review]
Patch v0.4

>--- a/toolkit/components/passwordmgr/src/storage-mozStorage.js
...
>-    // The current database schema
>+    // The current database schema. Changes must be reflected in
>+    // this._dbAreExpectedColumnsPresent

nit: literally add this right at the end of moz_logins, so that the next time a column is added someone will be staring at this line, and it shows up in the diffs.


>--- a/toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_3.js
...
>+/* ========== 16 ========== */
...
>+/* ========== 16 ========== */

Too much cut and paste! 17. :-)


>--- a/toolkit/components/passwordmgr/test/unit/test_storage_mozStorage_migrate.js
...
>+function getEncTypeForID(conn, id) {
>+    dump("SELECT encType from moz_logins WHERE id = " + id + "\n\n");

Remove the dump.


One final nit... I guess it would be helpful to replace the magic numbers (0 and 1) the JS uses for encType with some const aliases. Say, something like |const ENCTYPE_BASE64 = 0| and |const ENCTYPE_SDR = 1|.

Comment 25

[Justin Dolske [:Dolske]]

Comment on attachment 367086 [details] [diff] [review]
Patch v0.4

Bouncing over to gavin.  Since a fair chunk of this is based on my original patch, and I shouldn't technically review my own code.

Comment 26

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

Comment on attachment 367086 [details] [diff] [review]
Patch v0.4

Agree with dolske about using named constants for encType. Also seems like the "Check that we do have 2 base64 logins here" code should be factored out in the test rather than repeated 3 times, but that's not a big deal.

Comment 27

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.5 (for checkin) [Checkin: Comment 29]

Made encryption types constants & extracted the encryption counting into a method in tests.

Comment 28

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.5 (for checkin: mozilla-1.9.1)

Clean patch for branch (just updates a few offsets, nothing major).

Comment 29

[Serge Gautherie (:sgautherie)]

Comment on attachment 369296 [details] [diff] [review]
Patch v0.5 (for checkin)
[Checkin: Comment 29]


http://hg.mozilla.org/mozilla-central/rev/443975920e1b

Comment 30

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dcb770c209ac