Last Comment Bug 263546 - Security risk: TB uses IE to open javascript pop-up in news-feed items
: Security risk: TB uses IE to open javascript pop-up in news-feed items
Status: RESOLVED FIXED
[sg:nse]
: fixed-aviary1.0, fixed1.7.5
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 Windows 2000
: -- major (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
:
Mentors:
: 234117 260895 (view as bug list)
Depends on:
Blocks: 239965 248511
  Show dependency treegraph
 
Reported: 2004-10-08 15:24 PDT by Tom Braun
Modified: 2005-01-06 11:04 PST (History)
8 users (show)
chofmann: blocking‑aviary1.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Testcase (680 bytes, message/rfc822)
2004-10-09 03:43 PDT, Ben Bucksch (:BenB)
no flags Details
keep javascript and data urls in TB (961 bytes, patch)
2004-10-09 22:50 PDT, Daniel Veditz [:dveditz]
mscott: review+
mscott: superreview+
Details | Diff | Review
Move external warning checks from channel into the service (46.10 KB, patch)
2004-10-15 10:22 PDT, Daniel Veditz [:dveditz]
jst: review+
darin.moz: superreview-
asa: approval‑aviary+
Details | Diff | Review
Address Darin's comments (43.39 KB, patch)
2004-10-17 23:31 PDT, Daniel Veditz [:dveditz]
dveditz: review+
darin.moz: superreview+
dveditz: approval‑aviary+
Details | Diff | Review

Description Tom Braun 2004-10-08 15:24:45 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.7.3) Gecko/20040913 Firefox/0.10.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; rv:1.7.3) Gecko/20040913 Firefox/0.10.1

Hello!

I have submitted a more detailed description of this issue as bug 260895 already.
But somehow, it has remained unconfirmed (and therefore I assume it has not been
looked at) since then.  I think it is a major security risk, that TB chooses to
open anything with IE, even though Firefox is the default browser.  At first
I thought this was specific to RSS feeds, but it is not.  I have taken one of
those links and placed it into an HTML e-mail.  After I receive it, it is the
same effect:  The pop-up is opened in IE.  Here is the text of the e-mail.

- - - - -

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
  <meta content="text/html;charset=ISO-8859-1" http-equiv="Content-Type">
  <title></title>
</head>
<body bgcolor="#ffffff" text="#000000">
<br>
Here is an HTML e-mail with a link:<br>
<a
 href="javascript:commonPopup('printerFriendlyPopup.jhtml?type=scienceNews&storyID=6456643',
540, 525, 1, 'printerPopup')">Test</a><br>

- - - - -

Note that the Reuters page, from which I originally took this link, works
correctly when I open it directly in Firefox.  I can then click on the pop-up
link, and it is all displayed nicely.

But if I get this kind of link in TB, it calls IE, which is a terrible thing!

Interestingly, IE can for some reason not properly display the content, if
it is envoked that way.

Tom



Reproducible: Always
Steps to Reproduce:
Please see bug 260895, or send yourself an HTML e-mail with the above
specified text.


Actual Results:  
IE is used to open the pop-up, when it really should be Firefox.


Expected Results:  
Open pop-up with FIrefox.
Comment 1 Asa Dotzler [:asa] 2004-10-08 15:57:55 PDT
IS is the registered handler for javascript: links. If Firefox is set as your
default browser, we fail to register javascript: links so they remain with IE.
This is bug 241387.

I don't believe that this is a security bug.
Comment 2 Asa Dotzler [:asa] 2004-10-08 15:58:24 PDT
*** Bug 260895 has been marked as a duplicate of this bug. ***
Comment 3 Asa Dotzler [:asa] 2004-10-08 15:59:01 PDT
Sorry, that "IS" was supposed to be "IE", Internet Explorer. 
Comment 4 Ben Bucksch (:BenB) 2004-10-09 02:18:57 PDT
Unsurprisingly, could not reproduce on Linux. MacOS anyone?

Simple HTML not vulnerable.

Severity major (if not critical).

Unhide? The other bugs was (and still is) public for over 2 weeks. And it's
something user can protect themselves against:

Workaround: Do not click on javascript: links (check statusbar before clicking).
Comment 5 Ben Bucksch (:BenB) 2004-10-09 02:20:47 PDT
> Workaround: Do not click on javascript: links (check statusbar before clicking).

or better yet use Simple HTML, of course
Comment 6 Ben Bucksch (:BenB) 2004-10-09 03:41:12 PDT
Confirmed on Windows 2000 with Thunderbird 0.8.
Comment 7 Ben Bucksch (:BenB) 2004-10-09 03:43:56 PDT
Created attachment 161557 [details]
Testcase
Comment 8 Ben Bucksch (:BenB) 2004-10-09 03:52:43 PDT
Sorry, missed that comment:

Asa wrote:
> I don't believe that this is a security bug.

I do. Thunderbird should not invoke MSIE, because otherwise you can use MSIE
exploits in Thunderbird, and the reason for the user to use Thunderbird might be
to avoid exactly that. MSIE should never be invoked from Mozilla programs unless
specifically asked for by the user.

BTW: I don't see a way to make Mozilla Suite the default handler for javascript:
links either.
Comment 9 Ben Bucksch (:BenB) 2004-10-09 11:06:14 PDT
Should be fixed with bug 173010, but it's not, appearantly. Happens with latest
Thunderbird nightly.
Comment 10 Asa Dotzler [:asa] 2004-10-09 11:12:19 PDT
How is launching the default browser for javascript URLs a security
vulnerability. Isn't that simply expected behavior? 
Comment 11 Asa Dotzler [:asa] 2004-10-09 11:15:01 PDT
And I disagree with this being marked security confidential. Bug 241387 explains
this exact same issue and it's not confidential. This isn't an exploit or a
hole. It's simply doing what's expected. Is it a security hole that clicking an
http: URL in an email launches your default browser? If I've got IE set as my
default browser then is it a Thunderbird security hole that when I click on
links in mails it launches IE? What about Firefox as my default browser?
Comment 12 Tom Braun 2004-10-09 12:16:51 PDT
I think the problem is that people who want to become 'IE free'
are installing Firefox and Thunderbird in the hope of doing
just that.  It is a security issue, because it does something
unexpected.  No matter how much of a 'default behaviour' this
might be, after installing Firefox and Thunderbird, most people
will simply not expect IE to be the 'default handler' for those
javascript pop-ups.  As a result, they will not be as diligent
anymore in patching up IE security holes, assuming that they
are not using IE for anything anyway, and there you go.

It's the 'unexpectedness' of his default behaviour, which makes
it risky.  Principle of least surprise, and all that good stuff...

Alternatively, TB and Firefox should be marketed to the public
as 'more secure than IE, and helping people to stay free of
all those IE security issues EXCEPT if they click on javascript
pop-ups in messages', and not the way it is done now.  I mean,
if this is default, then you should also tell people about this
behaviour by default...
Comment 13 Ben Bucksch (:BenB) 2004-10-09 13:07:58 PDT
Right. Apart from all that: If Thunderbird renders a HTML page inline, a
JavaScript URL should be executed within the context of that page, not launch a
browser (where the URL will fail). That is, if JavaScript is enabled at all in
the mail client.
Comment 14 Daniel Veditz [:dveditz] 2004-10-09 22:47:39 PDT
Yes, this is security related, but it does not need to be confidential. Ben's
right, javascript (and data) urls should be handled by Thunderbird itself. The
javascript urls should do nothing unless javascript is enabled. This is what
keeps this bug from being a dupe of 241387.
Comment 15 Daniel Veditz [:dveditz] 2004-10-09 22:50:10 PDT
Created attachment 161646 [details] [diff] [review]
keep javascript and data urls in TB

This solves it for me. You can test this yourself simply by adding the lines in
this patch to the defaults\pref\all-thunderbird.js in your install directory.
Comment 16 Tom Braun 2004-10-10 01:24:22 PDT
Yes, I added that and now the javascript pop-ups don't come up in IE
anymore, which is great!  Maybe something like this could become the
default for the TB installation?  Anyway, the links now don't open at
all, and it appears as if exactly nothing is happening when one clicks
on them.  Is there a way to teach TB to open those things in Firefox?
Comment 17 Ben Bucksch (:BenB) 2004-10-10 02:00:35 PDT
Thanks, dveditz, for the patch.

Tom Braun wrote:
> Maybe something like this could become the default for the TB installation?

The patch does exactly this.

> it appears as if exactly nothing is happening when one clicks on them.

Probably because you have JavaScript disabled in Thunderbird (which is sane and
the default).

> Is there a way to teach TB to open those things in Firefox?

Not that I know of. Firefox needs the page itself as context to be able to
execute the URLs, but the "page" is part of the mail in your case (RSS). Maybe
try rightclicking on the page, select This Frame|Copy Location to Clipboard (not
sure if that option is offered), paste it in Firefox's addressbar, then click on
the link.
Comment 18 Mike Cowperthwaite 2004-10-10 09:43:20 PDT
*** Bug 234117 has been marked as a duplicate of this bug. ***
Comment 19 Mike Cowperthwaite 2004-10-10 09:56:06 PDT
Bug 234117 was about this same basic issue.  Note that it's not necessarily IE 
that gets opened.

This bug looks very similar to bug 252563, which is about *any* link.
Comment 20 Daniel Veditz [:dveditz] 2004-10-10 10:22:23 PDT
I'm not so sure about having tbird handle data: urls. It would be useful to
handle them if they were in-line images, but on links they'll replace the mail
document or even launch an external helper app.

Handling just javascript: would be a more conservative change. Broken data: urls
would be at most a minor bug. On the other hand if tbird had been handling data
urls last week it would have been vulnerable to the security bug 259708 along
with Firefox.
Comment 21 Scott MacGregor 2004-10-10 20:09:47 PDT
Comment on attachment 161646 [details] [diff] [review]
keep javascript and data urls in TB

my gut says we should disable data protocols from being handled internally like
we currently do today. I'd just expose javascript urls FWIW....

Thanks for fixing this Dan.
Comment 22 Scott MacGregor 2004-10-11 15:44:07 PDT
hey Dan, Darin sent some e-mail with some questions about solving the problem
this way. We should look at those before we proceed with this particular fix. 
Comment 23 Daniel Veditz [:dveditz] 2004-10-15 10:22:37 PDT
Created attachment 162217 [details] [diff] [review]
Move external warning checks from channel into the service

Better to move the fix for bug 173010 into the external protocol service
instead of the external protocol handler/channel so that link clicks and other
places that launch external programs don't bypass the mechanism.

Because thunderbird, for one, needs to sometimes handle the same scheme
internally *and* externally (e.g. http) we can't use a single "external" pref
for double-duty to cover the warning. Added an explicit warning set of prefs,
and suppressed warnings for standard types we want Thunderbird and Firefox to
handle transparently.

This requires explicitly whitelisting
Comment 24 Johnny Stenback (:jst, jst@mozilla.com) 2004-10-15 13:28:01 PDT
Comment on attachment 162217 [details] [diff] [review]
Move external warning checks from channel into the service

- In destroyExternalLoadEvent():

+  extLoadRequest* req =
+      NS_STATIC_CAST(extLoadRequest*, PL_GetEventOwner(event));
+  if (req)
+    delete req;

No need to null check there, delete is null safe.

- In nsExternalHelperAppService::LoadURI():

- In nsExternalHelperAppService::promptForScheme():

+  nsresult rv = sbSvc->CreateBundle("chrome://global...,
+			    getter_AddRefs(appstrings));

Fix up next-line indentation.

r=jst
Comment 25 Darin Fisher 2004-10-15 15:32:41 PDT
Comment on attachment 162217 [details] [diff] [review]
Move external warning checks from channel into the service

>Index: calendar/sunbird/app/profile/sunbird.js

> pref("network.protocol-handler.expose-all", true);
> pref("network.protocol-handler.expose.mailto", false);
>+pref("network.protocol-handler.expose.news", false);
>+pref("network.protocol-handler.expose.snews", false);
>+pref("network.protocol-handler.expose.nntp", false);
>+// XXX shouldn't browser schemes be unexposed too?

yes, these prefs are wrong.  expose-all should default to false for
all applications except browsers.  perhaps we should make that the
default in all.js.  in fact, this should be

pref("network.protocol-handler.expose-all", false);
pref("network.protocol-handler.expose.{some-calendar-protocol}", true);

where {some-calendar-protocol} is the protocol scheme of some
URI type that the calendar can render.


>Index: browser/app/profile/firefox.js

> pref("network.protocol-handler.expose-all", true);
> pref("network.protocol-handler.expose.mailto", false);
>+pref("network.protocol-handler.expose.news", false);
>+pref("network.protocol-handler.expose.snews", false);
>+pref("network.protocol-handler.expose.nntp", false);

this is unnecessary, right?  afterall, you can only really
expose a protocol type that is built in.

all this would seem to do is shortcut link clicks to bypass
the default protocol handler and go straight to LoadUrl.


>Index: uriloader/exthandler/nsExternalHelperAppService.cpp

>+struct extLoadRequest {
>+    nsCOMPtr<nsIURI>        uri;
>+    nsCOMPtr<nsIPrompt>     prompt;
>+};

use inheritance so you only have to do one heap allocation when posting
the event:

struct extLoadRequest : PLEvent


then this,

>+static void PR_CALLBACK destroyExternalLoadEvent(PLEvent *event)
>+{
>+  extLoadRequest* req =
>+      NS_STATIC_CAST(extLoadRequest*, PL_GetEventOwner(event));
>+  if (req)
>+    delete req;
>+  delete event;
>+}

becomes:

static void PR_CALLBACK destroyExternalLoadEvent(PLEvent *event)
{
  delete NS_STATIC_CAST(extLoadRequest*, event);
}


>+NS_IMETHODIMP nsExternalHelperAppService::LoadURI(nsIURI * aURL, nsIPrompt * aPrompt)
...
>+  PLEvent *event = new PLEvent;
>+  if (!event)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  extLoadRequest* req = new extLoadRequest;
>+  if (!req)
>+  {
>+    delete event;
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

this also gets simplified.


>+  PRBool allowLoad  = PR_FALSE;
>+  nsresult rv = prefs->GetBoolPref(externalPref.get(), &allowLoad);
>+  if (NS_FAILED(rv))
>+  {
>+    // no scheme-specific value, check the default
>+    rv = prefs->GetBoolPref(kExternalProtocolDefaultPref, &allowLoad);
>+  }
>+  if (NS_FAILED(rv) || !allowLoad)
>+    return PR_FALSE; // explicitly denied or missing default pref

nit: no need to initialize |allowLoad| since you are checking |rv|.


>+  PRBool warn = PR_TRUE;

same here.


>Index: uriloader/exthandler/nsExternalHelperAppService.h

>+  virtual nsresult LoadUriInternal(nsIURI * aURL) = 0;
>+  PRBool isExternalLoadOK(nsIURI* aURI, nsIPrompt* aPrompt);
>+  PRBool promptForScheme(nsIURI* aURI, nsIPrompt* aPrompt, PRBool *aRemember);

nit: when declaring member functions that will not be called from another DSO,
you may want to mark them as hidden to give GCC a hand in optimizing codesize.

  virtual NS_HIDDEN_(nsresult) LoadUriInternal(nsIURI * aURL) = 0;
  NS_HIDDEN_(PRBool) isExternalLoadOK(nsIURI* aURI, nsIPrompt* aPrompt);
  NS_HIDDEN_(PRBool) promptForScheme(nsIURI* aURI, nsIPrompt* aPrompt, PRBool
*aRemember);


>Index: uriloader/exthandler/nsIExternalProtocolService.idl

>-[scriptable, uuid(100FD4B3-4557-11d4-98D0-001083010E9B)]
>+[scriptable, uuid(33d824bc-638a-42ae-9452-19e531147854)]
> interface nsIExternalProtocolService : nsISupports

I thought the plan was to not change this interface on the stable
branches?  Why do we need to expose LoadURI for this patch?

I don't see LoadURI being called from outside this DSO, so perhaps
you could just QI extProtocolService to some private UUID, and have
QueryInterface return a pointer to nsExternalHelperAppService.	Then
make the protocol handler cast the result of QueryInterface back to
nsExternalHelperAppService.  We use this trick in many other places
throughout the tree to safely cast an interface pointer to the
concrete implementation class type.


sr- only because of the interface change.  The rest of my review 
comments are just nits.
Comment 26 Asa Dotzler [:asa] 2004-10-15 21:27:24 PDT
Comment on attachment 162217 [details] [diff] [review]
Move external warning checks from channel into the service

a=asa for aviary checkin.
Comment 27 Daniel Veditz [:dveditz] 2004-10-17 23:31:06 PDT
Created attachment 162443 [details] [diff] [review]
Address Darin's comments
Comment 28 Daniel Veditz [:dveditz] 2004-10-17 23:33:08 PDT
I think I'll skip changes to sunbird.js, filed bug 264831 instead.

If we don't list the mail protocols as exposed=false in firebird.js couldn't we
end up in an XRemote loop if firefox happens to be listed before thunderbird?
The os gives news: to firefox, firefox eventually figures out we don't have a
handler and ships it to the OS, which gives it back because it's exposed.

Fixed the interface issue and most of the nits
Comment 29 Darin Fisher 2004-10-18 20:41:37 PDT
> If we don't list the mail protocols as exposed=false in firebird.js couldn't we
> end up in an XRemote loop if firefox happens to be listed before thunderbird?

hmm... yeah, there could be problems with that.  i think that the xremote code
should probably reject any URL that does not correspond to a built-in protocol
handler.  we may want to have IsExposedProtocol perform that check.

mozTXTToHTMLConv::ShouldLinkify has code to test for built-in protocol handlers.

there is at least one savings grace for xremote, and that is the fact that we
check the application type when processing xremote requests. 
mozilla-xremote-client -a firefox openURL\(http://blah/\) will be ignored by
thunderbird.  the equivalent of the -a argument is enabled whenever the -remote
command line is used with firefox, thunderbird, or seamonkey.

my main concern with listing non-exposed protocols in firefox is that it is not
a very scalable solution.  what about 'webcal' or some other protocol handler
specific to some other mozilla based application?  we can't expect to go and
fixup all the deployed firefox instances, so we need a better solution :-(
Comment 30 Darin Fisher 2004-10-18 20:45:34 PDT
Comment on attachment 162443 [details] [diff] [review]
Address Darin's comments

>+interface nsPIExternalProtocolService : nsIExternalProtocolService

maybe add a big comment warning embedders or extension authors that
this interface is temporary... that it will be merged into
nsIExternalProtocolService in the future.


sr=darin
Comment 31 Daniel Veditz [:dveditz] 2004-10-20 06:33:25 PDT
aviary branch fix checked in
Comment 32 Daniel Veditz [:dveditz] 2004-10-22 12:40:29 PDT
This was checked in the same day to the 1.7-branch. The trunk patch is ready but
waiting for smoketest blockers to clear.
Comment 33 amano 2004-10-24 18:56:29 PDT
This may have caused a regression. See bug 265839 for Details.
Comment 34 Daniel Veditz [:dveditz] 2004-10-25 00:22:42 PDT
Fixed on trunk
Comment 35 Christian :Biesinger (don't email me, ping me on IRC) 2004-10-25 05:10:11 PDT
I had kinda intentionally removed the windowwatcher dependency from
exthandler... oh well :-/

nsIExternalProtocolService.idl
+  Replaces loadUrl()

why keep loadUrl, then? This interface must get a new IID on trunk, btw.
Comment 36 Christian :Biesinger (don't email me, ping me on IRC) 2004-10-29 09:49:51 PDT
> This interface must get a new IID on trunk, btw.

I now made a checkin to update the IID of nsIExternalProtocolService.

Note You need to log in before you can comment on or make changes to this bug.