1411732 - (CVE-2018-5170) TBE-01-009: Filename Spoofing for external Attachments

Status: RESOLVED FIXED

(MailNews Core :: Attachments, defect)

Description

[Ben Bucksch (:BenB)]

Thunderbird implements external attachments via the X-Mozilla-External- Attachment-URL. The actual resource is specified via this header. It was discovered that the GUI displays the filename outlined in the “Content-Type” header, which is not related to the real resource. This can be abused to trick the user into opening an attachment, believing that this item is a safe resource like an image, HTML file or similar. Conversely, as illustrated next, Thunderbird would fetch a completely different file

--------------A320A96F6639F3C578F35383
Content-Type: message/external-body; access-type=whatever;
name="NameWhichIsShownInTheGui.html"
X-Mozilla-External-Attachment-URL: data:application/pdf,aaaaaaaaaaaaaaaaaaaaa
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="thisigui3.html"
Content-Transfer-Encoding: binary
Cure53, Berlin · 10/11/17
[Ghost Body1234]<h1>asdf</h1><script>alert(1)</script>

One solution to this problem would be to display a warning box to the user before fetching the external resource, making user clearly aware of the potential risks. Another approach could be to extract the filename via the X-Mozilla-External-Attachment-URL instead of the Content-Type header.

For the original report as PDF; see bug 1411701.

Comment 1

[alta88]

Bug 1411716 disallows the feed parser from creating X-Mozilla-External-Attachment-URI values that are not http* (the only other use case for creating such a mimepart with that header is internal detachment of mail attachments, to file:// urls).

Bug 1345167 enables the url to be viewable in statusbar on mouseover, like any other protocol link.

Comment 2

[Patrick Brunschwig]

Cure53 tested the fix and reported the following back:

The GUI still displays the filename that is outlined in the content-type header. Considering unfixed.

Comment 3

[alta88]

For the record, it is completely legitimate to have a display name. This is no different from a web page text link where the text could be some spoofy wording to evilsite.com url. Since only http links are accepted now, a click will open the url in a default browser whose infrastructure for handling malicious sites takes over.

The enhancement needed is to enable the user to at least see the url link, like in Fx, as per comment 1.

Comment 4

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Note this specific bug will be a non-issue with the patch in bug 1411748. The better solution is likely bug 1345167, but the former is less risky to uplift.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

http links are being disabled for the time being until we can fix bug 1345167. With that, the risk is pretty low for this bug and the testcase above would not work.

What would still work is an external file: url, I have a patch that prefers the leafName of the file in that case which is low risk and easy to uplift.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v1

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 8950114 [details] [diff] [review]
Fix - v1

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

::: mail/base/content/msgHdrViewOverlay.js
@@ +1841,5 @@
> +      let file = uri.QueryInterface(Components.interfaces.nsIFileURL).file;
> +      this.name = file.leafName;
> +    } else {
> +      // best approximation otherwise
> +      match = url.match(/(?:\/|^)([^\/]+)$/);

not sure I get what this is about.
what's the ?:\/ matching?

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Magnus Melin from comment #7)
> > +      match = url.match(/(?:\/|^)([^\/]+)$/);
> 
> not sure I get what this is about.
> what's the ?:\/ matching?

?: means it shouldn't be a capture group. The intent is to get the stuff after the last slash. Come to think of it, this might have to be a bit more complicated to catch possible query refs, hashmarks, etc. Right now this won't be exposed since I removed external urls in the other patch, so this is somewhat untested.

Comment 9

[Magnus Melin [:mkmelin]]

Thought some more on this, and I'm coming back to comment 3. 

I don't really see the (solvable) problem here. It doesn't really matter what filename is displayed, since nothing prevents the evil site from redirecting to something else in the end. Not much we can do to prevent that, it's all just a try at social engineering. 

The same solution that's proposed for bug 1411748 would mitigate it - always launch the browser for external http attachments.

Or is there something I'm not seeing here? Opinions?

Comment 10

[Magnus Melin [:mkmelin]]

Opinions on wontfix? See comment 9.

Comment 11

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I think we should take this fix as a mitigation short term, then we can consider other options like bug 1411748 going forward.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 8950114 [details] [diff] [review]
Fix - v1

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

Like I wrote in comment 9, I don't see what this is fixing, so minusing for now.

Comment 13

[Jorg K (CEST = GMT+2)]

As per comment #3, this would be fixed by bug 1345167 since the spoof would be mitigated when the url is revealed/visible.

Comment 14

[Magnus Melin [:mkmelin]]

Yes, the most important thing to communicate is that it's not a local attachment. We can't know anything about the real content that url is going to lead to.

Comment 15

[Ben Bucksch (:BenB)]

> Yes, the most important thing to communicate is that it's not a local attachment.

I concur.

Comment 16

[Magnus Melin [:mkmelin]]

Attachment: bug1411732_testcase_filename.eml - testcase

Comment 17

[Magnus Melin [:mkmelin]]

Attachment: bug1411732_attachment_fn_spoof.patch

In the case there are any remote external attachments in mails, make the name of the attachment be of the format "URL - name". Don't do it for feeds, where it's used for enclosures, but those are always remote.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8970666 [details] [diff] [review]
bug1411732_attachment_fn_spoof.patch

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

::: mail/base/content/msgHdrView.js
@@ +1825,5 @@
>      match = GlodaUtils.PART_RE.exec(url);
>      this.partID = match && match[1];
>    }
>  
> +  // Make sure to communicate is external http an attachment is not a local

// Make sure to communicate *if* ?

Comment 19

[Magnus Melin [:mkmelin]]

Attachment: bug1411732_attachment_fn_spoof.patch

Improved + fixed the text

Comment 20

[Jorg K (CEST = GMT+2)]

I'm quite confused by this. You used an attachment of bug 1342838 as an example here.

Where is the modified name
  this.name = url + " - " + this.name;
displayed in the UI? I see no difference with the patch. Despite having four attachments, one "inline", the attachment area claims to have three. I see the original filenames displayed:
externalbody-attachment.html
thisigui3.html
LA-MAR_Cebiche-Clássico_foto-Henrique-Peron-470x120-1371827671.jpg
OK, if I switch "Display Attachment Inline" off, I get four attachments.

When I double-click thisigui3.html, it tries to open http://example.com/abc.doc in the preview pane.

Comment 21

[Magnus Melin [:mkmelin]]

Yeah, that just happened to be handy.

You should see the modified format for all the attachments. In the attachments area, once you click open the "3 attachments". Are you sure you had the patch successfully applied? At least it works for me.

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: attachment-names.png

No difference, sorry.

Comment 23

[Jorg K (CEST = GMT+2)]

Actually, it works for an IMAP folder, but not a local folder.

Comment 24

[Jorg K (CEST = GMT+2)]

Looks like the message is treated as feed message in a local folder. The behaviour changes when you remove 0x40 (FeedMsg) from the X-Mozilla-Status.

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 8970662 [details]
bug1411732_testcase_filename.eml - testcase

Based on your comments, I assume it's all good then

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8970662 [details]
bug1411732_testcase_filename.eml - testcase

... unless the attacker were to spoof that header as well.

Comment 27

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/759b892f48b80de23a4d5a80c959f72b9c25f0ad
prevent file name tricks for external http attachments. r=jorgk

Comment 28

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 6):
https://hg.mozilla.org/releases/comm-beta/rev/700521028a064b3a6ff44c16581b95087542dad0

Comment 29

[Jorg K (CEST = GMT+2)]

TB 52.8 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/d81c085727503d7ea48b9bda88cfb42dc3013552