1854684 - Get unencrypted message data from pgp encrypted messages via API

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Description

[maximiliansteinert]

Steps to reproduce:

messenger.mailTabs.getSelectedMessages() or or messenger.messages.getRaw()
will only get the encrypted messages.

Actual results:

API only gets encrypted message although message has already been unencrypted for reading in Thunderbird.

Expected results:

Using the Web extension Api to deal with unencrypted messages

Comment 1

[info]

I very much would love to use this. It would open a lot of opportunities for add-on developers, leaving the tough crypto to Thunderbird while being able to integrate with external systems.

Comment 2

[John Bieling (:TbSync)]

messages.getRaw() will most likely always return the raw encrypted message, but I will work on messages.getFull() to return the decrypted parts. We are also working on messages.getBody() which will return relevant body parts of the message, without having to walk through the mime tree, and that will also include the decrypted parts.

I will most likely introduce a new permission to allow extension access to decrypted messages.

Comment 3

[John Bieling (:TbSync)]

Attachment: Bug 1854684 - Remove inadequate performance optimization and add some minor improvements for the MimeTreeEmitter and the MimeTreeDecrypter. r=kaie

These are changes which I found helpful while actually using these
classes for the decryption support of getRaw().

• adding a few named constants
• changing the dependency for the handleAttachment mode
  EXCLUDE_ALL_BUT_ENCRYPTED_ATTACHMENTS: enforce the encryption support
  instead of changing the mode to EXCLUDE_ALL_ATTACHMENT, if encryption
  support is not enabled
• use private class members
• no longer expose the used MimeTreeDecrypter instance
• set decryptFailure also in pgpDecryptAttachment()

Comment 4

[John Bieling (:TbSync)]

Attachment: Bug 1854684 - Add decryption support to getRaw(). r=#thunderbird-reviewers

Since we need to parse messages in order to decrypt them, this patch
introduces the MsgHdrProcessor class, to ensure parsing and encryption
operations are executed only once, re-using already computed results.

The messages API currenlty expects a MimeMessagePart instead of a
MimeTreePart for further processing. To minimize code changes, this
patch adds temporary glue code and re-parses the results extracted from
the MimeTreePart, to obtain a MimeMessagePart. This will be removed
in Bug 1878620.

This patch re-enabes the test_openpgp test in
mail\components\extensions\test\xpcshell\test_ext_messages_get.js

Depends on D202130

Comment 5

[Kai Engert (:KaiE:)]

While reviewing the patch, and re-reviewing the older change from bug 1876008, I got confused about the various decoding flags. Thanks to John for explaning me the intention of the patches.

I think it's necessary that I give you a broader background explanation to enable you to write the correct patch.

First, let's clearly distinguish between an encrypted message, and a message that has an encrypted attachment.

If a message is encrypted, all attachments are encrypted, too. In this scenario, the outermost MIME layer already provides the encryption for everything. (And if you check whether an attachment is encrypted, you will see it (usually) isn't. The sender MUA didn't have to encrypt the attachment.)

John told me, he wants a mode that can decrypt messages, and obtain the decrypted message, but ignore all other attachments for efficiency. He explained that it's necessary to always do some processing of attachments - because all encrypted messages are technically transported as attachments. For this reason, the current code will collect incoming attachment data (via deliverPartData), until the type of attachment can be derived.

I think you should use a different strategy.

I suggest that you decide based on the content type headers. If you know that you are processing an encrypted message, then you should set a flag that allows the next (nested) MIME part to be collected (via deliverPartData) and treated as an encrypted message. You clear the flag after you have processed that part. You can then ignore attachments that are processed without the flag being set.

This way you will ignore all attachments that are separately encrypted. This should be limited to those scenarios, when the sender has encrypted a file externally, and attaches the encrypted data as a file to the message. I think you don't want to automtically decrypt that.

If your intention is to obtain the message text that is shown by Thunderbird, then the above strategy should give you what you want.

Unfortunately, that strategy gives you MORE than you want, and you should improve the strategy further.

If you apply the above strategy for parsing all levels of the MIME tree, then you might also decrypt nested messages.

You need to be careful with that, because it can be abused by attackers.

Let me briefly describe the attack:

Let's say Alice has sent an encrypted message to Bob.
Eve cannot decrypt that message, but Eve has obtained a copy of the encrypted (unreadable for her) message.
Eve could send an encrypted email to Bob that uses a carefully crafted MIME layering. Along Eve's message, she includes Alice' message on a nested MIME layer.
If Bob receives the message, and Bob's MUA decrypts all nested layers recursively, then the decrypted text content of all messages may be concatenated and appear as a single message. Bob might reply to that message. Bob's MUA might quote all the text content, including the part that originates from Alice' message. Bob sends the reply to Eve. Eve can read the original message from Alice, which wasn't intended to go to Eve.

Because of this class of attack, Thunderbird is currently very careful to avoid automatic decryption of nested messages.

When displaying a message with nested encrypted contents into Thunderbird mail viewer, it will decrypt the toplevel decrypted message, only. We have added user interface to inform the user that there are nested encrypted messages, and we show a button that allows the user to open and view the decrypted messages. Each of those parts will be opened in a separate message window. This avoids that the user accidentally mixes contents from multiple messages and accidentally quotes and sends the messages in the wrong place.

As I understand it, your intention is to implement a flexible API for message decryption. We don't know what an Add-on might do with the decrypted message. It might also want to take the decrypted message and use it in a new message.

I think you must prevent this class of attack in your new API.

I think you should also limit decryption of messages to the topmost MIME level. If the result contains additional encrypted parts at nested MIME layers, the Add-on should be required to request decryption of those parts separately. Then at least it's clear to the Add-on that those parts should be treated separately.

There is another complication. The decision, whether an encrypted MIME part is the topmost level or not, isn't always easy. For example, a transport gateway might add a wrapper MIME layer. Or, we have seen some MTAs adding an automatic signature layer around the encryption (intentionally or unintentionally). Thunderbird's streaming MIME processing code currently contains the logic for some exceptions. I'm afraid, if you try to rebuild the same logic, you'll have a difficult time to keep the behavior of both modes of processing (display and add-on) in sync.

Comment 6

[Kai Engert (:KaiE:)]

Being more pragmatic, and referring directly to your patch, I suggest your MimeTreeEmitterOptions could contain two flags:

• bool includeAttachments
• bool decryptMessage

If the flag decryptMessage is given, you should check whether the topmost MIME part has headers that look like an encrypted message. That is, after all headers have been received, check whether that's encrypted or not. If it's encrypted, set a flag (e.g. allowToDecryptNextLayer), that will cause the attachment at the next nested MIME layer to be collected, and afterwards decrypted.

If you are processing a MIME part that is further down the structure, ignore flag decryptMessage and never decrypt.

If your flag includeAttachments is true, then always collect the attachments. Only decrypt the attachment, if the flag allowToDecryptNextLayer is true.

If your flag includeAttachments is false, then collect the attachment ONLY if allowToDecryptNextLayer is true.

Make sure you clear the flag allowToDecryptNextLayer immediately after you have processed the next (immediate) layer.

Comment 7

[John Bieling (:TbSync)]

Attachment: Bug 1854684 - Introduce the MsgHdrProcessor class. r=#thunderbird-reviewers

The MsgHdrProcessor class will be the new main entry point to get
raw or parsed data from a message. It does not depend on
MsgHdrToMimeMessage, but on the MimeTreeEmitter and the
MimeTreeDecryptor.

It re-implements access to the raw message, the parsed message, and
attachment retrieval.

The function MsgHdrProcessor.getOriginalMessage() is a clone of
getRawMessage(), to keep review of the follow-up patches simple (where
it gets removed).

Depends on D202130

Comment 8

[John Bieling (:TbSync)]

Attachment: Bug 1854684 - Add decryption support to getRaw(). r=#thunderbird-reviewers

r=#thunderbird-reviewers

This patch rips out usage of MsgHdrToMimeMessage in the WebExtension
messages API and uses the newly introduced MsgHdrProcessor class
instead. Every function which needs access to the raw or parsed message
data is modified to accept a MimeTreePart instead of a
MimeMessagePart.

This fixes a bunch of inconsistencies.

As the first happy consumer, messages.getRaw() gets decryption
support.

This patch re-enabes the test_openpgp test in
mail\components\extensions\test\xpcshell\test_ext_messages_get.js

Depends on D202595

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Bug 1854684 - Process encrypted MIME parts based on headers, avoid peeking into MIME parts. r=john.bieling

Comment 10

[Kai Engert (:KaiE:)]

I've implemented my suggestion to process the MIME parts based on the MIME headers. It's based on top of your phab D202130.

Your code from D202130 couldn't work, because isPgpMime() requires a full MIME tree to work. It wants to assert that the number of subparts matches the expectation (2). However, you are using a streaming approach, where you build the MIME tree as you go. At the time you get the signal that headers are ready, you don't know yet whether there will (really) be sub parts (or how many). That means you cannot yet decide using isPgpMime(). But you want to decide at this time, because you said your intention is that you want to avoid building full MIME trees in memory unnecessarily, for performance reasons.

To solve this scenario, I think it's acceptable to take a shortcut. You can use a modified version of isPgpMime(), which excludes the check for the number of subparts.

This way, you can decide based on headers, and in the worst case scenario, you collect additional MIME parts - but this shouldn't happen with well formed messages.

The code uses variable currentlyProcessingEncryptedPartNum to signal that we're currently processing an encrypted MIME part. We should disable the usual checking while this flag is set - to ensure no malformed message can confuse our processing logic. We reset the flag after we're done recursively processing all subparts.

Processing PGP/INLINE should be limited to the top level part, which this patch implements.

Attachments may be decrypted, only if you have asked for attachment data.

Comment 11

[Kai Engert (:KaiE:)]

After more reading of the involved code, I realize:

Because MimeTreeEmitter is a base class for both your new mode of operation, but also for the existing PersistentCrypto code (which uses MimeTreeEmitter() without any options), we need to double check that we aren't changing any default behavior.

For example, the early return in deliverPartData(), is that really avoided (and all data is processed) with no options set?

(I should have paid better attention to that aspect in my review in bug 1876008.)

I would prefer if there was a global flag used in MimeTreeEmitter, to switch between the classic "plain emit everything" mode, in which none of the new flags are checked, and your new "filtering mode" (in which you set various options).

Comment 12

[Kai Engert (:KaiE:)]

I will work on comment 11 tomorrow, and work on an improved revision D202755.

Comment 13

[John Bieling (:TbSync)]

For example, the early return in deliverPartData(), is that really avoided (and all data is processed) with no options set?

(I should have paid better attention to that aspect in my review in bug 1876008.)

That was the concept of that bug yes, if no options are set, none of the additional features are enabled.

Edit: I will double check.

Comment 14

[Phabricator Automation]

Comment on attachment 9382704 [details]
Bug 1854684 - Process encrypted MIME parts based on headers, avoid peeking into MIME parts. r=john.bieling

Revision D202755 was moved to bug 1882832. Setting attachment 9382704 [details] to obsolete.

Comment 15

[John Bieling (:TbSync)]

try: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=2ee7747b366a1136eaef1e0ce7256a7131a3e8b9

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/17a80b3415d8
Remove inadequate performance optimization and add some minor improvements for the MimeTreeEmitter and the MimeTreeDecrypter. r=kaie
https://hg.mozilla.org/comm-central/rev/d23fe900760e
Introduce the MsgHdrProcessor class. r=mkmelin
https://hg.mozilla.org/comm-central/rev/5ec41bbeb2ea
Add decryption support to getRaw(). r=mkmelin