Closed Bug 1650551 Opened 5 years ago Closed 5 years ago

If OpenPGP is enabled, encrypt draft messages

Categories

(MailNews Core :: Security: OpenPGP, enhancement, P1)

Product:
MailNews Core
Component:
Security: OpenPGP
Type:
enhancement

Tracking

(thunderbird_esr78+ fixed, thunderbird80 wontfix, thunderbird81 fixed)

Status:
RESOLVED FIXED
Milestone:
81 Branch
Tracking Flags:
Tracking Status
thunderbird_esr78 + fixed
thunderbird80 --- wontfix
thunderbird81 --- fixed

People

(Reporter: dab, Assigned: KaiE)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1650551 - If OpenPGP is enabled, encrypt draft messages. r=PatrickBrunschwig
47 bytes, text/x-phabricator-request
wsmwk
: approval-comm-esr78+
Details | Review
Bug 1650551 - Follow-up to restore a try/catch block, fixes tests. r=mkmelin
47 bytes, text/x-phabricator-request
wsmwk
: approval-comm-esr78+
Details | Review

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Firefox/78.0

Steps to reproduce:

As a long-time Enigmail user, I've been trying out the new PGP stuff in Thunderbird 78. Start composing a message and examine the message that's saved in the Drafts folder.

Actual results:

The message in the Drafts folder is in clear-text.

Expected results:

The message in the Drafts folder should be encrypted, especially if the outgoing message is flagged to be encrypted (arguably they should all be encrypted). Otherwise, if the Drafts folder is on a server machine, the message travels across the net in the clear each time it's saved before it's finally encrypted and sent. Makes the whole encryption thing kinda pointless.

Component: Message Compose Window → Security: OpenPGP
Product: Thunderbird → MailNews Core

Yes this is an important TODO.

Assignee: nobody → kaie
Status: UNCONFIRMED → NEW
Ever confirmed: true

Changing to blocker for enabling PGP by default per Kai

Severity: -- → S1
status-thunderbird80: --- → affected
status-thunderbird81: --- → fixed
status-thunderbird_esr78: --- → affected
tracking-thunderbird_esr78: --- → +
Priority: -- → P1

I think we should get this fixed before updating 68.x users to 78.x

Summary: drafts are not encrypted → If OpenPGP is enabled for an account, then draft messages should be stored encrypted on the servers.

The attached patch works in my testing. I've tested reading drafts that were saved using tb 68 + enigmail,
and I've also tested saving drafts with 78 and opening them again.

Summary: If OpenPGP is enabled for an account, then draft messages should be stored encrypted on the servers. → If OpenPGP is enabled, encrypt draft messages

The storage for encryption/signing/attachment flags is kept the same was it was used with latest Enigmail.
We interpret the flags slightly differently if the flag for signing or encryption is in the "undefined" state.
That is, if Enigmail TB 68 automatically decided that encryption is possible, because of the entered email address, then it stored the flag as "undefined". (Neither never = disabled nor always = enabled.) In this scenario, we don't automatically require encryption after opening the message. Because the user didn't explicitly request encryption when saving the draft in enigmail tb 68, I think that's acceptable.

The code also remembers and restores the previously configured flags.

For S/MIME messages, no draft flags are saved. Instead, we rely on the current encryption/signing properties, and restore the same.

Hmm, those flags are in a plain header, so trusting them seems like an exploit vector for the thing encrypted drafts try to protect against, no? (Could easily be swapped to a downgrade by the server admin.)

True, but if you don't trust the admin of your email provider, you should better change the provider...

Sure, but then, why encrypt drafts in the first place... the idea behind that is protect against a bad admin.

yes, you're right :-) Maybe we should consider moving the draft status into the encrypted message (just like the protected subject)?

Instead of continuing to support the Enigmail draft status, you might want to follow the Autocrypt specification for storing the draft status:
https://autocrypt.org/level1.html#message-drafts

I'm probably missing something, why can't we just check if the draft was indeed encrypted - and if it was, continue on with that?

The primary attack vector here is the admin's ability to read the draft, which this patch fixes.
By encrypting the draft, we ensure the mail server admin cannot read the message.

If the admin changes the flags, the admin still cannot read the draft.
Even if the user edits the draft, and saves the draft again, we will still encrypt the draft.

The effect of an admin changing the flags is:
If the user is not careful, they might not notice that the settings in TB composer is no longer "encrypt", and might accidentally send the message unencrypted. The admin must still hope for the user's inattention to succeed.

This attack vector isn't new. All current Enigmail users are exposed to this attack vector. Therefore I think it's OK to handle that in a follow-up. I'll file a bug for that.

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

I'm probably missing something, why can't we just check if the draft was indeed encrypted - and if it was, continue on with that?

Because the user might not intend to send this message with encryption. And the user might not be able to send the message with encryption, because no recipient keys are available. This conclusion (can or cannot encrypt) cannot be made until the user finally attempts to send the message. Only then we'll know who are the intended recipients.

There's no need to bother the user while they are editing the draft.

The user might start typing a sensitive message. And they might not be aware that the message leaves their computer prior to sending (autosave on server). Only before sending the user might set the encryption flag. By encrypting draft messages, we protect the contents, regardless of the time the user decides to switch on the encrypt flag.

Filed bug 1660191.

Blocks: 1660191

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

The effect of an admin changing the flags is:
If the user is not careful, they might not notice that the settings in TB composer is no longer "encrypt", and might accidentally send the message unencrypted. The admin must still hope for the user's inattention to succeed.

Isn't it enough that the user doesn't notice the flag was set to not encrypt, and then wait until we autosave a draft, which could be just one minute?

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

Isn't it enough that the user doesn't notice the flag was set to not encrypt, and then wait until we autosave a draft, which could be just one minute?

We will always encrypt the draft, even if the message settings say "do not encrypt".

Encrypting the draft isn't driven by individual message settings.
It's driven by "openpgp key has been configured for the user's account".

Also, IIUC we only read the flags from the saved draft, after the user closes the composer window, and we need to load the draft from the save stored.

If the user continues to edit the draft, there is no need to read the flags stored on the server.

Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/67037561c66b
If OpenPGP is enabled, encrypt draft messages. r=PatrickBrunschwig DONTBUILD

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch

Comment on attachment 9171034 [details]
Bug 1650551 - If OpenPGP is enabled, encrypt draft messages. r=PatrickBrunschwig

Important OpenPGP security feature for feature parity with past Enigmail. Little risk for non-OpenPGP users, and any risk limited to the ability to save drafts.

Attachment #9171034 - Flags: approval-comm-esr78?

Comment on attachment 9171034 [details]
Bug 1650551 - If OpenPGP is enabled, encrypt draft messages. r=PatrickBrunschwig

[Triage Comment]
approved for esr78

Attachment #9171034 - Flags: approval-comm-esr78? → approval-comm-esr78+

Hmm, this is causing some test failures (verified by local backout).

Check the bc3's at https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=67037561c66b4c16a0b3a016297a32ec7cdca736 which are new

comm/mail/test/browser/composition/browser_emlActions.js | A promise chain failed to handle a rejection: Component returned failure code: 0x80040154 (NS_ERROR_FACTORY_NOT_REGISTERED) [nsIMessenger.messageServiceFromURI] - stack: getMsgHdr@chrome://openpgp/content/ui/enigmailMsgComposeOverlay.js:448:24

Flags: needinfo?(kaie)

As well as bc5 comm/mail/test/browser/smime/browser_multipartAlternative.js at least

I understand why my patch introduced the failure.
Apparently the call to messenger.messageServiceFromURI(msgUri).messageURIToMsgHdr(msgUri); is know to fail sometimes.
This code is inside enigmail function getMsgHdr.

Before my patch, there was a try/catch block around the call to getMsgHdr.
My code moving has removed that try/catch block.

Restoring the try/catch restores the original behavior (which ignores some failues) and makes that pass again.

However, the question remained, why is this happening only on trunk, not on comm-esr78.
I have done some analysis on that, and will report the results in bug 1635648.

I suggest that we restore the try/catch block as part of this bug here, and solve the general problem in bug 1635648.

Flags: needinfo?(kaie)
Pushed by kaie@kuix.de: https://hg.mozilla.org/comm-central/rev/4beebecc71b7 Follow-up to restore try/catch for getMsgHdr, fixes tests. r=mkmelin

Comment on attachment 9171494 [details]
Bug 1650551 - Follow-up to restore a try/catch block, fixes tests. r=mkmelin

Restore exception suppression, because of failures on trunk in automated tests. Uplift for code consistency.

Attachment #9171494 - Flags: approval-comm-esr78?

Greetings from the Thunderbird team at p≡p. One question: Are there any plans to make draft encryption optional? It seems unnecessary to encrypt drafts which so far have no recipient or are stored in the Drafts folder in Local Folders, like for POP3 users or those IMAP users with a "flaky" connection who have configured their drafts storage to be local. BTW, p≡p as the concept of a "trusted server", so messages (including drafts) stored on such a server are not encrypted (especially since Gloda and folder search doesn't work on encrypted messages, bug 188988 and bug 1562737). Local Folders would generally be considered "trusted".

Comment on attachment 9171494 [details]
Bug 1650551 - Follow-up to restore a try/catch block, fixes tests. r=mkmelin

[Triage Comment]
Approved for esr78

Attachment #9171494 - Flags: approval-comm-esr78? → approval-comm-esr78+

I'm curious why the pref temp.openpgp.protectedHeaders (and temp.openpgp.protectedSubjectText) was dropped as part of this change. It actually seems unrelated to me. Any ideas?

I'm asking because there is some doubt around the readiness for forcing this change. My understanding is that far from all OpenPGP-enabled email client support MemoryHole at this point. So some distributions might want to (easily) be able to change this default, and then this pref is great to have.

Apologies if it's bad style to comment on a closed ticket!

See Also: → 1671830
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: