1610390 - Add Thunderbird specific code to allow master password unlocking at startup time

Status: RESOLVED FIXED

(Thunderbird :: Security, enhancement)

Description

[Kai Engert (:KaiE:)]

Given the complexity required for a "clean" solution, this is a hack attempt.

It works just like the former StartupMaster Add-on.

It adds equivalent to the C++ startup code of the application, forcing a master password prompt, before any windows are shown, before other activity is started.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: 177175-like-startupmaster-v1.patch

Comment 2

[Kai Engert (:KaiE:)]

tb 68.x branch try build

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8e3c47e9bd3db7dc4a5d16e291262f7079846349

Comment 3

[Kai Engert (:KaiE:)]

try build refers to this gecko try patch
https://hg.mozilla.org/try/rev/0ec4379daad092e342b1c7369f0544fe0c2c3250

Comment 4

[Kai Engert (:KaiE:)]

Attachment: 177175-like-startupmaster-v1.patch (optional, with pref)

v1 prompts unconditionally (only if master password is set)

this v2 doesn't change the behavior by default.
Only if user adds hidden boolean pref security.prompt_for_master_password_on_startup and sets it to true, prompt will be enforced on startup.

Comment 5

[Kai Engert (:KaiE:)]

Attachment: 177175-like-startupmaster-v2.patch (optional, with pref)

Comment 6

[Kai Engert (:KaiE:)]

Attachment: 177175-like-startupmaster-v2.patch (optional, with pref)

Comment 7

[Kai Engert (:KaiE:)]

Comment on attachment 9121938 [details] [diff] [review]
177175-like-startupmaster-v2.patch (optional, with pref)

I suggest to take this patch for THUNDERBIRD_68_VERBRANCH.

By default, it's a no-op.

For anyone annoyed by multiple master password prompts on startup, it gives them a way to experiment with this potential fix.
The potential fix works equivalently to the former "startup master" add-on ( https://addons.thunderbird.net/de/thunderbird/addon/startupmaster/ ), by forcing a master password login prompt during startup, prior to any parallel network activity kicking off.

(We don't have an appropriate approval flag for the tb-esr68 branch, so please comment with words, if you support or reject the suggestion.)

Comment 8

[Kai Engert (:KaiE:)]

However, prior to shipping, we should test the try build on all platforms. I've tested Linux 64, which works for me. (Reduces 5 prompts to 1.)

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9121938 [details] [diff] [review]
177175-like-startupmaster-v2.patch (optional, with pref)

Review of attachment 9121938 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a reviewer for this code, but looks alright

::: toolkit/xre/nsAppRunner.cpp
@@ +4445,5 @@
> +        do_GetService("@mozilla.org/security/pk11tokendb;1");
> +      nsCOMPtr<nsIPK11Token> token;
> +      nsresult rv = db->GetInternalKeyToken(getter_AddRefs(token));
> +      if (NS_SUCCEEDED(rv)) {
> +        rv = token->Login(false);

the rv is unused, I guess you'd just want to drop that

Comment 10

[Kai Engert (:KaiE:)]

Attachment: 1610390-v3.patch

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 9121983 [details] [diff] [review]
1610390-v3.patch

Dana, Nathan, what's your thought on adding this code to the startup path, until we have a better solution?

We consider to add it to THUNDERBIRD_68_VERBRANCH as a band aid, for people suffering from multiple prompts for the master password on startup. Having it on comm-central would be nice, to avoid repeated merging into TB's gecko branch.

As explained in 177175 comment 384, the real solution would be more work. I'd like to help with that work, but likely won't be able to make that a priority during the next 6 months, because of important feature development for Thunderbird 78.

Comment 12

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:KaiE:) from comment #11)

Having
it on comm-central would be nice, to avoid repeated merging into TB's gecko
branch.

... and to have it in nightly and TB betas.

If you don't want to have this code active for Firefox - would you accept #ifdef MOZ_THUNDERBIRD ?

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 9121983 [details] [diff] [review]
1610390-v3.patch

Review of attachment 9121983 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think I'd accept this without an `#ifdef MOZ_THUNDERBIRD` or similar.

Do we care that something can set the appropriate pref and (seemingly) lock people out of their mail client/web browser?

Comment 14

[Kai Engert (:KaiE:)]

(In reply to Nathan Froyd [:froydnj] from comment #13)

Do we care that something can set the appropriate pref and (seemingly) lock
people out of their mail client/web browser?

The call to token->login will be a no-op, if no master password is set.

If a MP is set and the pref is set, this code will bring up the prompt, with the choice to cancel. If the user cancels, startup will proceed normally (and as usual, will prompt for the MP at the time it's needed).

Comment 15

[Kai Engert (:KaiE:)]

(just curious if the additional information from comment 14 changes your opinion)

Comment 16

[Nathan Froyd [:froydnj]]

No, I think this would still need to be #ifdef MOZ_THUNDERBIRD, at least at first. It's possible it would be appropriate for Firefox, but I think such a change has implications that would need broader review before getting turned on for Firefox.

Comment 17

[Kai Engert (:KaiE:)]

Attachment: 1610390-v4.patch

Thanks for your feedback. MOZ_THUNDERBIRD seems fine to me. Being allowed to have this in m-c would still be helpful with the #ifdef.

I'm marking this as obsolete, and will submit a patch in phabricator.

Comment 18

[Kai Engert (:KaiE:)]

Attachment: Bug 1610390 - Add Thunderbird specific code to allow master password unlocking at startup time. r?froydnj

Comment 19

[Kai Engert (:KaiE:)]

Attachment: 1610390-default-prefs.patch

If we get the code accepted in toolkit (#ifdef'ed), then we should probably set a default pref value in Thunderbird. Makes the pref discoverable in config editor.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9122138 [details] [diff] [review]
1610390-default-prefs.patch

Review of attachment 9122138 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/app/profile/all-thunderbird.js
@@ +362,5 @@
>  pref("security.warn_leaving_secure", false);
>  pref("security.warn_viewing_mixed", false);
>  pref("security.aboutcertificate.enabled", false);
>  
> +pref("security.prompt_for_master_password_on_startup", false);

Wouldn't you make it true? Or is there a performance impact?
Please also add a comment about what it does, even if it's fairly self explanatory.

Comment 21

[Kai Engert (:KaiE:)]

(In reply to Magnus Melin [:mkmelin] from comment #20)

+pref("security.prompt_for_master_password_on_startup", false);

Wouldn't you make it true? Or is there a performance impact?

For comm-central, you're right, we should set it to true, so we get testing.

For a backport to comm-esr68, maybe we should be conservative and have it opt-in (false), so we can get some feedback, just in case it introduces a problem for some users?

Please also add a comment about what it does, even if it's fairly self
explanatory.

Will do

Comment 22

[Kai Engert (:KaiE:)]

Attachment: 1610390-pref-v2.patch

Comment 23

[Kai Engert (:KaiE:)]

Magnus, based on comment 21, can you please state your opinion for TB 68, false or true?
(If false, I'll attach a separate patch with comment.)

Comment 24

[Magnus Melin [:mkmelin]]

What's the pref impact when set to true but not master password used?

Comment 25

[Kai Engert (:KaiE:)]

(In reply to Magnus Melin [:mkmelin] from comment #24)

What's the pref impact when set to true but not master password used?

No user visible action should get triggered.
We tell NSS that we want to login, and NSS will detect that it doesn't require a password, no prompt.

I tested the above on TB 68, both with existing profiles and new profiles, and it works.

However, I just did some testing on comm-central, and I get some confusing results. I don't know yet if this is related, or another regression. I'll do some additional testing.

Comment 26

[Magnus Melin [:mkmelin]]

Yeah I'm asking how many milliseconds does a no-op cost?

Comment 27

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:KaiE:) from comment #25)

However, I just did some testing on comm-central, and I get some confusing results. I don't know yet if this is related, or another regression. I'll do some additional testing.

Because of bug 1610807, comm-central currently doesn't prompt for the master password at startup.

Comment 28

[Kai Engert (:KaiE:)]

(In reply to Magnus Melin [:mkmelin] from comment #26)

Yeah I'm asking how many milliseconds does a no-op cost?

With pref=false, zero ms (as expected, very quick for checking the pref value, only).

With pref enabled, I don't think we don't have to pay any price either.

Although with pref enabled, I measure 6 ms for this code (Intel i7, 2.8 ghz), it's because it triggers NSS library init at an earlier time. This time will be saved later on.

I'll attach the patch I used for measuring.
With pref disabled, NSS init is triggered in nsMsgAccountManager::Init calling net_EnsurePSMInit.

With pref disabled:
time for startup login: 0
time for net_EnsurePSMInit: 14

With pref enabled:
time for startup login: 6
time for net_EnsurePSMInit: 1

edit: fixed labels for timing results

Comment 29

[Kai Engert (:KaiE:)]

Attachment: timing patch for gecko

Comment 30

[Kai Engert (:KaiE:)]

Attachment: timing patch for comm

Comment 31

[Kai Engert (:KaiE:)]

Note, because of bug 1610807, the timing experiment was done on the 68.x branch.

Comment 32

[Kai Engert (:KaiE:)]

question from comment 23 is still open

Comment 33

[Magnus Melin [:mkmelin]]

I think probably true is fine, but let's see how it goes on trunk/beta first.

Comment 34

[Kai Engert (:KaiE:)]

Comment on attachment 9122309 [details] [diff] [review]
1610390-pref-v2.patch

ok, then lets start with "true" on c-c

Comment 35

[Kai Engert (:KaiE:)]

(In reply to Magnus Melin [:mkmelin] from comment #33)

I think probably true is fine, but let's see how it goes on trunk/beta first.

We don't use a gecko fork/branch for TB beta, so this can be tested on beta 64 as the earliest, IIUC.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4b71625fcfd
Add Thunderbird specific code to allow master password unlocking at startup time. r=froydnj

Comment 37

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/b4b71625fcfd

Comment 38

[Magnus Melin [:mkmelin]]

Comment on attachment 9122309 [details] [diff] [review]
1610390-pref-v2.patch

Review of attachment 9122309 [details] [diff] [review]:
-----------------------------------------------------------------

I guess, though this further aggravates bug 992569.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/64e8cb1308b1
Add default value for security.prompt_for_master_password_on_startup. r=mkmelin DONTBUILD

Comment 40

[Kai Engert (:KaiE:)]

Magnus, you asked for testing, prior to enabling this by default (pref true) to TB 68. That sounds good.

However, I wonder if we should land with pref set to false into TB 68 immediately, to allow TB 68 users to opt in to testing.

Comment 41

[Wayne Mery (:wsmwk)]

(In reply to Kai Engert (:KaiE:) from comment #40)

However, I wonder if we should land with pref set to false into TB 68 immediately, to allow TB 68 users to opt in to testing.

Comment 42

[Wayne Mery (:wsmwk)]

(We don't have an appropriate approval flag for the tb-esr68 branch, so please comment with words, if you support or reject the suggestion.)

I agree with taking it preffed off, if it's not difficult, because I think the preffed on patch should bake for the full beta cycle.

Comment 43

[Magnus Melin [:mkmelin]]

I would keep it on beta/trunk first, at least until the bug 1612456 regression is solved. In the wild we'd otherwise get a bunch of reports about that from people fiddling with prefs and don't understanding what went wrong.