1633251 - Processing of "Re: " not done correctly in OpenPGP code

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, defect)

Description

[Jorg K (CEST = GMT+2)]

The current OpenPGP implementation in Thunderbird needs to restore a subject from a decrypted message. For treating subjects with Re: in them, it uses this hack:
https://searchfox.org/comm-central/rev/d9cc983e281ba4bfbb34722bb1f080124caf6263/mail/extensions/openpgp/content/ui/enigmailMsgHdrViewOverlay.js#1261

which then makes another hack necessary:
https://searchfox.org/comm-central/rev/51eaff8b48e029097e38475155cb43390a32405b/mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js#3936

The correct processing would be to strip the Re: prefix from the subject and set the HasRe flag on the header as can be seen in attachment 9143399 [details] [diff] [review].

I can submit a patch once bug 1633205 is done.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1633251-re-process.patch

Sadly I couldn't test this at all. Sending an unencrypted message crashed, and sending encrypted stops with a failure :-( - That's on Windows.

Sending encrypted failed "Sending of the message failed":
console.debug: (new Error("failure in finishCryptoEncapsulation", "chrome://openpgp/content/modules/mimeEncrypt.jsm", 537))
JavaScript error: chrome://openpgp/content/modules/mimeEncrypt.jsm, line 537: Error: failure in finishCryptoEncapsulation

Sending unencrypted, at least I thought that, not sure why it goes through finishCryptoEncapsulation. Crash at the end:
console.debug: (new Error("failure in finishCryptoEncapsulation", "chrome://openpgp/content/modules/mimeEncrypt.jsm", 537))
JavaScript error: chrome://openpgp/content/modules/mimeEncrypt.jsm, line 537: Error: failure in finishCryptoEncapsulation
[1828, Main Thread] WARNING: NS_ENSURE_TRUE(root) failed: file c:/mozilla-source/comm-central/layout/base/nsDocumentViewer.cpp, line 2955
[1828, Main Thread] WARNING: NS_ENSURE_TRUE(root) failed: file c:/mozilla-source/comm-central/layout/base/nsDocumentViewer.cpp, line 2955
[1828, Main Thread] WARNING: IInputPane2::TryHide is failure: 'result', file c:/mozilla-source/comm-central/widget/windows/OSKInputPaneManager.cpp, line 69
[1828, Main Thread] ###!!! ASSERTION: not-null m_mime_delivery_state: 'm_mime_delivery_state != nullptr', file c:/mozilla-source/comm-central/comm/mailnews/compose/src/nsMsgAttachmentHandler.cpp, line 930

Comment 2

[Kai Engert (:KaiE:)]

(In reply to Jorg K (CEST = GMT+2) from comment #1)

Sadly I couldn't test this at all. Sending an unencrypted message crashed, and sending encrypted stops with a failure :-( - That's on Windows.

Sending encrypted failed "Sending of the message failed":
console.debug: (new Error("failure in finishCryptoEncapsulation", "chrome://openpgp/content/modules/mimeEncrypt.jsm", 537))
JavaScript error: chrome://openpgp/content/modules/mimeEncrypt.jsm, line 537: Error: failure in finishCryptoEncapsulation

This error is expected, if you don't have a valid and accepted key for at least one recipient.
We haven't yet worked on a better error message.

Are you sure you have all the keys, and you have used openpgp key manager to define each recipient key as accepted?

Sending unencrypted, at least I thought that, not sure why it goes through finishCryptoEncapsulation. Crash at the end:
...
[1828, Main Thread] ###!!! ASSERTION: not-null m_mime_delivery_state: 'm_mime_delivery_state != nullptr', file c:/mozilla-source/comm-central/comm/mailnews/compose/src/nsMsgAttachmentHandler.cpp, line 930

I see you crash because you hit an assertion.
You were using a debug build?

I haven't used a debug build in a while, so I I'll build one and test if I can reproduce.

Comment 3

[Jorg K (CEST = GMT+2)]

It was a "message to self": Created the key, encryption was enabled, then I sent to myself. Do I need to "accept" anything in this case?
Yes, of course a debug build. I believe all the assertions (NS_ASSERTION, MOZ_ASSERT, etc.) are there for developers to trip them.
Sorry to mention the issue here, it really belongs into another bug. I just wanted to create a message and reply with obfuscated subject to test the Re. processing.

Comment 4

[Kai Engert (:KaiE:)]

Thanks. I'm able to reproduce. The assertion seems to be harmless, because we're already tearing down the sending action (failed/abort) when we reach that code path.

I'm working on a fix that will avoid reaching that code path, by checking availability of recipient keys, prior to trying to send.

Comment 5

[Kai Engert (:KaiE:)]

A fix for the composer window has landed. You should be able to proceed testing your patch.

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 9145237 [details] [diff] [review]
1633251-re-process.patch

I've tested this now, it works as expected. Linting passes as well. I can even move messages between local and IMAP folders thanks to the fix in bug 1633205.

Comment 7

[Jorg K (CEST = GMT+2)]

Just for some background here: The subject is used for threading if an appropriate thread can't be found by other means, so it's important that the HasRe flag is set properly and that the subject doesn't contain the "Re:":
https://searchfox.org/comm-central/rev/2006bce43f7350f93c37e18a136e149f9a7ffb6c/mailnews/db/msgdb/src/nsMsgDatabase.cpp#4021

Having the HasRe flag set correctly would also allow to remove this code:
https://searchfox.org/comm-central/rev/51eaff8b48e029097e38475155cb43390a32405b/mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js#3933-3944
It would be good to get Patrick's opinion to that. I can add the removal to the patch, or we can leave it for now since it doesn't have negative effects.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9145237 [details] [diff] [review]
1633251-re-process.patch

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

If the code you referenced is not needed, certainly we should remove it.

::: mail/extensions/openpgp/content/ui/enigmailMsgHdrViewOverlay.js
@@ +1260,5 @@
>        gFolderDisplay.selectedMessage
>      ) {
> +      // Strip multiple localised Re: prefixes. This emulates NS_MsgStripRE().
> +      let newSubject = subject;
> +      let prefixes = Services.prefs.getStringPref("mailnews.localizedRe", "");

let prefixes = Services.prefs.getStringPref("mailnews.localizedRe", "Re");

then you can avoid some stuff below

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1633251-re-process.patch (v1b)

Thanks, Magnus, I made the simplification. As for removing
https://searchfox.org/comm-central/rev/51eaff8b48e029097e38475155cb43390a32405b/mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js#3933-3944

Current Enigmail has produced messages where "Re: this is the subject" is stored in the database (referring to the message database in Mork/MSF) without the HasRe flag. If you remove the code and then reply to such a message, you'll get "Re: Re: this is the subject" since a reply prepends a "Re:" if the message doesn't have the flag. I don't want to make the call to get bad results on pre-existing data.

Comment 10

[Patrick Brunschwig]

Comment on attachment 9149600 [details] [diff] [review]
1633251-re-process.patch (v1b)

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

::: mail/extensions/openpgp/content/ui/enigmailMsgHdrViewOverlay.js
@@ +1273,5 @@
>        }
> +      let hadRe = newSubject != subject;
> +
> +      // Update the header pane.
> +      this.updateHdrBox("subject", hadRe ? "Re: " + newSubject : newSubject);

You're adding a non-localized "Re: " here. I believe this should be localized (something like prefixes[0])?

Comment 11

[Jorg K (CEST = GMT+2)]

No. Thunderbird removes all localised prefixes and exclusively adds "Re:", see:
https://searchfox.org/comm-central/search?q=%22Re%3A+%22&path=mail&case=true&regexp=false

EDIT: IOW, It wouldn't make sense to show "Aw:" in the header pane and "Re:" in the thread pane.

There are two decisions to be taken here:

• Use case-insensitive matching or not. If not, we need to start off with "Re,RE,re,rE", see: https://searchfox.org/comm-central/rev/2006bce43f7350f93c37e18a136e149f9a7ffb6c/mailnews/base/util/nsMsgUtils.cpp#547. I think that case-insensitive matching is fine.
• Whether to remove the said code in enigmailMsgComposeOverlay.js#3933-3944.

The reviewers got removed again :-(

Comment 12

[Magnus Melin [:mkmelin]]

"RE:" is unfortunately pretty common.

Comment 13

[Patrick Brunschwig]

I'd suggest that we use case-insensitive RegExp in this case.

Assuming that the patch here works as expected, in my eyes, the code in enigmailMsgComposeOverlay.js#3933-3944 is not needed anymore and should be removed.

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to Patrick Brunschwig from comment #13)

Assuming that the patch here works as expected, in my eyes, the code in enigmailMsgComposeOverlay.js#3933-3944 is not needed anymore and should be removed.

Actually, I prefer not to touch the compose processing. fixMessageSubject() is called here
https://searchfox.org/comm-central/rev/41733433edd6a3a2d0369ce03eb4fab62bf38c3b/mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js#4019
after setOriginalSubject() and I have the impression that that code will add a second "Re:" prefix under certain circumstances:
https://searchfox.org/comm-central/rev/41733433edd6a3a2d0369ce03eb4fab62bf38c3b/mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js#630
https://searchfox.org/comm-central/rev/41733433edd6a3a2d0369ce03eb4fab62bf38c3b/mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js#654.
I'm not sure why this function has code to add a "Re:" prefix when "normal" Thunderbird reply processing does so already. IOW, the lines in question should be removed in the context of code review of the entire area.

EDIT: I believe there is some room for improvement. For example:
https://searchfox.org/comm-central/rev/41733433edd6a3a2d0369ce03eb4fab62bf38c3b/mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js#647-648
could be replaced by using msgHdr.mime2DecodedSubjectto start with.
https://searchfox.org/comm-central/rev/41733433edd6a3a2d0369ce03eb4fab62bf38c3b/mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js#650-652
doesn't appear to be correct. Why only check "Re: " here? As far as I can tell, those three lines can be removed since there is a check a few lines further down.

EDIT 2: It's also not clear why getOriginalMsgUri() distinguishes between gMsgCompose.compFields.draftId and gMsgCompose.originalMsgURI. IMHO, using the latter would suffice. But that's all material for another bug.

Comment 15

[Jorg K (CEST = GMT+2)]

Somehow the review request got lost here, or more precisely, the reviewers got removed from the request. Let me know if you're interested in fixing this issue, otherwise I'll mark it WONTFIX.

Comment 16

[Patrick Brunschwig]

Comment on attachment 9149600 [details] [diff] [review]
1633251-re-process.patch (v1b)

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

Apart from this, the patch is fine with me.

::: mail/extensions/openpgp/content/ui/enigmailMsgHdrViewOverlay.js
@@ +1266,5 @@
> +      if (!prefixes.includes("Re")) {
> +        prefixes.push("Re");
> +      }
> +      for (let p of prefixes) {
> +        let regEx = new RegExp(`^(${p}: )+`, "i");

This removes for example "Re: Re: Re:", but it would not remove "Re: AW: Re:".

I would propose a single regexp like this:
let replaceStr = "^(" + prefixes.join(":|")  + ": )"
let regEx = new RegExp(replaceStr, "i");

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: 1633251-re-process.patch (v1c)

Good idea. Like this then.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2bcb19f79562
Correct processing of Re: prefix, set HasRe flag if necessary. r=patrick

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 9168594 [details] [diff] [review]
1633251-re-process.patch (v1c)

[Approval Request Comment]
Regression caused by (bug #): This was never right in Enigmail.
User impact if declined: Incorrect view flags leading to potential threading issues.
Testing completed (on c-c, etc.): Yes, code has been in use in the pEp Add-on for months.
Risk to taking this patch (and alternatives if risky): Low.

Note: No rush uplifting this since it's a minor issue and has been wrong forever.

Comment 20

[Wayne Mery (:wsmwk)]

Comment on attachment 9168594 [details] [diff] [review]
1633251-re-process.patch (v1c)

[Triage Comment]
Approved for beta

Comment 21

[Rob Lemley [:rjl]]

Thunderbird 80.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/d97dc9d2f677

Comment 22

[Wayne Mery (:wsmwk)]

Comment on attachment 9168594 [details] [diff] [review]
1633251-re-process.patch (v1c)

[Triage Comment]
Approved for esr78

Comment 23

[Geoff Lankow (:darktrojan)]

Thunderbird 78.1.2:
https://hg.mozilla.org/releases/comm-esr78/rev/dca9f3d0c590