451267 - Master password dialog is shown on each website with at least one text field due to sqlite import

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080819020844 Minefield/3.1a2pre ID:20080819020844

Even if there is no username/password to fill in the master password dialog pops-up each time you load a page. Looks like that this is a regression from the fix on bug 288040.

Steps:
1. Set a master password (and store some passwords)
2. Restart Firefox
3. Open http://www.google.com

=> There is no need for a user/password combination but the master password dialog is displayed. That's really annoying because on each page it is shown again until you have entered the correct master password.

Comment 1

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

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080819020844 Minefield/3.1a2pre

I can't seem to reproduce this.

Are you doing this with a new profile, or a profile with signons prior to the change for bug 288040?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Paul, yes these are existing profiles. All of them have a signons3.txt but no signons.sqlite inside their folder. It looks like that the latter one will not be created. Following errors appear inside the Error Console after canceling the master password dialog:

Error: [Exception... "'User canceled Master Password entry' when calling method: [nsILoginManagerStorage::getAllLogins]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: file:///Users/henrik/Desktop/Minefield.app/Contents/MacOS/components/storage-mozStorage.js :: anonymous :: line 750"  data: no]
Source File: file:///Users/henrik/Desktop/Minefield.app/Contents/MacOS/components/storage-mozStorage.js
Line: 750

Error: [Exception... "'Initialization failed' when calling method: [nsILoginManagerStorage::countLogins]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: file:///Users/henrik/Desktop/Minefield.app/Contents/MacOS/components/nsLoginManager.js :: anonymous :: line 486"  data: no]
Source File: file:///Users/henrik/Desktop/Minefield.app/Contents/MacOS/components/nsLoginManager.js
Line: 486

If you still cannot reproduce this issue I should have a look again later today and try to find out why that happens. There is no add-on involved. Each one is disabled.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Paul, I can even reproduce it on my Windows box at work. Just using an existing profile from Firefox 3.0.x and starting it directly with Minefield nags me each time I open a website with a contained text field.

Comment 4

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

Henrik, I know exactly what you're talking about now. This was actually a conscious decision. What we try to do is import the legacy signons the first time password manager gets used, which is on first page load. This requires the master password if it was set.

If it fails, we defer initialization (and thus import) until the next time password manager gets used. Since countLogins is run on every page load, you'll actually get the prompt on every page load, not just ones with text boxes (I think, though maybe just with textboxes - I didn't think we looked at forms until after countLogins)

I agree that this is annoying, but we figured it was such an edge case, and that it was more important to import the signons that it was a tradeoff we were willing to make.  If you have other suggestions though, I'd love to hear them.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Loading pages without a text field (form) the dialog doesn't pop-up. For example you can load: http://labs.google.com/suggestfaq.html

Any Firefox 3.0.x user with a master password set will be confronted with this bug when upgrading to Firefox 3.1 at some time. If the conversion has to be done, we could probably wait until the user requests the user/password data the first time, e.g. fill-in to text fields or just open show passwords. IMHO there is no need to do this earlier and nagging the user all the time. I know it's an edge case and it's up to you if it will be more user-friendly or not. 

Comment 6

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

You're right, it's only on pages with forms. But the way password manager works is if a form is found, we call countLogins, which needs the data to be imported first. Thus the problem you're encountering. Maybe some sort of alert could be shown telling the user to go to options in order to import if we fail the first time but that feels awkward and would add clutter to UI. Ideally modifying the master password prompt to say that we're importing would be best so users at least know why we keep asking.

Justin- I know we talked about this before, but can you think of another way we might go about all this? Otherwise we might have to wontfix this.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Paul, so it also should happen that way when signon.autofillForms is set to false? I thought that I can explicitly say: Please don't nag me with any master password dialog or pre-filled form unless I want that by double clicking the text field. 

Comment 8

[Justin Dolske [:Dolske]]

I suspect this is just due to page loads calling countLogins, which is enough to trigger the limport-from-legacy process. We could make this a little nicer by first calling legacy.countLogins(), and deferring the import (again) if the result is 0.

Comment 9

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

(In reply to comment #7)
> Paul, so it also should happen that way when signon.autofillForms is set to
> false? I thought that I can explicitly say: Please don't nag me with any master
> password dialog or pre-filled form unless I want that by double clicking the
> text field. 
> 

So that pref isnt checked until _fillForm (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/nsLoginManager.js#993), which is called from
_fillDocument (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/nsLoginManager.js#947) - which gets fired on page load. This way we can notify observers that we _can_ fill a form, but didn't (and enable the listener to fill it).

Maybe we can change when this pref gets checked and bail out before countLogins get called. An extension could come in and handle the "double click" or other action (which they already can do in trunk). It redefines the meaning of the pref just a little - the result is still the same to end users, but might effect extension devs (maybe?)

I think that might be a better solution than checking legacy if import fails... that would just make the logic too convoluted, and seems like too much confusion to catch this one case.

As much as I can see this being annoying, it only happens on the very first time you use 3.1, unless you keep canceling. And once the user does put in their password, we're just maintaining code that is hardly used.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to comment #9)
> As much as I can see this being annoying, it only happens on the very first
> time you use 3.1, unless you keep canceling.

It's not just annoying -- it's downright scary.  This bug's behavior is NOT consistent with how I expect Firefox to behave, which initially made me wonder if some extension (or spyware, or an evil patch that made it into the nightlies, or something) was trying to steal my passwords.  And that *definitely* didn't make me want to enter my master password.  So I kept hitting this bug (and being annoyed by it), until I figured out what was going on.

Keep in mind that this isn't just any old dialog.  When dealing with something as sensitive as the master password, which protects *so much* sensitive data, Firefox should be VERY careful not to act in a way that appears buggy / bizarre / questionable.  IMHO this bug is a major issue.

Comment 11

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

Ok, so since there was more complaining about this, I guess something should be done. Based on discussion in #firefox, I think we have two viable choices:

1. Try to import. If the user cancels master password, revert to using the legacy storage for that session. On next session start, try again. Repeat.

2. Try to import. On failure revert to legacy until the user has to enter master password (on the few pages the user actually uses master password - traditional behavior). After master password is entered, import.

Path one is the path of least resistance, but may take a while for the user to actually import and switch over. Path 2 is more likely to get the user switched over, but will be much trickier to implement.

Either way it seems like this should definitely get in before 3.1 or there will be much backlash from the security conscious.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Daniel, that's true. Thinking about your words gives me the feeling that all other users who intent on their security will also be scared about what's going on. We are doing something with their privacy without informing them. Lets add the privacy keyword to reflect the concerns of users with this hidden feature.

Comment 13

[Justin Dolske [:Dolske]]

Talked with Paul, we'll try something along the lines of comment 8.

This isn't a privacy issue.

Comment 14

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

Just a quick update - I have a version of this "working" but still has major flaws (like always using legacy until something other than countLogins is called). I should have something up later this week; school decided to start killing me right after I started.

Comment 15

[Travis Saling]

At the risk of making an irrelevant comment: I was seeing this exact same behavior (which I came to report here), but eventually noticed in the status bar that whenever a spurious master password dialog popped up, the Foxmarks add-on was trying to synch my bookmark information. When I disabled Foxmarks, the problem went away.

Comment 16

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

Attachment: Patch v0.1

This should fix the issue. It will now use the legacy storage module in countLogins iff we haven't created the sqlite file. It will use the legacy storage just for countLogins, and only until we initialize.

This should only effect upgrade use cases until the user enters their master password at a prompt. Once the master password prompt comes up, it means we're importing, and then continuing. The new mozStorage module will be used from then out.

This change also has the added benefit of now filling the form on the page when import occurs.

Not asking for review yet since I haven't run unit tests (have to do full build & figured I would get it up ASAP)

Comment 17

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

Comment on attachment 338378 [details] [diff] [review]
Patch v0.1

unit tests all pass, requesting review

Comment 18

[Justin Dolske [:Dolske]]

Comment on attachment 338378 [details] [diff] [review]
Patch v0.1

>     countLogins : function (hostname, formSubmitURL, httpRealm) {
>+        // Use legacy storage if we haven't created DB file yet
>+        if (!this._signonsFile.exists()) {
>+            this.log("Using legacy storage to countLogins");
>+            return this._legacyStorage.countLogins(hostname, formSubmitURL, httpRealm);
>+        }
>+
>         this._checkInitializationState();

Hmm. A couple issues here:

* It's kind of sucky to be doing a exists() on each call (which is presumably going to be doing a stat() on the directory, which isn't the best thing to be doing perf wise). Ideally we would want to just have a boolean to let us know if we've imported the old DB yet or not. That would be a bit more direct and understandable, too.

* Another crappy situation here is people who don't have any stored logins. With this scheme, we'll never end up migrating from the legacy format (ie, we'll be deferring the import indefinitely)... I'd rather get people migrated onto the new code in a timely manner, and this might interfere with adding new features to the mozstorage bits.

I'm almost starting to think it might be simpler to add a getAllEncryptedLogins() API to nsILoginManagerStorage... That would avoid triggering a master password, but we'd have to reopen bug 316084.

Comment 19

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

(In reply to comment #18)
> Hmm. A couple issues here:
> 
> * It's kind of sucky to be doing a exists() on each call (which is presumably
> going to be doing a stat() on the directory, which isn't the best thing to be
> doing perf wise). Ideally we would want to just have a boolean to let us know
> if we've imported the old DB yet or not. That would be a bit more direct and
> understandable, too.

Agreed. I'll try to come up with some way of doing this.

> * Another crappy situation here is people who don't have any stored logins.
> With this scheme, we'll never end up migrating from the legacy format (ie,
> we'll be deferring the import indefinitely)... I'd rather get people migrated
> onto the new code in a timely manner, and this might interfere with adding new
> features to the mozstorage bits.
> 
> I'm almost starting to think it might be simpler to add a
> getAllEncryptedLogins() API to nsILoginManagerStorage... That would avoid
> triggering a master password, but we'd have to reopen bug 316084.

Might be an alternative. Though we'd have to modify how some other things work (namely addLogins). Since we'd be importing the encrypted passwords, we wouldn't want to re-encrypt them. If there's some way we can check the login, then that would take care of both issues (since on addLoing it would get encrypted if it's not already, else just save).

Comment 20

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

Hmm, I get the feeling this might get a bit more traffic with this still not done for beta1. Maybe I'll find some free time tonight to work on this and see if we can get it landed before the freeze, but I'm not banking on it.

Justin - Should we take that route and add the getAllEncryptedLogins() interface? Keep in mind we would have to reopen that bug and my thoughts in comment #19...

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Paul, any ETA on that part? Looks like you weren't able to finish it for beta1.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Justin can you give a status update? Paul seems to be off and it looks like that this one will even not get it into beta2. Can you also please answer the question comment 20? Thanks.

Comment 23

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

If we don't go ahead with Justin's last idea from comment #18, then this can probably be a relatively painless change from attachment 338378 [details] [diff] [review] to store the boolean somewhere as opposed to checking the file every time. I started to do some work on this, then school caught up with me and it's not quite done.

If we do go ahead with this using the general process from the patch, we still have the problem of not bringing users up to date (as Justin pointed out). Maybe we can check if they have any saved passwords at all, and if not just "import" - that shouldn't cause a MP dialog since nothing needs to be decrypted/encrypted. That may take care of some of the users, and the others will just have to wait until they get to a page where they have a password before the upgrade process happens. None of the mozStorage features will be changing before v.next, so maybe it's a safe enough move, and we can worry about that then.

That said, I haven't heard a whole lot from anybody using else using nightlies or Beta 1 having any issues with this. I would have expected a rather loud group to cry out in anger. Maybe Beta 2 will result in more community feedback.

Comment 24

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

Clear regression that we should either band-aid or fix, IMO.

Comment 25

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

Attachment: Patch v0.2

Per discussion with Dolske, I went ahead and added the getAllEncryptedLogins API, and use that when importing. This method has not been implemented in storage-mozStorage, but if we should, then that's ~ 2 minutes work.

Bug 316084 may have to be reopened since base64 (and all) logins are now imported in the encrypted state. They do get opportunistically reencrypted when they are encountered in _decryptLogins, so they will get upgraded ASAP.

This patch ensures that all users will be upgraded to using storage-mozStorage.

Comment 26

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

Dolskinator: review, bitte?

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

While running tests with the beta2 I was able to stop Gmail from working. These are simple steps:

1. Use an older Firefox 2/3 profile with saved passwords and the master password enabled.
2. Start beta 2
3. Log into Gmail and don't enter the master password!
4. Click a heading to open the message
5. Click on e.g. delete at the top

Even after step 4 you will be asked twice for the master password. If you cancel both dialogs nothing will happen. A second click opens the message finally. Doing a click on "Delete" stops Gmail from working. You can click on each of the links but none will trigger an action.

Following errors are shown:

Error: [Exception... "'User canceled Master Password entry' when calling method: [nsILoginManagerStorage::getAllLogins]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: file:///C:/Program%20Files/Mozilla%20Firefox/components/storage-mozStorage.js :: anonymous :: line 784"  data: no]
Source File: file:///C:/Program%20Files/Mozilla%20Firefox/components/storage-mozStorage.js
Line: 784

Error: [Exception... "'Initialization failed' when calling method: [nsILoginManagerStorage::getLoginSavingEnabled]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: file:///C:/Program%20Files/Mozilla%20Firefox/components/nsLoginManager.js :: anonymous :: line 536"  data: no]
Source File: file:///C:/Program%20Files/Mozilla%20Firefox/components/nsLoginManager.js
Line: 536

Error: Permission denied for <http://www.google.de> to call method UnnamedClass.toString on <>.

Error: uncaught exception: unknown (can't convert to string)

Comment 28

[Justin Dolske [:Dolske]]

Comment on attachment 348358 [details] [diff] [review]
Patch v0.2

>--- a/toolkit/components/passwordmgr/src/storage-Legacy.js
...
>+        for each (var hostLogins in this._logins) {
>+            // Return copies to the caller. Prevents callers from modifying
>+            // our internal stoage, and helps avoid keeping decrypted data in
>+            // memory (although this is fuzzy, because of GC issues).

The last half of this comment ("and helps...") doesn't apply here.

s/stoage/storage/.


>--- a/toolkit/components/passwordmgr/src/storage-mozStorage.js
...
>+    _addLogin : function (login, isEncrypted) {
>+        let userCanceled, encUsername, encPassword;
>+        if (isEncrypted) {
>+            [encUsername, encPassword] = [login.username, login.password];
>+        } else {
>+            // Throws if there are bogus values.
>+            this._checkLoginValues(login);

Move the _checkLoginValues() up (so it's always called). It shouldn't matter much (because already-encrypted values are coming straight from legacy storage), but it might help catch imports of badly corrupted files.

>@@ -854,16 +858,30 @@
>+            // If the login is base64, opportunisitically reencrypt it.

s/opportunisitically/opportunistically/

How did all these misspellings get into the code you cut'n'pasted?! :)

>+            // May cause minor perf slowdowns, but only for old base64 logins

Comment not needed, since this is just a one-time hit.

>+            let isBase64 = login.username.charAt(0) == '~' ||
>+                           login.password.charAt(0) == '~';

You'll need to move this test up a few lines, because |username| and |password| are the decrypted values at this point.

Add a test that exercises this code path. (import from a legacy file with base64 values, check that the imported login has the expected cleartext user/pass, poke at the DB to confirm that the base64 encoding was removed).

>+            if (isBase64) {
>+                let newLogin = Cc["@mozilla.org/login-manager/loginInfo;1"].
>+                createInstance(Ci.nsILoginInfo);

Indent. But... I think you don't need to create a new login anyway. |this.modifyLogin(login, login);| should suffice.

Maybe wrap this in a try/catch block, lest the modifyLogin throw? It's rather unlikely to happen, but there's more code being invoked now (vs how the legacy module did it), so doing so might be good anyway.

Side note: can we set a global flag on the DB (similar to |PRAGMA user_version|)? Not required to fix this bug, but the complete fix for the base64 issue might be to have a "base64_migrated" flag, and if it's false _decryptLogins could just make a one-time fixup to the whole DB.


>     _decrypt : function (cipherText) {
...
>+            if (cipherText.charAt(0) == '~') {
>+                // The older file format obscured entries by
>+                // base64-encoding them. These entries are signaled by a
>+                // leading '~' character.

I suppose this comment is inaccurate now, because the "older format" here isn't storage-Legacy. "The old Wallet file format..."


Oh, one more thing. Importing legacy logins this way means we can get rid of _deferredInit() [by merging it back into init()], and with it _checkInitializationState().

Comment 29

[Justin Dolske [:Dolske]]

(In reply to comment #27)
> While running tests with the beta2 I was able to stop Gmail from working.

I don't think that has anything to do with this bug. The chrome exceptions are normal and expected when canceling a master password, and shouldn't have any effect on content. More likely a glitch, or some general problem with B2 and Gmail.

Comment 30

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

I'm busy graduating this week into next, so I won't have time to work on this before maybe Thursday...

Since b3 freeze is looking like January, this won't be too much of a problem will it?

Comment 31

[Justin Dolske [:Dolske]]

Should be fine.

Comment 32

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

Attachment: Patch v0.3 - WIP

This is a WIP (things just commented out). My xpcshell keeps crashing and I think it's something with my OSX, so until I figure that out, I can't run unit tests.

Could somebody test this for me?

This addresses the changes from comment #28.

Regarding PRAGMA - I don't think so. I'm not sure if we can define our own pragmas but I'll look into it.

Regarding unit tests for base64 - there is a test for it already: tests 3 & 4 in test_storage_mozStorage_3

Comment 33

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

Attachment: Patch v0.4

Ok, here it is. Addresses the issues, but takes out the opportunistic reencryption for base64 logins - which means we'll have to reopen bug 316084.

Some tests had to be modified to account for when errors get thrown. Small change to test framework (because for errors weren't bubbling out like they were supposed to).

Comment 34

[Justin Dolske [:Dolske]]

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

>--- a/toolkit/components/passwordmgr/src/storage-Legacy.js
...
>+    getAllEncryptedLogins : function (count) {

storage-mozStorage.js should have a dummy stub for this too.

>+            // Return copies to the caller. Prevents callers from modifying
>+            // our internal storage, and helps avoid keeping decrypted data in
>+            // memory (although this is fuzzy, because of GC issues).

Oops, this comment (added with patch) was the one that should have been updated, not the original comment that you modified below.

>             // Return copies to the caller. Prevents callers from modifying
>-            // our internal stoage, and helps avoid keeping decrypted data in
>-            // memory (although this is fuzzy, because of GC issues).
>+            // our internal storage

See previous.

>--- a/toolkit/components/passwordmgr/src/storage-mozStorage.js
>-    _deferredInit : function () {
>         let isFirstRun;
>-        // Check that we are not already in an initializing state
>-        if (this._initializing)
>-            throw "Already initializing";
...
>             this._initialized = true;

._initialized and ._initializing can be removed from the prototype now.

r+ with these nits. Can you attach an updated patch? (Find me on IRC, if I find time I just might go ahead and fix the nits myself and check it in).

Comment 35

[Justin Dolske [:Dolske]]

(In reply to comment #33)
> ... takes out the opportunistic reencryption for base64 logins - which
> means we'll have to reopen bug 316084.

Just a note to self: Paul and I had discussed this. Since we're going to need a solution for 316084 to take care of base64 logins (because mailnews is switching to password manager, and such logins are common there), we won't need the opportunistic stuff. There will be a single mechanism to handle this. I've got a plan. :)

Comment 36

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

Attachment: Patch v0.5

Comment 37

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

Comment on attachment 354763 [details] [diff] [review]
Patch v0.5

Since I probably should ask for review even if the changes were just nits.

Comment 38

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/2abbd4951630

And realized right after pushing that the interface uuid should have been updated too: http://hg.mozilla.org/mozilla-central/rev/87d32ea05ca2

Comment 39

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7816ad83f5ac

Comment 40

[Aakash Desai [:aakashd]]

Verified FIXED on build agent:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3) Gecko/20090304 Firefox/3.1b3 ID:20090304214853

Comment 41

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

(removing relnote keyword as this bug is fixed for b3)

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Marking verified based on comment 40.