1464056 - (CVE-2018-12373) [efail] S/MIME plaintext can be leaked through HTML reply/forward

Status: RESOLVED FIXED

(Thunderbird :: Security, defect)

Description

[Jens Müller]

Attachment: smime-reply-exfiltration.eml

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20180326230345

Steps to reproduce:

A variant of Efail's direct exfiltration attack still work in TB nightly (2018-05-22). S/MIME: decrypted MIME parts can be completely hidden with CSS; if user replies/forwards in HTML format the (non-visible) plaintext is leaked, otherwise (in text-only reply mode) the victim could be fooled by inserting newlines
---------------------------------------------------
Content-Type: multipart/mixed; boundary="BOUNDARY"

--BOUNDARY
Content-Type: text/html

Reply please to this email
<div style="color: red;">

--BOUNDARY
Content-Type: application/pkcs7-mime; name="smime.p7m"; smime-type=enveloped-data
Content-Transfer-Encoding: base64
...
--BOUNDARY--
---------------------------------------------------


Actual results:

Quoted S/MIME plaintext is hidden and leaked when user replies/forwards the message


Expected results:

Plaintext should in other MIME parts than the very first one should not be quoted

Comment 1

[Jens Müller]

Attachment: smime-reply-exfiltration.png

Comment 2

[Daniel Veditz [:dveditz]]

Wayne: please CC additional Thunderbird folks as necessary

Comment 3

[Magnus Melin [:mkmelin]]

I don't see that - I get no "THIS IS A S/MIME ENCRYPTED MESSAGE....". Are you sure you're using 62 nightly trunk? Or which nightly did you mean? From here http://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-central/

As I've written elsewhere for replies/forwards efail is only a bit mitigated now. But the sample eml you provided here - that case is fixed by parsing parts separately.

Comment 4

[Jens Müller]

Reproduced with http://ftp.mozilla.org/pub/thunderbird/nightly/latest-comm-central/thunderbird-62.0a1.en-US.linux-x86_64.tar.bz2 (of course the S/MIME base64 blob needs to be replaced by a S/MIME message you have the secret key for)

Comment 5

[Ben Bucksch (:BenB)]

From bug 1419417 comment 57:

3. Only decrypt, if the entire message is encrypted, not attachments

Normally, if we receive an encrypted email, the entire email will be encrypted. There's little point in leaving part of the message encrypted.

Part of the bug here was that we are automatically decrypting and inlining MIME subparts. That led to that "mixed content". Patrick had no way to stop that "mixed content" in Enigmail, because the code is in libmime.

We should only automatically decrypt a message, if the entire message is encrypted. That's true for all normal encrypted emails.
If an attachment is encrypted, we should simply show it as external attachment. The user can click on it to open or save it.
(In the first version of the patch, I might only allow saving the attachment, not opening it directly by double-click - depending on how difficult it is to implement.)

-----

I think that would fix the bug here.

We can either fix that in libmime, or in Enigmail.

Enigmail would have to use Part numbers, which is a little hacky, but advantage is that it's easier to implement than in libmime.

Comment 6

[Patrick Brunschwig]

(In reply to Ben Bucksch (:BenB) from comment #5)
> From bug 1419417 comment 57:
> 
> 3. Only decrypt, if the entire message is encrypted, not attachments
> 
> Normally, if we receive an encrypted email, the entire email will be
> encrypted. There's little point in leaving part of the message encrypted.
> 
> Part of the bug here was that we are automatically decrypting and inlining
> MIME subparts. That led to that "mixed content". Patrick had no way to stop
> that "mixed content" in Enigmail, because the code is in libmime.

I actually did fix it in Enigmail 2.0.5 a way that works for all use-cases, except for forwarding. The solution works like this: I receive the message URL (including the &part=1.2.3) from libmime, and I also receive the MIME part number that I'm decrypting. From this I can derive if the message is encrypted as a whole (MIME part number == "1") or if the message is an encrypted attachment (URL contains &part=1.2.3, and MIME part number matches /^1.2.3.1/)

Comment 7

[Magnus Melin [:mkmelin]]

Why is the red color from the unclosed <div style="color: red;"> from part1 leaking into the rest of the message? Is the bug 1419417 working as expected? I would expect parsing and outputting to close that div.

Comment 8

[Jorg K (CEST = GMT+2)]

As per bug 1462488 comment #20: I don't think |<div style="color: red;">| makes anything hidden. A possible attack is |<style>div { display: none; } pre { display: none; }</style>|.

BTW, why is this bug access restricted, if bug 1462488 is not?

Comment 9

[Jens Müller]

Of course the red color is not meant to hide anything but just to show the problem: Basically, attacker-controlled CSS is leaked into other MIME parts, it defines how sensitive information such as decrypted mails are displayed. This is bad and there are lots of variants of how to actually exploit it.

Comment 10

[Magnus Melin [:mkmelin]]

Yes we need to see why the red color still shows. It shouldn't.

Comment 11

[Jorg K (CEST = GMT+2)]

Please read bug 1462488 comment #20. There is not red colour. If instead you use |<style>body { color: red; }</style>| then the whole thing is read since all parts share the same HTML document. That's expected.

Comment 12

[Ben Bucksch (:BenB)]

> I actually did fix it in Enigmail 2.0.5
> I can derive if the message is encrypted as a whole (MIME part number == "1")

@Patrick: nice!
Could you please verify that this fixes the bug here? Then we can set this FIXED and unhide the bug.

Comment 13

[Patrick Brunschwig]

The fix does work for most but not all cases in Enigmail 2.0.5 because I wasn't restrictive enough. The rules I described are implemented on master and will be part of the next release. And yes, this fixes the bug in Enigmail. But I can't fix this for S/MIME.

Comment 14

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) (bustage-fix and urgent reviews only) from comment #11)
> Please read bug 1462488 comment #20. There is not red colour. 

But there is! See https://bugzilla.mozilla.org/attachment.cgi?id=8980273 and I see this too on trunk

So there is still a problem here

This is the html we're producing for the above test mail on trunk: 
---
<body>

<meta http-equiv="content-type" content="text/html; ">Reply please to this email
<div style="color: red;">
<plaintext>
</plaintext></div></body></html><BR><FIELDSET CLASS="mimeAttachmentHeader"></FIELDSET><div class="moz-text-plain" wrap=true graphical-quote=true style="font-family: -moz-fixed; font-size: 14px;" lang="x-western"><pre wrap class="moz-quote-pre">
</pre></div></body>
</html>
</plaintext></div></body>
---

Appears the plaintext is closed, but then due to the second </plaintext>, the "could be decrypted part" is interpreted as text.

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #14)
> See https://bugzilla.mozilla.org/attachment.cgi?id=8980273
That's a picture. Kindly supply a message.

Comment 16

[Jorg K (CEST = GMT+2)]

Oh, you mean attachment 8980272 [details] (no need to past the entire link).

Comment 17

[Magnus Melin [:mkmelin]]

Yes, the mail is attachment 8980272 [details], as I assume you saw now.

Comment 18

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #14)
> But there is!
OK, I see it now. Comment #0 DID NOT have the <plaintext> tag that causes all the problems.

But that's not a sign that the fix in bug 1419417 didn't work. The HTML is parsed correctly. I ran it in the debugger and the HTML that comes from parsing is this:
<html><head>
<meta http-equiv="content-type" content="text/html; charset="></head><body>
 Reply please to this email
<div style="color: red;">
<plaintext>
</plaintext>
</div>
</body></html>

So the code from bug 1419417 DOES work, it closes the <div> and the <plaintext> tags SUCCESSFULLY!!

In the Inspector I see:

<html><head>
<title>Testcase 'reply-mix-css' (S/MIME)</title>
<link rel="important stylesheet" href="chrome://messagebody/skin/messageBody.css">
</head>
<body>
<div class="moz-text-html" lang="x-western">
<meta http-equiv="content-type" content="text/html; ">
  Reply please to this email
  <div style="color: red;">
    <plaintext>
    </plaintext>
  </div>
</div> <-- inserted by my code from bug 1462895.
</body></html>

<BR><FIELDSET CLASS="mimeAttachmentHeader"></FIELDSET>
<div class="moz-text-plain" wrap=true graphical-quote=true style="font-family: -moz-fixed; font-size: 14px;" lang="x-unicode">
  <pre wrap class="moz-quote-pre">(Decrypted text)</pre>
</div>
</body>
</html>
</plaintext>    <- pseudo tag inserted by Dev tools.
</div>          <- pseudo tag inserted by Dev tools.
</div>          <- pseudo tag inserted by Dev tools.
</body></html>  <- pseudo tag inserted by Dev tools.

The problem comes from the <plaintext> tag which the rendering engine interprets according to:
https://html.com/tags/plaintext/
===
The <plaintext> element was used to render HTML code as plain text. Since everything after the opening tag was rendered as plain text, there was no closing <plaintext> tag. This element is obsolete and should not be used.
===

If you remove the plaintext tag from the message, everything becomes good.

So it's interpreted as:
<html><head>
<title>Testcase 'reply-mix-css' (S/MIME)</title>
<link rel="important stylesheet" href="chrome://messagebody/skin/messageBody.css">
</head>
<body>
<div class="moz-text-html" lang="x-western">
<meta http-equiv="content-type" content="text/html; ">
  Reply please to this email
  <div style="color: red;">
    <plaintext>

|    </plaintext>
|  </div>
|</div> <-- inserted by my code from bug 1462895.
|</body></html>
|<BR><FIELDSET CLASS="mimeAttachmentHeader"></FIELDSET>
|<div class="moz-text-plain" wrap=true graphical-quote=true style="font-family: -moz-fixed; font-size: 14px;" lang="x-unicode">
|  <pre wrap class="moz-quote-pre">(Decrypted text)</pre>
|</div>
|</body>
|</html>

</plaintext>    <- pseudo tag inserted by Dev tools.
</div>          <- pseudo tag inserted by Dev tools.
</div>          <- pseudo tag inserted by Dev tools.
</body></html>  <- pseudo tag inserted by Dev tools.

All the stuff marked | is displayed verbatim as "plain text". So despite closing the <plaintext> tag correctly, by definition it cannot be closed.

This goes into the same category of problems which can be created with CSS leaking into other message parts. CSS leaks by definition, and <plaintext> has a similar effect.

Comment 19

[Jorg K (CEST = GMT+2)]

Note: Stripping the <plaintext> tag would be easy, but that still doesn't fix the CSS attack vector.

Comment 20

[Magnus Melin [:mkmelin]]

Every day you learn something new. Nice analysis!

I filed bug 1464667 to really remove <plaintext> support. This can't be the only case where someone can use that to do bad things.

Comment 21

[Magnus Melin [:mkmelin]]

Attachment: bug1464056_efail_decrypt2.patch

This patch disallows encrypted sub parts. Only top level encryption will be allowed. If an encrypted subpart is found it is shown as an attachment.

I added a pref mail.decrypt_children to disable this, in case there is a need for it, which I don't think we know before we deploy it at large scale.

I've tested that 
 - normal mails work
 - normal encrypted mails work (as produced by Thunderbird)
 - messages such as attachment 8980272 [details] will be prevented from showing encrypted content inline

I've not tested that an encrypted message containing an encrypted sub part will work. But I have no reason to think it wouldn't. I'm not sure how to produce such a message though.

Patrick, what do you think? And is there a similar check we should add for PGP?

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8985832 [details] [diff] [review]
bug1464056_efail_decrypt2.patch

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

I haven't tried, but it looks reasonable by visual inspection. I'll test when review is requested.

::: mailnews/mime/src/mimei.cpp
@@ +718,5 @@
> +        if (!Preferences::GetBool("mail.decrypt_children", false) &&
> +            opts->is_child) {
> +          // We do not allow encrypted parts except as top level.
> +          // Allowing them would leak the plan text in case the part is
> +          // clevely hidden and the decrypted content gets included in

Two typos: plan text and clevely.

::: mailnews/mime/src/modlmime.h
@@ +176,5 @@
>                   xlated for them doesn't work.  We have to
>                   dexlate it before sending it.
>                 */
>  
> +  bool is_child = false; /* Whether this part is a child and not top level. */

Grammar?

Comment 23

[Patrick Brunschwig]

Comment on attachment 8985832 [details] [diff] [review]
bug1464056_efail_decrypt2.patch

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

The idea is very good, and -- given my limited knowledge of libmime -- I think the implementation is reasonable. I don't think we need it in the PgpMimeProxy, I already do something very similar within the Enigmail code.

What will happen if the user wants to open an "S/MIME attachment"?

Comment 24

[Magnus Melin [:mkmelin]]

(In reply to Patrick Brunschwig from comment #23)
> What will happen if the user wants to open an "S/MIME attachment"?

It's opened like an attachment and you get to select how to open it. I'm not sure there's any way to actually open it for viewing decrypted. (The file seems binary.)

Comment 25

[Magnus Melin [:mkmelin]]

Attachment: bug1464056_efail_decrypt2.patch

Fixed language/typos.

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: bug1464056_efail_decrypt2.patch - rebased

Rebased after https://hg.mozilla.org/comm-central/rev/01215911a27e turned mimei.cpp upside down.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: bug1464056_efail_decrypt2.patch - rebased (v2)

I've done some whitespace adjustments here.

Comment 28

[Jorg K (CEST = GMT+2)]

Comment on attachment 8985852 [details] [diff] [review]
bug1464056_efail_decrypt2.patch - rebased (v2)

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

This works for me. I've tested with S/MIME encrypted messages and encrypted subordinate parts.

::: mailnews/mime/src/mimemult.cpp
@@ +434,5 @@
>     auto-uudecode-hack won't ever be done for subparts of a
>     multipart, but only for untyped children of message/rfc822.
>     */
> +  if (!obj->options)  // Just to make sure.
> +    obj->options = new MimeDisplayOptions();

Are you sure this line is correct? I didn't find any other call site where the options are set up that way.

@@ +436,5 @@
>     */
> +  if (!obj->options)  // Just to make sure.
> +    obj->options = new MimeDisplayOptions();
> +
> +  obj->options->is_child = true;

Have you considered:
if (obj->options)
  obj->options->is_child = true;
?

I understand that options have a plethora of fields. Would setting just one field to the right thing? I don't think so.

Comment 29

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #28)
> Comment on attachment 8985852 [details] [diff] [review]
> bug1464056_efail_decrypt2.patch - rebased (v2)
> Have you considered:
> if (obj->options)
>   obj->options->is_child = true;
> ?

That should probably be ok to. So far I didn't find a case where options isn't set. It's hard to verify though.

> I understand that options have a plethora of fields. Would setting just one
> field to the right thing? I don't think so.

As far as I understand, it's quit a mess. Lots of options that can be set at some point, but often at the points you really need a field from there, it's not set (yet) :/

Comment 30

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #29)
> That should probably be ok to. So far I didn't find a case where options
> isn't set. It's hard to verify though.
Can we go with that then, please. Please use the patch where I did the white-space adjustments.

As I said in comment #28, I didn't find any other call site where the options are set up that way. Can you show me an example where those options are created to motivate the "new MimeDisplayOptions();"? If it's hard to verify, we should at least run something which is well understood.

Comment 31

[Magnus Melin [:mkmelin]]

Attachment: bug1464056_efail_decrypt2.patch - v3

Now not creating the options. No null check is needed, it's used later in the function without a check.

Comment 32

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #31)
> Now not creating the options. No null check is needed, it's used later in
> the function without a check.
Really?

  body = mime_create(((ct && *ct) ? ct : (dct ? dct: TEXT_PLAIN)),
***           mult->hdrs, obj->options);
  PR_FREEIF(ct);
  if (!body) return MIME_OUT_OF_MEMORY;
  status = ((MimeContainerClass *) obj->clazz)->add_child(obj, body);
  if (status < 0)
  {
    mime_free(body);
    return status;
  }

#ifdef MIME_DRAFTS
***  if ( obj->options &&
     obj->options->decompose_file_p &&
     obj->options->is_multipart_msg &&
     obj->options->decompose_file_init_fn )
  {

In the first line it could be passed as NULL, then it's tested before dereferencing it.

BTW, is MIME_DRAFTS defined, I've never done the research.

Comment 33

[Magnus Melin [:mkmelin]]

Attachment: bug1464056_efail_decrypt2.patch - v4

Yes MIME_DRAFTS is defined.

Comment 34

[Jorg K (CEST = GMT+2)]

Comment on attachment 8986030 [details] [diff] [review]
bug1464056_efail_decrypt2.patch - v4

Thanks. I'll get this landed and uplifted.

Comment 35

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/b474dfcfa256b5f5800083d592a8307b430d1303
Don't decrypt S/MIME sub parts. Only allow decryption at top level. r=jorgk DONTBUILD
(will be build with the next push)

Comment 36

[Jorg K (CEST = GMT+2)]

TB 60 beta 8 (BETA_60_CONTINUATION branch):
https://hg.mozilla.org/releases/comm-beta/rev/b938c7733c51941c8033603a46efc2c1b2d02574

Comment 37

[Jorg K (CEST = GMT+2)]

This caused a test failure:
TEST-UNEXPECTED-FAIL | comm/mailnews/mime/test/unit/test_alternate_p7m_handling.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | comm/mailnews/mime/test/unit/test_alternate_p7m_handling.js | thunderbird_default/< - [thunderbird_default/< : 41] false == true

Looking at the test it seems that all that was required to fix this bug was to set the preference mailnews.p7m_external to true.

Comment 38

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #37)
> Looking at the test it seems that all that was required to fix this bug was
> to set the preference mailnews.p7m_external to true.
That's incorrect. With that pref set, not even the body in the top part is decrypted.

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: 1464056-follow-up.patch

I suggest to rename the new preference to mailnews.p7m_child_external so it lines up with the existing mailnews.p7m_external.

This patch also fixes the failing test and removes duplicated code.

Comment 40

[Magnus Melin [:mkmelin]]

Comment on attachment 8986076 [details] [diff] [review]
1464056-follow-up.patch

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

LGTM, with the below. r=mkmelin

Came to mind, mailnews.p7m_child_external actually seems obsolete now (though leave it for the moment). With mailnews.p7m_child_external, p7m attachments are always external so you don't need to set the mailnews.p7m_external pref.

::: mailnews/mime/test/unit/test_alternate_p7m_handling.js
@@ +53,5 @@
>  
>  /* ===== Driver ===== */
>  
>  var tests = [
> +  parameterizeTest(worker, [{ messages, pref1: false, pref2: false, count: 0 }]),

Naming these pref1 and pref2 makes this very hard to read. Please use the names like p7m_external instead.

Comment 41

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #40)
> Came to mind, mailnews.p7m_child_external actually seems obsolete now
> (though leave it for the moment). With mailnews.p7m_child_external, p7m
> attachments are always external so you don't need to set the
> mailnews.p7m_external pref.
No. When you set mailnews.p7m_external even the top part is made external. Try it now on a secure BMO message, assuming that you have S/MIME in use. Then you will see it doesn't decode at all.

If the test were more sophisticated, that is with an S/MIME encoded body, you'd see the difference there.

New patch coming. I'll also rename mailnews.p7m_child_external to mailnews.p7m_subparts_external since users don't know of the MIME tree where subparts are children. Agreed, even the concept of a subpart is difficult to graps.

Comment 42

[Jorg K (CEST = GMT+2)]

Attachment: 1464056-follow-up.patch

Changes discussed in the previous comment.

Comment 43

[Jorg K (CEST = GMT+2)]

C-C: https://hg.mozilla.org/comm-central/rev/6332ffbfd6518278cf3e49b99b6bcd0b87c1ebbb
C-B: https://hg.mozilla.org/releases/comm-beta/rev/acfbb53a1e1a597954b3999288251f292b40b0c1 (BETA_60_CONTINUATION branch)

Comment 44

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #41)
> (In reply to Magnus Melin from comment #40)
> > Came to mind, mailnews.p7m_child_external actually seems obsolete now
> > (though leave it for the moment). With mailnews.p7m_child_external, p7m
> > attachments are always external so you don't need to set the
> > mailnews.p7m_external pref.
> No. When you set mailnews.p7m_external even the top part is made external.
> Try it now on a secure BMO message, assuming that you have S/MIME in use.
> Then you will see it doesn't decode at all.

Sorry I confused the pref names now. It's  mailnews.p7m_external that became obsolete.

Comment 45

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #44)
> Sorry I confused the pref names now. It's mailnews.p7m_external that became
> obsolete.
Hmm, how can I convince you otherwise:

1) mailnews.p7m_external==true => all p7m parts are made external, including the top part.
2) mailnews.p7m_external==false, mailnews.p7m_subparts_external==true => (attacking) subparts are made external.

Or are you saying 1) is not a useful option? We'd need to research how people are using it:
https://www.google.com.au/search?q=mailnews.p7m_external
There are a few hits and as you know, people cry out when we make the slightest change :-(
We could also research why this was added.

Comment 46

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #45)
> 1) mailnews.p7m_external==true => all p7m parts are made external, including
> the top part.

Yes, but I imagine that's not the intended use case. 
It's used for (in particular in Italian administration) allowing signed pdf files to get received properly and not have thunderbird trying to decode them.

Comment 47

[Ben Bucksch (:BenB)]

My 2 cents to the pref names: Prefs are much easier to use when they are in the positive. "external" is essentially "do not show inline", and therefore a negation, and "external false" essentially a double-negation. Even I had a hard time to understand what the pref means.

"show_foo_inline" is something that most advanced users can understand, because they know the "show images inline" options.

Comment 48

[Jorg K (CEST = GMT+2)]

Agreed, but the pref was pre-existing.

Comment 49

[Jorg K (CEST = GMT+2)]

TB 52.9:
https://hg.mozilla.org/releases/comm-esr52/rev/fdefe5691f32ddfc7a8035661ff9a5bfd4c23765
Merged patch. Landed with preference set to false, so no change in behaviour.
People worried about the issue can set the pref to true, this will be stated in the release notes.

Comment 50

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/974ee91603eb
https://hg.mozilla.org/releases/comm-esr60/rev/4698a00fad7a

Comment 51

[Kai Engert (:KaiE:)]

For the record, the change that added an early return in mime_find_clazz can trigger crashes. The code must be allowed to execute until the end of the function, to ensure mimeExternalObjectClass gets initialized correctly, if it's the first time it's being used. I'll fix that in another bug.