Closed Bug 534462 Opened 10 years ago Closed 10 years ago

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

Categories

(MailNews Core :: Security, defect)

x86
Linux
defect
Not set

Tracking

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

RESOLVED FIXED
Thunderbird 3.1a1
Tracking Status
blocking-thunderbird3.0 --- .1+
thunderbird3.0 --- .1-fixed

People

(Reporter: htamas, Assigned: htamas)

References

()

Details

(Keywords: fixed-seamonkey2.0.3, Whiteboard: [has patch][gs])

Attachments

(3 files, 8 obsolete files)

1.37 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
1.21 KB, patch
neil
: review+
Details | Diff | Splinter Review
1.26 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
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.
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: --- → ?
Assignee: nobody → htamas2
Status: UNCONFIRMED → NEW
Ever confirmed: true
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 :)
Attachment #417306 - Attachment is obsolete: true
Attachment #417371 - Flags: review?(dmose)
Attachment #417306 - Flags: review?(bugzilla)
Attached patch new patch for suite (obsolete) β€” β€” Splinter Review
Attachment #417372 - Flags: review?(neil)
Attached patch new patch for calendar (obsolete) β€” β€” Splinter Review
Attachment #417373 - Flags: review?
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?
(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.
Attached patch patch for mail, version 3 (obsolete) β€” β€” Splinter Review
Attachment #417371 - Attachment is obsolete: true
Attachment #417452 - Flags: review?(dmose)
Attachment #417371 - Flags: review?(dmose)
Attached patch patch for suite, version 3 (obsolete) β€” β€” Splinter Review
Attachment #417372 - Attachment is obsolete: true
Attachment #417453 - Flags: review?(neil)
Attachment #417372 - Flags: review?(neil)
Attached patch patch for calendar, version 3 (obsolete) β€” β€” Splinter Review
Attachment #417373 - Attachment is obsolete: true
Attachment #417454 - Flags: review?(philipp)
Attachment #417373 - Flags: review?(philipp)
Attachment #417453 - Flags: review?(neil) → review+
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+
Whiteboard: [has patch][needs review 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.
Attachment #417452 - Flags: review?(bienvenu) → review+
Attached patch patch that applies (obsolete) β€” β€” Splinter Review
carrying forward my review; I'll land this soon on the trunk.
Attachment #417452 - Attachment is obsolete: true
Attachment #418260 - Flags: review+
fixed on trunk. changeset:   4556:c4775a7bc5bf
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review bienvenu] → [has patch]
Target Milestone: --- → Thunderbird 3.1a1
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+
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.)
sorry, it was a side effect of trying to apply the patch by hand. I'll try to clean it up.
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);
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 ;-)
[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?
Attachment #417453 - Attachment is obsolete: true
Attachment #417454 - Attachment is obsolete: true
Attachment #417454 - Flags: review?(philipp)
Attachment #418423 - Flags: review?(bienvenu)
Attachment #418424 - Flags: review?(neil)
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+
Whiteboard: [has patch] → [has patch][needs review bienvenu][attachment 418260 is obsolete]
Attachment #418423 - Flags: review?(bienvenu) → review+
tb patch checked in.
Whiteboard: [has patch][needs review bienvenu][attachment 418260 is obsolete] → [has patch][attachment 418260 is obsolete]
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
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Ok, using bug 349641 for further calendar issues.
Attachment #418424 - Flags: approval-seamonkey2.0.2?
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+
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
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]
Attachment #418424 - Attachment description: [checked in] patch for suite, version 4 → [checked in, trunk + branch] patch for suite, version 4
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
Whiteboard: [has patch] → [has patch][gs]
Duplicate of this bug: 537490
Duplicate of this bug: 540526
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)
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.
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?
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?
(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.