Closed Bug 1411748 Opened 7 years ago Closed 5 years ago

TBE-01-007: "Reload Page" dialog runs Javascript with external attachment because we only disable JavaScript for nsIMsgMessageUrls

Categories

(MailNews Core :: Attachments, defect)

Product:
MailNews Core
Component:
Attachments
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0

People

(Reporter: BenB, Assigned: alta88)

Details

(Keywords: privacy, sec-low, testcase)

Attachments

(2 files, 8 obsolete files)

bug1411748_External_attachment.eml (test case)
852 bytes, text/plain
Details
reloadMessage.patch
2.49 KB, patch
alta88
: review+
Details | Diff | Splinter Review 
Thunderbird supports rendering of HTML tags inside an email body but it disables the support to execute JavaScript. It was discovered possible to enable JavaScript execution inside the email body by exploiting an issue with external attachments and “Reload Page” functionality.

First, the user needs to click on “Save” on an external message body of a received email. The specified HTTP resource redirects to file://smb/share URI. This will display the “Corrupted Content Error” page in Thunderbird, in essence letting a user reload the HTTP resource. In case the user decides to reload the page, the HTTP resource can return a normal HTML page, meaning one that can execute JavaScript.

Steps to reproduce (tested on Windows 7):
1. Load external_attachment.eml in Thunderbird.
2. Right click on “NameWhichIsShownInTheGui.html” and opt for “Save”.
3. Thunderbird will fetch http://example.com/redir.php and the attacker’s server answers with the following HTTP response:

GET http://example.com/redir.php HTTP/1.1
[...]
HTTP/1.1 302 Found
Date: Tue, 19 Sep 2017 20:46:23 GMT
Server: Apache/2.4.18 (Ubuntu)
Location: file://example.com/thunderbird/file

4. The “Corrupted Content Error” page is shown in Thunderbird.
5. Click on “Try Again”.
6. Thunderbird will fetch http://example.com/redir.php again and the attacker’s
server answers with the following HTTP response:
GET http://example.com/redir.php HTTP/1.1
[...]
HTTP/1.1 200 OK
Content-Type: text/html; charset=UTF-8
<!DOCTYPE html>
<h1>123</h1>
<script> alert(1); </script>
7. The page will be displayed in Thunderbird and JavaScript is executed.

File:
External_attachment.eml

Code:
Content-Type: multipart/alternative; boundary="------------
2DEE3F98D70BD2C65FBA7373"
MIME-Version: 1.0
Subject: Link
From: email@email.com
To: email@email.com
--------------2DEE3F98D70BD2C65FBA7373
Content-Type: multipart/related; boundary="------------A320A96F6639F3C578F35383"
--------------A320A96F6639F3C578F35383
Content-Type: text/html
Content-Transfer-Encoding: 7Bit
<!DOCTYPE html>
<body>
<h1>dummy body</h1>
--------------A320A96F6639F3C578F35383
Content-Type: message/external-body; access-type=whatever;
name="NameWhichIsShownInTheGui.html"
X-Mozilla-External-Attachment-URL: http://example.com/redir.php
Content-Transfer-Encoding: 7bit
Content-Disposition: inline; filename="thisigui3.html"
Content-Transfer-Encoding: binary
Cure53, Berlin · 10/11/17
24/35Dr.-Ing. Mario Heiderich, Cure53
Bielefelder Str. 14
D 10709 Berlin
cure53.de · mario@cure53.de
[Ghost Body1234]
--------------A320A96F6639F3C578F35383--
--------------2DEE3F98D70BD2C65FBA7373--

A different handling should concern HTTP/HTTPS URLs specified in an external attachment URL. Specifically, when HTTP/HTTPS URLs are being opened, Thunderbird should utilize the default web browser without fetching the resource beforehand. Another approach could be to either disable or modify the “Corrupted Content Error” page, so as to disallow the reloading of the corrupted HTTP resource.

For the original report as PDF; see bug 1411701.
Which security context does the javascript run from? If it's normal content principal, like webpages, I don't think this is something to worry about.

If the Javascript runs with chrome privileges, this should be fixed.
Even then, this needs a lot of very unlikely user interaction.
Can somebody reproduce this and check which security context this runs in?

Or just fix it?
Group: mail-core-security
Why is the "reload" enabling javascript? If the user is clicking "Save" why are we displaying it at all?

If we are going to display it presumably we're limited to a web context and the origin is the remote origin, but two bad outcomes which are possible are 1) it's a chrome context (unlikely) or 2) it's the mailbox: context which raises the possibility of the attachment being able to read other mail.

Why are we fetching remote content without making it clear to users we're fetching remote content? This looks like an "attachment" to users which they think are local things sent to their mail server, not ratting them out about exactly when they opened it.
Group: mail-core-security
Flags: needinfo?(ben.bucksch)
Keywords: privacy
Summary: "Reload Page" dialog runs Javascript → TBE-01-007: "Reload Page" dialog runs Javascript
I can reproduce and check this. 
JavaScript is run because, we only disable JavaScript for nsIMsgMessageUrls, and this is displaying a remote page. It's limited to the web context and has the remote origin (window.location.origin says localhost when I'm testing a local page). 

(In reply to Daniel Veditz [:dveditz] from comment #3)
> Why is the "reload" enabling javascript? If the user is clicking "Save" why
> are we displaying it at all?

Since getting the file to save fails, we end up with the error page shown. When the "Try again" button is used, the content is loaded, but at that point that we were really saving and not trying to view the page is forgotten.

> Why are we fetching remote content without making it clear to users we're
> fetching remote content? This looks like an "attachment" to users which they
> think are local things sent to their mail server, not ratting them out about
> exactly when they opened it.

Yes there is room for improvements here.
clearing NI, answered by magnus
Flags: needinfo?(ben.bucksch)
Keywords: testcase
Summary: TBE-01-007: "Reload Page" dialog runs Javascript → TBE-01-007: "Reload Page" dialog runs Javascript with external attachment because we only disable JavaScript for nsIMsgMessageUrls
Attachment #8927019 - Attachment mime type: message/rfc822 → text/plain
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attached patch Fix - v1 (obsolete) — — Splinter Review 
I was only able to reproduce this with content disposition attachment. What I think we should go ahead and do is hide external attachments that are not file:// attachments for now.

It seems to me that bug 1345167 might allow us to restore this functionality as it would display external links differently, but I would be inclined to remove the feature to have remote external attachments, since this sounds to me like an under-tested feature that has a lot of attack surface.
Attachment #8950112 - Flags: review?(mkmelin+mozilla)
I have to say, I'm not sure. Basically this bug is just about bad UI as a result of an unexpected response. Disabling external attachments (feed enclosures) seems like an overkill.
Of course I don't have telemetry to back it up, but allowing this via http seems to be a rather obscure feature to me, and the way we present it in the UI is also not very great (to be fixed in bug 1345167). As much as I hate quick fixes, my point with this bug was to provide a patch that serves as a quick fix that we could backport, possibly down to 52.

Maybe I am unaware of what else external attachments are really used for, do you know? My understanding was that this is for detached attachments, which will always be behind a file:// url. If there is no use case for remote external attachments, and no way to create them without forging a message, I think we should remove the feature and any code associated with it.
Flags: needinfo?(mkmelin+mozilla)
I believe the primary use case is for enclosures in feeds (kind of like attachments, media for the article), so you will certainly find valid cases where it's used. Podcasts an such. Of course, it can be debated how important it is.

While this bug is about a kind of odd behavior, it's a bit questionable if there's anything really wrong, apart from that a failed save-as shouldn't load the "failed" page in the UI, avoiding the whole situation.

I wonder if a reasonable thing would be to, at least for 52, just launch the browser for http external attachments - both for save and open.
Flags: needinfo?(mkmelin+mozilla)
(In reply to Magnus Melin from comment #10)
> I believe the primary use case is for enclosures in feeds (kind of like
> attachments, media for the article), so you will certainly find valid cases
> where it's used. Podcasts an such. Of course, it can be debated how
> important it is.
What kind of feeds use this? How common is this? If this is optimizing for an edge case I think we should rather remove the feature.

> 
> While this bug is about a kind of odd behavior, it's a bit questionable if
> there's anything really wrong, apart from that a failed save-as shouldn't
> load the "failed" page in the UI, avoiding the whole situation.
We could also set the docShell useErrorPages = false. I tried that and it showed an ugly confirm alert we'd have to see if we can get rid of.


> 
> I wonder if a reasonable thing would be to, at least for 52, just launch the
> browser for http external attachments - both for save and open.
I think this would be reasonable even past 52. 

Let me know what approach you prefer.
Flags: needinfo?(mkmelin+mozilla)
(In reply to Philipp Kewisch [:Fallen]  from comment #11)
> What kind of feeds use this? How common is this? If this is optimizing for
> an edge case I think we should rather remove the feature.

E.g. http://feeds.wnyc.org/radiolab
I'd say podcasts are fairly common in general, but I have no feel for how much they are used in Thunderbird.

> Let me know what approach you prefer.

Either way is ok for me.
Flags: needinfo?(mkmelin+mozilla)
So I checked, useErrorPages = false shows an ugly modal alert on each failure, which can get annoying when accidentally loading feed while offline. Looking at the code, there seems no way to prevent the confirm dialog: https://searchfox.org/comm-central/source/mozilla/docshell/base/nsDocShell.cpp#4924

In that case I'd rather go for removing this feature. I'm sure there are valid use cases, but as this is not supported by other major email clients I don't think there is a lot of motivation for publishers to use this feature.

Magnus, if you agree, please continue with the review at your convenience.
Flags: needinfo?(mkmelin+mozilla)
review ping on this, can we agree to move forward in disabling external attachments? Given this is an edge case feature not supported by other major clients I think it should be ok.
Hmm, Magnus just landed a patch in bug 1411732, so I see no move to disabling external attachments.
Keywords: sec-low
Attachment #8950112 - Attachment is obsolete: true
Flags: needinfo?(mkmelin+mozilla)
Attachment #8950112 - Flags: review?(mkmelin+mozilla)

I believe alta88 is on this.

AFAICT, bug 1345167 doesn't fix this per se since while that checks the remote file (using HEAD) there's nothing preventing the next call to that URL to lead somewhere else. Or that HEAD/GET would give different results.

But I think it could fairly easily be modified to do so. We need to do the fetch before accessing, then make sure to access the result we already have.

Assignee: philipp → alta88
Status: ASSIGNED → NEW
Attachment #9055378 - Flags: review?(mkmelin+mozilla)

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

(In reply to alta88 from comment #23)

You're missing that the response.url will be the redirect url, and will
differ from request.url. For fetch() it is one call only but 2 calls in
gecko, which you can even see in the console with xhr and request toggled
on. There is one response to fetch() which is for the redirect url. I don't
know what 'response can vary between calls' means, it doesn't work that way.

It means that the attacker can have call #1 return A and call #2 return B even if the url is the same. It also means he can set HEAD to respond C and GET to respond D.

I see finally what you're describing, a timing attack where a successful HEAD response is turned into a malicious response on the open/GET to the same url. And for such an attack, doing a GET instead of HEAD doesn't necessarily prevent that either, so testing fetch() etc etc isn't the answer.

nsMessenger::OpenURL() loads the url in messagepane docShell so a content handler can be invoked on it. The url could be sent to a browser via nsIExternalProtocolService/nsIWebNavigation loadURI() but this would be a very degraded UX.

How about an override of netError.xhtml in aboutRedirector.js and simply removing the retry button or testing the errors in a netError.js stub and hiding it for certain conditions.

Edit: Btw, sorry for being slow to get this, I was somehow fixated on a specific flow..

Attachment #9055489 - Attachment is obsolete: true
Attachment #9055509 - Attachment is obsolete: true

Ok, so putting up a redirect server, pointing it to a file:// redirect url and using the test eml above, will make fetch() get a 200 response for HEAD (and continue to the corrupt page as a result of OpenURL()) but will catch with a Network Error with GET.

I guess overriding netError could work, maybe to make Retry open the URL open in the external browser for error conditions?

Another possible route would be doing the fetch, putting the verified ok'd data into a data: url and have sMessenger::OpenURL open that.

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

I guess overriding netError could work, maybe to make Retry open the URL open in the external browser for error conditions?

I think it's not great UX to load the error page and leave the message; there's then no way to get back without deselect/select. If the error occurs in the first place, it won't work in the browser either. If we go this route, I'd say the button should say Return to Message or such.

Another possible route would be doing the fetch, putting the verified ok'd data into a data: url and have sMessenger::OpenURL open that.

Even if the link is to a 100M video (like the now obsolete testcase I attached before)?

If we replace HEAD with GET, a redirect to both an existing and non existing local file fails with Network Error, and there will be no open and no error page. Do we agree that fixes the case here?

In the case of a long server non response (fetch eventually times out) I made it so if the message has changed (likely) the response will just die. And if not, it will be a caught error.

What potential issues remain?

(In reply to alta88 from comment #31)

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

I guess overriding netError could work, maybe to make Retry open the URL open in the external browser for error conditions?

I think it's not great UX to load the error page and leave the message;
there's then no way to get back without deselect/select. If the error occurs
in the first place, it won't work in the browser either. If we go this
route, I'd say the button should say Return to Message or such.

Sounds ok, or just disable the button.

Another possible route would be doing the fetch, putting the verified ok'd data into a data: url and have sMessenger::OpenURL open that.

Even if the link is to a 100M video (like the now obsolete testcase I
attached before)?

Might not be so nice, no.

If we replace HEAD with GET, a redirect to both an existing and non existing
local file fails with Network Error, and there will be no open and no error
page. Do we agree that fixes the case here?

I'm not sure I follow. If you're replacing HEAD with GET you do need to use the result of the GET. But, perhaps we won't take this route. I'm open to suggestions.

What potential issues remain?

Can't think of any.

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

(In reply to alta88 from comment #31)

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

I guess overriding netError could work, maybe to make Retry open the URL open in the external browser for error conditions?

I think it's not great UX to load the error page and leave the message;
there's then no way to get back without deselect/select. If the error occurs
in the first place, it won't work in the browser either. If we go this
route, I'd say the button should say Return to Message or such.

Sounds ok, or just disable the button.

Another possible route would be doing the fetch, putting the verified ok'd data into a data: url and have sMessenger::OpenURL open that.

Even if the link is to a 100M video (like the now obsolete testcase I
attached before)?

Might not be so nice, no.

If we replace HEAD with GET, a redirect to both an existing and non existing
local file fails with Network Error, and there will be no open and no error
page. Do we agree that fixes the case here?

I'm not sure I follow. If you're replacing HEAD with GET you do need to use the result of the GET. But, perhaps we won't take this route. I'm open to suggestions.

In the case here, with a redirect to file, the GET fetch() will fail and the attack cannot proceed.

Hypothetically, the fetch() (first GET) has to succeed, and then the url response changed for the open (second GET) to show the error page. But how can the attacker know there's a second GET coming. This seems implausible.

Anyway, I suggest we change it to GET (http only) for the first case (this bug), and change netError to return to message for the second case.

What potential issues remain?

Can't think of any.

But how can the attacker know there's a second GET coming. This seems implausible.

If there is just a distinction between data from HEAD vs GET that would be easy. For GET+GET, I suppose one could match up on remote IP and take an educated guess... but sure, harder.

Anyway, I suggest we change it to GET (http only) for the first case (this bug), and change netError to > return to message for the second case.

Sounds good.

Attached patch reloadMessage on netError button (obsolete) — — Splinter Review

Yes, it's sure possible to trick even 2 GETs: read this bug, user agent, ip, second GET within a second..

The button should Return to Messge in general, as Retry usually fails, and the UX is nicer now for that page.

I don't think we want to clone netError.xhtml and netError.js, so did it this way.

Attachment #9056000 - Flags: review?(mkmelin+mozilla)
Attachment #9056000 - Attachment is patch: true
Comment on attachment 9056000 [details] [diff] [review]
reloadMessage on netError button

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

::: mail/locales/en-US/chrome/overrides/netError.dtd
@@ +5,5 @@
>  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd">
>  %brandDTD;
>  
>  <!ENTITY loadError.label "Problem loading page">
> +<!ENTITY retry.label "Return to Message">

Since you'd need to bump the l10n key, it would seem easier to just add a new button dynamically and hide the original retry button.

Or, we wouldn't change the string but only swap the click handling to do the Reload instead. Perhaps that's preferable.
Attachment #9056000 - Flags: review?(mkmelin+mozilla)
Attached patch reloadMessage.patch (obsolete) — — Splinter Review

no string change.

Attachment #9056000 - Attachment is obsolete: true
Attachment #9056130 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9056130 [details] [diff] [review]
reloadMessage.patch

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

::: mail/base/content/mailWindow.js
@@ +214,5 @@
> +  messagepane.addEventListener("DOMContentLoaded", (event) => {
> +    if (!event.target.documentURI.startsWith("about:neterror?")) {
> +      return;
> +    }
> +    let button = event.target.getElementById("errorTryAgain");

Could you add an explanation in the code for this. It's not exactly clear to someone unfamilar with this bug why we would change the event handler.

::: mail/base/content/msgHdrView.js
@@ +1871,5 @@
>  
>      let url = this.url;
>      let empty = true;
>      let size = 0;
> +    let options = { method: "GET" };

I think this should stay HEAD, or otherwise we could fetch a large file more than once (uncless cache kicks in). 

When using HEAD, and then a GET that fails, you still get the neterror page and can reload the message so it seem to me this is all fixed even when this stays as HEAD
Attachment #9056130 - Flags: review?(mkmelin+mozilla) → review+
Status: NEW → ASSIGNED
Attached patch reloadMessage.patch (obsolete) — — Splinter Review
Attachment #9056130 - Attachment is obsolete: true
Attachment #9056540 - Flags: review+
Keywords: checkin-needed

Sorry, this doesn't pass linting:
$ ../mach eslint mail/base/content/
c:\mozilla-source\comm-central\comm\mail\base\content\mailWindow.js
219:54 error 'retryThis' is not defined. no-undef (eslint)
? 1 problem (1 error, 0 warnings)

... and I wouldn't know how to fix it. This seems to come from netError.js.

Keywords: checkin-needed

/* global retryThis */ on top of the file should do it. But perhaps we don't need the retryThis at all, since removing the eventListener doesn't look at the actual function.

declare global.

Attachment #9056540 - Attachment is obsolete: true
Attachment #9056657 - Flags: review+
Flags: needinfo?(jorgk)
Keywords: checkin-needed

https://hg.mozilla.org/comm-central/rev/5e3c486189d48c8cfa6084be270236cd807c8205
On netError page change retry button to reload message. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(jorgk)
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0
Group: mail-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: