1837247 - TB115 can no longer decrypt a forwarded encrypted OpenPGP message

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, defect, P2)

Description

[Kai Engert [:KaiE:]]

Use a profile that has the Alice and Bob example secret OpenPGP keys imported.

Open test file mail/test/browser/openpgp/data/eml/fwd-unsigned-encrypted.eml

With 102, it was possible to open the attached message, and its decrypted contents were shown.

This is broken in 115.

Comment 1

[Kai Engert [:KaiE:]]

I've tested nightly build 2023-01-12, which contains the commit from bug 1746579, which touched the handling of nested encrypted messages. It still works with that version, bug 1746579 isn't the cause of the regression.

Comment 2

[Kai Engert [:KaiE:]]

Nightly 2023-01-17 still worked.
Nightly 2023-01-19 was broken.

I blame the Ash branch merging from Jan 18

Comment 3

[Kai Engert [:KaiE:]]

Looking at the source of the attachment, I see the data already got decrypted, but the content-type header and boundaries are no longer appropriately set.

Apparently isn't a decryption issue, but rather a MIME level breakage.

Comment 4

[Magnus Melin [:mkmelin]]

Very likely from https://hg.mozilla.org/comm-central/rev/8689605fa4c8901006b16e50aaa5e4d9bf6ce700

Comment 5

[Kai Engert [:KaiE:]]

It seems the following happens:

We process a MIME part with content-type: multipart/encrypted; protocol="application/pgp-encrypted";

The decrypt the inner data, that works. We correctly return the result to the caller.

At this point, the caller should replace the whole part with the result. However, apparently the caller keeps that content-type wrapper. It simply replaces the inner data with the decrypted data.

In the next step, we apparently attempt to process that result part again. Because it still has the same outer content-type, we again run into the same PGP decryption handler - which fails, because there's no encrypted data (rather there's cleartext, which is unexpected for that content-type).

I'm still trying to find where that caller is, and where the mistake (keep original MIME type) is made.

Comment 6

[Kai Engert [:KaiE:]]

I'm still searching for the responsible code.

I see that the decrypt code works correctly: It only returns the inner data.

I see the code in mail/modules/AttachmentInfo.sys.mjs
loads the following URL:
mailbox:///home/user/.thunderbird/..profile../Mail/Local%20Folders/fwd-attach-encrypt?number=1&part=1.2&filename=attached-message.eml

(The contents of that mailbox is one message, the message we have in tree with name: fwd-unsigned-encrypted.eml)

The data returned from that URL contains the false extra content-type wrapper.

I need to find the code that handles that mailbox:// URL, takes the decryption result, and returns the raw bytes. That code must be responsible for adding the wrong outer MIME content-type.

Comment 7

[Kai Engert [:KaiE:]]

I already spent 12 hours searching for the place that adds/keeps that extra header, but I couldn't find it yet.

I'd greatly appreciate any hints, do we have someone who understands the data flow well enough to help?

Comment 8

[Magnus Melin [:mkmelin]]

Maybe not the answer to your question, but noting it down here as I noticed it recently:
Atm we load each url twice (yes we should rework this somehow). First one normal load and then OpenPGP notices we have a message to check, and loads the same url once again. Triggered from https://searchfox.org/comm-central/rev/fe988e6b57d63dde2a550676304716769d2bfacf/mail/extensions/openpgp/content/ui/enigmailMsgHdrViewOverlay.js#573

Comment 9

[Magnus Melin [:mkmelin]]

With the keys available, I'm not sure what's not working?
If I load the fwd-unsigned-encrypted.eml message I get a "This message includes encrypted parts" + "Decrypt and Show" button. If i click the button, the attached message from Bob opens and I can see the decrypted content.
If I just click the attachment, then it doesn't decrypt. Obviously would be better if it did show when I open it yes.

Comment 10

[Kai Engert [:KaiE:]]

P1 based on bug 1825869 comment 8

Comment 11

[Kai Engert [:KaiE:]]

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

If I load the fwd-unsigned-encrypted.eml message I get a "This message includes encrypted parts" + "Decrypt and Show" button. If i click the button, the attached message from Bob opens and I can see the decrypted content.

Yes, that was recently added, new functionality, which allows to decrypt any nested parts at any nesting level.

If I just click the attachment, then it doesn't decrypt. Obviously would be better if it did show when I open it yes.

Right, that's the regression.

Comment 12

[Wayne Mery (:wsmwk)]

Not a blocker for 115.0, per matrix chat

Comment 13

[betterbird.project+11]

Kai's observations from comment #5 and comment #6 are correct: The (inner) decrypted data is still embedded into a defective multipart/encrypted wrapper when written out to a file, as one can also see in the subPart.eml file in the temp directory:

Content-Type: multipart/encrypted;
 protocol="application/pgp-encrypted";
 boundary="------------pDRPNQMqISsWnmd0BK2t6P1L"

Overall the file contains a broken MIME tree since this boundary is actually never used.

The way opening attachments is implemented now, that is "save to file, open file", the entire bug has become an exact duplicate of bug 1689326 where the incorrect MIME structure was already noted in the bug description. Bug 1689326 comment #2 (quote): 'best to just save the "raw" attachment' may also be the way to go here, since then, the message would (hopefully) be decrypted when being opened from file.

For a full history, read below, although that knowledge won't produce a fix:

Before the Ash/Supernova merge, attachments were opened like this:
https://hg.mozilla.org/comm-central/file/43752e8d8e55fd237c5bc78f1f73f6dd167ce4de/mail/base/content/msgHdrView.js#l1514
via messenger.openAttachment() (also in version 102).
After the Ash merge on 19-01-2023, attachments were opened like this:
https://hg.mozilla.org/comm-central/file/5abcc1359eb7d4122f958a823e29b43de5f60b2e/mail/base/content/msgHdrView.js#l1514
via top.messenger.openAttachment(). Surprisingly that already didn't work for encrypted message attachments since the regression range from comment #2 is correct.

This was later changed to the currently implemented method "save to file, open file" in March 2023:
https://hg.mozilla.org/comm-central/rev/1b437e7aa80e83ff321d020dc651498f8db157e9#l6.60
As bug 1689326 from 2021 shows, storing half-decrypted originally encrypted attachments to a file never worked.

The openAttachment() methods were later removed later in March 2023:
https://hg.mozilla.org/comm-central/rev/907ab135e85912c5c642db8879a42ad03963d3f5#l4.54

Finally the attachment handling code moved to AttachmentInfo.sys.mjs in April 2023.

Comment 14

[betterbird.project+11]

https://github.com/Betterbird/thunderbird-patches/blob/main/115/bugs/1837247-1689326-save-encrypted-attachments.patch

Comment 16

[Fabian Dellwing]

This bug also happens if your mail server injects an "external mail" warning into the mail body. To do this, the original encrypted mail is put into an attachment.

This attachment can't currently be opened with TB 115.

This is a pretty big issue for us.

Comment 17

[Kai Engert [:KaiE:]]

When loading the attachment in the new window, I see we're going through the MIME decryption twice. First time, we decrypt, and second time, we try to decrypt the already decrypted data, which fails.

We must prevent the decryption on the first time, which I think is supposed to deliver only the raw message (keeping the encryption wrapper).

When loading mail URIs, the flags influence whether a message is processed (rendered for loading), or whether it's loaded in a raw format.

When opening the attached message, we arrive in nsStreamConverter::DetermineOutputFormat,
which sees the "part=" parameter, and concludes we must use nsMimeOutput::nsMimeMessageRaw.

In both scenarios, we run through the code in mimeDecrypt.jsm.

I cannot see any checks in any PGP code for the raw output format.

I think we need to prevent that we arrive in the mimeDecrypt code.
I think if we see the request for the raw output format, we must prevent routing to any PGP handlers completely.

Where could this be done?

Maybe it could be done in nsPgpMimeProxy.cpp - which instead of routing into a processing handler, could just return the raw data.

Maybe AttachmentInfo.sys.mjs, when calling NetUtil.asyncFetch, we should add a parameter to the URI, such as "&outputformat=raw", and add code to nsPgpMimeProxy.cpp to detect that parameter.

Comment 18

[Kai Engert [:KaiE:]]

Regarding the output of the header, including the output of Content-Type: multipart/encrypted, which we earlier (above) described as wrong:

I think that output is actually correct, because we're supposed to produce the full (raw) message.

That initial header section is written by the MIME code (I'll attach a stack that documents where).
By the time we arrive in MimePgpe_init, it was already written to the stream.

Comment 19

[Kai Engert [:KaiE:]]

Attachment: Stack for comment 18

Comment 20

[Fabian Dellwing]

Is there any progress on this? Still stuck on 102 ESR because of this issue.

Comment 21

[Kai Engert [:KaiE:]]

Fabian, did you try to use the button "decrypt and show"?
Does that allow you to view the decrypted message/attachment?

Comment 22

[Kai Engert [:KaiE:]]

Finally coming back to this bug.
I think my past analysis was correct, but I didn't find time yet to fully implement my suggestion from comment 17.
I found that the link from comment 14 contains a simplification that is working.
I'm taking inspiration from that patch (combining it with my suggestion to append a "&outputformat=raw" parameter), and will modify mimeDecrypt to be able to return the raw original data, when the caller has requested to do so (in the saveToFile function as I had suggested).

Comment 23

[Kai Engert [:KaiE:]]

Attachment: Bug 1837247 - Fix viewing of attached encrypted messages. r=mkmelin

Loosely inspired by a patch from Betterbird
https://github.com/Betterbird/thunderbird-patches/blob/main/115/bugs/1837247-1689326-save-encrypted-attachments.patch

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/946719acdd85
Fix viewing of attached encrypted messages. r=mkmelin

Comment 25

[Kai Engert [:KaiE:]]

Attachment: 1837247-esr115.patch

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

Comment 26

[Kai Engert [:KaiE:]]

Comment on attachment 9424301 [details]
Bug 1837247 - Fix viewing of attached encrypted messages. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): 1803255
User impact if declined: functionality broken, cannot open/view attached encrypted messages
Testing completed (on c-c, etc.): tests pass, could reenable an test that was disabled in the past
Risk to taking this patch (and alternatives if risky): there is a functional change on how attached encrypted messages are treated, so there is a small risk that some untested scenario might behave differently, but the risk it limited to messages with an encrypted attachment. The benefit of this change outweighs the risk

Comment 27

[Kai Engert [:KaiE:]]

Comment on attachment 9425135 [details] [diff] [review]
1837247-esr115.patch

[Approval Request Comment]
see comment 26

(try build worked)

Comment 28

[Corey Bryant]

Comment on attachment 9424301 [details]
Bug 1837247 - Fix viewing of attached encrypted messages. r=mkmelin

[Triage Comment]
Approved for beta

Comment 29

[Corey Bryant]

Kai, we weren't planning any more 115 releases. We could, of course, but we already stated in the release notes that 115.15.0esr is the final release.

Comment 30

[Daniel Darnell [:dandarnell]]

Thunderbird 131.0b5:
https://hg.mozilla.org/releases/comm-beta/rev/affe74f82732

Comment 31

[Corey Bryant]

Comment on attachment 9424301 [details]
Bug 1837247 - Fix viewing of attached encrypted messages. r=mkmelin

[Triage Comment]
Approved for esr128

Comment 32

[Daniel Darnell [:dandarnell]]

Thunderbird 128.3.0esr:
https://hg.mozilla.org/releases/comm-esr128/rev/f34779ec4171

Comment 33

[Corey Bryant]

Comment on attachment 9425135 [details] [diff] [review]
1837247-esr115.patch

[Triage Comment]
Approved for esr115

Comment 34

[Corey Bryant]

Thunderbird 115.16.3esr:
https://hg.mozilla.org/releases/comm-esr115/rev/b3b141377ec5

Comment 35

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Hello,

I tried to reproduce this issue using the affected build from 2023-06-07(20230607034834) on macOS 14.
Kai, if possible , could you help us with narrowed/simplified STR in order for us to encounter the faulty behaviour and properly verify the fix on 115.16.3esr?

Thank you in advance!