1665475 - crash [@ mime_LineBuffer ] caused by nsPgpMimeProxy vs MimeObj lifetime issue

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, defect)

Description

[Kai Engert (:KaiE:)]

This MIGHT be the same bug as bug 1659919 and bug 1660556, but we might have different scenarios that trigger it.

I have been given a example message with the following structure:

• outer S/MIME signature
• inner OpenPGP encryption and signature layer

We crash when our JS implementation of OpenPGP decryption calls nsPgpMimeProxy::OutputDecryptedData to send the decrypted output to our C++ MIME code.

This is a callback function, which has been initialized in MimePgpe_init and stores a pointer to MimeObj* / MimeEncrypted*.

However, at the time the callback is reached, the MimeEncrypted object has already been destroyed. We received a call to MimePgpe_free, which currently does nothing.

Consequently nsPgpMimeProxy::OutputDecryptedData will access a destroyed pointer.

I have a fix that will remove the reference, but it also causes the message body to remain empty. We show an outer S/MIME signature to be shown.

I think as part of this bug we should also implement the EFAIL measure that was implemented in the past - which forbids decryption of MIME parts that aren't the top level. (I haven't yet checked if we already forbid it.)

Comment 1

[Kai Engert (:KaiE:)]

Attachment: outer-smime-inner-openpgp.eml

sample message.

The user agent header says that Thunderbird 78.2.2 was used.

I'm guessing that a mail agent has automatically added an outer S/MIME signature layer.

Comment 2

[Kai Engert (:KaiE:)]

Attachment: 1665475-v1.patch

This patch avoids the crash.
As explained above, more work is probably necessary.

Comment 4

[Kai Engert (:KaiE:)]

Florian, the test message you sent contained an additional, outer S/MIME signature layer (in addition to inner OpenPGP encryption/signature).
I would like to understand which software has added that, do you know?

The S/MIME signature was done using a certificate in your name at your company.
Are you using any software on your computer that adds that?
Or, does your company automatically add such signatures to all outgoing messages?

Comment 5

[f.fainelli]

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

Florian, the test message you sent contained an additional, outer S/MIME signature layer (in addition to inner OpenPGP encryption/signature).
I would like to understand which software has added that, do you know?

Yes, we just recently switched to have our GSuite account add S/MIME certificates and this appears to be done "transparently" when the email is sent.

The S/MIME signature was done using a certificate in your name at your company.
Are you using any software on your computer that adds that?
Or, does your company automatically add such signatures to all outgoing messages?

Gmail does that AFAICT though I do not know all the details on how this is done.

Comment 7

[Kai Engert (:KaiE:)]

Multiple scenarios can trigger this crash.
I can also be reproduced using the test case from bug 1660556, which only has a combination of OpenPGP message parts.
The attached patch fixes the crash from bug 1660556, too.

Comment 9

[Kai Engert (:KaiE:)]

This regression was introduced by the localization conversion to fluent from bug 1638822.

The existing code relied MIME processing actions to execute synchronously.

The fluent conversion code used the JS ".then" method to obtain strings asynchronously.
This had the effect, that some processing is done after the underlying MIME code has already been destroyed.

The best fix is to restore synchronous execution.

Magnus, Khushil, you might want to re-review the fluent conversion for other places that could have introduced similar bugs.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Bug 1665475 - Restore synchronous execution of emptyAttachment() to prevent a crash. r=mkmelin

Comment 11

[Kai Engert (:KaiE:)]

Magnus, can you please review in phab?

Comment 12

[Magnus Melin [:mkmelin]]

Why not use the crash fix from attachment 9176143 [details] [diff] [review]?
Requiring the js code to be sync seems quite a hack. That code shouldn't really have to think about internal c++ object life cycles.

Comment 13

[Kai Engert (:KaiE:)]

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

Why not use the crash fix from attachment 9176143 [details] [diff] [review]?

Because it doesn't fix the situation correctly!

With the newer fix, we actually get the original, intended behavior - which is, the attached encrypted email is shown with an explanation text in the message body - which says "this is an encrypted message part, you must open it in a new window to view it".

Without the newer patch, with only the earlier band-aid, we just get an empty message.

We need these strings immediately, because we are processing MIME data in a streaming manner, and objects get cleaned up after streaming is complete.

Comment 14

[Kai Engert (:KaiE:)]

Magnus, that JS code is executed as part of data processing code. That code expects us to do the action immediately. The MIME streaming processing isn't an asychronous implementation. Therefore the requirement to implement the MIME processing in JS isn't a hack, it's necessary.

Comment 15

[Kai Engert (:KaiE:)]

Attachment: Bug 1665475 - Add safeguards to discover flaws related to OpenPGP JS MIME handlers. r=mkmelin

Comment 16

[Kai Engert (:KaiE:)]

Comment on attachment 9176373 [details]
Bug 1665475 - Restore synchronous execution of emptyAttachment() to prevent a crash. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): 1638822
User impact if declined: crashes when reading encrypted messages, use-after-free
Testing completed (on c-c, etc.): locally
Risk to taking this patch (and alternatives if risky): no risk

Comment 17

[Wayne Mery (:wsmwk)]

Comment on attachment 9176373 [details]
Bug 1665475 - Restore synchronous execution of emptyAttachment() to prevent a crash. r=mkmelin

[Triage Comment]
Approved for esr78

Comment 18

[Kai Engert (:KaiE:)]

Comment on attachment 9176545 [details]
Bug 1665475 - Add safeguards to discover flaws related to OpenPGP JS MIME handlers. r=mkmelin

This is an optional patch that can avoid additional (yet unknown) crashes. I recomend to take it on comm-esr78 soon, but we can let it bake on c-c for a few days, if desired.

Comment 19

[Wayne Mery (:wsmwk)]

Comment on attachment 9176545 [details]
Bug 1665475 - Add safeguards to discover flaws related to OpenPGP JS MIME handlers. r=mkmelin

[Triage Comment]
Approved for esr78

Comment 20

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/comm-central/rev/1e29b764ab5b18da530d69fb5ce050186f0c6dcc
https://hg.mozilla.org/comm-central/rev/caec5afd0bf13c746573329bfd7d1b3e0dc2fe62
https://hg.mozilla.org/comm-central/rev/98b112f740837bd12111a52e92251a584114e5f8

Comment 21

[Rob Lemley [:rjl]]

Thunderbird 78.3.0:
https://hg.mozilla.org/releases/comm-esr78/rev/e2bf8d0a5561cda6321aa3c372c9d22ad71c227e

That is rev 1e29b764 from comment 20. Waiting for the okay from :kaie to push the other two.

Comment 22

[Kai Engert (:KaiE:)]

the patch that landed on esr78 is sufficient to mark it fixed

Comment 23

[f.fainelli]

I can confirm this is fixed in Thunderbird 78.3.1, thanks a lot!

Comment 24

[Kai Engert (:KaiE:)]

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

Bug 1665475 - Add safeguards to discover flaws related to OpenPGP JS MIME handlers. r=mkmelin

it's time to finally land this additional patch

Comment 25

[Kai Engert (:KaiE:)]

Wayne, can I consider your approval to still be valid, despite the 6 week delay?

Comment 26

[Wayne Mery (:wsmwk)]

yes

Comment 27

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/releases/comm-esr78/rev/b64de2d0d84fba0597df42942767b08f678fad53
>= 78.4.1