437904 - secretDecoderRing sometimes fails after clearing private data or exiting private browsing mode

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

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

I was just editing the users that are allowed to log in to my web site statistics page, and was using "Clear private data" with "Authenticated sessions" to test the changes. I deleted the username that I was logged in as  (and had a password stored for) on the server, then cleared authenticated sessions, then signed in with a newly created account. Once that was successful, clicking on the "Remember password" button in the notification gives me:

Error: [Exception... "'Couldn't write to file, login not added.' when calling method: [nsILoginManagerStorage::addLogin]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: file:///Users/gavin/moz/mozilla/ff-opt/dist/Minefield.app/Contents/MacOS/components/nsLoginManager.js :: anonymous :: line 388"  data: no]
Source File: file:///Users/gavin/moz/mozilla/ff-opt/dist/Minefield.app/Contents/MacOS/components/nsLoginManager.js
Line: 388

and the notification bar doesn't close. This traces back up to _writeFile, which fails only if _encrypt returns true for userCanceled, which happens when nsISecretDecoderRing::encryptString returns NS_ERROR_FAILURE.

I saw this on two computers (Windows and Mac), but I can't seem to reproduce at the moment, so I'm not sure exactly what's up.

Comment 1

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

I didn't have the signon.debug pref set, and the storage component doesn't seem to watch it live, so I won't be able to get that output without restarting. I'm afraid that I won't be able to reproduce it again after that, though.

Comment 2

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

I don't have a master password set in either of the profiles were I saw this.

Comment 3

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

Attachment: console log with signon.debug set

Turns out the other profile I was seeing this in does have the debug pref set.

Comment 4

[Justin Dolske [:Dolske]]

Are you sure this was on a current nightly?

Sounds a lot like bug 400238, which should be fixed. :(

Comment 5

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

I'm seeing this with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008053106 Minefield/3.0pre , which should include the fix for bug 400238 if I'm reading http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/client.mk&rev=HEAD&mark=1.381 and bug 400238 comment 39 correctly.

Comment 6

[(no longer active)]

Possibly related: bug 248970 comment 261.

Comment 7

[Justin Dolske [:Dolske]]

I'm having a hard time reproducing this...

The closest I can come is that if I set a master password, then unset the master password, then clear private data, I get the same failure (these steps all have to be done in the same session, quitting and restarting makes it work again). We either of you doing the set&remove MP steps when hitting this? Might be 2 bugs here. :(

Comment 8

[(no longer active)]

This is what happens on sanitize (or entering/leaving the PB mode):

    let sdr = Cc["@mozilla.org/security/sdr;1"].
              getService(Ci.nsISecretDecoderRing);
    sdr.logoutAndTeardown();

Is this possible that SDR can't recover successfully after logoutAndTeardown?

BTW, I really feel this bug belongs in the NSS product...

Comment 9

[Justin Dolske [:Dolske]]

I hit this a couple days ago with a 1.9.1 nightly shortly after exiting private browsing mode. I don't have a master password set (nor had I been fiddling with it). I had password manager logging running, symptoms were the same as comment 0. The failure looks sticky, even calling getAllLogins() failed because SDR was failing when trying to decrypt the login data.

Still can't reliably reproduce this; I expect it's some race condition between the logoutAndTeardown() and something else. I tried writing a little xpcshell test that calls logoutAndTeardown() and decryptString() in a loop, but that won't trigger it either.

But yeah, over to NSS. Not sure what else to try. There's no master password set, so decryptString shouldn't ever be failing...

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Ehsan & Justin, 
In between your code and NSS is a layer of code known as PSM.
NSS is not PSM and PSM is not NSS.  
Your analysis so far shows that calls to PSM functions are not behaving as 
you expect, but you have not yet shown evidence that this is an NSS failure.  

A bug report filed against NSS should at least name the NSS function that 
was called and what error code it returned (by calling PR_GetError).  
Have you done those steps? If not, then you should not assign this to NSS.

Comment 11

[Justin Dolske [:Dolske]]

I filed this against NSS because it looks almost identical to the problem previously fixed (?) in bug 400238. Given that PSM implemention of SDR is basically just a lightweight wrapper, that seemed sensible enough.

Comment 12

[Serge Gautherie (:sgautherie)]

Bug 521263 is 100% reproducible, all 3 platforms.
Hope this helps to at least diagnose what is going on.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Serge, how is your comment 12 relevant to this bug?
How is bug 521263 relevant to this bug?

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)

> Serge, how is your comment 12 relevant to this bug?

This bug seemed to lack a reproducible case.
Hopefully, the underlying cause is the same for both.

> How is bug 521263 relevant to this bug?

Both fail in "nsILoginManagerStorage::addLogin" with "Failed to encrypt string. (NS_ERROR_FAILURE)".
Both seems to point to bug 400238.

Comment 15

[Michael Kraft [:morac]]

I can reproduce this bug very easily with the following steps:

1. Set a master password.
2. Go into private browsing mode
3. Clear the master password.
4. Exit private browsing mode

At this point any attempt to use secretDecoderRing fails until the browser is restarted or a new master password is set.  The problem seems to be that secretDecoderRing thinks it has put up a master password prompt when none was displayed.  As such it thinks the user canceled the prompt.

This fails in the Gecko/20091106 trunk load as well as all branches that have the private browsing mode functionality.

The simplest test is to type the following in the error console after step 4 above.  It should encrypt, but throws an exception instead:

Components.classes["@mozilla.org/security/sdr;1"].getService(Components.interfaces.nsISecretDecoderRing).encryptString("")

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

The problem described in comment 15 is definitely NOT an NSS problem.
It is definitely a PSM problem.  This bug is presently classified as an NSS 
bug.  Either 
a) this bug needs to be reclassified as a PSM bug, or 
b) a new and separate PSM bug should be filed about the problem described in 
comment 15. 

I vote for choice a.

Comment 17

[(no longer active)]

(In reply to comment #16)
> The problem described in comment 15 is definitely NOT an NSS problem.
> It is definitely a PSM problem.  This bug is presently classified as an NSS 
> bug.  Either 
> a) this bug needs to be reclassified as a PSM bug, or 
> b) a new and separate PSM bug should be filed about the problem described in 
> comment 15. 
> 
> I vote for choice a.

OK, let's do that.  I took the liberty of keeping the NSS guys CCed to this bug, in case further investigation shows possible NSS ties as well.

Comment 18

[Justin Dolske [:Dolske]]

This is an NSS bug after all. I took a good hard look at what was going on, as a result of my newly-written test for bug 499233 failing in the same was as reported here...

The basic issue is that softtoken has some complicated accounting for handling the token login state, and it's not quite right in a couple of places. Specifically, when a "master password" is not set (ie, the PIN is an empty string), SFTKSlot.isLoggedIn and SFTKSlot.needsLogin are normally FALSE -- but there are a couple of ways to get into a state where there's no master password set but isLoggedIn is TRUE. That, in turn, leads to problems.

These are the 3 main spots I believe are incorrect:

1) When NSC_SetPIN() is called to clear the master password (ie, change the PIN from "mysecretvalue" to ""), it sets .needsLogin back to the correct state, but not .isLoggedIn.

2) If NSC_Login() is ever called when there's no master password set, it will end up setting SFTKSlot.isLoggedIn to TRUE.

3) NSC_SetPIN() never calls sftk_update_all_states(). [I need to doublecheck if this was really needed, or if it was just a side effect of the other problems.]

The value of .isLoggedIn is important, because if NSC_Logout() is called in this state, it will flush the cached passwordKey data (used for decrypting private objects). That's bad, because PK11_Authenticate() normally takes care of reinitializing it by requesting a PIN authentication. But when there's no master password set, .needsLogin is false so it does nothing at all. Thus, the SecretDecoderRing functions all end up failing deep in softtoken, because the token has no cached passwordKey, can't decrypt the key anew, and can't create a new key (it tries, thinking the key just hasn't been initialized).

The sftk_update_all_states() call seems to be needed to avoid confusing PK11_IsLoggedIn(), but as I noted I need to check to see if that was just a side-effect of the other bugs.

Notable ways to trigger these functions:

* Change the master password [calls NSC_ChangePIN]
* Exit Private Browsing mode, which uses nsISecretDecoderRing.logoutAndTeardown() [calls NSC_Logout]
* Open the password manager UI and click Show Passwords, which uses nsIPK11Token.checkPassword() to see if it should force a MP entry. [calls PK11_CheckUserPassword, which actually forces a C_Logout and then attempts a C_Login]

Triggering this bug takes some combination of the above.

Comment 19

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Comment 20

[Justin Dolske [:Dolske]]

Oops, meant to note with the attachment:

This passes the existing Firefox password manager tests (plus those added in bug 499233). Are there any relevant NSS tests to try running?

Comment 21

[Justin Dolske [:Dolske]]

Also, the steps to reliably reproduce this problem in Firefox (starting from a clean profile plus 1 login saved for, say, Gmail):

1) Start browser
2) Open pwmgr UI (Tools -> Options, Security, View Passwords)
3) Click "Show Passwords"
4) Enter Private Browsing mode
5) Exit Private Browsing mode
6) Visit site with saved login to use it, or use pwmgr UI again.

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Because this patch includes softoken sources, it's a FIPS patch.
However, the timing is perfect for a FIPS bug fix, IINM.  

Bob Relyea really needs to review this bug, Justin's analysis and Justin's 
proposed fix.  Bob's the only one who's claimed to fully understand all the 
relevant state variables.  Maybe Justin is becoming the next heir-apparent 
to that throne! :)  

Justin,  A number of SM users who upgraded to SM 2.0 when it released, who 
claimed they had no MP set prior to that upgrade, found that they were being
prompted to enter their MP on startup after the upgrade, as if they had had
an MP all along.  I wonder if your findings here can help explain that also.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 447449 [details] [diff] [review]
Patch v.1

Will be easier to review this once it's converted to a CVS diff.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Justin's patch converted to CVS diff and to C89.

Converted a // style comment to /* style */
No other intentional differences.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

I should have written "no other intentional non-whitespace differences".

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Justin's patch converted to CVS diff and to C89, whitespace fixed

NSS builds with C89 compilers, which means no // comments and all variable
declarations must occur at the beginning of basic blocks, right after {'s.  
This patch fixes those issues and also fixes the remaining whitespace issues
I didn't catch in the previous attempt.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 447449 [details] [diff] [review]
Patch v.1

review is requested on the cvs diff copy of this patch.

Comment 28

[[e-mail]]

(In reply to comment #8)
> This is what happens on sanitize (or entering/leaving the PB mode):
> 
>     let sdr = Cc["@mozilla.org/security/sdr;1"].
>               getService(Ci.nsISecretDecoderRing);
>     sdr.logoutAndTeardown();
> 
> Is this possible that SDR can't recover successfully after logoutAndTeardown?
For SeaMonkey we replaced all our calls to logoutAndTeardown with checkPassword("") because we couldn't log back in after logoutAndTeardown.
(See my m.d.t.crypto thread 'How to "log out" of SDR?')

Comment 29

[Wan-Teh Chang]

Comment on attachment 447462 [details] [diff] [review]
Justin's patch converted to CVS diff and to C89, whitespace fixed

In pk11wrap/pk11auth.c:

>     /* if we're already logged in, we're good to go */
>     case CKR_OK:
> 	slot->authTransact = PK11_Global.transaction;
>+	/* Fall through */
>+    case CKR_USER_ALREADY_LOGGED_IN:
> 	slot->authTime = currtime;
> 	rv = SECSuccess;
> 	break;

The original comment "if we're already logged in, ..."
needs to be updated to reflect the difference between
the CKR_OK and CKR_USER_ALREADY_LOGGED_IN cases.

In softoken/pkcs11.c:

>@@ -3348,8 +3346,22 @@ CK_RV NSC_SetPIN(CK_SESSION_HANDLE hSess
>     /* Now update our local copy of the pin */
>     if (rv == SECSuccess) {
> 	slot->needLogin = (PRBool)(ulNewLen != 0);
>+	/* Reset login flags and passwordKey. */
>+	if (ulNewLen == 0) {
>+	    PRBool tokenRemoved = PR_FALSE;
>+	    slot->isLoggedIn = PR_FALSE;
>+	    slot->ssoLoggedIn = PR_FALSE;
>+
>+	    rv = sftkdb_CheckPassword(handle, "", &tokenRemoved);
>+	    if (tokenRemoved) {
>+		sftk_CloseAllSessions(slot, PR_FALSE);
>+	    }
>+	}
>+	sftk_update_all_states(slot);
> 	return CKR_OK;
>     }
>+    sftk_freeDB(handle);
>+    handle = NULL;
>     crv = CKR_PIN_INCORRECT;
> loser:
>     if (sp) {

Two points about moving the
    sftk_freeDB(handle);
    handle = NULL;
code.

1. We should call
    sftk_freeDB(handle);
before the
    return CKR_OK;
statement at the end of the outer "if" block.

2. Since there is already the code
    if (handle) {
        sftk_freeDB(handle);
    }
under the "loser" label, it is not necessary to add
    sftk_freeDB(handle);
    handle = NULL;
before the
    crv = CKR_PIN_INCORRECT;
statement.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 447462 [details] [diff] [review]
Justin's patch converted to CVS diff and to C89, whitespace fixed

I'm entering an r- here for Wan-Teh's review comments in comment 29.
Thanks, Wan-Teh.

Comment 31

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Updated Patch v.1 with comments from Nelson and Wan-Teh, now in CVS format.

Comment 32

[Robert Relyea]

Thanks for the patch, My first look noted a couple of things:

I need to verify that those 'high-level BOOLs are still the correct BOOLs to look at. I think the current softoken is expecting to have to ask the low level database code if it is logged in or not. My worry is the set password case isn't the only place where the logged in state could change and that the correct fix is to remove the bools altogether. If it's not the case that the password state can change without the bools being set, then I believe your patch to SetPIN is fine.

The second thing is C_Login is undefined if you call it when the token doesn't need to be logged in. This means that the PK11_CheckUserPassword is incorrect because it shouldn't be making the C_Logout() C_Login() calls if the token doesn't support login anyway. I'm curious why it are being called in this case to begin with. There's a question about what should they return as there really isn't a password in this case. (An error because there is not password, or OK because any password is OK?).

NOTE on PK11_CheckUserPassword... it's a function that was created because sometimes you just need to verify a password, but it has a nasty side effect --- if the password is wrong you loose your login state. This is not a bug in softoken, but a deficiency in the PKCS #11 spec-- and therefore a limitation of the CheckUser function itself. 

Looking at Neil's comment, it may be a work around to deal with a different NSS bug.

Comment 33

[Robert Relyea]

> I need to verify that those 'high-level BOOLs are still the correct BOOLs to
> look at. 

OK, I've verified that the high level bools are still supposed to be correct. Login verifies that the cached password got set correctly before it changes to IsLoggedIn. I now believe the code for NSC_SetPIN is correct, though I think I would have preferred using the sftk_hasNullPassword() function to do the heavy lifting of setting needLogin correctly. With needLogin set, we can correctly set isLoggedin.

C_Logout is not the the only place the passwords get cleared. It also happens when we close the last session (either in C_CloseSession() or C_CloseAllSessions) We probably shouldn't clear those passwords if needLogin is set.

bob

Comment 34

[Robert Relyea]

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

r+ for the NSC_SetPIN part of the patch.

We shouldn't return CKR_USER_ALREADY_LOGGED_IN for NSC_Login r-)

sftkdb_ClearPassword(), we should reframe if the password is NULL.

PK11_CheckUserPassword should not call C_Login/C_Logout if the token doesn't need login.

bob

Comment 35

[Robert Relyea]

Justin if you want to submit another patch, I will be happy to review it. If you want me to submit one, I'll have to get wtc or nelson to review it.

bob

Comment 36

[Justin Dolske [:Dolske]]

(In reply to comment #34)

> sftkdb_ClearPassword(), we should reframe if the password is NULL.

I'm not sure what this comment means... "Reframe"?

Comment 37

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

* Removed CKR_USER_ALREADY_LOGGED_IN return previously added
* Guard other sftkdb_ClearPassword() calls to prevent calling when needLogin is false.
* PK11_CheckUserPassword avoids calling C_Logout/C_Login when not needed.

As noted in comment 32, it's not clear what should happen when no password is needed, but PK11_CheckUserPassword / C_Login is called with a non-blank password. This patch returns an error in such cases, just to be careful, but it could be allowed.

Comment 38

[Justin Dolske [:Dolske]]

ping? It's too late for this to make Firefox 4 beta 1, but I'd really like to get it into the next beta.

Comment 39

[Justin Dolske [:Dolske]]

Robert: ping again?

Comment 40

[Justin Dolske [:Dolske]]

Robert Relyea: ping x 3? Is there something we can do to get this review to move along?

Comment 41

[Robert Relyea]

re: I'm not sure what this comment means... "Reframe"?

Refrain from calling it. (bad spelling on my part).

Comment 42

[Wan-Teh Chang]

Comment on attachment 452788 [details] [diff] [review]
Patch v.3

dolske: I'll review your patch.  Since I am not familiar with
this code, I am counting on you to get to the bottom of this
bug and fully understand the code you're modifying.

Before I give review+, I have some questions and suggested changes
for your patch.

In security/nss/lib/pk11wrap/pk11auth.c:

>+    if (!slot->needLogin) {
>+        return (len == 0) ? SECSuccess : SECFailure;
>+    }

Please call PORT_SetError() before returning SECFailure.
Do you know what error code we should set here?  Perhaps
SEC_ERROR_INVALID_PASSWORD or SEC_ERROR_LIBRARY_FAILURE?
NSS error codes are listed at
http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslerr.html

Note: to be consistent with your change to NSC_Login
below, you should set the SEC_ERROR_BAD_PASSWORD error.

In security/nss/lib/softoken/pkcs11.c:

In NSC_SetPIN:

>+        /* Reset login flags and passwordKey. */

What is "passwordKey"?  I didn't find that string in
this file.

>+        if (ulNewLen == 0) {
>+            PRBool tokenRemoved = PR_FALSE;
>+            slot->isLoggedIn = PR_FALSE;
>+            slot->ssoLoggedIn = PR_FALSE;
>+
>+            rv = sftkdb_CheckPassword(handle, "", &tokenRemoved);
>+            if (tokenRemoved) {
>+                sftk_CloseAllSessions(slot, PR_FALSE);
>+            }
>+        }
>+        sftk_update_all_states(slot);
>+        sftk_freeDB(handle);
> 	return CKR_OK;

I guess if slot->needLogin is false, we need to set slot->isLoggedIn
to false?  Is that our convention?  That would answer one of my
questions below.

We may need to modify slot->isLoggedIn while holding slot->slotLock.
I also found that in NSC_Login, we call sftkdb_CheckPassword()
while holding slot->pwCheckLock.  Please find out if we need to
acquire any locks here.

In NSC_Login:

>     if (slot->isLoggedIn) return CKR_USER_ALREADY_LOGGED_IN;
>+    if (!slot->needLogin) {
>+        return ulPinLen ? CKR_PIN_INCORRECT : CKR_OK;
>+    }
>     slot->ssoLoggedIn = PR_FALSE;

Before we return CKR_OK, should we do the following to set
slot->isLoggedIn to PR_TRUE?

    PZ_Lock(slot->slotLock);
    slot->isLoggedIn = PR_TRUE;
    PZ_Unlock(slot->slotLock);

I don't know if we should also set slot->ssoLoggedIn.

>-    if (handle) {
>+    if (slot->needLogin && handle) {
> 	sftkdb_ClearPassword(handle);
>     }

What's the purpose of this change?  (There are several
occurrences of it in the patch.)  Is it harmful to call
sftkdb_ClearPassword(handle) if slot->needLogin is false?

Comment 43

[Justin Dolske [:Dolske]]

(In reply to comment #42)

> dolske: I'll review your patch.  Since I am not familiar with
> this code, I am counting on you to get to the bottom of this
> bug and fully understand the code you're modifying.

Understood! (In both ways. :)


> In security/nss/lib/pk11wrap/pk11auth.c:
> 
> >+    if (!slot->needLogin) {
> >+        return (len == 0) ? SECSuccess : SECFailure;
> >+    }
> 
> Do you know what error code we should set here?  Perhaps
> SEC_ERROR_INVALID_PASSWORD or SEC_ERROR_LIBRARY_FAILURE?

SEC_ERROR_INVALID_PASSWORD makes sense, and also keeps this consistent with when this function [PK11_CheckUserPassword] is called with an invalid password and slot->needLogin == true.


> In security/nss/lib/softoken/pkcs11.c:
> 
> >+        /* Reset login flags and passwordKey. */
> 
> What is "passwordKey"?  I didn't find that string in
> this file.

That's referring to what the softtoken is doing, by way of sftkdb_CheckPassword(), see comment 19. Doesn't need to be mentioned here, so I removed it.

> I guess if slot->needLogin is false, we need to set slot->isLoggedIn
> to false?  Is that our convention?

Correct.


> We may need to modify slot->isLoggedIn while holding slot->slotLock.
> I also found that in NSC_Login, we call sftkdb_CheckPassword()
> while holding slot->pwCheckLock.  Please find out if we need to
> acquire any locks here.

Hmm, I'm not really sure why slot->slotLock is being obtained in the other cases. Lots of code is reading read->isLoggedIn without obtaining the lock, and the code that sets isLoggedIn+lock just blindly sets it without any conditionals inside the critical section.

Maybe this was just for older architectures, where you can get odd effects from races that set adjacent variables which are each less than word? EG given struct { int16 aaa; int16 bbb } and 32 bit memory accesses (with a = b = 0 initially), ISTR that a thread which just sets aaa and a thread which just sets bbb can end up stomping on each others values?

But I don't think it's _unsafe_ to grab slotLock here, so I'll just do that anyway.


> In NSC_Login:
> 
> >     if (slot->isLoggedIn) return CKR_USER_ALREADY_LOGGED_IN;
> >+    if (!slot->needLogin) {
> >+        return ulPinLen ? CKR_PIN_INCORRECT : CKR_OK;
> >+    }
> >     slot->ssoLoggedIn = PR_FALSE;
> 
> Before we return CKR_OK, should we do the following to set
> slot->isLoggedIn to PR_TRUE?

No, per above (when needLogin==false, isLoggedIn should also be false).


> >-    if (handle) {
> >+    if (slot->needLogin && handle) {
> > 	sftkdb_ClearPassword(handle);
> >     }
> 
> What's the purpose of this change?  (There are several
> occurrences of it in the patch.)  Is it harmful to call
> sftkdb_ClearPassword(handle) if slot->needLogin is false?

Yes, see comment 18. If the (cached) password is cleared, it never gets reentered by PK11_Authenticate(). The semantics seem to be that when the token has a blank password set (aka "no master password set"), then it should never be logged out from nor needs logged into -- it must just remain in this limbo state.

I'll attach the minor updates to this patch in a bit.

Comment 44

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Comment 45

[Wan-Teh Chang]

I checked in patch v.4 on the NSS trunk (NSS 3.13) and NSS_3_12_BRANCH (NSS 3.12.8).

Checking in pk11wrap/pk11auth.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11auth.c,v  <--  pk11auth.c
new revision: 1.13; previous revision: 1.12
done
Checking in softoken/pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.169; previous revision: 1.168
done

Checking in pk11wrap/pk11auth.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11auth.c,v  <--  pk11auth.c
new revision: 1.12.2.1; previous revision: 1.12
done
Checking in softoken/pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.167.6.1; previous revision: 1.167
done

Comment 46

[Justin Dolske [:Dolske]]

woot!

Will there be a NSS tag so we can pull this into mozilla-central asap? Firefox 4 Beta 3 freezes on Monday.

Comment 47

[Wan-Teh Chang]

Please ask Kai Engert to push a new NSS tag (NSS_3_12_8_BETA1)
to mozilla-central.

If he cannot do that in time, you may use the mozilla/security/patches
method for Firefox 4 Beta 3:
http://mxr.mozilla.org/mozilla-central/source/security/patches/

Comment 48

[Robert Relyea]

Comment on attachment 460741 [details] [diff] [review]
Patch v.4

r+ 

yes, this patch is fine.

bob

Comment 49

[Justin Dolske [:Dolske]]

Kaie: what's needed to do push a tag (see comment 47)?

Comment 50

[Wan-Teh Chang]

Comment on attachment 460741 [details] [diff] [review]
Patch v.4

dolske: I can push a new NSS tag for you.  It seems that
we need to either get this bug marked as blocking2.0 or
get the approval-2.0+ flag for this patch.

drivers: I'm requesting approval2.0 for this patch.  The
benefit of this patch is that it fixes a bug, decribed in
dolske's comment 18, that affects the Password Manager
under some conditions.  The compatibility risk of this
patch is low.  The regression risk of this patch is low
to moderate.  dolske can give a better assessment of the
regression risk.

Comment 51

[Wan-Teh Chang]

Comment on attachment 460741 [details] [diff] [review]
Patch v.4

I pushed this patch to mozilla-central in patchset 8638116b698f:
http://hg.mozilla.org/mozilla-central/rev/8638116b698f

Comment 52

[Nelson Bolyard (seldom reads bugmail)]

Using Thunderbird version:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.13) Gecko/20101207 Thunderbird/3.1.7

I have it configured with an aladdin PKCS#11 module.  I added the module using TBird's UI, which does not offer any way to mark the module as "Friendly".

With that configuration, when no token is inserted into the slot, I cannot 
send an email message at all (not even a plain unsigned, unencrypted, message sent to an SMTP server that requires NO SSL/TLS and no user name or password).
I get a failure that says it could not contact the server, and I should check my settings to be sure they are correct.  

Changing the module with modutil to be marked as "Friendly" solves the problem.

This seems like a new problem.  I do not have this problem with a year+ old 
version of TBird.  Hence it seems to be a regression.  I wonder if it could 
be due to the fix for this bug.  

It's not clear to me why TBird would be checking that module at all, given 
that no SSL is required with the particular SMTP server.  :-(

Comment 53

[Serge Gautherie (:sgautherie)]

(In reply to comment #52)

> Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.13)
> Gecko/20101207 Thunderbird/3.1.7

Can you reproduce with TB 3.2pre? Or SeaMonkey 2.0/2.1pre?

> This seems like a new problem.

You should probably file a new bug.