534462 - loadStartFolder() asks for master password even if already logged into the software security device

Status: RESOLVED FIXED

(MailNews Core :: Security, defect)

Description

[Tamas Hubai [:htamas]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.5) Gecko/20091103 SUSE/3.5.5-1.1.2 Firefox/3.5.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091212 Shredder/3.1a1pre

If an add-on logs you into the software security device before the main Thunderbird/SeaMonkey 3-pane window appears, the application will forget about it and ask the master password again.

Reproducible: Always

Steps to Reproduce:
1. Set a non-empty master password in preferences.
2. Ask Thunderbird/SeaMonkey to remember the password for at least one account and check for new messages at startup.
3. Install the StartupMaster extension from https://addons.mozilla.org/en-US/firefox/addon/9808 (requires overriding compatibility).
4. Restart the application.
Actual Results:  
Two master password prompts appear: one before the 3-pane window is displayed and another one after it.

Expected Results:  
Only one master password prompt appears.

Thunderbird 2 did not exhibit this problem, so I assume it's a regression.

Comment 1

[Tamas Hubai [:htamas]]

Attachment: one-liner patch (don't call token.checkPassword() if already logged in)

I think the bug is caused by the call to token.checkPassword("") before token.login(false) in the loadStartFolder() function in mail/base/content/msgMail3PaneWindow.js. Apparently token.checkPassword() logs one out from the software security device if the supplied password is incorrect, which causes another login dialog even if we have been logged in before.

I'm attaching a one-liner patch that should fix the problem in the current trunk. It might be beneficial to also modify the wording of the comment above the changed line.

Comment 2

[Ludovic Hirlimann [:Usul]]

(In reply to comment #1)

> I'm attaching a one-liner patch that should fix the problem in the current
> trunk. It might be beneficial to also modify the wording of the comment above
> the changed line.

If you want your patch to get in, you'll need to request reviews as described at https://developer.mozilla.org/en/comm-central#Requirements.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 417306 [details] [diff] [review]
one-liner patch (don't call token.checkPassword() if already logged in)

Ooooh, this looks like how I've been thinking of fixing a few issues around this.

Comment 4

[Tamas Hubai [:htamas]]

Browsing through the sources it appears that this login pattern occurs a few more times:
* suite/common/src/nsSuiteGlue.js line 205
* calendar/base/src/calCalendarManager.js line 215
in addition to the aforementioned
* mail/base/content/msgMail3PaneWindow.js line 598

I don't think that an additional check for being logged on would hurt anyone, but it might reduce some additional master password prompts. It might also address bug 349641.

Any objections to adding this additional check to all three files?

Comment 5

[Ludovic Hirlimann [:Usul]]

I'm don't think people will object; make sure to make three patches .

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Looking forward to the calendar patch, I hope this reduces the master password stuff for calendar too :)

Comment 7

[Tamas Hubai [:htamas]]

Attachment: updated patch for mail (added comments in code)

Comment 8

[Tamas Hubai [:htamas]]

Attachment: new patch for suite

Comment 9

[Tamas Hubai [:htamas]]

Attachment: new patch for calendar

Comment 10

[[e-mail]]

Comment on attachment 417372 [details] [diff] [review]
new patch for suite

I think we might have over-engineered this. Can we not simply call .login(false) since that will not prompt if we have already logged in?

Comment 11

[Tamas Hubai [:htamas]]

(In reply to comment #10)
> (From update of attachment 417372 [details] [diff] [review])
> I think we might have over-engineered this. Can we not simply call
> .login(false) since that will not prompt if we have already logged in?

Yes, you're right. I just checked that removing the checkPassword() condition does the same as adding the isLoggedIn() one, both with and without a master password. Updating the three patches.

Comment 12

[Ludovic Hirlimann [:Usul]]

Once this lands we'll need to go thru password related bugs.

Comment 13

[Tamas Hubai [:htamas]]

Attachment: patch for mail, version 3

Comment 14

[Tamas Hubai [:htamas]]

Attachment: patch for suite, version 3

Comment 15

[Tamas Hubai [:htamas]]

Attachment: patch for calendar, version 3

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 417452 [details] [diff] [review]
patch for mail, version 3

David is probably the best option to look at this code - he and I wrote it between us iirc (and my review queue is big).

Comment 17

[Mark Banner (:standard8)]

I think we should try and get this into 3.0.1 as this will help resolve potential problems with multiple master password prompts and add-ons.

Comment 18

[David :Bienvenu]

Comment on attachment 417452 [details] [diff] [review]
patch for mail, version 3

this looks fine, thx; for some reason, I had to apply it by hand, for some reason, so I'm going to attach the patch I'm going to land on the trunk so we'll have a clean patch.

Comment 19

[David :Bienvenu]

Attachment: patch that applies

carrying forward my review; I'll land this soon on the trunk.

Comment 20

[David :Bienvenu]

fixed on trunk. changeset:   4556:c4775a7bc5bf

Comment 21

[David :Bienvenu]

Comment on attachment 418260 [details] [diff] [review]
patch that applies

a=3.01, after some baking on trunk.

Comment 22

[Tamas Hubai [:htamas]]

Thank you, David. By the way, did you intentionally re-add the following two lines of comments or was it only a side-effect of your cleanup?

// If an empty string is valid for the internal token, then we don't
// have a master password, else, if it does, then try to login.

(It doesn't matter much, I'm just curious.)

Comment 23

[David :Bienvenu]

sorry, it was a side effect of trying to apply the patch by hand. I'll try to clean it up.

Comment 24

[Tamas Hubai [:htamas]]

I've successfully checked out and built Thunderbird (Shredder) from the latest trunk. It compiled cleanly and the bug no longer occurs, either with or without a master password, on my i686 Linux box. I consider this half the battle won.

While waiting with the backport to 3.0.1, we could concentrate our efforts on the other two patches. Neil has already reviewed the one for SeaMonkey, so I assume it's ready to be applied. Could someone with commit access land it on trunk?

The calendar patch still needs a review from Philipp. Are you available or should I reassign it to someone else?

Comment 25

[Mark Banner (:standard8)]

I've just backed this out due to failures on our bloat builders:

http://hg.mozilla.org/comm-central/rev/6368ab714c7c

Looking around the builders, I saw one of them had Thunderbird displayed with a "Change Master Password" dialog. I'll try and add some more details in a bit.

Comment 26

[Mark Banner (:standard8)]

Ok, I did a quick pass through the bloat tests. It turns out that with the token.login() only version of the patch, it tries to get the user to set a master password in fresh profiles.

I'm guessing this is probably being done if the user hasn't previously saved any passwords, as it didn't happen on my dev profile, but it did on a fresh profile, even after I restarted Thunderbird.

If we go back to the original version of:

        if (!token.isLoggedIn() && (!token.checkPassword("")))
          token.login(false);

then it seems to work correctly for the bloat tests on a new profile at least (I haven't had time to verify existing profiles).

Comment 27

[[e-mail]]

So, just for the record, PSM/NSS is being pedantic.
* When PSM/NSS starts up, you are never logged in.
* When you call checkPassword you are always logged out if the password does not match or if you have not set a password and you explicitly logged out by calling logoutSimple. Otherwise you are logged in and it returns true.
* When you call login the password is not allowed to be empty, and it will throw if there is no password or if it was not correctly entered.

(In reply to comment #26)
> If we go back to the original version of:
> 
>         if (!token.isLoggedIn() && (!token.checkPassword("")))
>           token.login(false);
Actually the original version had even more superfluous brackets. Try
if (!token.isLoggedIn() && !token.checkPassword(""))
  token.login(false);

Comment 28

[David :Bienvenu]

I'm not quite following. Does the original patch fix the original bug, and the bloat tests? If so, we should land it, right?

Comment 29

[[e-mail]]

(In reply to comment #28)
> Does the original patch fix the original bug, and the bloat tests?
Yes.
> If so, we should land it, right?
But preferably with fewer ()s ;-)

Comment 30

[Tamas Hubai [:htamas]]

[I was writing this while the last few comments were submitted. Neil already mentioned some of the key points, but I'll still send it unchanged.]

Oops, the change master password dialog really pops up for a fresh profile.

Looks like it was an oversimplification to think that a master password can only be "set" or "unset". According to the NSS sources, a slot corresponding to a security token can be in one of the following six states, as indexed by the nsIPKCS11Slot.SLOT_* constants:

* DISABLED (0)
* NOT_PRESENT (1)
* UNINITIALIZED (2)
* NOT_LOGGED_IN (3)
* LOGGED_IN (4)
* READY (5)

I think DISABLED and NOT_PRESENT are not applicable to the PSM private key store (NSS internal module, slot 2). But UNINITIALIZED is indeed used for a new profile until a (possibly empty) master password is set. After that, the status becomes READY for an empty password and either NOT_LOGGED_IN or LOGGED_IN for a non-empty one.

According to my autopsy, calling token.login(false) in a LOGGED_IN or READY state does nothing, while doing so in a NOT_LOGGED_IN state shows the usual master password prompt. And in the UNINITIALIZED state, it tries to initialize the database, asking you to set a master password. If you cancel this dialog, the login call fails.

You can verify these claims by trying to log in interactively from the device manager. Note that you can reset the software security device to an UNINITIALIZED state by deleting the 'key3.db' file from the profile folder, and quickly access the device manager by running:
thunderbird -chrome 'chrome://pippki/content/device_manager.xul'

Back to the patch:
We need some check around token.login(false) in order not to call it in an UNINITIALIZED state. This can either be a combination of token.isLoggedIn() and token.checkPassword("") as in the first version of the patch, or just a conditional based on the token.needsUserInit attribute. This latter one seems simpler to me, so I'm attaching a new patch for all the three components again.

Mark, can you run the bloat tests for the new version without checking it in?

Comment 31

[Tamas Hubai [:htamas]]

Attachment: [checked in, trunk + branch] patch for mail, version 4

Comment 32

[Tamas Hubai [:htamas]]

Attachment: [checked in, trunk + branch] patch for suite, version 4

Comment 33

[Tamas Hubai [:htamas]]

Attachment: [checked in, trunk + branch] patch for calendar, version 4

Comment 34

[[e-mail]]

Comment on attachment 418424 [details] [diff] [review]
[checked in, trunk + branch] patch for suite, version 4

Sure, this seems to work.

Comment 35

[David :Bienvenu]

tb patch checked in.

Comment 36

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 418426 [details] [diff] [review]
[checked in, trunk + branch] patch for calendar, version 4

Looks fine for calendar. Taking this for comm-central and comm-1.9.1 (default branch). If we need to release 1.0b1rc2, then this patch should be contained. 

If all other patches are checked in and calendar 1.0b1 has not been released, please move this bug to Product Calendar and request blocking-calendar1.0.

Comment 37

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 418426 [details] [diff] [review]
[checked in, trunk + branch] patch for calendar, version 4

Pushed to calendar's comm-central: <http://hg.mozilla.org/comm-central/rev/2de08c283083>

and comm-1.9.1 (default branch): <http://hg.mozilla.org/releases/comm-1.9.1/rev/fecde353a673>

Comment 38

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Forgot to push comm-1.9.1, new rev id is 4234d9af7434

Comment 39

[Mark Banner (:standard8)]

Comment on attachment 418424 [details] [diff] [review]
[checked in, trunk + branch] patch for suite, version 4

Checked in: http://hg.mozilla.org/comm-central/rev/d1fa0f378721

Comment 40

[Mark Banner (:standard8)]

This is now fixed so marking as such.(In reply to comment #36)

> If all other patches are checked in and calendar 1.0b1 has not been released,
> please move this bug to Product Calendar and request blocking-calendar1.0.

I'd prefer just to file a new bug for that as we need to keep this one here for QA purposes.

Comment 41

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Ok, using bug 349641 for further calendar issues.

Comment 42

[Mark Banner (:standard8)]

Thunderbird patch checked in on branch: http://hg.mozilla.org/releases/comm-1.9.1/rev/fca195ba701a

Comment 43

[Jens Hatlak (:InvisibleSmiley)]

SeaMonkey part checked in on branch:
http://hg.mozilla.org/releases/comm-1.9.1/rev/fadc50b3945d

Comment 48

[Ronny Perinke]

With Tb 3.0.1 and the Lightning build from 2010-01-20 the problem was gone, but with the new Lightning build from 2010-01-24 I'm asked again two times for the master password.


Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.1.7) Gecko/20100111 Lightning/1.0b2pre Thunderbird/3.0.1

Lightning 1.0b2pre + Gdata Provider 0.6b2pre (2010-01-24)

Comment 49

[Milind Rao]

I have the same problem as Ronny.  This defect needs to be re-opened.

Comment 50

[Mark Banner (:standard8)]

(In reply to comment #49)
> I have the same problem as Ronny.  This defect needs to be re-opened.

Please file a new bug. This bug did fix something even if it hasn't fixed your specific issue.

Comment 51

[Charles Cox]

So which version is this actually fixed in?  I'm using the public release of Thunderbird 3.0.4 and I'm still getting prompted 4 times every time I start TB.  If the fix is only in 3.1 dev builds, is there any way we can get it backported into 3.0?

Comment 52

[Tamas Hubai [:htamas]]

As far as I remember, this bug was fixed in Thunderbird 3.0.1. Are you sure you have the same issue? Can you reproduce the bug using the steps in comment 0?

Comment 53

[Charles Cox]

(In reply to comment #52)
> As far as I remember, this bug was fixed in Thunderbird 3.0.1. Are you sure you
> have the same issue? Can you reproduce the bug using the steps in comment 0?

I don't use the StartupMaster extension, but I do use Lightning 1.0b1 with TB 3.0.4 on both Linux and Solaris.  Both give the same behavior, i.e. 4 or 5 prompts for the master password every time I start TB.  I think it's one prompt for each IMAP account plus one for calendar.

Comment 54

[Mark Banner (:standard8)]

(In reply to comment #53)
> (In reply to comment #52)
> > As far as I remember, this bug was fixed in Thunderbird 3.0.1. Are you sure you
> > have the same issue? Can you reproduce the bug using the steps in comment 0?
> 
> I don't use the StartupMaster extension, but I do use Lightning 1.0b1 with TB
> 3.0.4 on both Linux and Solaris.  Both give the same behavior, i.e. 4 or 5
> prompts for the master password every time I start TB.  I think it's one prompt
> for each IMAP account plus one for calendar.

iirc the Lightning fix missed 1.0b1. Thunderbird 3.1 has reworked how it manages those prompts, so should be a lot better, although I don't know if Lightning has picked up those changes yet (which is a separate bug).