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
680 bytes, message/rfc822
Details
Address Darin's comments
43.39 KB, 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 —
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]
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+
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: