Closed Bug 1464056 (CVE-2018-12373) Opened 3 years ago Closed 3 years ago

[efail] S/MIME plaintext can be leaked through HTML reply/forward

Categories

(Thunderbird :: Security, defect)

defect
Not set
normal

Tracking

(thunderbird_esr52 fixed, thunderbird_esr60 fixed, thunderbird60 fixed, thunderbird61 wontfix, thunderbird62 fixed)

RESOLVED FIXED
Thunderbird 62.0
Tracking Status
thunderbird_esr52 --- fixed
thunderbird_esr60 --- fixed
thunderbird60 --- fixed
thunderbird61 --- wontfix
thunderbird62 --- fixed

People

(Reporter: jens.a.mueller, Assigned: mkmelin)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-disclosure, sec-high)

Attachments

(4 files, 6 obsolete files)

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
Wayne: please CC additional Thunderbird folks as necessary
Flags: needinfo?(vseerror)
Flags: needinfo?(ben.bucksch)
Flags: needinfo?(vseerror)
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.
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)
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.
Flags: needinfo?(ben.bucksch)
(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/)
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
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.
Yes we need to see why the red color still shows. It shouldn't.
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.
> 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.
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.
(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.
(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.
Attachment #8980272 - Attachment mime type: message/rfc822 → text/plain
Oh, you mean attachment 8980272 [details] (no need to past the entire link).
Yes, the mail is attachment 8980272 [details], as I assume you saw now.
(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.
Note: Stripping the <plaintext> tag would be easy, but that still doesn't fix the CSS attack vector.
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.
Attached patch bug1464056_efail_decrypt2.patch (obsolete) — Splinter Review
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?
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #8985832 - Flags: feedback?(patrick)
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?
Attachment #8985832 - Flags: feedback+
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"?
Attachment #8985832 - Flags: feedback?(patrick) → feedback+
(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.)
Attached patch bug1464056_efail_decrypt2.patch (obsolete) — Splinter Review
Fixed language/typos.
Attachment #8985832 - Attachment is obsolete: true
Attachment #8985845 - Flags: review?(jorgk)
Summary: [efail] S/MIME plaintext can be leaked trough HTML reply/forward → [efail] S/MIME plaintext can be leaked through HTML reply/forward
Attachment #8985845 - Attachment is obsolete: true
Attachment #8985845 - Flags: review?(jorgk)
I've done some whitespace adjustments here.
Attachment #8985850 - Attachment is obsolete: true
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.
Attachment #8985852 - Flags: feedback+
(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) :/
(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.
Now not creating the options. No null check is needed, it's used later in the function without a check.
Attachment #8985852 - Attachment is obsolete: true
Attachment #8986026 - Flags: review?(jorgk)
(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.
Yes MIME_DRAFTS is defined.
Attachment #8986026 - Attachment is obsolete: true
Attachment #8986026 - Flags: review?(jorgk)
Attachment #8986030 - Flags: review?(jorgk)
Comment on attachment 8986030 [details] [diff] [review]
bug1464056_efail_decrypt2.patch - v4

Thanks. I'll get this landed and uplifted.
Attachment #8986030 - Flags: review?(jorgk)
Attachment #8986030 - Flags: review+
Attachment #8986030 - Flags: approval-comm-esr60+
Attachment #8986030 - Flags: approval-comm-esr52+
Attachment #8986030 - Flags: approval-comm-beta+
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)
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 62.0
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Attached patch 1464056-follow-up.patch (obsolete) — Splinter Review
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.
Attachment #8986076 - Flags: review?(mkmelin+mozilla)
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.
Attachment #8986076 - Flags: review?(mkmelin+mozilla) → review+
(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.
Changes discussed in the previous comment.
Attachment #8986076 - Attachment is obsolete: true
Attachment #8986101 - Flags: review+
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)
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
(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.
(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.
(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.
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.
Agreed, but the pref was pre-existing.
Group: mail-core-security → core-security-release
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.
Alias: CVE-2018-12373
Blocks: 1463360

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.

Blocks: 1594253
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.