31925 - Single Signon database becomes unusable

Status: VERIFIED FIXED

(SeaMonkey :: Passwords & Permissions, defect, P3)

Description

[Stephen P. Morse]

Steps to reproduce:

1. Start with a fresh profile
2. Go to any login page (http://scopus.mcom.com/bugsplat/login.html is my 
favorite) and log in
3. Answer YES to the dialog that asks if you want to save the login
4. Repeat step 2
5. Answer "NEVER" to the dialog that asks if you want to save the login
6. Exit the browser
7. Re-enter the browser
8. Go back to the login page

expected result: username and password would be prefilled
actual result: no prefilling (in debug build you get an assertioj about the 
header information in the .u and .p file being out of synch).

Comment 1

[Stephen P. Morse]

Problem was an erroneous check.  It is perfectly normal for the two files to be 
out of synch and will occur whenever you click on NEVER in the 
do-you-want-to-save dialog (in that case the .u file is updated but not the .p 
file).

The error is in doing the check in the first place and refusing to load the 
sigle-signon database when the check fails.  Having understood this, the fix is 
obvious and has already been checked into the M15 trunk.

Raising this issue here for consideration in the beta1 branch.  The reasons I 
think this should be fixed in beta1 are:

1. The consequence is complete loss of all single-signon data at frequent 
intervals.

2. The fix is well understood and very safe.  To prove that, the diffs are 
shown below:

Index: singsign.cpp
===================================================================
RCS file: /cvsroot/mozilla/extensions/wallet/src/singsign.cpp,v
retrieving revision 1.119
diff -c -r1.119 singsign.cpp
*** singsign.cpp	2000/03/12 08:00:49	1.119
--- singsign.cpp	2000/03/14 16:30:04
***************
*** 1655,1670 ****
    lineBuffer = nsAutoString("");
  
    /* read the line */
!   PRUnichar c, c2;
    for (;;) {
      if (inHeader) {
        c = Wallet_UTF8Get(strmu);
        if (obscure) {
!         c2 = Wallet_UTF8Get(strmp);
!         if (c != c2) {
!           NS_ASSERTION(0, ".p and .u files differ in header");
!           return -1;
!         }
        }
      } else if (obscure) {
        c = Wallet_UTF8Get(strmu); /* get past the asterisk */
--- 1655,1666 ----
    lineBuffer = nsAutoString("");
  
    /* read the line */
!   PRUnichar c;
    for (;;) {
      if (inHeader) {
        c = Wallet_UTF8Get(strmu);
        if (obscure) {
!         c = Wallet_UTF8Get(strmp);
        }
      } else if (obscure) {
        c = Wallet_UTF8Get(strmu); /* get past the asterisk */

Comment 2

[Stephen P. Morse]

Here are more details on this bug:

I create a pair of files -- a .u file for the cleartext info (url, fieldnames) 
and a .p file for the encoded info (values for username and password).  Both of 
these files have a of header that contains the seed information needed to decode 
the encoded file.  Of course that seed information is not needed for the 
cleartext file but for consistency it has one.

Now I made a bad assumption somewhere along the line.  Namely, I concluded that 
the seed value in the two files will always be the same because they are always 
written out together.  So when I read in the files, I check for identical seed 
values and I reject the file if it's not.

Bad assumption.  It's true that whenever I'm writing out password info, I am 
update the two files together.  But the files also contains my reject list -- 
the list of sites for which I never want to save passwords.  Whenever an entry 
is added to that list, I update the cleartext file only -- nothing's changed in 
the encoded file so I don't write it.  And at this point the seed values in the 
two files are out of synch.

The fix involves the following two changes.

1. Remove the code that tests for identical seed value in the two files.

2. Make sure that the seed value I wind up saving is from the encoded file and 
not from the cleartext one.

This means that my algorithm now changes as follows:

OLD ALGORITHM:

- collect seed from cleartext file
- compare it to seed in encoded file
- if no match, ignore files

NEW ALGORITHM:

- collect seed from cleartext file
- collect seed again from encoded file (overwriting value just collected)

Comment 3

[Jim Roskind]

IMO, the seed info should either not be stored twice, or should be used exactly
as you have (re: checking for consistency).
The error seems to be that you change the seed value when you are not re-writing
the obscured file.  Your fix seems to be to ignore this error, which will
probably have worse consequences.
The right fix seems to be to either re-write both files (and change the seed)
whenever you update the datebase, or re-write only the plaintext file, but not
update the seed (when you're not changing the obscured file).
Clearly if there is a mis-match between the two files, due to an inoportune
crash, you would not want to use passwords from one vendor in the input fields
of another.  I agree you are stopping a loss of data that can appear without a
crash, but the currently described fix sounds like it removes the alignment
redundancy that you should be desparate to preserve.

Comment 4

[Stephen P. Morse]

I can't rewrite both files all the time because the user might not yet have 
entered his database password.  It would look strange to suddenly request his 
password just because he asked not to be prompted to save his password for a 
particular site.

Keeping the cleartext file's seed unchanged is a good suggestion but will take 
more investigation, coding, and testing.  That's a much higher-risk fix than the 
one I am proposing for beta1.  What you are suggesting is something to consider 
for the future.

Comment 5

[leger]

Putting on PDT- radar for beta1.

Comment 6

[Stephen P. Morse]

Jan, why?  I can't present any counter-arguments if I don't know what made you 
decide to not include this for beta1.  It seems to me like data loss would be a 
pretty serious thing.  All the signons for all the sites the user has visited, 
for all the http and ftp authenticiations, and for mailnews would be lost.

And the fix is very safe, no-risk, and ready to go. 

Comment 7

[leger]

PDT made this PDT- per jar's comments above.  That was the reason.  jar will 
come by and talk with you.

Comment 8

[leger]

Putting on PDT- radar.

Comment 9

[Stephen P. Morse]

Trying for PDT+ once again.  After discussing this with jar, we realized that 
the correct change (i.e., not updating the seed if only the cleartext file is 
being written) was actually a very simple and safe one-line change.  The diff 
for that change is shown below and jar has already code-reviewed it.  I tested 
it and it indeed solves the problem of preventing the two files from getting out 
of synch while at the same time prevents us from accepting the files if they do 
get out of synch due to a catastrophic failure.  jar agrees that this is the 
correct thing to do.

Requesting PDT+ status once again.


Here's the diff:

*** 2090,2096 ****
  
    nsAutoString buffer;
    buffer = "";
!   saveCountP += 16; /* preserve low order four bits which designate the file 
type */
    buffer.Append(PRInt32(saveCountP),10);
    si_WriteLine(strmu, strmp, buffer, PR_FALSE, fullSave2, 0, 0, PR_TRUE);
    si_WriteLine(strmu, strmp, buffer, PR_FALSE, fullSave2, 0, 0, PR_TRUE);
--- 2090,2098 ----
  
    nsAutoString buffer;
    buffer = "";
!   if (fullSave2) {
!     saveCountP += 16; /* preserve low order four bits which designate the file 
type */
!   }
    buffer.Append(PRInt32(saveCountP),10);
    si_WriteLine(strmu, strmp, buffer, PR_FALSE, fullSave2, 0, 0, PR_TRUE);
    si_WriteLine(strmu, strmp, buffer, PR_FALSE, fullSave2, 0, 0, PR_TRUE);

Comment 10

[Phil Peterson]

[PDT+] w/b minus on 3/17

Comment 11

[Stephen P. Morse]

OK, correct fix is not checked into the branch.  Also I undid the previous fix 
that I made on the tip (i.e., not doing the check for identical seed in both 
files) and put the correct fix onto the tip as well.

Comment 12

[Stephen P. Morse]

Oooops!  The above should have read:

"The correct fix is NOW checked into the branch. ... "

Thanks to jar for keeping me honest about that. ;-)

Comment 13

[sairuh (rarely reading bugmail)]

steve, i'm unclear as to how to verify this bug. going over the original steps
for this, i don't understand how you can do step 4 (repeating login) and step 5.
namely, if you're within the same session, those fields will be prefilled for
you anyhow. did you mean remove what's prefilled and enter another
username/passwd?

in addition, with a new profile the user is going to get the Master Password
dialog, prompting for a passwd (occurs after step 3 is completed).

Comment 14

[Stephen P. Morse]

Yes, step 4 is with a different username than was used in step 2.  That's what 
will cause the dialog in step 5 to appear.  No need to remove the prefilled 
username -- simply appending a character to the end of it will do.

And, yes, I intentionally skipped documenting the master-password prompt that 
would occur after step 3 since it wasn't germaine to this bug.  Depending on 
whether or not you previously unlocked the database, you might or might not get 
the master-password dialog.  I hoped that that it would be obvious to anyone 
reproducing the bug but maybe I should have been more specific.  

Comment 15

[sairuh (rarely reading bugmail)]

thx steve!

verified fixed w/ today's beta1 branch bits on linux, winNT and mac (opt comm,
2000.03.22.06-nb1b). the username/passwd (initial ones that were saved) were
prefilled after restarting.