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

RESOLVED FIXED in Thunderbird 3.1a1

Status

MailNews Core
Security
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: htamas, Assigned: htamas)

Tracking

({fixed-seamonkey2.0.3})

unspecified
Thunderbird 3.1a1
x86
Linux
fixed-seamonkey2.0.3

Firefox Tracking Flags

(blocking-thunderbird3.0 .1+, thunderbird3.0 .1-fixed)

Details

(Whiteboard: [has patch][gs], URL)

Attachments

(3 attachments, 8 obsolete attachments)

1.37 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
1.21 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
1.26 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
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.
(Assignee)

Comment 1

8 years ago
Created attachment 417306 [details] [diff] [review]
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.
(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 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.
Attachment #417306 - Flags: review?(bugzilla)
blocking-thunderbird3.0: --- → ?
status-thunderbird3.0: --- → ?
Assignee: nobody → htamas2
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 4

8 years ago
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?
Status: NEW → ASSIGNED
I'm don't think people will object; make sure to make three patches .
Looking forward to the calendar patch, I hope this reduces the master password stuff for calendar too :)
(Assignee)

Comment 7

8 years ago
Created attachment 417371 [details] [diff] [review]
updated patch for mail (added comments in code)
Attachment #417306 - Attachment is obsolete: true
Attachment #417371 - Flags: review?(dmose)
Attachment #417306 - Flags: review?(bugzilla)
(Assignee)

Comment 8

8 years ago
Created attachment 417372 [details] [diff] [review]
new patch for suite
Attachment #417372 - Flags: review?(neil)
(Assignee)

Comment 9

8 years ago
Created attachment 417373 [details] [diff] [review]
new patch for calendar
Attachment #417373 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #417373 - Flags: review? → review?(philipp)
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?
(Assignee)

Comment 11

8 years ago
(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.
Once this lands we'll need to go thru password related bugs.
(Assignee)

Comment 13

8 years ago
Created attachment 417452 [details] [diff] [review]
patch for mail, version 3
Attachment #417371 - Attachment is obsolete: true
Attachment #417452 - Flags: review?(dmose)
Attachment #417371 - Flags: review?(dmose)
(Assignee)

Comment 14

8 years ago
Created attachment 417453 [details] [diff] [review]
patch for suite, version 3
Attachment #417372 - Attachment is obsolete: true
Attachment #417453 - Flags: review?(neil)
Attachment #417372 - Flags: review?(neil)
(Assignee)

Comment 15

8 years ago
Created attachment 417454 [details] [diff] [review]
patch for calendar, version 3
Attachment #417373 - Attachment is obsolete: true
Attachment #417454 - Flags: review?(philipp)
Attachment #417373 - Flags: review?(philipp)

Updated

8 years ago
Attachment #417453 - Flags: review?(neil) → review+
(Assignee)

Updated

8 years ago
Attachment #417452 - Flags: review?(dmose) → review?(bugmail)
Attachment #417452 - Flags: review?(bugmail) → review?(bienvenu)
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).
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.
blocking-thunderbird3.0: ? → .1+
status-thunderbird3.0: ? → ---
Whiteboard: [has patch][needs review bienvenu]

Comment 18

8 years ago
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.
Attachment #417452 - Flags: review?(bienvenu) → review+

Comment 19

8 years ago
Created attachment 418260 [details] [diff] [review]
patch that applies

carrying forward my review; I'll land this soon on the trunk.
Attachment #417452 - Attachment is obsolete: true
Attachment #418260 - Flags: review+

Comment 20

8 years ago
fixed on trunk. changeset:   4556:c4775a7bc5bf
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review bienvenu] → [has patch]
Target Milestone: --- → Thunderbird 3.1a1

Comment 21

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

a=3.01, after some baking on trunk.
Attachment #418260 - Flags: approval-thunderbird3.0.1+
(Assignee)

Comment 22

8 years ago
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

8 years ago
sorry, it was a side effect of trying to apply the patch by hand. I'll try to clean it up.
(Assignee)

Comment 24

8 years ago
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?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #418260 - Flags: approval-thunderbird3.0.1+ → approval-thunderbird3.0.1-
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).
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

8 years ago
I'm not quite following. Does the original patch fix the original bug, and the bloat tests? If so, we should land it, right?
(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 ;-)
(Assignee)

Comment 30

8 years ago
[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?
(Assignee)

Comment 31

8 years ago
Created attachment 418423 [details] [diff] [review]
[checked in, trunk + branch] patch for mail, version 4
(Assignee)

Comment 32

8 years ago
Created attachment 418424 [details] [diff] [review]
[checked in, trunk + branch] patch for suite, version 4
Attachment #417453 - Attachment is obsolete: true
(Assignee)

Comment 33

8 years ago
Created attachment 418426 [details] [diff] [review]
[checked in, trunk + branch] patch for calendar, version 4
Attachment #417454 - Attachment is obsolete: true
Attachment #417454 - Flags: review?(philipp)
(Assignee)

Updated

8 years ago
Attachment #418423 - Flags: review?(bienvenu)
(Assignee)

Updated

8 years ago
Attachment #418424 - Flags: review?(neil)
(Assignee)

Updated

8 years ago
Attachment #418426 - Flags: review?(philipp)
Comment on attachment 418424 [details] [diff] [review]
[checked in, trunk + branch] patch for suite, version 4

Sure, this seems to work.
Attachment #418424 - Flags: review?(neil) → review+
(Assignee)

Updated

8 years ago
Whiteboard: [has patch] → [has patch][needs review bienvenu][attachment 418260 is obsolete]

Updated

8 years ago
Attachment #418423 - Flags: review?(bienvenu) → review+

Comment 35

8 years ago
tb patch checked in.
Whiteboard: [has patch][needs review bienvenu][attachment 418260 is obsolete] → [has patch][attachment 418260 is obsolete]

Updated

8 years ago
Attachment #418423 - Attachment description: patch for mail, version 4 → patch for mail, version 4 - checked in.
Attachment #418423 - Flags: approval-thunderbird3.0.1?
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.
Attachment #418426 - Flags: review?(philipp) → review+
Attachment #418426 - Attachment description: patch for calendar, version 4 → [checked in] patch for calendar, version 4
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>
Forgot to push comm-1.9.1, new rev id is 4234d9af7434
Attachment #418423 - Attachment description: patch for mail, version 4 - checked in. → [checked in] patch for mail, version 4
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
Attachment #418424 - Attachment description: patch for suite, version 4 → [checked in] patch for suite, version 4
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.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Ok, using bug 349641 for further calendar issues.
Attachment #418424 - Flags: approval-seamonkey2.0.2?

Updated

8 years ago
Attachment #418424 - Flags: approval-seamonkey2.0.2? → approval-seamonkey2.0.2+
Attachment #418426 - Attachment description: [checked in] patch for calendar, version 4 → [checked in, trunk + branch] patch for calendar, version 4
Attachment #418260 - Attachment is obsolete: true
Attachment #418423 - Flags: approval-thunderbird3.0.1? → approval-thunderbird3.0.1+
(Assignee)

Updated

8 years ago
Whiteboard: [has patch][attachment 418260 is obsolete] → [has patch]
Thunderbird patch checked in on branch: http://hg.mozilla.org/releases/comm-1.9.1/rev/fca195ba701a
status-thunderbird3.0: --- → .1-fixed
Keywords: checkin-needed
Whiteboard: [has patch] → [has patch][seamonkey patch needs checking in on branch]
SeaMonkey part checked in on branch:
http://hg.mozilla.org/releases/comm-1.9.1/rev/fadc50b3945d
Keywords: checkin-needed
Whiteboard: [has patch][seamonkey patch needs checking in on branch] → [has patch]
(Assignee)

Updated

8 years ago
Attachment #418424 - Attachment description: [checked in] patch for suite, version 4 → [checked in, trunk + branch] patch for suite, version 4
(Assignee)

Updated

8 years ago
Attachment #418423 - Attachment description: [checked in] patch for mail, version 4 → [checked in, trunk + branch] patch for mail, version 4
Duplicate of this bug: 538336
Duplicate of this bug: 483455

Updated

8 years ago
Whiteboard: [has patch] → [has patch][gs]
Duplicate of this bug: 537490
Duplicate of this bug: 540526

Comment 48

7 years ago
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)

Updated

7 years ago
Keywords: fixed-seamonkey2.0.3

Comment 49

7 years ago
I have the same problem as Ronny.  This defect needs to be re-opened.
(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

7 years ago
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?
(Assignee)

Comment 52

7 years ago
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

7 years ago
(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.
(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).
You need to log in before you can comment on or make changes to this bug.