383229 - Password Manager should honor the master password timeout prefs

Status: RESOLVED DUPLICATE of bug 222408

(Toolkit :: Password Manager, defect)

Description

[Nelson Bolyard (seldom reads bugmail)]

Seen on both trunk and current releases.

Password manager is dishonoring the user's Master Password timeout preference
and forcing the Master Password to be re-entered every time the user bonks
the "show passwords" button.

To reproduce this bug, you need to have a master password set, and 
(for those clients that give you this option) you need to select that
saved passwords are encrypted.    Then set the Master Password timeout
pref to prompt you for the master password either :
  (.) The first time it is needed
  ( ) If it has not been used for [ 60] minutes or longer

Then, restart your browser.  Then go into password manager.  It will prompt
you for your Master password before opening the Password Manager dialog.
(It's quite unclear why it does that at this point, since no decryption
is performed to show you the password manager contents.  Decryption occurs
only when you attempt to see the passwords themselves.)

Then in the password manager, bonk the "show passwords" button.  

Actual result: You are prompted for your Master Password again, immediately,
regardless of the Master Password timeout preference.  

Expected result: Since you are already logged into the "software encryption
device" (That's what the master password actually does) and the timeout 
time has no yet expired, you should NOT be prompted for the master password
when you bonk that button.

Comment 1

[Johnathan Nightingale [:johnath]]

It seems that the culprit may be here:

http://mxr.mozilla.org/mozilla/source/toolkit/components/passwordmgr/content/passwordManager.js#156

Specifically:

// If there is no master password, still give the user a chance to opt-out of displaying passwords
if (token.checkPassword(""))
     return AskUserShowPasswords();
 
   // So there's a master password. But since checkPassword didn't succeed, we're logged out (per nsIPK11Token.idl).
   try {
     // Relogin and ask for the master password.
     token.login(true);  // 'true' means always prompt for token password. User will be prompted until
                         // clicking 'Cancel' or entering the correct password.

So once the check of an empty password fails, it has also killed the login.  I suspect this is not the way to do things.  

We probably want to check isLoggedIn(), but I bet the current code exists to work around the fact that isLoggedIn() returns true for users without a master password, and we still wanted to prompt those people (who have not yet seen any prompt).

Nelson - is there a better way you know of to check for "do I have a master password?" than logging in with an empty string (and implicitly logging out those who DO have one set)?  Assuming there is, this should be a straightforward patch.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Johnathan,  As you know, the "master password" is the login password for one 
of NSS's virtual (pure software) PKCS#11 cryptographic "devices" (tokens, in 
the jaron).  It is possible to ask if the device has been given a password,
and it is possible to ask if the device is presently in a logged in state.

The device occupies a virtual "slot".  To ask questions about the device,
you first need the slot handle.  

PK11SlotInfo * PK11_GetInternalKeySlot(void);
  returns the slot handle of the slot used with the "master password"

PRBool PK11_NeedUserInit(PK11SlotInfo *slot);
  returns true if the slot has no password, and false if it has a password.

PRBool PK11_IsLoggedIn(PK11SlotInfo *slot, void *wincx);
  returns true if the slot is presently in a logged in state.  It also checks
  to see if the slot has been unused for longer than the user-specified 
  master password timeout time, and reports that the device is not logged in
  if that time has elapsed.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Another useful function related to master passwords:

PRBool PK11_NeedLogin(PK11SlotInfo *slot);
Tells you if you need to login before you can use the keys in the 
internal key slot.  This is not quite the same thing as !PK11_IsLoggedIn
because when no master password has been set, then a login may be 
unnecessary.

Comment 5

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

It looks like all of the NSS APIs mentioned by Nelson are exposed by PSM through XPCOM via nsIPK11Token, which is how we're using them in the Firefox UI.

I think I understand the problem, so I'll try to explain it.

1) In Firefox, we give the users the choice of adding a master password, or not using one. To achieve the default "no master password" state, we apparently set the master password to "" by default when first retrieving the InternalKeyToken (see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js&rev=1.5#132).

2) When it comes time to display the passwords (after the user clicked on "Show Passwords"), we want to display a dialog warning the user if they have no master password. We can't use needsUserInit for this, because it will always be false, given 1). We therefore use checkPassword("") (passing in the empty string as the password), which succeeds if we're in the default state of having a password equal to "", but also has the side effect of logging out any users that have set a master password that is not "".

We could solve this problem by not setting the device's password to "" in 1), which would mean that we could use needsUserInit to answer the question "does the user have a master password set". It would let the token keep track of whether it's been given a password (via needsUserInit), instead of us comparing the password to a known-string (""). I'm not sure if there's a reason we chose to set an empty password. Does a token for which PK11_NeedUserInit returns true have different behavior, such that it wouldn't be suitable for use by Firefox's password manager?

Alternatively, we could solve this problem by continuing to use "" as a default master password, and exposing a way to validate a token's password without the side effect of being logged out if the password is wrong. I don't like this solution because it requires exposing another NSS API, which I would imagine is a more complicated matter (without even mentioning the work needed to get a new version of NSS into a shipping Firefox).

Comment 6

[Robert Relyea]

This was a purposeful design decision in PSM. There are certain events which are considered 'critical' events, where PSM wants to get reconfirmation of the password before it does some operation. I am aware of 2 cases there this happens:

1) Before export a user certificate/private key.
2) Before you display decrypted passwords in the password manager.

In both of these cases, the assumption is you don't want someone extracting your key or password while you are away, even if you are logged in.

These are both implementented by using C_Logout to force a new authentication before the operation can be completed. Neither looks at the password timeout/preference.

bob

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Bob, it may have been purposeful, but it's illogical.
The WHOLE POINT of giving the user the choices for master password timeout
is to PUT THE USER IN CONTROL of how often he gets asked for it.
The idea that "the user is in control, except when looking at his passwords"
is offensive.  It does NOT strengthen the security for users who have set 
their timeouts in a fashion appropriate to their environment. 
It is PURELY and unnecessarily IRRITATING for it to ask for the master 
password twice in rapid succession, as is common in password manager in the
case I cited in comment 0.  

Further, it creates a bad precedent.  It tells mozilla programmers not to 
trust or honor our carefully designed master password timeout logic, and
instead just make up their own rules.  Consequently, there is no apparent
rhyme or reason to the times when users are asked for the password.  
It leaves users wondering: who designed this nonsense?

IMO, this is just BAD UI.

Comment 8

[Sylvain Pasche]

Nelson, sorry if this may be a little off topic, but how to you get to the UI for setting the Master Password timeout pref? Looks like this is not available in Firefox preferences by default (otherwise, this extension would have little value ;-) https://addons.mozilla.org/en-US/firefox/addon/1275).

I was planning to work on an extension that could store the master password inside a password manager, so that you don't need to enter your master password every time, while still encrypting your password on disk. Thus, I looked into more details in these API.

I found out that the token.initPassword(""); is not setting the value of the master password, but is calling PK11_InitPin which is something different as the master password from my little understandings.

I did some tests with both a key with a master password set and one without, by running:

print("NeedUserInit: " + token.needsUserInit);
if (token.needsUserInit) {
    print("Initializing key3.db with default blank password.");
    token.initPassword("");
}
print("Logged in: " + token.isLoggedIn());
print("Needs login: " + token.needsLogin());
var res = token.checkPassword(theMasterPassword);
print("CheckPassword: " + res);
print("Logged in: " + token.isLoggedIn());
print("Needs login: " + token.needsLogin());


Results using a key with a master password:

NeedUserInit: false
Logged in: false
Needs login: true
CheckPassword: true
Logged in: true
Needs login: true

Results using a key without a master password:

NeedUserInit: false
Logged in: false
Needs login: false
CheckPassword: false
Logged in: false
Needs login: false

By evaluating both returned values of isLoggedIn() and needsLogin(), it should be possible to tell if the master password is set, without logging the user out.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #8)
> Nelson, sorry if this may be a little off topic, but how to you get to the UI
> for setting the Master Password timeout pref? 

Although this is a FireFox bug, we are discussing code that is common to 
FireFox and SeaMonkey, if I'm not mistaken.  

In SeaMonkey, there is a preferences panel with these choices:

   Master Password Timeout
   SeaMonkey will ask for your master password:
   ( ) The first time it is needed
   ( ) Every time it is needed
   (.) If it has not been used for [ 5 ] minutes or longer

In FireFox, I think it is necessary to use about:config to set these prefs:

security.ask_for_password    integer   2  
 (Note: 0 = first time it is needed, 1 = every time, 2 = not used for ...)
security.password_lifetime   integer   5  (minutes before master PW prompt)
signon.expireMasterPassword  boolean true (if ask_for_password = 1, else false)

> I found out that the token.initPassword(""); is not setting the value of the
> master password, but is calling PK11_InitPin which is something different as
> the master password from my little understandings.

I believe PK11_InitPin is called when you click the "Reset Master Password" 
button.  Do you know what that does?

> I did some tests with both a key with a master password set and one without, 

I'm not sure what you mean by "a key with a master password".  A key?

> By evaluating both returned values of isLoggedIn() and needsLogin(), it should
> be possible to tell if the master password is set, without logging the user
> out.

Agreed.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #5)

> I think I understand the problem, so I'll try to explain it.
> 
> 1) In Firefox, we give the users the choice of adding a master password, 
> or not using one. To achieve the default "no master password" state, we 
> apparently set the master password to "" by default when first retrieving 
> the InternalKeyToken (see
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js&rev=1.5#132).
> 
> 2) When it comes time to display the passwords (after the user clicked on 
> "Show Passwords"), we want to display a dialog warning the user if they 
> have no master password. We can't use needsUserInit for this, because it 
> will always be false, given 1). 

That's the underlying cause of the problem, IMO.
Unless someone can defend that decision, I propose we stop doing that.

> We therefore use checkPassword("") (passing in the empty string as the 
> password), which succeeds if we're in the default state of having
> a password equal to "", but also has the side effect of logging out any 
> users that have set a master password that is not "".

Yup. That's evil.

> We could solve this problem by not setting the device's password to "" 
> in 1), which would mean that we could use needsUserInit to answer the 
> question "does the user have a master password set". It would let the 
> token keep track of whether it's been given a password (via needsUserInit), 
> instead of us comparing the password to a known-string (""). 

I believe you mean: instead of trying to login with a known password string.

Although it uses PSM, this is the responsibility of Firefox's password 
manager, not PSM (per se').  I hope the password manager rewrite will fix it.
I want to see this annoying double-password prompt get fixed.