1606619 - (CVE-2020-6794) key3.db encryption key remains on disk from Thunderbird 52.x (becomes issue if adding a master password post 60.x)

Status: RESOLVED FIXED

(Thunderbird :: Security, defect)

Description

[Kai Engert [:KaiE:]]

We haven't yet fixed bug 1475775 in Thunderbird.

Because of the risk of key dataloss (e.g. bug 1510212), we're waiting for a solution that is more reliable than what Firefox had used as a fix.

Let's use this new bug to track fixing the issue in Thunderbird.

In separate NSS bug 1561368 we're working out an appropriate solution, potentially at the NSS code level.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: testing-bug-1606619.txt

Comment 2

[Kai Engert [:KaiE:]]

Attachment: 1606619-v1.patch (intended for the fork of the Mozilla repository used by Thunderbird 68.x)

Comment 3

[Kai Engert [:KaiE:]]

The attached patch applies to TB 68.x, only.

The patch doesn't make sense for any later Thunderbird versions, because:

• we didn't take bug 1561366 for comm-central, which means, thunderbird versions 69.x+ already deleted key3.db
• mozilla branch 73+ has disabled support for key3.db (dbm file format) completely (bug 1594931), which means, versions 73+ are no longer able to perform a migration from key3.db to key4.db

If my patch is wrong, it will cause dataloss for those users who will upgrade from thunderbird 52.x to a thunderbird 68.x having this fix.

However, with the testing steps listed in the attached text file, which I have executed locally, I try to show that the patch will delete key3.db only after the migration was successful.

Comment 4

[Masatoshi Kimura [:emk]]

Comment on attachment 9119188 [details] [diff] [review]
1606619-v1.patch (intended for the fork of the Mozilla repository used by Thunderbird 68.x)

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

::: security/manager/ssl/SecretDecoderRing.cpp
@@ +200,5 @@
> +  nsCOMPtr<nsIFile> file = do_CreateInstance("@mozilla.org/file/local;1");
> +  if (!file) {
> +    return NS_ERROR_FAILURE;
> +  }
> +  nsresult rv = file->InitWithNativePath(path);

See bug 1607652.

Comment 5

[Kai Engert [:KaiE:]]

Bob, thanks again for your very helpful comments in bug 1561368.

Bob, can I assume, if we aren't merging,

slot = PK11_GetInternalKeySlot()
bool done = !PK11_IsReadOnly(slot) && PK11_IsLoggedIn(slot, NULL);

and done is true, that the migration from key3.db to key4.db has already completed?

FYI, the attached patch implements the following strategy:

• don't remove key3.db by default (as Firefox does it)
• wait until we've successfully decrypted a string stored in the secret decoder. If that works, either no master password is configured, or the master password was correctly provided by the user.
• at this time, do a once-per-application-session check for the potential cleanup
• double-check that our internal token is really logged in (we assume this means the data was migrated), and that we aren't in read-only mode (this ensures that a migration to key4.db was allowed to be written to disk)
• confirm that key4.db exists on disk. If it doesn't exist, stop.
• check whether key3.db exists on disk. If it exists, delete it.

Comment 6

[Robert Relyea]

That should work. The only possible issue is if the update failed after the login in a way that it will try to update again. I don't see a good way of detecting this case.

Comment 7

[Kai Engert [:KaiE:]]

Comment on attachment 9119188 [details] [diff] [review]
1606619-v1.patch (intended for the fork of the Mozilla repository used by Thunderbird 68.x)

Magnus, given Bob approved the strategy, are you OK to take this fix for stable Thunderbird 68.x (only)?

Any suggestion who might want to double-check my fix, by potentially playing with the separate test instructions I've attached?

Comment 8

[Kai Engert [:KaiE:]]

regarding comment 4, I'd merge that fix into this patch once that one is reviewed

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9119188 [details] [diff] [review]
1606619-v1.patch (intended for the fork of the Mozilla repository used by Thunderbird 68.x)

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

Seems fine to me.

Maybe you can let Khushil go through the steps an see if he can verify.

Comment 10

[Masatoshi Kimura [:emk]]

Please port bug 1607652. Thunderbird 68 is the last chance to upgrade from DBM and clean-up key3.db correctly without dataloss.

Comment 11

[Kai Engert [:KaiE:]]

Attachment: 1606619-1607652.patch

emk, can you please review this incremental patch, to confirm it's correctly and completely merged for the key3.db scenario?

Comment 12

[Masatoshi Kimura [:emk]]

Comment on attachment 9120253 [details] [diff] [review]
1606619-1607652.patch

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

LGTM

Comment 13

[Kai Engert [:KaiE:]]

Attachment: 1606619-complete-v2.patch

This patch combines the two earlier patches into a single patch, and adds a review comment.

Comment 14

[Kai Engert [:KaiE:]]

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

Maybe you can let Khushil go through the steps an see if he can verify.

Hi Khushil, may I ask for your help to test this change?

I haven't been able to create a try build yet, so you'd have to build locally.
Only for comm-esr68, and the patch is for mozilla-esr68 and THUNDERBIRD_68_VERBRANCH.
Test instructions are attached, too.

Comment 15

[Kai Engert [:KaiE:]]

Khushil, please see comment 14.

Comment 16

[Kai Engert [:KaiE:]]

Looks like the try server is down, so maybe I'll be able to create a test build a bit later.

Comment 17

[Kai Engert [:KaiE:]]

They've fixed it.
Try build running here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=49cdb3d083ba692b84af334edc21a86a38c3e42d

Comment 18

[Kai Engert [:KaiE:]]

Attachment: 1606619-complete-v3.patch

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

Try build running here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=49cdb3d083ba692b84af334edc21a86a38c3e42d

The above has builds for Linux and Mac.

Windows build had failed because of a missing include. This updated patch fixes it. Successful try build for Windows is here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0264197e4bd32135e3eb454df21e70ccbe39a01d

Comment 19

[Kai Engert [:KaiE:]]

Comment on attachment 9120340 [details] [diff] [review]
1606619-complete-v3.patch

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: ESR 68.x is the last chance to fix this CVE for Thunderbird. We'll be unable to cleanup key3.db in TB 78, because TB 78 will no longer be able to perform a data migration.
• User impact if declined: Users having upgraded from TB 52.x will still be exposed to the security issue, because unprotected key3.db will remain on disk.
• Fix Landed on Version: n/a
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): There's a small potential risk for dataloss (of saved passwords and private keys), for people who'll upgrade directly from TB 52.x to TB 68.x with this fix. Bob Relyea stated that in theory, the logic we use to conclude that "migration to key4.db has completed" might produce a false positive in an edge case scenario, but he isn't aware of a way to test for that edge case.
  Not at risk are people who have already used TB 60.x or previous TB 68.x for a while, and have already entered their master password several times. Those should certainly have been migrated successfully already.
• String or UUID changes made by this patch: none

Comment 20

[Jorg K (CEST = GMT+2)]

Umm, is this a request for mozilla-esr68 trunk or THUNDERBIRD_68_VERBRANCH? For the latter, we don't ask for approval, we just land it there ;-)

Comment 21

[Kai Engert [:KaiE:]]

Comment on attachment 9120340 [details] [diff] [review]
1606619-complete-v3.patch

Yeah, sorry, approval request for THUNDERBIRD_68_VERBRANCH.
Jörg, those comments were intended for you, to clarify and document the risks.

Before landing, we should wait for a positive test feedback from either Khushil or emk.

Comment 22

[Khushil Mistry [:khushil324]]

Yes, it worked. I followed all the steps and finally, key3.db was not there in the profile.

Comment 23

[Kai Engert [:KaiE:]]

Khushil, thanks for testing.

Comment 24

[Kai Engert [:KaiE:]]

Hello Aryx, could you please commit attachment 9120340 [details] [diff] [review] on mozilla-esr68/THUNDERBIRD_68_VERBRANCH ?
Thanks in advance

Comment 25

[Magnus Melin [:mkmelin]]

Aryx, see comment 24.

Comment 26

[Kai Engert [:KaiE:]]

Updating summary to match description from original Firefox bug 1475775 (CVE-2018-12383).

Comment 27

[Kai Engert [:KaiE:]]

[deleted, double post]

Comment 28

[Kai Engert [:KaiE:]]

Hello Red Hat / CentOS people, cc'ing you FYI. If you have older RHEL branches that use patched NSS/FF/TB that is hardcoded to use key3.db/DBM, and which also gets Thunderbird 68.x updates, then you probably want to verify this patch is safe for you.

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/661669a9175b755a43bde8a1a6594cd93f3f75df

Shall this only land on mozilla-central?

Comment 30

[Kai Engert [:KaiE:]]

Shall this only land on mozilla-central?

No. Only for Thunderbird 68. Firefox already fixed this differently in bug 1475775.

Comment 31

[Kai Engert [:KaiE:]]

We'll attribute Jurgen as the original reporter of this Thunderbird issue, because it's the same as the Firefox issue reported in bug 1475775.