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

Steps to reproduce:

Dear Thunderbird team,

In the scope of academic research in cooperation with Ruhr-Uni Bochum and FH Münster, Germany we discovered a security issue in Thunderbird: Thunderbird quotes and includes CSS internal <style> elements in email replies. This allows an attacker to abuse Thunderbird as a signing oracle for arbitrary S/MIME or PGP signed emails.

The attack is outlined as follows:

*** Attack scenario ***

Digital signatures should guarantee integrity, authenticity, and non-repudiation of messages. To give an example, Johnny could be a commander-in-chief who takes information security seriously. All his emails are digitally signed, making it hard to impersonate him in order to send forged statements or instructions. The goal of our attacker Eve is to start false-flag warfare. Therefore she needs to obtain a digitally signed "declaration of war" which she can forward to the armed forces.

*** General idea ***

Eve now sends an email to commander Johnny, in which she hides her malicious content using CSS while a benign text message, such as "What's up Johnny?", is added to be shown by Thunderbird. Similarly, the benign text can be hidden while showing the malicious content, based on CSS conditional rules which are satisfied only for a third party. If Johnny replies to such a specially-crafted HTML/CSS email, he signs arbitrary covert content along with visible content. This signed message can then be forwarded by Eve to a third party (e.g., the armed forces) where it displays the previously hidden malicious content "I hereby declare war". A simple example email is given below:

From: eve@evil.com
To: johnny@good.com
Content-Type: text/html

<style>
/* hide malicious content on desktop devices /
@media (min-device-width: 835px) {
.covert {visibility: hidden;}
}
/ but show on mobile/small-screen devices */
@media (max-device-width: 834px) {

• {visibility: hidden;}
  .covert {visibility: visible !important}
  }
  </style>

What's up Johnny?
<div class="covert" style="visibility: hidden">
I hereby declare war.</div>

In this example, different content is shown based on the device's screen resolution. It can be used to obtain a signed email from a desktop client, where a benign message is shown. The reply message instead displays a (signed) declaration of war when shown on a mobile device.

*** Conditional CSS rules ***

The W3C specifies CSS conditional rules (e.g., @media) which allow different formatting based on conditions such as screen width or orientation. For example, a different text can be shown whether a mobile phone is hold in portrait or landscape mode or whether the document is displayed on a screen or printed out. But there are lots of other options: for example, mail clients can be fingerprinted based on the @support conditional rule or various proprietary conditional statements of certain clients can be applied.

Without going into detail here, in the scope of our research we found conditional CSS to match and therefore show/hide certain text for virtually every email client that exists.

A further conditional rule introduced by Mozilla is @-moz-document. It allows CSS code to be executed based on the document location. In the context of email clients this even allows us to show different text for each user because the location contains an imap:// URI scheme with the email address. For example, to apply a red color solely for the emails of general@good.com the following CSS code can be used:

@-moz-document url-prefix("imap://general@good.com") {* {color:red}}

This can be used to obtain a signed email which with a different displayed content for each user, for example, on a mailing list.

*** CSS blinding options ***

We identified seven CSS properties which can be used for covert content attacks as shown below. However, this list is unlikely to be complete because CSS is very complex and offers more possibilities to hide text.

display: none;
visibility: hidden;
opacity: 0;
clip-path: polygon(0px 0px, 0px 0px, 0px 0px, 0px 0px);
position: absolute; top: -9999px; left: -9999px;
color: transparent;
font-size: 0;

Feel free to contact me for any questions.

Greetings,
Jens Mueller

--
M.Sc. Jens Mueller
Research Assistant
Chair for Network and Data Security, Ruhr-University
Universitaetsstr. 150
Building ID 2/415
D-44780 Bochum
Phone: +49 (0) 234 / 32-29177

Actual results:

*** Impact ***

The attack allows Eve to obtain valid signatures for arbitrary content to be displayed. This can be used to trick a third party, which relies on the authenticity and integrity of signed messages, to perform certain actions (such as starting a war). A forensic analysis can reveal the deception, but then it may already be too late (the war is already declared).

Expected results:

*** Countermeasures ***

There are three options to counter the attack, each with a usability-security trade-off:

• 1. Drop CSS support in general: Conditional CSS makes it easy for an attacker to hide certain text within a signed message while showing different text. Ideally, clients would ignore CSS in received emails. However, this is an unrealistic scenario given today's usage of email. Sanitizing conditional CSS rules and properties which can be used to hide content is feasible, but it may be insufficient as web technologies are constantly evolving.

• 2. Only ASCII text in replies: It should not harm the user experience if mail clients converted quoted messages into ASCII text when replying to an email. Various clients are already doing this. Thus, we recommend that security-focused clients should adopt this behavior.

• 3. Remove CSS styles in replies: Email clients should not sign any quoted CSS <style> (or <link rel="stylesheet") from the original message, so that they cannot be used as signing oracles, based on blinding text with CSS conditional statements.

Email that shows "What's up Johnny?" in Thunderbird on a desktop device, but when replied to with a (PGP or S/MIME signed) email "I hereby declare war." is showed if the reply message is read on an iPad.

Magnus: what security rating keyword should this bug get?

Good question, as this is primarily not a technical flaw. Probably sec-low?

I'm not sure what a satisfactory solution would look like. Force plaintext for replying to signed? I'm not sure that would be very popular. Replying without quote when signed?

Update: Here's a full (public) report on reply-based signature oracles (and decryption oracles as mentioned in bug #1419417):
https://arxiv.org/ftp/arxiv/papers/1904/1904.07550.pdf

I've read the paper. Jens, thanks a lot for the great work by you and your team.

Regarding this bug, section 8.2 of the paper mentioned in comment 4 contains suggested countermeasures for signing oracles.

The suggested options are:
(a) drop CSS support altogether
(b) only ASCII text in replies

When receiving a signed email, the email might have been produced by a victim of an attack, sent with a client that includes conditional CSS. It is difficult, or impossible, to decide which contents was intended to be signed be the sender. Only approach (a) can potentially help for received email that was sent by a vulnerable client.

Disabling only a subset of CSS seems difficult, and has the risk to miss new mechanisms that might get introduced in the future.

However, if we disable the conditional or blinding rules, what's the consequence?

IIUC, all text contents will be shown. This means, both the text that was intended to remain hidden by the attacker, and also the text knowingly signed be the sender, both will be shown.

I believe this means, disabling all CSS can still result in text being shown that wasn't intended to be signed by the sender. Potentially, we could try to filter out all contents that was covered by conditional rules, and only show the contents that was knowingly signed by the sender. But I see two problems with that approach. First, if we excluded a couple of words, like "not", it can easily and completely change the meaning of the text. Second, the filtering is difficult, we're at risk that we'd miss to exclude some parts.

If we want to be on the safe side, I think our only chance is to inform the user, and not pretend to be certain about the message signature.

Suggestion:

• for all received signed message that contain any CSS style information,
  we display the "questionable signature" status, meaning,
  that we see a signature, but we are unable to confirm that the message
  contents match the signature.
  The advanced details can say that the uncertainty is based on CSS content.

• strip all CSS prior to displaying signed messages

Regarding replying, quoting and suggestion (b), I agree that we should strip all CSS information, and even better, strip away all HTML formatting, too. That seems like the only reliable way to prevent the user from unknowingly signing information.

Based on https://wiki.mozilla.org/Security_Severity_Ratings I'd rate this as sec-high.

The definition for sec-high includes a risk described as
"inject data or code into those sites, requiring no more than normal browsing actions"

With this bug, information can be injected into signed emails, and all that's required is that a user uses the regular approach to reply to an email, and digitally sign it, which many people do frequently.

Dan, Magnus, do you agree with sec-high based on the above explanation?

Technically there is no injection - it's all about perception.

I guess one option would be that for signed emails we wouldn't include a quote of the original. That might not be that bad actually. It would need some information bar telling why and allowing an informed override though.

Could maybe also throw in checking for encrypted too here. I guess the attack wouldn't be possible then(?).

Given how little "orders" are probably given over signed (and unencrypted) emails like this, it seems like a pretty fringe case for actual attacks.

Technically there is no injection - it's all about perception.

It's injection into the information visible to users. I think that's sufficient to claim practical injection, even if the information is technically always present.

I guess one option would be that for signed emails we wouldn't include a quote of the original. That might not be that bad actually. It would need some information bar telling why and allowing an informed override though.

Could maybe also throw in checking for encrypted too here.

"Not being able to quote at all" might need more explanation than "quoting only as ASCII".

However, given the additional risks related to quoting when replying to an encrypted email, I have also considered to disable quoting to encrypted emails, too (at least by default).

I guess the attack wouldn't be possible then(?).

The attack is still possible with encryption involved. The encryption is just an outer layer. The attacker can receive a signed-then-encrypted email from the victim, decrypt, and reuse the signed part.

Given how little "orders" are probably given over signed (and unencrypted) emails like this, it seems like a pretty fringe case for actual attacks.

I think you're trying to make a guess here. I think we don't know how many environments use signed email for relying on the authenticity of orders. I think this is a practical use case for signed emails, and I think we shouldn't rule out that it's used for such.

I think there are two aspects here:

1. Thunderbird includes the internal <style> elements in replies, this is the condition to be misued as a "signature oracle". Email clients that a) remove or b) convert internal to inline styles are not vulnerable (i.e., we were not able to use them as "signature oracles").

2. Like almost any mail client with HTML/CSS support, Thunderbird can be used to display a certain content based on conditional statements. This is hard to counter against, and is a somewhat philosophical problem: Does any data format that allows dynamic content (HTML, PDF, etc.) make sense at all for the purpose of signatures?

Aspect 1. is probably relatively easy to solve, aspect 2. is challenging (but would not be required if all email clients implemented countermeasures for 1. :)). As a minimal countermeasure, we should at least the remove @-moz-document, because it allows targeted content manipulation for specific Thunderbird users, based on their email addresses / IMAP username.

Personally, I'm a fan of reply/quote in ASCII in the default settings, but I guess that HTML email has become a reality for most users.

Regarding the impact, we were not able to measure the actual usage of S/MIME or PGP signed emails. My personal option (based on a look into my inbox) is similar to the one of @Magnus -- I think signatures are rarely used in the wild. On the other hand, we know of a government authority that has a policy for mandatory S/MIME signatures for "internal" emails (on the other hand, I don't think they're using Thunderbird as a client).

Info: this is not a duplicate of Bug 1548252, but an independent issue.

Because the issue was reported in February, we fully disclosed it in the scope of an academic paper (https://arxiv.org/abs/1904.07550) and a DEF CON talk (video demo here: https://media.defcon.org/DEF%20CON%2027/DEF%20CON%2027%20presentations/DEFCON-27-Jens-Mueller-Re-Whats-up-Johnny-Covert-Content-Attacks-Extras/Jens%20Mueller-demo-signing-oracle.ogv). Various other mail clients are affected to this bug/feature. AFAIK no one has fixed yet, because it's kinda "standard CSS behavior". Still bad if we take digital signatures seriously (as in mathematically unbreakable and unforgettable ;)).

This bug primarily reports issues related to conditional styles. You said, if we don't use <style>, but only inline style=, then this class of attack isn't possible.

For a while we considered to treat <style> differently.

However, Jörg just reminded us that ambiguous content can be produced using simpler inline style, too, for example by using
<p style="display: none;">

If the first victim uses a client supporting styles, then the hidden paragraph will be retained in the reply, and can be tricked to sign a message, including that sentence in the plaintext part.

If that signed message is then forwarded to a second victim, who uses a client that supports plain text only, the hidden paragraph will be shown.

I think this simpler scenario should also be handled, when identifying a fix for this bug.

True. Inline styles can be somewhat abused. I don't know of a way to actually build conditional statements (e.g., @-moz-document like) with inline styles but not supporting HTML could be considered as a condition by itself. However, the attack is limited because you cannot escalate into text added in the reply message. The attacker only has control over her own text:

[01] I'm fine, thanks.
[02] On 01/05/19 09:53, Eve wrote:
[03] > <p style="display: none;">What’s up Johnny?</p> 

If this reply message is signed, the display of lines 1 and 2 cannot be controlled by the attacker with inline styles. Therefore only the attacker's quoted text is subject to ambiguity. For us, such issues where to weak to consider them as vulnerabilities. But I totally agree that that it's not perfect that things like this are possible...

We did multiple rounds of suggestions and discussions by email, but it is very difficult to come up with a solution for this class of bugs.

There's a strong desire to keep the ability for pretty formatting of emails, and we couldn't agree yet on any approach that would automatically degrade all email to plaintext.

In theory, the digital signature only promises that the message was received exactly as it was sent. Displaying it identically as the sender has seen it is a different problem. I personally agree that it is a valid expectation of the user to be certain that the message display does indeed show what the sender intended to sign. But with the use of HTML and CSS, it seems impossible to guarantee that.

Maybe we need to use an alternative approach, and lower our promise for all HTML email.

Idea:

• Whenever we receive a plaintext email, we continue what we do today, show the signature status.

• Whenever we received an email with alternative text and html, but the user has configured Thunderbird to "view / message body as / plain text", then well also continue to show the signature status.

• We could introduce a new preference, that a user could set to opt in to "always show digitally signed email as plain text". If the user sets the pref, then for messages with alternative text and html, we'd show the plain text part. For messages that contain only html we'd convert the message to plain text.

• Whenever a signed email contains HTML, and we're displaying HTML, then we DON'T show a signature status. Instead we show a notification bar, which says something like: "Message was digitally signed, but this is a HTML message, it's uncertain if we display its text correctly. [more information]".

• If the user clicks "more information" in a signed HTML email, we list the signature status in more detail, but say, the display is uncertain, doublecheck with sender, etc., and offer to view the message as plaintext instead, which is more reliable"

• When composing a message, we continue to allow the user to sign any message, including HTML. We'll keep HTML and CSS by default, to ensure the pretty styling continues to work. However, whenever digital signing is enabled and we're in HTML mode, we show a notification/warning bar in the composer window. "It's recommended to use plain text for signed email, because it cannot be guaranteed that the recipient will see it exactly as you see it." Optional with a more information button, that explains about potentially hidden text when quoting/forwarding. There could also be a button that offers to convert the message being composed to plaintext.

• We could also have a second pref (or have the above pref be covert the following functionality, too), which the user could use to opt in to the following behavior: "Always use plaintext for digitally signed email"

• If the user replies and "always use plaintext for signed email" is set, and the user's configuration is "sign by default", then we'll automatically convert the quoted text to plain text.

• If the configuration is "don't sign by default", user replies to html, starts editing, then switches signing to on for this message, then we convert it to plain text at this time. We also will show a notification that says "message was converted to plain text [undo]".

• If the user clicks undo, we'll revert the conversion, and disable signing again (and tell the user). If the user continues to type after switching signing to enabled, we'll remove the notification bar and the user cannot undo any more.

Jens, what's your opinion on this potential approach? Nothing is decided yet, this is just brainstorming.

Jens, do you have any input to add?

Sorry for the delayed response.

It's a somewhat philosophical question on what "digital signature" means. If we're consequent, signatures of any "active" file format data cannot be taken seriously regarding their displayed content -- only regarding their bytestream (as already stated here). This is true not only for HTML but also for PDF and basically everything that is not plain text. And even for text/plain it may be possible to generate ambiguous content using Unicode hacks.

The question is what do we want to protect from?

It is imho really challenging to protect against ambiguous messages (a message that displays different content in different applications) signed by the attacker herself. There are so many ways to do this, for example:

• multipart/alternative displaying either text-part OR html-part as already mentioned
• Unicode tricks:
  • RTLO: I buy for $[U+202E]1337
  • BACKSPACE: I hereby declare war[U+0008][U+0008][U+0008]peace
  • etc.
• Terminal escape sequences:
  • CLEARSCREEN: I here by declare war[\033[2J]...
• File attachments with disambigous displayed content
  • "PDF Mirage" or "PDF Polyglot" style attacks
  • etc.

I personally would give up here, and accept the fact an attacker herself can sign a message with two meanings. This is hard to protect from, even though the first 5 ideas do actually try. It does not seem easy to find a trade-off between security and feature richness in practice. But we should at least be able to protect from being misused as a signing oracle (i.e., the attacker makes the victim sign a message that is displayed differently to a third party). The last 5 ideas try to accomplish that.

Personally, I think

• Reply-with-plain-text is a good practice anyway :)
• If reply-with-html is used, what about removing internal styles for signed messages (or converting them to to inline styles)? E.g., Outlook does this, therefore we cannot misuse this client as a signing oracle.
• Where possible, keep the user out of the loop (instead of displaying them warning messages which may be hard to handle/understand).

Jens, thanks for your feedback.

With all that has been said, I consider CSS conditional rules as the primary issue.

Proposals to completely remove CSS in general, to completely disable CSS in replies, and to completely drop down to plaintext in emails have been rejected.

We cannot support pretty HTML/CSS and avoid invisible text during HTML rendering.
Even without CSS, it's possible to use HTML tags that will render text as invisible, unreadable, or unnoticeable.
I think this means it's useless to remove CSS blinding rules from emails.

I suggest that we fix this bug by removing all CSS conditional rules, introduce a blacklist, and have a reminder to add future conditional rules, too. At this time, we'd blacklist @media, @supports, @-moz-document.

I have a patch in hand that does that.

While the bug primarily asks that creating signed emails with such rules should be prevented, I'd prefer to have a solution for incoming emails, too.
Therefore I suggest that we strip those rules when viewing email, and also when using an existing email for a new email (reply, forward, edit as new).
(Optionally, we could consider to show a notification, whenever we display a received email, in which we stripped CSS conditional rules prior to displaying it.)

With those rules removed, the attacker's options are limited to hiding text in the original message to Johnny, which Johnny might not see in the quoted text when replying, but which might be shown to the armed forces, if they view the message as plain text. However, as Jens explained in comment 14, let's consider it outside the scope of this bug.

I'll file a follow-up bug that explores to improve the situation with alternative text and html emails. We could indicate whenever an email has alternative presentations. When sending digitally signed emails, we could consider to never include alternatives, but either send plaintext only or html only.

Just FYI, I'd like to make everyone aware of the WYSIWYS approach (What You See Is What You Sign, https://en.wikipedia.org/wiki/WYSIWYS ). It's difficult to solve the problem that contents might be shown differently based on viewing software and hardware, and signing an image of the visual rendering of a message seems to be the only solution that really works.

Gecko code (Mozilla platform) that adds a new sanitizer, that can strip blacklisted CSS rules, and keeps everything else. Based on nsIParserUtils and nsTreeSanitizer. It's unclear if this could be added to the Mozilla platform (would be preferable for maintenance), or if it would have to live inside Thunderbird's code.

Mail part of the fix.

Note that both patches apply to the 68.x branches. Minor changes will be required for mozilla-central.

Let's start by adding the sanitizer code to comm. We can try to move it to mozilla core at a later time.

Magnus, Jörg, is it acceptable to you to always ignore the @media and @supports conditional rules when displaying email, and removing them when forwarding/replying to email?

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

Henri, maybe we can put this addition into nsIParserUtils? See comment 24.

It's not-obvious what the difference would be for nsTreeSanitizer, since the patch here both adds and removes stuff compared to nsTreeSanitizer.

What would this add if this was implemented as a new flag in nsTreeSanitizer itself?

Magnus, maybe it will be the right approach to add this code to the Mozilla core, but I'm uncertain if we're ready to go that path.

I think as a first step, we should test if the suggested new behavior is acceptable. We don't know yet if the suggested cleanup doesn't have any side effects, or if there will be complaints about the new behavior. Or maybe it doesn't work correctly yet.

It might be necessary to make adjustments to the sanitizing code. And if that code lives inside Thunderbird, we're more flexible while we potentially go to a number of iterations to get it right.

If this code is moved to the Mozilla core immediately, are we sufficiently flexible with testing and experimenting and amendments?

Could it be better to start with having this code live in comm-central, wait until we've confirmed it's working right (in nightly/beta), and only afterwards consider to move it over to Mozilla core?

I don't see much reason not to have it live at the right place from start. It's basically an additional flag and shouldn't cause side effects elsewhere.

(In reply to Henri Sivonen (:hsivonen) from comment #27)

It's not-obvious what the difference would be for nsTreeSanitizer, since the patch here both adds and removes stuff compared to nsTreeSanitizer.

What would this add if this was implemented as a new flag in nsTreeSanitizer itself?

The existing code uses this approach:
"You'll get most sanitizing, but only a subset of the changes can be influenced by the flags"

What we need here is:
"remove some CSS rules, but don't touch anything else"

Jörg, here is a try build with the patches:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=75027de420a29ec21cf4259b45555a9dc0122ee9

Would you like to test if it causes any unexpected side effects for HTML mail?
Thanks

Some mozmill tests are failing. I'll have to check what happens.

Indeed, the Z1 failures, mostly in compose and cloudfile (which also prepares messages), seem related to your patches. X4 is bug 1591677.

Yes, I'd like to check it out in some sample HMTL messages once ready. Why do you add the Sanitize() call in nsMsgCompose::ConvertAndLoadComposeWindow() for outgoing messages?

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

Why do you add the Sanitize() call in nsMsgCompose::ConvertAndLoadComposeWindow() for outgoing messages?

for "edit as new"

So that's different from reply for forward? Why?

(In reply to Henri Sivonen (:hsivonen) from comment #35)

/**

• • Flag for sanitizer: Only CSS conditional rules will be moved,
• • nothing else will be changed.
• • Can be combined with flag SanitizerLogRemovals, only.

Please MOZ_ASSERT this restriction in nsTreeSanitizer.

Does this test look correct?

nsTreeSanitizer::nsTreeSanitizer(uint32_t aFlags)
...
      mOnlyConditionalCSS(aFlags & nsIParserUtils::SanitizerRemoveOnlyConditionalCSS) {
...
  /* Ensure SanitizerRemoveOnlyConditionalCSS isn't combined with any
   * flags, except SanitizerLogRemovals. */
  MOZ_ASSERT(!mOnlyConditionalCSS || 
             0 == (aFlags & ~(nsIParserUtils::SanitizerRemoveOnlyConditionalCSS | nsIParserUtils::SanitizerLogRemovals)));

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

So that's different from reply for forward? Why?

The change to mimeTextHTMLParsed.cpp doesn't cover "edit as new", that seems to take a code path where the message is taken pretty much unchanged.

That's why I made an additional change to nsMsgCompose::ConvertAndLoadComposeWindow. However, the attached patch is too invasive.

I have a new patch for that function, which limits "strip CSS conditional" to the EditAsNew scenario. Below is a new try build that built this smaller patch, that works better. Ignoring intermetting failures, it passes the tests:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8b39dd50ea5b43ba2863349a35a68f9f9c6d465a

Jörg, I think you could use the binaries from this newer try build for some interactive testing.

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

My general question is: Why do we do the sanitise only for "edit as new" and
not for reply or forward?

That's a misunderstanding. The patch covers:

• reply
• forward
• edit as new

The change to mimeTextHTMLParsed.cpp doesn't cover "edit as new", that seems to take a code path where the message is taken pretty much unchanged.

Well, unless I'm missing something, mimeTextHTMLParsed.cpp is for display
only.

In my testing, the change to mimeTextHTMLParsed.cpp covered the reply/forward scenarios.

+static void remove_conditional_CSS(nsString &aBuf) {

Please inline this to save making copies, or give it an in and an out
parameter.

We could do that, but note this new function uses exactly the same style as the existing function "remove_plaintext_tag", which is just next to it in the same file.

+static void remove_conditional_CSS(nsString &aBuf) {

• nsString tmp = aBuf;

One copy here.

@@ +722,5 @@

     remove_plaintext_tag(newBody);
     htmlEditor->RebuildDocumentFromSource(newBody);
   } else {

•    if (mType == nsIMsgCompType::EditAsNew) {
  

•      nsString newBody(aBuf);
  

And another copy here.

In my understand the above don't copy. See this reference guide:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings

The nsString class uses reference counting of the underlying memory buffer, and do copy on write, only.

Does this test look correct?

Yes.

Can we do the string processing like this, please. BTW, remove_plaintext_tag manipulates the string passed in directly.

Also thanks for the sample e-mail. It shows what I said: Forward and "edit as new" are mostly the same code path and the patch doesn't work for forward, the @ lines are still red. It works for reply and I haven't added the debugging to see what's going on exactly, but as I said (and documented in the compose pipeline WIP document), reply is very different to the rest of the bunch.

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

the patch doesn't work for forward

Sorry, not sure how I had missed this while testing.

Here's a new patch, and a new try build:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c33326acd039210d5aeb327b8b566e06e3043abc

Updated gecko patch, added assertion from comment 40 which Henri confirmed in comment 45.

The test message that Jörg mentioned is from here:
https://github.com/RUB-NDS/Covert-Content-Attacks/blob/master/02-sig/02-conditional-rules.eml

Jörg, can you please test the new build?

This is a slightly simplified version. I noticed that removing the plaintext tag had a bug
https://hg.mozilla.org/comm-central/rev/fca92b0b06929d2a91a677546b1d282fd4d17321
It needs to be done everywhere where you have a HTML editor, but not in a plaintext context. That's why the original patch did:

-      if (aHTMLEditor)
-        mailEditor->InsertAsCitedQuotation(mMsgBody, EmptyString(), true,
+      if (aHTMLEditor) {
+        nsAutoString body(mMsgBody);
+        remove_plaintext_tag(body);
+        mailEditor->InsertAsCitedQuotation(body, EmptyString(), true,
                                            getter_AddRefs(nodeInserted));
-      else
+      } else {
         mailEditor->InsertAsQuotation(mMsgBody, getter_AddRefs(nodeInserted));
+      }
     }

but only sticking it into the if branch here seems wrong:
https://hg.mozilla.org/comm-central/rev/fca92b0b06929d2a91a677546b1d282fd4d17321#l1.81

Let's correct this now. I've tested reply, forward and "edit as new", but I still don't know why reply works since we're not changing that code path. I'll investigate that an post my findings into the next comment.

Oops, let's write it like this, even better.

I still don't know why reply works since we're not changing that code path.

OK, I added some debug. Reply goes through MimeInlineTextHTMLParsed_parse_eof() and that we've already covered. Weird and wonderful ways of composition. I guess I'm done here, I'd go for the last C-C version I've attached, well, after some clang reformatting.

OK, reformatted.

Thanks Jörg. Those changes look good to me.

You didn't say explicitly if you have tested with some advanced HTML emails, and checked if they still look pretty. I assume you did. (I had done some limited testing my myself, and didn't notice regressions.)

Jörg, you probably have sufficiently reviewed the flow of this code. Can we consider your work as an r+, or do you think we should get an additional review from Magnus? (I can give you r+ on the modifications you made, so the current patch could be seen as our combined work, peer reviewed by us.)

Once we have the r+ done, I'll submit a phabricator patch for the m-c gecko landing (and ask Henri to approve).

This raises another question. Should we consider fixing this bug for stable comm-esr68?
If yes: It seems unlikely that our functional enhancement (with IDL change) to gecko can be approved for the stable mozilla-esr68 branch. We'd probably have to add that Gecko code locally to the comm-esr68 tree, as I had originally suggested. (I think that's cleaner than patching THUNDERBIRD_68_VERBRANCH.)

Looks like the M-C patch doesn't apply any more.

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

Looks like the M-C patch doesn't apply any more.

Yes indeed, the code was changed:
https://hg.mozilla.org/mozilla-central/diff/5dc1f98435bb24f9bfe8eeabf4a3c2070e85a32e/dom/base/nsTreeSanitizer.cpp

Here is a merge attempt, but I haven't yet built.

Yes, gecko v4 builds and works.

OK, so I was asked whether the patch would behave on e-mail seen in the wild. The answer is: It of course destroys @media query based HTML messages to a certain extent.

In my mailbox I use for "bulk" e-mail, mailing lists, charities, etc., I currently have 141 messages. In those, I have 720 media queries, like:
@media only screen and (min-width:768px){
@media only screen and (max-width: 480px){

Those e-mail are designed to look good on small devices. I enclose one sample where you can see the responsive design by making the TB windows smaller. Display the message in the preview pane to see it.

With the patch, the responsiveness is gone. I haven't analysed all 720 media queries, but I think it's imaginable that information will get lost by stripping conditional CSS.

Jörg, is there an alternative that also fixes this bug?

Jörg, is there an alternative that also fixes this bug?

Well, you could do the removal of conditional CSS only for signed/encrypted messages. And if the user decides to sign a message which he edited as new from an existing message, strip the conditional CSS at that point from the editor. Of course that's more work.

Maybe after many comments of deliberation, you can summarise the risk that we're trying to avoid or a concrete example in one paragraph.

I don't think we need to treat all messages the same and there don't seem to be much point of affecting how a message is rendered for the "run of the mill" newsletters or messages from PayPal or charities I tested this on.

In an earlier discussion (possiby outside this bug), Magnus had stated his opinion, that a solution that treats signed messages differently from regular messages isn't a good solution.

I also think that we should treat both signed and unsigned message equivalently, for consistency.
Even when not using signed messages, the same kind of confusion can be created, which is an argument to always strip the conditional rules.

In an earlier discussion (possiby outside this bug), Magnus had stated his opinion, that ...

Sorry, I have many other duties and I'm really confused by all the discussions going on. Maybe we should jump onto a conference call or have an overview document or some such. I remember that I suggested to show signed HTML message as "simple HTML" by default, which AFAIK removes all CSS, with a big warning.

Frankly, and please excuse the ignorant comment, we also need to define what "signing" actually means. Does it mean that we guarantee that the message originates from the person it claims to originate from, or do we also need to ensure that it doesn't contain any malicious content? Why would people who's lives are at risk not revert to safe plaintext messages? Do we really need to bend backwards and eliminate every possible danger of using the "e-mail tool". Any tool has its dangers which can cause damage or death if used improperly. Again, I don't think Edward was using HTML mail in critical situations.

EDIT: That said: There's nothing wrong with making signed e-mail safer, along the lines of comment #0 point 2 iii: Remove CSS in replies.

Jörg, I was referring to the discussion that you, Magnus and I had a while ago. In those discussions, I had tried to argue for a more general solution. But we couldn't reach a consensus for a solution that would strip HTML/CSS styling in general, because supporting pretty email contents is considered important.

The consequence of those discussions were my comment 18, in which I gave up finding more general solutions, and suggested to limit our effort specifically to the conditional CSS rules.

Assuming that email software renders all fixed HTML/CSS rules, an email will probably be rendered in similar ways in all clients that render HTML/CSS, both for the sender and the recipient.

However, using CSS conditional rules, it's much easier to create different presentation based on environment properties. Making that impossible using CSS conditional rules is the purpose of this bug, and this is useful for both regular and signed email.

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

we also need to define what "signing" actually means. Does it mean that we guarantee that the message originates from the person it claims to originate from,

Yes, to know who sent it, and to be certain that it hasn't been modified.

We're unable to guarantee that it is shown exactly as the sender saw it, in order to do so, the sender would have to render all contents as bitmaps, as explained here https://en.wikipedia.org/wiki/WYSIWYS - which is something we don't do currently.

Currently, the risk that email contents are shown differently, based on plain text vs HTML vs HTML/CSS capabilities of the involved software, isn't addressed by most email software that supports digital signatures.

or do we also need to ensure that it doesn't contain any malicious content?

No. Signing means confirmation of ownership. Nobody can guarantee that something is malicious or not, and that also depends on the point of view. It's similar with https on the web. It only confirms from where the content was loaded, based on ownership of the associated server certificate. No promises are made for the contents.

Why would people who's lives are at risk not revert to safe plaintext messages?

I personally agree that they should, although they might not be aware that they should do so. However, protecting the user from any kind of potential misunderstanding apparently isn't seen as the purpose of Thunderbird. Apparently it's accepted that by default we attempt a best effort rendering approach, which might be different from what other clients show.

Do we really need to bend backwards and eliminate every possible danger of using the "e-mail tool". Any tool has its dangers which can cause damage or death if used improperly. Again, I don't think Edward was using HTML mail in critical situations.

I think we're trying to find a good middle ground. Apparently we aren't trying to avoid any possible danger. If that were our priority, then you and Magnus would have agreed to the suggestion to completely disable rendering of all HTML/CSS elements for digitally signed email and revert to plain text. But you didn't agree, you rejected that suggestion.

As a consequence, we're trying to find a middle ground, and disabling CSS conditional rules seems like an acceptable trade off, that has only limited implications for rendering HTML/CSS email on desktop devices.

EDIT: That said: There's nothing wrong with making signed e-mail safer, along the lines of comment #0 point 2 iii: Remove CSS in replies.

I personally would agree to that, but I thought this suggestion had been rejected by Magnus. It would have the consequence that the quoted contents when replying or forwarding would no longer look pretty. I believe Magnus was asking for a solution that uses the same behavior regardless whether using signing or not. Also, an implementation that dynamically changes the contents of the quoted part, which is influenced at runtime if the user toggles between signing or not signing, would require a much more complicated user interface.

So I agree the current approach of the patch (to drop conditional css on display as well as compose) should be alright. We may want to do more stuff later, but it's a good first step at least.

Can someone please explain to me why we're dropping the conditional CSS for all messages and not just signed ones? All I heard was (comment #67):

... but I thought this suggestion had been rejected by Magnus.

Seems like Kai and I are agreeing not to apply this globally.

I don't think we should make needless differences between signed/encrypted or not. Else you just put secure mails into a state where people could not accept secure emails due to such abnormalities.

You could do the same "funny" tricks un-signed too so it's good to protect against that.

Given Magnus supports the suggestion to always remove all conditional CSS rules (for both signed and unsigned), let's go with that, and let it bake on c-c and in beta. If we get reports about broken behavior as a consequence of that change, we can potentially reconsider.

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

• /* Ensure SanitizerRemoveOnlyConditionalCSS isn't combined with any
• • flags, except SanitizerLogRemovals. */

Not sure why you'd make it like that...? it could be one of the flags that
could be combined with others

Magnus, the arguments are:

(a) I'd like to begin with a minimal change. Because this bug is considered a security issue, we might consider to backport it to the stable TB 68.x branch eventually. The more changes we make, the more risk the patch will be, the more difficult the merging will be.

(b) Right now, we don't need that combination.

(c) Because nsTreeSanitizer will apply several modifications unconditionally, that aren't controlled by flags. We'd have to introduce new flags for each operation that is currently being applied by default, and that we want to prevent when removing CSS conditional rules, only.

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

(c) Because nsTreeSanitizer will apply several modifications unconditionally, that aren't controlled by flags. We'd have to introduce new flags for each operation that is currently being applied by default, and that we want to prevent when removing CSS conditional rules, only.

With the current approach, if our new flag is set, I can skip the changes that would usually be done unconditionally.

If we want to generalize the code, to allow this new flag to be combined with other code, I'd prefer to do that in a follow-up, that doesn't need to be backported.

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

It would also be good to put this behind a pref, say
mail.html_sanitize.drop_conditional_css

Will do and will attach a new c-c patch.

If my comment on the other patch is addressed, also add a pref for each flag
to say, drop style, drop legacy (the non-CSS presentational stuff)

You haven't said why you're suggesting this fine grained flexibility for users to customize the behavior, in addition to the current ability to use the "simple html" configuration. If this is suggested as "nice to have", then I'd prefer to do that in a follow-up to this security bug.

It would indeed be more of a nice-to-have. But more generally, it's a bit odd code-wise to have one flag that can't be combined with other flags. Flags are usually used exactly so you can bit-wise combine behaviours.

I'll file a follow-up once this has landed.

added the pref.
tested manually.

Do I have to run some kind of c++ prettier on this?

Do I have to run some kind of c++ prettier on this?

Yes. clang-format. At least on Linux that appears to need a special setup, BenC has the details (or even posted to #maildev or published elsewhere).

Thanks for reminding me about clang-format.
This patch was processed with it.

updated for Jörg's suggestion

Henri, now that we're ready to land the Thunderbird part, I've submitted the TreeSanitizer patch (which you already had r+'ed here) in a separate bug and phabricator request: Bug 1600429. Thanks for your help.

https://hg.mozilla.org/comm-central/rev/55d957f1d6befeb94b0806a45340d96d25fb4eef
Let's call this fixed for now.

IIUC we missed 72, and the first Beta that will have this change will be Beta 73.

Any thoughts on uplifting this to TB 68.x after a while?

BTW, the preference should have gone into mailnews.js, not all-thunderbird.js. Effectively it doesn't matter, but architecturally what we have is wrong now.

You don't know what (theoretically) seamonkey would want to do...

I'm not sure why we're discussing something which is quite obviously wrong. You know as well as I that the particular client can redefine the preference if they want. A preference exclusively read in MailNews belongs into mailnews.js.

I haven't yet tested this change.

https://hg.mozilla.org/comm-central/rev/98d3639a346e8049888368e789e56c58f6273153
follow-up to move the mail.html_sanitize.drop_conditional_css pref to mailnews.js. r=mkmelin

Please don't forget to record relevant changesets.

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

Any thoughts on uplifting this to TB 68.x after a while?

We probably should. Maybe still give it more beta time first. We'll need

• https://hg.mozilla.org/mozilla-central/rev/53cfede1065c
• https://hg.mozilla.org/comm-central/rev/55d957f1d6befeb94b0806a45340d96d25fb4eef + https://hg.mozilla.org/comm-central/rev/98d3639a346e8049888368e789e56c58f6273153

The patch as written is not quite a complete fix... I can just use left: 100vh - 1000px to show an element only to big enough viewports or what not :/

This is also very incomplete in other ways, like the <style media> attribute not being sanitized, or @import url(..) <media-query>.

Emilio, do you have ideas how this could be improved further, to handle the scenarios you mention?

I think he filed bug 1603299 for that.

I summarized many of the aspects discussed in this issue here. The problem is not just conditional styles and multipart/alternative messages but also different implementations by different vendors. I crafted an email which is displayed differently in Thunderbird, Apple Mail, Gmail, Outlook, and Yahoo. The solution to these problems is not to remove conditional styles but to force all content to render – and have other mail clients do the same.

This really needs to be reverted. I go as far as saying that 99.99% of all emails that contain such media queries are no security risk as they are informations only (mostly newsletters). Nobody's replying to those emails either.

I'd also like to chime in and hope that mail.html_sanitize.drop_conditional_css could default to false. If a user makes a conscious choice to disable conditional css, some mails being displayed weirdly can be expected, but someone without that knowledge will just see the mails looking weird in Thunderbird.
Similarly, for remote content the user always see's the notification bar right in the mail and can easily understand why something looks like it's missing. If the conditional css ought to be disabled by default, a similar info at least might inform the user why something looks weird.

The second mentioned counter measure strikes me as a lot more user-friendly as well: Only sanitizing conditional css when replying or even replying ASCII-only when using a signature. It could even suffice to disable the conditional css in the reply editor and display a warning that conditional css was used and to take another good look at the complete mail before after hitting send, or letting the user choose to sanitize the conditional css right there.
Any of these options would mean the best experience for the majority of users, while retaining security for those that might be affected by such an attack.