1710290 - (CVE-2021-29956) For OpenPGP secret keys imported with Thunderbird versions 78.8.1 - 78.10.1, the master password isn't effective

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, defect)

Description

[Kai Engert [:KaiE:]]

The default behavior of Thunderbird 78 is to use an automatic protection mechanism for OpenPGP secret keys. To protect the keys at rest, users are required to set a master password.

Versions from 78.8.1 until 78.10.1 contain a regression bug (introduced by bug 1673239) that imports keys without protection. As a result, even having a master password set doesn't protect the local storage of the secret key.

I'm initially filing this as a security senstitive bug, however, I suggest to open this bug immediately (today), because several users have publicly reported on the Thunderbird e2ee mailing list that the master password protection is ineffective for them.

Comment 1

[Kai Engert [:KaiE:]]

The fix for this bug should:

• fix the import mechanism, to ensure all imported secret keys are immediately protected
• introduce an automatic repair mechanism, which scans for unprotected keys, and automatically enables protection for those keys

We should carefully prepare the fix, which may take a few days to create, review, test and release.

In the meantime, as a hotfix, it might be possible to offer advanced users a hotfix, which they can execute using the JavaScript/Error console.

Comment 2

[Kai Engert [:KaiE:]]

To check if you are affected by this bug, you may use the following procedure:

Step 1: Select and copy the following block of text:

function reportUnprotectedKeys() {
  const { RNP } = ChromeUtils.import(
    "chrome://openpgp/content/modules/RNP.jsm"
  );
  let [prot, unprot] = RNP.getProtectedKeysCount();
  console.log("Number of protected keys: " + prot);
  console.log("Number of unprotected keys: " + unprot);
}
reportUnprotectedKeys();

Step 2: Use the menu to open the error console. Either use the menu shown on the top of the Thunderbird window, or click the "burger menu" (three horizontal lines) in the upper right of the Thunderbird Window. Select Tools, Developer Tools, Error Console.

Step 3:
Click the space after the two blue arrows. Paste the block that you have selected above, then press the enter key.

The console will show a message saying "Number of unprotected keys". If the number shown is zero (0), you are not affected.

If a number other than zero is shown, you are affected by this bug.

Edit, 2021-05-11: Improved logging code as suggested in comment 5 and comment 6.

Comment 3

[Wayne Mery (:wsmwk)]

FYI we plan to do a 78.10.2 this week - but didn't plan to do another beta until next week.

Would we want to ship on both channels at same time? Or do we want it on beta first for a few days? The later would imply that we won't ship the patch on release channel until roughly next week.

Comment 4

[Kai Engert [:KaiE:]]

Attachment: hotfix-js-1710290-v1.txt

As a possible hotfix for advanced users, I am attaching a file with commands for repairing, that very likely should be safe for everyone to execute, whether affected or not affected by this bug. Let's test and review the hotfix.

Intended to be executed by affected users on the JS error console, if they don't want to wait for a fixed release.

Comment 5

[Rob Lemley [:rjl]]

I suggest outputting the number of initial and post repair protected keys as well. It was the first question that popped into my head after running the repair function.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9220997 [details]
hotfix-js-1710290-v1.txt

I'd lint it for readability.
Also move p declaration down to where it's first used. Or well, you don't need it at all. Just do if(!OpenPGPMasterpass._ensureMasterPassword())
For running in the console, the try catch is not useful I think.

What's the "repeat this procedure" about? To verify it worked? Maybe write that out in the msg if so.

Comment 7

[Kai Engert [:KaiE:]]

Thanks, I will update the diagnosis and hotfix repair commands.

To fix the primary bug in Thunderbird, the following change needs to be reverted (only lines 1.97 to 1.104, not the whole patch):
https://hg.mozilla.org/releases/comm-esr78/rev/0c8606e7f45d805bf12d8e67828c24a343b34f11#l1.88

That will ensure that importing keys in the future will again result in protected keys.

It was my mistake to remove those lines. I had assumed that the new calls to rnp_key_protect would be sufficient to protect the keys. I should have verified my assumption. (The protection got lost when exporting the keys from the temporary area, although it shouldn't have gotten lost.)

I still need to work on the automatic repair. That's a bit tricky. The important decision is: at what time should the automatic repair be made? This decision is important, because repairing requires that the user has successfully entered the master password. I'm evaluating options.

Comment 8

[Kai Engert [:KaiE:]]

While working on a new automated test, I discovered an additional angle of this bug:

If the imported secret is unprotected (can be imported without entering the password), then the current code apparently works. RNP reports the imported key as a protected key.

Only if the imported secret is protected with a password, then the current code fails, and the imported key is reported as unprotected.

Comment 9

[Kai Engert [:KaiE:]]

Attachment: hotfix-js-1710290-v2.txt

Updated hotfix code, as suggested in comment 5 and comment 6.

Comment 10

[Kai Engert [:KaiE:]]

Note I have edited comment 2 with improved diagnostic logging, as suggested in comment 5 and comment 6.

Comment 11

[Kai Engert [:KaiE:]]

Attachment: Bug 1710290 - Part 1, preparations: Moving code and APIs, log number of unprotected keys, update a test key. r=mkmelin

Comment 12

[Kai Engert [:KaiE:]]

Attachment: Bug 1710290 - Part 2: Protect keys after import into permanent store, and add a test. r=mkmelin

Depends on D114823

Comment 13

[Kai Engert [:KaiE:]]

Attachment: Bug 1710290 - Part 3: Repairing, automatically protect any unprotected keys in storage. r=mkmelin

Depends on D114824

Comment 14

[Kai Engert [:KaiE:]]

An experimental test build is available from the links below.
If you'd like to test it, please backup your profile.

linux64: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/bBK_v-TPRMeINEQxDR5DOw/runs/0/artifacts/public/build/target.tar.bz2
linux32: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/OOOv6eRCSiqsuNcTYIrwAQ/runs/0/artifacts/public/build/target.tar.bz2
win64: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/HJkBP0nFT-OBFonTikh_Sg/runs/0/artifacts/public/build/target.zip
win32: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/OlEaYmE5TI6ZfCb8w7afnA/runs/0/artifacts/public/build/target.zip
macos: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/Jo1uSQkISxaiU5_EgYkzAw/runs/0/artifacts/public/build/target.dmg

Comment 15

[Kai Engert [:KaiE:]]

To clarify, the experimental build mentioned in the previous comment is based on latest 78.x

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0ab383eab07b
Part 1, preparations: Moving code and APIs, log number of unprotected keys, update a test key. r=mkmelin
https://hg.mozilla.org/comm-central/rev/5c58842e025a
Part 2: Protect keys after import into permanent store, and add a test. r=mkmelin
https://hg.mozilla.org/comm-central/rev/d5de99115896
Part 3: Repairing, automatically protect any unprotected keys in storage. r=mkmelin

Comment 17

[Kai Engert [:KaiE:]]

Comment on attachment 9221284 [details]
Bug 1710290 - Part 1, preparations: Moving code and APIs, log number of unprotected keys, update a test key. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): 1673239
User impact if declined: Local storage of OpenPGP secret keys may be unprotected despite master password.
Testing completed (on c-c, etc.): new automated test for regression fix. No feedback yet for automatic repairing.
Risk to taking this patch (and alternatives if risky): low to medium. Mostly code moving, new logging, and restoring a previously removed code block (adjusted after the offline primary key feature). Automatic repair code uses existing functions and is straightforward.

Comment 18

[Wayne Mery (:wsmwk)]

Comment on attachment 9221284 [details]
Bug 1710290 - Part 1, preparations: Moving code and APIs, log number of unprotected keys, update a test key. r=mkmelin

[Triage Comment]
Approved for beta

Comment 19

[Wayne Mery (:wsmwk)]

Comment on attachment 9221286 [details]
Bug 1710290 - Part 3: Repairing, automatically protect any unprotected keys in storage. r=mkmelin

[Triage Comment]
Approved for beta

Comment 20

[Wayne Mery (:wsmwk)]

Comment on attachment 9221285 [details]
Bug 1710290 - Part 2: Protect keys after import into permanent store, and add a test. r=mkmelin

[Triage Comment]
Approved for beta

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/04f57699828a
follow-up - linting fix. rs=linting

Comment 22

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/releases/comm-beta/rev/08585e2d0c668251f00212ce81d082a32d480320
https://hg.mozilla.org/releases/comm-beta/rev/498ff73e90a3c81f2a89c9012196ab58b2d49f7a
https://hg.mozilla.org/releases/comm-beta/rev/be0ed093191adfa7c91515bca973714858bb8dca
89.0b4

I missed the lint fix. Rob, can you please push comment 21 to beta when finalizing the beta? I want to avoid a separate push.

Comment 23

[Rob Lemley [:rjl]]

Thunderbird 89.0b4:
https://hg.mozilla.org/releases/comm-beta/rev/019b920cd5e0

Comment 24

[Wayne Mery (:wsmwk)]

Comment on attachment 9221286 [details]
Bug 1710290 - Part 3: Repairing, automatically protect any unprotected keys in storage. r=mkmelin

[Triage Comment]
Approved for esr78

Comment 25

[Wayne Mery (:wsmwk)]

Comment on attachment 9221284 [details]
Bug 1710290 - Part 1, preparations: Moving code and APIs, log number of unprotected keys, update a test key. r=mkmelin

[Triage Comment]
Approved for esr78

Comment 26

[Wayne Mery (:wsmwk)]

Comment on attachment 9221285 [details]
Bug 1710290 - Part 2: Protect keys after import into permanent store, and add a test. r=mkmelin

[Triage Comment]
Approved for esr78

Comment 27

[Kai Engert [:KaiE:]]

78.10.2
https://hg.mozilla.org/releases/comm-esr78/rev/36ed28f4ec88f8d747399b538d5afd3e73702c94
https://hg.mozilla.org/releases/comm-esr78/rev/248ac0ef78f64b0c6a57dad759a34977fb73d481
https://hg.mozilla.org/releases/comm-esr78/rev/6657aa43703aaa1de16003c4ab7e92a1cbc6dff8

Comment 28

[Kai Engert [:KaiE:]]

The RNP advisory can be found here:
https://www.rnpgp.org/advisories/ri-2021-001/