Closed Bug 263546 Opened 20 years ago Closed 20 years ago

Security risk: TB uses IE to open javascript pop-up in news-feed items

Categories

(Thunderbird :: General, defect)

Product:
Thunderbird
Component:
General
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: tombraun, Assigned: dveditz)

References

Details

(Keywords: fixed-aviary1.0, fixed1.7.5, Whiteboard: [sg:nse])

Attachments

(2 files, 2 obsolete files)

Testcase
(deleted), message/rfc822
Details
Address Darin's comments
(deleted), patch
dveditz
: review+
darin.moz
: superreview+
dveditz
: approval-aviary+
Details | Diff | Splinter Review 
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.
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.
*** Bug 260895 has been marked as a duplicate of this bug. ***
Sorry, that "IS" was supposed to be "IE", Internet Explorer. 
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).
Severity: normal → major
Flags: blocking-aviary1.0?
> Workaround: Do not click on javascript: links (check statusbar before clicking).

or better yet use Simple HTML, of course
Confirmed on Windows 2000 with Thunderbird 0.8.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached file Testcase (deleted) — 

    
    

    
  
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.

    
    

    
  
Should be fixed with bug 173010, but it's not, appearantly. Happens with latest
Thunderbird nightly.

    
    

    
  
How is launching the default browser for javascript URLs a security
vulnerability. Isn't that simply expected behavior? 

    
    

    
  
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?

    
    

    
  
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...


    
    

    
  
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.

    
    

    
  
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.
Assignee: mscott → dveditz
Group: security
Whiteboard: [sg:nse]

    
    

    
  

      
      
      
      

        Attached patch
          
          
        keep javascript and data urls in TB (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
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.

    
  

        Attachment #161646 -
        Flags: superreview?(mscott)

        Attachment #161646 -
        Flags: review?(mscott)

    
    

    
  
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?


    
    

    
  
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.

    
    

    
  
*** Bug 234117 has been marked as a duplicate of this bug. ***

    
    

    
  
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.

    
    

    
  
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.
Status: NEW → ASSIGNED

    
    

    
  
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.

        Attachment #161646 -
        Flags: superreview?(mscott)

        Attachment #161646 -
        Flags: superreview+

        Attachment #161646 -
        Flags: review?(mscott)

        Attachment #161646 -
        Flags: review+

    
    

    
  
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. 

    
  
Flags: blocking-aviary1.0? → blocking-aviary1.0+

    
  
Blocks: 239965

    
    

    
  


  
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

        Attachment #161646 -
        Attachment is obsolete: true

    
  

        Attachment #162217 -
        Flags: superreview?(darin)

        Attachment #162217 -
        Flags: review?(jst)

        Attachment #162217 -
        Flags: approval-aviary?

    
    

    
  
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

        Attachment #162217 -
        Flags: review?(jst) → review+

    
  
Whiteboard: [sg:nse] → [sg:nse] [have patch] need review darin

    
    

    
  
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.

        Attachment #162217 -
        Flags: superreview?(darin) → superreview-

    
    

    
  
Comment on attachment 162217 [details] [diff] [review]
Move external warning checks from channel into the service

a=asa for aviary checkin.

        Attachment #162217 -
        Flags: approval-aviary? → approval-aviary+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Address Darin's comments (deleted)
        — 
          — Splinter Review
      

    


  

        Attachment #162217 -
        Attachment is obsolete: true

    
    

    
  
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

    
  

        Attachment #162443 -
        Flags: superreview?(darin)

        Attachment #162443 -
        Flags: review+

        Attachment #162443 -
        Flags: approval-aviary+

    
  
Whiteboard: [sg:nse] [have patch] need review darin → [sg:nse] [have patch] need sr= darin

    
    

    
  
> 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 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

        Attachment #162443 -
        Flags: superreview?(darin) → superreview+

    
    

    
  
aviary branch fix checked in
Keywords: fixed-aviary1.0
Whiteboard: [sg:nse] [have patch] need sr= darin → [sg:nse] [have patch] need landing

    
    

    
  
This was checked in the same day to the 1.7-branch. The trunk patch is ready but
waiting for smoketest blockers to clear.
Keywords: fixed1.7.x
Whiteboard: [sg:nse] [have patch] need landing → [sg:nse] need trunk landing

    
    

    
  
This may have caused a regression. See bug 265839 for Details.

    
    

    
  
Fixed on trunk
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: [sg:nse] need trunk landing → [sg:nse]

    
    

    
  
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.

    
    

    
  
> This interface must get a new IID on trunk, btw.

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

    
  
Blocks: 248511

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: