1571481 - (CVE-2019-11739) Covert Content Attack on S/MIME are (still) possible for multipart/alternative

Status: RESOLVED FIXED

(Thunderbird :: Security, defect)

Description

[Jens Müller]

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

Steps to reproduce:

As a long-term fix for bug 1419417 (EFAIL direct exfiltration), partially encrypted S/MIME emails are no longer being decrypted (which is good). However, a bypass exists if the attacker uses a multipart/alternative message structure.

---

Proof of concept: Leaking plaintext through reply/forward

/Attacker model/: The attacker is in possession of an S/MIME encrypted message, which she may have obtained as passive man-in-the-middle or by actively hacking into the victim's mail server or gateway, etc.

/Attacker's goal/: Obtain the plaintext

/Prerequisites:/

• victim reads and composes messages in HTML (default setting in decent TB versions)
• victim replies or forwards the message (which is what email is used for, right?)

/Attack:/

The attacker modifies the outer MIME structure of the email: She wraps the captured ciphertext into her own specially crafted MIME email of type multipart/alternative as shown below.

multipart/alternative
|--- Content-Type: text/html
| Please answer to this email
| <style>.moz-text-plain, .moz-quote-pre, fieldset {display: none;}</style>
+--- Content-Type: application/pkcs7-mime

This modified message is now re-send to the victim (e.g., the orginal recipient or anyone else who can act as a decryption oracle). Because of the CSS style, the existence of the ciphertext part is hidden to the user (while it still is transparently decrypted by Thunderbird). The plaintext is quoted within the reply message (while still hidden to the user).

Actual results:

The Plaintext of S/MIME encrypted messages can be leaked by a single reply to a benign looking email.

Expected results:

Multipart messages should not be decrypted. Instead, decryption should only be performed if the whole message (= single MIME root element) is encrypted. This countermeasure to bug 1419417 has already been applied for multipart/mixed, however it is not correctly enforced for multipart/alternative.

Comment 1

[Jens Müller]

Can you add [e-mail] to this bug? thanks!

Comment 2

[Jens Müller]

Attachment: 07-dec-smime-alternative.eml

Proof-of-concept email file

Comment 3

[Jens Müller]

Attachment: screenshots.png

Screenshots of the attack

Comment 4

[Jens Müller]

Note that in the above example, Eve would of course change the Sender address from Alice to her very own address.
Also note that Eve must also encrypt her malicious part (Content-Type: text/html) to be able to completely hide the second (ciphertext) part.

Comment 5

[Daniel Veditz [:dveditz]]

(In reply to Jens Müller from comment #1)

Can you add [...]

They will have to create an account on this site first.

Comment 6

[Jens Müller]

sorry, wrong email address. [e-mail] should work / already have an account. thanks.

Comment 7

[Kai Engert [:KaiE:]]

I confirm the bug.

I can reproduce using both Thunderbird 60.8 and also using a Thunderbird 68.0 candidate build.

Comment 8

[Magnus Melin [:mkmelin]]

Attachment: fix part 1 from Magnus (requires additional changes, see attachment part 2)

Possibly this would fix it?
I'm not set up to test this now. Kai, if you have a proper test case, can you check it?

Comment 9

[Kai Engert [:KaiE:]]

Attachment: TestCA.pem

Comment 10

[Kai Engert [:KaiE:]]

Attachment: Alice.p12

Comment 11

[Kai Engert [:KaiE:]]

Attachment: test-mbox-1571481

Comment 12

[Kai Engert [:KaiE:]]

How to reproduce yourself:

• Create a test profile
• copy the full path to your test profile, PROFILE-PATH
• Download file test-mbox-1571481 and copy it to directory
  PROFILE-PATH/Mail/Local\ Folders/
• Download the attached files TestCA.pem and Alice.p12, save to /tmp
  (or take them from comm-esr68/mozilla/comm/mailnews/test/data/smime )
• Run the following NSS utilities as installed on Linux, or run them
  from inside a local thunderbird build, they are in dist/bin
• certutil -d sql:$PROFILE-PATH -A -n ca -t C,C, -a -i /tmp/TestCA.pem
• pk12util -d sql:$PROFILE-PATH -i /tmp/Alice.p12
  (press enter when prompted for password)

With the above configuration, using a default build, if click the
message inside the test-mbox-1571481 folder, you'll see the
decrypted contensts "Please reply to this harmless looking message".

Reply to the message, change the "To:" field and send to yourself.
Open the message you just sent, and look at the source.

If it contains "SECRET-TEXT from second part injected by the attacker"
then you have reproduced the bug.

Comment 13

[Kai Engert [:KaiE:]]

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

Possibly this would fix it?

It crashes when opening the multipart message.

Crash is at MimeMultipartAlternative_display_part_p in mimemalt.cpp

  // We must pass 'true' as last parameter so that text/calendar is
  // only displayable when Lightning is installed.
  MimeObjectClass *clazz = mime_find_class(ct, sub_hdrs, self->options, true);
  if (clazz && clazz->displayable_inline_p(clazz, sub_hdrs)) {

(gdb) print clazz
$3 = {class_name = 0x7fffe6c8d79a "MimeExternalObject", instance_size = 104, superclass = 0x7ffff4153138 <mimeLeafClass>, class_initialize = 0x7fffea9a7330 <MimeExternalObjectClassInitialize(MimeExternalObjectClass)>, class_initialized = false,
initialize = 0x0, finalize = 0x0, parse_begin = 0x0, parse_buffer = 0x0, parse_line = 0x0, parse_eof = 0x0, parse_end = 0x0, displayable_inline_p = 0x0, debug_print = 0x0}

Note that class_initialized = false and the function pointer displayable_inline_p is null.,

Comment 14

[Kai Engert [:KaiE:]]

Attachment: nssdb-1571481.zip

As an easier alternative to downloading testca.pem and alice.p12 and running certutil/pk12util to import, you can download this file nssdb-1571481.zip , extract the two files and move them to PROFILE-PATH (replacing the default files in your test profile).

Comment 15

[Kai Engert [:KaiE:]]

In https://hg.mozilla.org/comm-central/rev/b474dfcfa256b5f5800083d592a8307b430d1303 an early return from mime_find_class was added. In this scenario, it skips the init of the mimeExternalObjectClass which would be done at the end of the function. I'll try to undo the early return, and allow the function to walk to the end.

Comment 16

[Kai Engert [:KaiE:]]

Yes, that works. I'll attach an updated patch.

The patch has the effect that the entire message body is shown as empty. No parts are made available as attachments. Is that a good behavior?

Magnus, was there a particular reason that you gave the child property to MimeSunAttachment? What about other variations of multipart?

Comment 17

[Kai Engert [:KaiE:]]

Comment on attachment 9087288 [details] [diff] [review]
fix part 1 from Magnus (requires additional changes, see attachment part 2)

This works, if we also add patch part 2. The fact that we display a completely empty message might be a little confusing, though. But that could be improved at a later time.

I've checked the various other multipart types, and they're all treated like multipart/mixed, and seem fine (both parts treated as attachments).

Comment 18

[Kai Engert [:KaiE:]]

Attachment: 1571481-part2.patch

Part 1 and Part 2 together fix the issue.

Note this change looks big, but it's simply adding an indent level.

If you ignore whitespace, the change is just this:

@@ -673,9 +673,7 @@ MimeObjectClass *mime_find_class(const c
         // cleverly hidden and the decrypted content gets included in
         // replies and forwards.
         clazz = (MimeObjectClass *)&mimeExternalObjectClass;
-        return clazz;
-      }
-
+      } else {
       char *ct = hdrs ? MimeHeaders_get(hdrs, HEADER_CONTENT_TYPE, false, false)
                       : nullptr;
       char *st =
@@ -711,6 +709,7 @@ MimeObjectClass *mime_find_class(const c
       PR_Free(st);
       PR_Free(ct);
     }
+    }
 #endif
     /* A few types which occur in the real world and which we would otherwise
     treat as non-text types (which would be bad) without this special-case...

Comment 19

[Kai Engert [:KaiE:]]

I think we should have automated tests for these issues. Not just this new issue, but also for all the old EFAIL scenarios. I'll file a separate bug for that.

Comment 20

[Magnus Melin [:mkmelin]]

(In reply to Kai Engert (:kaie:) from comment #12)
Even after doing this (apparently successfully) attachment 9087344 [details] still shows just blank on trunk (no patches applied). Am I missing something?

Re MimeSunAttachment: added setting the child setting there, since it was the only other multipart implementation we have that was not already covered. If shouldn't really hurt... OT, but MimeSunAttachment might be ripe to remove entirely. Wonder if that's really supported elsewhere.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 9087722 [details] [diff] [review]
1571481-part2.patch

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

This looks correct either way though.

Comment 22

[Kai Engert [:KaiE:]]

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

Even after doing this (apparently successfully) attachment 9087344 [details] still shows just blank on trunk (no patches applied). Am I missing something?

I think you made a mistake in following the instructions from comment 12.

If the message is shown blank, then you haven't imported the private key to decrypt the attachments.

You can check certificate manager, "your certificates" tab. If it's empty, you haven't imported the private key.

Comment 23

[Magnus Melin [:mkmelin]]

Imported it manually now, and things work as they should. We should get these landed.

Comment 24

[Kai Engert [:KaiE:]]

Jens, did you already publicly disclose this security issue?

Is there any coordination required from your point of view, or are we allowed to publish any time that works for us?

Comment 25

[Jens Müller]

Kai, we publicly disclosed the original "Leaking plaintext through reply/forward" attacks from Bug 1419417 in an academic paper (https://arxiv.org/abs/1904.07550). Note that it was not a Thunderbird-only issue, a lot of mail clients were affected. We did not disclose that TB is still vulnerable to the multipart/alternative variant this bug. We have no intent for a quick disclosure from our side, so publish any time that works for you.

That said, however, we made all testcases public when giving a DEF CON talk a couple of weeks ago: https://github.com/RUB-NDS/Covert-Content-Attacks. These are all EFAIL direct exfiltration / reply exfiltration scenarios we know of (which can also be used for automated tests). The testbed includes a -- more generic -- 01-reply-mix-alt.eml test. If someone tries it on TB, they will realize TB still vulnerable to this attack variant (and that they only need some CSS tricks to actually hide the existence of the ciphertext part, which is minimal engineering effort).

Comment 26

[Kai Engert [:KaiE:]]

Thanks Jens.

Comment 27

[Kai Engert [:KaiE:]]

Attachment: Combined patch - commit to TB 70/69/68

[Approval Request Comment]
Regression caused by (bug #): none
User impact if declined: covert attack on encrypted email
Testing completed (on c-c, etc.): manually tested using attached data
Risk to taking this patch (and alternatives if risky): doesn't break existing S/MIME tests

Comment 28

[Kai Engert [:KaiE:]]

Attachment: Combined patch - commit to TB 60

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky):

Comment 29

[Kai Engert [:KaiE:]]

Comment 28 should have been: Same statements as comment before.
We don't have automatic S/MIME testing on the TB 60 branch. But I performed the same manual testing with a local TB 60 build.
The patch for the 60.x branch was merged for context changes.

Comment 30

[Kai Engert [:KaiE:]]

Because Jens stated that no embargo is necessary, we can go ahead and land immediately.
Nevertheless, I used a disguising comment, that doesn't immediately point out the problem.
We'll add a test case later, probably with bug 1576167.

Comment 31

[Kai Engert [:KaiE:]]

Al, can you please assign a CVE ID?

Comment 32

[Jorg K (CEST = GMT+2)]

The problem is that we did the last TB 69 beta yesterday. So this will go out in TB 70 beta, and therefore it appears a little risky to put it into TB 60.9 and TB 68.1, right? We'd have to run a 60.9.1 and 68.1.1 for it.

Comment 33

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/2694f1118d8fb502ad32afc96ad85b45ba1aae7e
Improve multipart/alternative. r=kaie DONTBUILD

Comment 34

[Kai Engert [:KaiE:]]

Jens, who should be attributed as the reporter(s) of this bug? Should we set it to the list of authors of your paper, which is:
"Jens Müller, Marcus Brinkmann, Damian Poddebniak, Sebastian Schinzel, Jörg Schwenk" ?

Comment 35

[Kai Engert [:KaiE:]]

(In reply to Jorg K (GMT+2) from comment #32)

The problem is that we did the last TB 69 beta yesterday. So this will go out in TB 70 beta, and therefore it appears a little risky to put it into TB 60.9 and TB 68.1, right? We'd have to run a 60.9.1 and 68.1.1 for it.

Doing 60.9.1 and 68.1.1 seems like a lot of extra work.
The issue is sec-high, not sec-critical. How soon will TB 70 beta go out? Maybe it's acceptable to delay the fix for beta?

Comment 36

[Jorg K (CEST = GMT+2)]

TB 70 beta is scheduled for next week. We could run another TB 69 beta 5 just to include this fix.

Maybe it's acceptable to delay the fix for beta?

I was saying that we don't usually uplift patches to ESR unless they spent some time on beta. Given that TB 70 beta, 68.1 and 60.9 are all next week, it can't go into the latter if it needs to spend time in the former.

This isn't exactly a small patch and messing with MIME should get some exposure before hitting the ESR versions, don't you agree? Let's see whether it passes the test suite. I don't see a try run here.

Comment 37

[Kai Engert [:KaiE:]]

Jörg, the patch is small, the change to mimei.cpp is simply indent change, caused by another "else" instead of an early return. See comment 18 for the details.

Comment 38

[Jorg K (CEST = GMT+2)]

I see, thanks. Sometime it pays to attach a -w patch. Anyway, you tell me where you want it. We have 70 beta, 60.9 and 68.1 all shipping next week.

Comment 39

[Kai Engert [:KaiE:]]

Jörg, when I discussed this issue yesterday with Magnus, his opinion was to include it in 60.9 for next week, and he considers the change low risk. Based on that, I suggest to ship it everywhere next week.

Comment 40

[Jorg K (CEST = GMT+2)]

Comment on attachment 9088660 [details] [diff] [review]
Combined patch - commit to TB 70/69/68

Fine by me.

Comment 41

[Jens Müller]

Kai, yeah "Jens Müller, Marcus Brinkmann, Damian Poddebniak, Sebastian Schinzel, Jörg Schwenk" is great if you want to attribute the issue. Thanks.

Comment 42

[Jorg K (CEST = GMT+2)]

TB 68.1 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/555887eee94ced63beb52b6b83b9d3781475277e

Comment 43

[Al Billings [:abillings - ex-MoCo]]

CVE assigned.

Comment 44

[Jorg K (CEST = GMT+2)]

TB 60.9 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/05de7d275364688e59231184c342998a39ed5fb9

Comment 45

[Kai Engert [:KaiE:]]

Jörg, do you understand why this fix isn't present on comm-beta?

Has something gone wrong with merges?

Comment 46

[Jorg K (CEST = GMT+2)]

Nothing ever goes wrong with merges, we'd be totally hosed.

https://hg.mozilla.org/comm-central/rev/2694f1118d8fb502ad32afc96ad85b45ba1aae7e
https://hg.mozilla.org/releases/comm-beta/rev/2694f1118d8fb502ad32afc96ad85b45ba1aae7e

Comment 47

[Kai Engert [:KaiE:]]

Sorry, my local comm-beta tree is stuck on another "hg head", no idea what I did to get there. Thanks, I'll just need to clean up locally.

Comment 48

[Jorg K (CEST = GMT+2)]

Of course. After every merge you need to: hg pull and hg up -C default.