186237 - manage stored passwords preference option does not work in this build

Status: RESOLVED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[L.W. Brown]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.2.1) Gecko/20021130
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.2.1) Gecko/20021130

Preferences -> Privacy & Security -> Passwords -> Manage Stored Passwords:
gets either no reaction at all after the tiny B&W spinning icon appears (and
even it does not spin), or only asks for master password & then stops (again,
the tiny icon appears but does not spin)

Reproducible: Always

Steps to Reproduce:
see above
Actual Results:  
see above

Expected Results:  
it worked prior to V.1.2

Comment 1

[Matthias Versen [:Matti]]

.

Comment 2

[John Unruh]

If you have deleted your Security folder, or some of the files in it, while at
the same time selecting "Use Encryption when storing sensitive data" in the
prefs, this can be the result. If that is the case, this is a dupe of another bug.

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Reassigning unconfirmed form and password manager bugs to the new owner

Comment 4

[John Unruh]

A workaround seems to be to be to deselect "Use Encryption when storing 
sensitive data" and click Ok, then go back in and reset your Master Password.

Comment 5

[Nicolas Pioch]

Have this problem on Win32 (windows 2000 sp3) with Mozilla/1.2.1

workaround doesn't work -- if I unselect "Use encryption to store passwords"
and click OK, I get "Unable to convert stored data"
after the popup window asking for the master password for the software security
device.

Comment 6

[Nicolas Pioch]

this seems related to bug 173568
as I'm experiencing both.

Probably a duplicate ?

Comment 7

[Nicolas Pioch]

Also I wouldn't be too surprised if this was related to bug 169777
and general XUL FastLoad corruption, even if the symptoms are different
(dialog won't show up, but not eating 100% CPU and hogging memory)

Will have to see if fixes such as bug 188744 change the reproductibility
of this bug.

Comment 8

[Paul Fischer]

This looks the same as bug 191622. I have added a lot of info to bug 191622
and this one should probably be considered a duplicate. Or 191622 is a dupe.
Either way they look the same to me.

Comment 9

[Paul Fischer]

I got a duplicate in my .s file when I re-entered a password in 1.3b. When I
removed both entries for that site I was able to use the workaround.

Comment 10

[David Barr]

Resetting the Master Password and de-selecting password encryption does allow
the "Manage Stored Password" window to open.

No modifications have been made to the security folder, so that doesn't appear
to be a factor here.

Comment 11

[David Barr]

Ack! My comment that I'm seeing this didn't appear to take. I'm seeing the same
problem under build 2003030311:

Mozilla 1.4a
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4a) Gecko/20030303

Comment 12

[Benedikt Kantus]

Maybe bug 203663 is of importance. Just a thought.

Comment 13

[Nicolas Pioch]

bug 203663 is a duplicate of this one -
check comment 5 above, the OS should be changed on this bug from Macintosh only
to at least Mac & Win32.

Comment 14

[Michael Eibl]

The bug is still there in 1.4RC2 (Mozilla/5.0 (Macintosh; U; PPC Mac OS X
Mach-O; en-US; rv:1.4) Gecko/20030612)

Regards,
Michael

Comment 15

[Corey Leighton]

Its caused by a corrupt entry in the password file.  By moving the entries off
one by one, I isolated three different entries that were causing this issue. 
From memory, all three seem to have been created around the same time, in a
long-ago forgotten build.

This causes problems in both Mozilla 1.4 and Firebird 0.6, on Windows and Mac
platforms, which lead me to think its the file and not the app.

The work around is as follows:  1) Backup the password file in the profile
directory.  2) Remove entries one by one (or groups if you have many entries),
keeping track of which ones you remove, using a text editor, save, then reload
the browser and go into the password manager.  Eventually, you will pick out the
entries that cause problems.

Comment 16

[Mabry Tyson]

For normal users, this workaround is virtually useless.  It seems better to just
cause all the passwords to be flushed by reseting the master password BUT this
flushes much more (like certificates).

This is still causing headaches as of 1.5a.  If you virtually never put in a bad
password so you don't have to remove bad passwords, this may bite you much later
when you have lots of passwords.  I never used any of the nightly builds so it
was a release (or alpha/beta/release candidate) that caused these problems.

According to the initial bug report, the "long-ago forgotten build" that was
buggy is less than one year old (between 1.1 and 1.2.1).  I know I used 1.2a and
wonder if that was it.  Fall 2002 is consistent with the bad passwords I found.

Editing the file was a VERY painful workaround.  I had 33 bad passwords and 48
good ones. It took a very long time to work through them with that kind of
density of bad ones. Too bad the code fails in a useless and painful way rather
than just dropping passwords that currently cause it to fail.  [Each bad
password had to be tested by having it be the only password in the file.  So I
restarted Mozilla at least 33 times and probably closer to 80 times.]

For others suffering this problem who try to do the workaround...  On MacOSX,
the file was at
  ~/Library/Mozilla/Profiles/<Mozilla_Id>/<random>.slt/*.s
The lines before the first "." line are the sites for which passwords aren't
stored.  After that, each set of lines between the periods has the URL and then
pairs of lines with input name and encrypted value.   Multiple entries for the
same URL may be either different inputs or a good set followed by a bad set. 
That is, for some URLs that were duplicated, the first was valid while the
second caused Manage Stored Passwords to fail.  There were other sets that were
bad which were the only entry for that URL.

Comment 17

[Mike Calmus]

Attachment: Fix to better deal with corrupt passwords

This patch lets the Password Manager ignore corrupt entries when parsing the
password file. Instead of not showing the password manager when an error
occurs, it is now shown. Also, the decrypt routine will work properly when
corrupt entries exist so the database can be converted from encrypted to
unencrypted.

Comment 18

[Mabry Tyson]

FYI:  Netscape 7.01 (MacOSX) creates corrupt passwords.

I had a set of passwords that I could manage with Moz 1.5a.  I then started up
NS 7.01 and checked my email and sent mail.  I was prompted for passwords for
SSL for SMTP and IMAP which were already stored (from Mozilla 1.5a).  It added
new entries for those passwords (leaving the old ones intact). Those new entries
then prevented Moz 1.5a from managing the stored passwords.  Deleting the new
entries then allowed Moz 1.5a to manage the stored passwords.

I tried NS 7.1 (starting from a valid set of passwords) and it didn't prompt me
for the passwords, so I suspect 7.1 is ok.

(Thanks for working towards ignoring corrupt entries.)

Comment 19

[Nicolas Pioch]

Sharing a profile between Netscape (any version) and Mozilla
is specifically documented as something NOT to do anyway,
so the just-above corruption isn't a valid test case...

Check the release notes...
Netscape should have its own profile directory.

Moving a stored password file from Mozilla to Netscape 7.0
and back to Mozilla doesn't sound like a good idea.

Comment 20

[Daniel Veditz [:dveditz]]

*** Bug 203663 has been marked as a duplicate of this bug. ***

Comment 21

[Mike Calmus]

Comment on attachment 130070 [details]
Fix to better deal with corrupt passwords

Index: extensions/wallet/src/nsPasswordManager.cpp
===================================================================
RCS file: /cvsroot/mozilla/extensions/wallet/src/nsPasswordManager.cpp,v
retrieving revision 1.16
diff -u -r1.16 nsPasswordManager.cpp
--- extensions/wallet/src/nsPasswordManager.cpp 7 Sep 2003 22:05:28 -0000     
1.16
+++ extensions/wallet/src/nsPasswordManager.cpp 14 Sep 2003 01:15:27 -0000
@@ -66,6 +66,8 @@
       PRUnichar * pswd;
       nsresult rv = SINGSIGN_Enumerate(mHostCount, mUserCount++, &host, &user,
&pswd);
       if (NS_FAILED(rv)) {
+	 mUserCount = 0;
+	 mHostCount++;
	 return rv;
       }
       if (mUserCount == SINGSIGN_UserCount(mHostCount)) {
@@ -222,9 +224,11 @@
   // Emumerate through set of saved logins
   while (hasMoreElements) {
     rv = enumerator->GetNext(getter_AddRefs(passwordElem));
+    /*
     if (NS_FAILED(rv)) {
       return rv;
     }
+    */

     if (NS_SUCCEEDED(rv) && passwordElem) {

Index: extensions/wallet/signonviewer/SignonViewer.js
===================================================================
RCS file: /cvsroot/mozilla/extensions/wallet/signonviewer/SignonViewer.js,v
retrieving revision 1.52
diff -u -r1.52 SignonViewer.js
--- extensions/wallet/signonviewer/SignonViewer.js	24 Sep 2002 20:45:53
-0000	   1.52
+++ extensions/wallet/signonviewer/SignonViewer.js	14 Sep 2003 01:17:12
-0000
@@ -82,7 +82,6 @@
   kSignonBundle = document.getElementById("signonBundle");
   var element;
   if (isPasswordManager) {
-
     // be prepared to reload the display if anything changes
     kObserverService =
Components.classes["@mozilla.org/observer-service;1"].getService(Components.int
erfaces.nsIObserverService);
     kObserverService.addObserver(signonReloadDisplay, "signonChanged", false);
@@ -236,39 +235,41 @@
     var nextPassword;
     try {
       nextPassword = enumerator.getNext();
-    } catch(e) {
-      /* user supplied invalid database key */
-      window.close();
-      return false;
-    }
-    nextPassword =
nextPassword.QueryInterface(Components.interfaces.nsIPassword);
-    var host = nextPassword.host;
-    var user = nextPassword.user;
-    var rawuser = user;
-
-    // if no username supplied, try to parse it out of the url
-    if (user == "") {
-      var unused = { };
-      var ioService = Components.classes["@mozilla.org/network/io-service;1"]
-		     .getService(Components.interfaces.nsIIOService);
-      var username;
-      try {
-	 username = ioService.newURI(host, null, null).username;
-      } catch(e) {
-	 username = "";
+
+      nextPassword =
nextPassword.QueryInterface(Components.interfaces.nsIPassword);
+      var host = nextPassword.host;
+      var user = nextPassword.user;
+      var rawuser = user;
+
+      // if no username supplied, try to parse it out of the url
+      if (user == "") {
+	 var unused = { };
+	 var ioService =
Components.classes["@mozilla.org/network/io-service;1"]
+		       .getService(Components.interfaces.nsIIOService);
+	 var username;
+	 try {
+	   username = ioService.newURI(host, null, null).username;
+	 } catch(e) {
+	   username = "";
+	 }
+	 if (username != "") {
+	   user = username;
+	 } else {
+	   user = "<>";
+	 }
       }
-      if (username != "") {
-	 user = username;
-      } else {
-	 user = "<>";
+
+      if (encrypted) {
+	 user = kSignonBundle.getFormattedString("encrypted", [user], 1);
       }
-    }

-    if (encrypted) {
-      user = kSignonBundle.getFormattedString("encrypted", [user], 1);
+      signons[count] = new Signon(count++, host, user, rawuser);
+    } catch(e) {
+      /* user supplied invalid database key or
+	  an entry is corrupt. Go to next element. */
+      //window.close();
+      //return false;
     }
-
-    signons[count] = new Signon(count++, host, user, rawuser);
   }
   signonsTreeView.rowCount = signons.length;

Index: extensions/wallet/src/singsign.cpp
===================================================================
RCS file: /cvsroot/mozilla/extensions/wallet/src/singsign.cpp,v
retrieving revision 1.235
diff -u -r1.235 singsign.cpp
--- extensions/wallet/src/singsign.cpp	30 Jul 2003 22:44:53 -0000	1.235
+++ extensions/wallet/src/singsign.cpp	14 Sep 2003 01:17:58 -0000
@@ -2938,7 +2938,8 @@
			       user->signonData_list.ElementAt(k));
	 nsAutoString userName;
	 if (NS_FAILED(si_Decrypt(data->value, userName))) {
-	   return PR_FALSE;
+	   //Ignore the error and go to the next one.
+	   //return PR_FALSE;
	 }
	 if (NS_FAILED(si_Encrypt(userName, data->value))) {
	   return PR_FALSE;

Comment 22

[Victor Bobier]

Passwords do not show up, user names, if any do show up. I'm using Mozilla/5.0
(Windows; U; Windows NT 5.1; en-US; rv:1.6a) Gecko/20031002 Firebird/0.7+ It
just mean I have to type in a password everytime I use ebay or some other site.

Comment 23

[Boris Zbarsky [:bzbarsky]]

dwitte, caillon, any idea who should review this?  dveditz is a black hole...

Comment 24

[Marc Bejarano]

though the patch is a huge improvement over current behavior, it would be
extra-swell if mozilla would at least warn the user that corrupted passwords are
being passed over and, better yet, give the user the option of throwing out the
corrupted entries.  of course since i'm not offering to write this
functionality, i'll shut up, now ;) 

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 130070 [details]
Fix to better deal with corrupt passwords

Looks like the whole file instead of a patch, I'll look at the one inline
instead,

(Next time attach the patch rather than enter it as a comment. Also more
context is good, try "cvs diff -up6" or even more context for tricky patches).

Comment 26

[Daniel Veditz [:dveditz]]

r=dveditz for the in-line patch.

Comment 27

[Mike Calmus]

Attachment: Correct patch instead of whole files

This is the same as the previous patch with the new location of SignonViewer.js
reflected and my name added as a contributer. No other changes have been made.
It also has additional lines of context.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 133702 [details] [diff] [review]
Correct patch instead of whole files

>Index: extensions/wallet/src/singsign.cpp
>===================================================================
>@@ -2935,13 +2936,13 @@ SINGSIGN_ReencryptAll()
>       for (PRInt32 k=0; k<dataCount; k++) {
>         data = NS_STATIC_CAST(si_SignonDataStruct *,
>                               user->signonData_list.ElementAt(k));
>         nsAutoString userName;
>         if (NS_FAILED(si_Decrypt(data->value, userName))) {
>-          return PR_FALSE;
>+          //Ignore the error and go to the next one.
>         }
>         if (NS_FAILED(si_Encrypt(userName, data->value))) {
>           return PR_FALSE;
>         }
>       }

Your comment doesn't match what happens, in the error case you fall through and
encrypt the already-encrypted stuff. I guess there's no hope of doing it right
if si_Decrypt fails, but why waste time encrypting it?	Worse, next time
through we'll be able to decrypt it fine but it'll decrypt to the
corruptly-encrypted garbage and any future garbage-detection-cleanup stuff we
might want to write won't know it's garbage.

Please add a continue after your comment, or rearrange with the encrypt inside
a NS_SUCCEEDED(si_Decrypt()) test instead of the reverse.

Sorry I didn't notice this before.

Comment 29

[Brendan Eich [:brendan]]

Also, don't comment out code.  Remove it -- CVS will remember.

/be

Comment 30

[Mike Calmus]

Attachment: Patch to ignore corrupt passwords

This patch fixes the issue raised by dveditz. Changing a passowrd file from
encrypted to decrypted and back now does not make the corrupt passwords show up
with junk in the password list (which it did with the previous patch). However,
the corrupt entries in the underlying data store are different after than
before the transformation. This makes it far less likely they could be
resurrected by reversing some previous algorithm.

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 133727 [details] [diff] [review]
Patch to ignore corrupt passwords

>         if (NS_FAILED(si_Decrypt(data->value, userName))) {
>-          return PR_FALSE;
>+          //Ignore the error and go to the next one.
>         }
>-        if (NS_FAILED(si_Encrypt(userName, data->value))) {
>+        else if (NS_FAILED(si_Encrypt(userName, data->value))) {
>           return PR_FALSE;
>         }

As a matter of style "if (!foo) /*nothing*/ else" is an ugly and verbose
equivalent of "if (foo)". Since hard-to-read code leads to future bugs please
rewrite this in one of the two ways suggested earlier, using either a continue
(so testing for decrypt failure makes some sense) or swapping the sense of the
first test using NS_SUCCEEDED (and then of course putting the second test
inside, or combining the two). Either way you don't need that "else" you added.

Comment 32

[Mike Calmus]

Attachment: Change to use continue

Ugh. I should know better. I added in a continue and changed the comment to
better reflect why we were moving on to the next.

Comment 33

[Daniel Veditz [:dveditz]]

Comment on attachment 133756 [details] [diff] [review]
Change to use continue

r=dveditz -- thanks!

Comment 34

[Brendan Eich [:brendan]]

Comment on attachment 133756 [details] [diff] [review]
Change to use continue

This looks like an improvement.

I wonder what bryner thinks (he rewrote password manager for Firebird).

/be

Comment 35

[Scott Tran (Wildcard)]

*** Bug 204009 has been marked as a duplicate of this bug. ***

Comment 36

[Mike Calmus]

Could someone with CVS check-in permissions please do so with this patch?

Comment 37

[[e-mail]]

you'll need to ask for approval1.6b? first.

Comment 38

[Asa Dotzler [:asa]]

How well has this been tested. We don't have much time to respond to any
regression fallout between now and 1.6 final.

Comment 39

[Mike Calmus]

I've tested it pretty extensively. The only different behavior I've seen from
the current system (other than fix itself) is that when decrypting and
re-encrypting passwords, corrupted entries are stored with different values.

Comment 40

[Asa Dotzler [:asa]]

Comment on attachment 133756 [details] [diff] [review]
Change to use continue

a=asa (on behalf of drivers) for checkin to 1.6beta

Comment 41

[Scott Tran (Wildcard)]

This going to get checked in any time soon ?

Comment 42

[Sean Aitken]

Not sure of the current status (as I am not keeping up to speed on the detailed
developments in the current release (1.6), but I can confirm that it is a
problem with my install. I'm using 1.6 (Jan 15th) on the Win32 platform and am
seeing this behavior.
Hope it can be fixed soon!  Keep up the good work guys!

Comment 43

[Mike Calmus]

This was approved for 1.6b but never got checked in. Can someone please check
this into CVS?

Comment 44

[timeless]

Comment on attachment 133756 [details] [diff] [review]
Change to use continue

unfortunately you managed to miss the window. when this gets approval, visit
irc.mozilla.org #mozilla and ask for someone to check it in. if this misses
1.7a, ask on irc for info about what time period you'd be able to check it in
w/o approval. sorry.

Comment 45

[Daniel Veditz [:dveditz]]

-> Mike since he's written the patch

Comment 46

[Daniel Veditz [:dveditz]]

Comment on attachment 133756 [details] [diff] [review]
Change to use continue

1.7a+ a=dveditz for drivers

Comment 47

[Daniel Veditz [:dveditz]]

Oh, I get it now.... sorry, back to mvl

Comment 48

[timeless]

Checking in
mozilla/extensions/wallet/signonviewer/resources/content/SignonViewer.js;
/cvsroot/mozilla/extensions/wallet/signonviewer/resources/content/SignonViewer.js,v
 <--  SignonViewer.js
new revision: 1.4; previous revision: 1.3
done
Checking in mozilla/extensions/wallet/src/nsPasswordManager.cpp;
/cvsroot/mozilla/extensions/wallet/src/nsPasswordManager.cpp,v  <-- 
nsPasswordManager.cpp
new revision: 1.17; previous revision: 1.16
done
Checking in mozilla/extensions/wallet/src/singsign.cpp;
/cvsroot/mozilla/extensions/wallet/src/singsign.cpp,v  <--  singsign.cpp
new revision: 1.238; previous revision: 1.237
done

Comment 49

[Scott Tran (Wildcard)]

Appears to be fixed on Windows XP Build 2004021308

Comment 50

[chris hofmann]

marking fixed to get on the testing radar