Last Comment Bug 384384 - (IDEF2595) Multiple Vendor Multiple Browser Interaction URI Handler Command Injection Vulnerability
(IDEF2595)
: Multiple Vendor Multiple Browser Interaction URI Handler Command Injection Vu...
Status: RESOLVED FIXED
[sg:critical] (aka SA25984)
: verified1.8.1.5
Product: Toolkit
Classification: Components
Component: Startup and Profile System (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Robert Strong [:rstrong] (use needinfo to contact me)
:
Mentors:
http://larholm.com/2007/07/10/interne...
Depends on:
Blocks: 389257 389613 418150
  Show dependency treegraph
 
Reported: 2007-06-13 20:13 PDT by Daniel Veditz [:dveditz]
Modified: 2012-01-12 08:53 PST (History)
36 users (show)
dveditz: blocking1.9?
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
dveditz: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
PoC (open in IE with FF not running) (421 bytes, text/html)
2007-06-14 14:33 PDT, Daniel Veditz [:dveditz]
no flags Details
IE's http regkey (2.38 KB, text/plain)
2007-06-14 16:09 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details
patch (1.42 KB, patch)
2007-06-20 00:55 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
patch - use a regexp (1.37 KB, patch)
2007-06-20 01:35 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
Billy Rios testcase (2.28 KB, text/html)
2007-06-20 13:35 PDT, Daniel Veditz [:dveditz]
no flags Details
patch rev 3 (11.64 KB, patch)
2007-06-21 10:57 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
Extended Billy Rios testcase (command execution) (3.37 KB, text/html)
2007-06-21 17:47 PDT, Daniel Veditz [:dveditz]
no flags Details
branch patch ver 4 (19.16 KB, patch)
2007-06-27 13:33 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
branch patch rev 5 (fixed a typo) (19.16 KB, patch)
2007-06-27 14:41 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
trunk patch rev 4 (20.92 KB, patch)
2007-06-27 22:07 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
trunk patch rev 5 (comment fix) (20.93 KB, patch)
2007-06-27 22:11 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
patch rev 6 (diff from trunk) (23.19 KB, patch)
2007-06-27 22:50 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
Thunderbird patch in progress (12.59 KB, patch)
2007-06-27 23:22 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
patch rev 6 (diff from branch) (26.26 KB, patch)
2007-06-28 12:54 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
benjamin: review-
Details | Diff | Splinter Review
patch rev 7 (diff from branch) (29.74 KB, patch)
2007-07-05 12:29 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
patch rev 7 (diff from branch) (30.03 KB, patch)
2007-07-05 12:57 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
benjamin: review+
Details | Diff | Splinter Review
Thunderbird patch (12.69 KB, patch)
2007-07-10 16:00 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
mscott: review+
Details | Diff | Splinter Review
Branch as checked in (30.03 KB, patch)
2007-07-10 23:14 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review
Trunk as checked in (30.01 KB, patch)
2007-07-10 23:14 PDT, Robert Strong [:rstrong] (use needinfo to contact me)
no flags Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2007-06-13 20:13:35 PDT
iDefense reports a security problem that arises from a combination of IE7 and Firefox 2. The attached PoC was unreadable (working on getting a copy) but the description sounds like Thor Larholm's Safari-for-Windows exploit blogged at http://larholm.com/2007/06/12/safari-for-windows-0day-exploit-in-2-hours/ and is presumably similar.

Excerpt:
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
II. DESCRIPTION

Remote exploitation of an input handling vulnerability in the handling
of external application URIs by multiple browsers could allow remote
code execution as the local user.

A remote attacker can cause an Internet Explorer user, with Mozilla
Firefox installed, to execute arbitrary commands. By creating an iFrame
with a specially constructed link, attackers may execute arbitrary
programs and files may be downloaded, uploaded, or modified as the
current user.

This vulnerability is due to the interaction between programs. Internet
Explorer does not escape double-quote characters in some URLs. This can
allow a specially constructed link to be interpreted by Firefox as
multiple arguments.

III. ANALYSIS

Exploitation of this vulnerability allows an attacker to execute
arbitrary commands as the current user. To exploit this vulnerability,
the attacker must create a website containing a maliciously constructed
link. The victim must have Firefox installed, but not currently running.
When Internet Explorer requests a frame with a 'firefoxurl:' URI,
Internet Explorer will launch Firefox with the arguments supplied from
the SRC (source) parameter.

When opening a URL, Firefox is started with the following command line:

[path/to/firefox.exe] -url "%1" -requestPending

The "%1" is replaced with the source URL. If the URL contains a
double-quote character followed by a space, the quoting will be closed,
and the rest of the source URL will be treated as new arguments. By
supplying the '-chrome' (user interface) option, followed by JavaScript
that calls XPCOM interface functions, an attacker can execute arbitrary
commands.

XPCOM (Cross Platform Component Object Model) is the framework used by
Mozilla-based products that allow the same base code to be used for
multiple applications on multiple platforms. The XPCOM user interface
code for Firefox accesses the underlying code on the system, providing
a standardized interface across operating systems. Code interacting
with the browser at this level can perform any operation the user
running the browser can.

This vulnerability is related to a similar vulnerability in the Beta
version of Apple's Safari Web browser for Windows.

In Vista with IE 7 in Protected Mode, which is the default setting, a
dialog will ask you to confirm that you want to run Firefox.

IV. DETECTION

iDefense has confirmed the existence of this vulnerability to be
exploitable with Firefox version 2.0.0.4 from Internet Explorer 7 on
Windows XPSP2. Previous versions may also be affected.

While this vulnerability is due to Internet Explorer not escaping
double-quotes in the URL passed to external programs, the actual code
execution comes from Firefox. The interaction between the two programs
is the source of this vulnerability.

V. WORKAROUND

iDefense is currently unaware of any workaround for this issue, other
than keeping Firefox running while browsing the Web with Internet
Explorer. iDefense Labs is researching other potential workarounds and
will update this report should any become available.

[...]

IX. CREDIT

This vulnerability was discovered by Greg MacManus (iDefense Labs).
Comment 1 Daniel Veditz [:dveditz] 2007-06-13 20:20:42 PDT
As mentioned I haven't see the PoC, but assuming it's like Larholm's I have to wonder why we allow javascript: urls with the -chrome command-line argument, or really anything but an actual chrome: url.

But we shouldn't focus on -chrome alone, there could be other problematic command-line arguments now or in the future we should protect. Perhaps require -url to be the last command-line argument if used and ignore any options that follow.
Comment 2 Jesse Ruderman 2007-06-13 21:10:12 PDT
Does Firefox really register a "firefoxurl:" protocol?  That's not very nice; it prevents users from safely using other browsers when Firefox has a known hole.
Comment 3 Gervase Markham [:gerv] 2007-06-14 01:14:09 PDT
Given that the attacker is already executing commands on the target box, is it really worth changing Firefox to make it harder to use for this? Won't they just use some other app to do their dirty work instead?

Jesse: I don't _think_ we do...
http://lxr.mozilla.org/mozilla/search?string=firefoxurl

Gerv
Comment 4 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2007-06-14 03:08:11 PDT
We allow opening web content with the -chrome flag because it's a common operation on intranets. If anything, the bug is that the javascript URL gets chrome privs instead of about:blank privs, but I don't know how we pick a principal in that case.

Really though this has to be a windows/IE bug... there are many other ways this could be exploited given the number of commandline options we support, and they are obviuosly passing us bogus data.
Comment 5 Daniel Veditz [:dveditz] 2007-06-14 10:01:22 PDT
(In reply to comment #3)
> Given that the attacker is already executing commands on the target box,

The attacker is not, they have a web page with a URL in it.

It happens to be something that will cause IE to use an external handler (Larholm's Safari PoC used gopher:) and Firefox is the registered handler for that scheme. IE and Safari apparently are not escaping quotes properly, but Firefox accepts arguments wholly inappropriate for handling remote URLs.

> Is it really worth changing Firefox to make it harder to use for this? Won't
> they just use some other app to do their dirty work instead?

Some other hypothetical handler that will turn an innocent text string into code execution? I doubt it. If they find a different vulnerability, though, hackers will definitely try that instead (a while back it was a buffer overrun in a default rtsp: handler; QuickTime I think, but maybe Real).

(In reply to comment #2)
> Does Firefox really register a "firefoxurl:" protocol?

We register HKLM/SOFTWARE/Classes/FirefoxURL with a shell/open/command subkey and then set the values of ftp, gopher, http, and https to FirefoxURL under HKLM/SOFTWARE/Clients/StartMenuInternet/FIREFOX.EXE/Capabilities/URLAssociations 

This, amazingly, does cause firefoxurl: to be sent to Firefox, which then doesn't recognize it and loops trying to open it in an external app which turns out to be Firefox. Definitely do NOT want to check the "remember" box on _that_ one! We can stop the looping by setting network.protocol-handler.external.firefoxurl to false, but it's too late by then since the malicious -chrome command-line has already executed.

We seem to be the only browser setting the "Capabilities" key under StartMenuInternet, at least on WinXP -- is it a Vista thing? This was added for bug 354005

If it's not a Vista requirement we should save our placeholders in a new place, because anything under HKR with a shell/open/command can be used as a scheme. (e.g. giffile://something will attempt to launch IE)

1) why do we register firefoxurl ?

2) even if we didn't, the problem still exists if Firefox is the default browser. Other browsers will slurp up http(s)/ftp regardless of default handler, but we've still got gopher: sitting out there.
Comment 6 Daniel Veditz [:dveditz] 2007-06-14 10:04:58 PDT
I'd prefer to keep the original advisory title for the summary because people will eventually be searching for this once the advisory is public.

"passing spaces" isn't really the problem, it's the quotes.
Comment 7 Daniel Veditz [:dveditz] 2007-06-14 14:33:05 PDT
Created attachment 268418 [details]
PoC (open in IE with FF not running)
Comment 8 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-14 14:52:34 PDT
(In reply to comment #5)
>...
> We seem to be the only browser setting the "Capabilities" key under
> StartMenuInternet, at least on WinXP -- is it a Vista thing? This was added for
> bug 354005
This is a Vista requirement and we set it so that if the OS is upgraded to Vista from a previous OS we need these keys under HKLM for Vista integration. MS did a similar implementation for local files (see FirefoxHTML) a while back and added this for protocol handlers with Vista.

> If it's not a Vista requirement we should save our placeholders in a new place,
> because anything under HKR with a shell/open/command can be used as a scheme.
> (e.g. giffile://something will attempt to launch IE)
The Capabilities and FirefoxURL keys are under HKLM. You are seeing the abstracted HKCR which is a combination of HKLM and HKCU Software\Classes.

> 1) why do we register firefoxurl ?
For Vista Integration.
Comment 9 Daniel Veditz [:dveditz] 2007-06-14 15:35:13 PDT
(In reply to comment #8)
> The Capabilities and FirefoxURL keys are under HKLM.

Yes, I mentioned that.

> You are seeing the abstracted HKCR which is a combination of
> HKLM and HKCU Software\Classes.

Which is what windows uses to determine the "active" handlers, should some user have customized HKCU settings.

> > 1) why do we register firefoxurl ?
> For Vista Integration.

Vista requires that there be a way to launch any specific browser even if the user doesn't want that browser handling stuff by default? Does IE do the same thing on Vista (IE7 doesn't in WinXP)? Yowza, that means even if you've switched to Firefox people could try to launch IE exploits. Yes, we'll put up a dialog first but that's pretty thin protection.

Please let us know what key name IE uses, I'd like to set network.protocol-handler.external.<ieurl> to false in the default prefs to protect against this. Safari and Opera, too, if they do this on Vista.
Comment 10 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2007-06-14 15:54:23 PDT
> We register HKLM/SOFTWARE/Classes/FirefoxURL with a shell/open/command subkey

> This, amazingly, does cause firefoxurl: to be sent to Firefox, which then

Does this mean that any Named key under HKLM/SOFTWARE/Classes automatically becomes a protocol handler? That sounds like a pretty serious bug in Vista to me: There are many keys there that have nothing to do with URI protocols. That's a huge (and pretty stupid) attack vector.

On my system IE has "htmlfile" even when Firefox is my default browser.

Setting .external = false on whackamole bad handlers sounds like a bad way to deal with this.
Comment 11 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-14 16:09:43 PDT
Created attachment 268432 [details]
IE's http regkey

(In reply to comment #9)
> (In reply to comment #8)
> > The Capabilities and FirefoxURL keys are under HKLM.
> 
> Yes, I mentioned that.
That was for clarification in regards to

(In reply to comment #5)
> If it's not a Vista requirement we should save our placeholders in a new place,
> because anything under HKR with a shell/open/command can be used as a scheme.
> (e.g. giffile://something will attempt to launch IE)
and to point out that the statement of "we should save our placeholders in a new place" wouldn't work. The system level default value must be under HKLM\Software\Classes which is then abstracted to HKCR unless it is over-ridden by a value under HKCU\Software\Classes.

> > You are seeing the abstracted HKCR which is a combination of
> > HKLM and HKCU Software\Classes.
> 
> Which is what windows uses to determine the "active" handlers, should some user
> have customized HKCU settings.
yes

> > > 1) why do we register firefoxurl ?
> > For Vista Integration.
> 
> Vista requires that there be a way to launch any specific browser even if the
> user doesn't want that browser handling stuff by default?
No, the purpose is to have a system level default for protocol handlers in the same way as earlier versions of windows expect a system level default for file handlers. 

> Does IE do the same thing on Vista (IE7 doesn't in WinXP)?
yes

> Yowza, that means even if you've
> switched to Firefox people could try to launch IE exploits. Yes, we'll put up a
> dialog first but that's pretty thin protection.
> 
> Please let us know what key name IE uses, I'd like to set
> network.protocol-handler.external.<ieurl> to false in the default prefs to
> protect against this. Safari and Opera, too, if they do this on Vista.
For protocols IE uses

IE.FTP
IE.HTTP
IE.HTTPS
Comment 12 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-14 16:13:49 PDT
(In reply to comment #10)
> > We register HKLM/SOFTWARE/Classes/FirefoxURL with a shell/open/command subkey
> 
> > This, amazingly, does cause firefoxurl: to be sent to Firefox, which then
> 
> Does this mean that any Named key under HKLM/SOFTWARE/Classes automatically
> becomes a protocol handler? That sounds like a pretty serious bug in Vista to
> me: There are many keys there that have nothing to do with URI protocols.
> That's a huge (and pretty stupid) attack vector.
No. With Vista protocol handlers have a HKLM\Software\Classes key similar to how file handlers had a value there. There must also be a value name of URL Protocol for it to be a URL protocol.

> On my system IE has "htmlfile" even when Firefox is my default browser.
> 
> Setting .external = false on whackamole bad handlers sounds like a bad way to
> deal with this.
> 

Comment 13 Daniel Veditz [:dveditz] 2007-06-19 12:25:08 PDT
rstrong said he'd look at the command handler here.
Comment 14 Daniel Veditz [:dveditz] 2007-06-19 17:45:23 PDT
Billy Rios of VeriSign independently reported similar abuse. At the following URL he has fun with various command-line options we support, several of which work even if Firefox is currently running.

http://www.xs-sniper.com/sniperscope/Cross-Browser-Scripting.html
Comment 15 Daniel Veditz [:dveditz] 2007-06-19 17:56:06 PDT
following up to comment 13, we're thinking that given an indication that "this is a call via a registered protocol/mimetype handler" we would only honor the -url commandline option (and perhaps a small whitelisted set of others).

We already register these handlers with -requestPending and that option has little use outside that context so we may be able to use that as our signal. But we'll have to reorder the options so that comes first before any potential bad quoting.
Comment 16 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-20 00:16:21 PDT
deveditz, I believe that nothing should be using firefoxurl as an -url param so how about if we just close that off completely by not handling any command line arguments when that is present instead of trying to handle the url and not handling specific arguments, etc.? btw: the url in comment #14 is no longer available.
Comment 17 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-20 00:55:00 PDT
Created attachment 269050 [details] [diff] [review]
patch

dveditz, this patch implements what I suggested in comment #16 and does the same thing for firefoxhtml while we are there. Does this seem like a reasonable approach?
Comment 18 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-20 01:35:35 PDT
Created attachment 269054 [details] [diff] [review]
patch - use a regexp

Changed to use a regexp. I don't think we need the check for firefoxhtml and will remove it of desired.
Comment 19 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-20 02:34:59 PDT
This patch doesn't handle the gopher / safari case. To properly handle urls when we need to restart during startup We remove requestPending so we can't use that to differentiate. We could do one of the following
1) add a new flag when we remove requestPending to handle that case
2) remove the gopher protocol reg keys when it points to our app
3) other?

There has been some discussion about removing the gopher protocol reg keys previously which is the direction I think we should take.

Also, I'm not entirely certain that the case when the app is already open and DDE is used isn't also vulnerable... I'll verify that tomorrow.
Comment 20 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-20 03:04:41 PDT
(In reply to comment #19)
>...
> Also, I'm not entirely certain that the case when the app is already open and
> DDE is used isn't also vulnerable... I'll verify that tomorrow.
When using DDE I get the same results and we don't pass requestPending or any other flags via DDE so just using a command line flag isn't an option. The current patch already fixes launching with command line flags and DDE and the only additional thing that would need to be done is the removal of the gopher protocol reg keys.
Comment 21 Daniel Veditz [:dveditz] 2007-06-20 13:35:44 PDT
Created attachment 269120 [details]
Billy Rios testcase

The URL from comment 14 was moved to
  http://www.xs-sniper.com/sniperscope/IE-Pwns-Firefox.html

and made public at
  http://sla.ckers.org/forum/read.php?3,12752,12752#msg-12752

I'm attaching the testcase in case it moves again.

For the record, that's not exactly what "Mozilla Security" (me) told him:

  "Thanks for sending the details. Ultimately we view this as bad behavior
   on the other app's part (as in the Safari bug Thor Larholm blogged about)
   but we are pragmatically working on protecting users from this on our end
   for a future security update.

   The hook you used was added for Vista compatibility and you will no doubt
   start seeing many more apps adding similar features."

the "hook" referred to his use of firefoxurl: which I didn't want to explicitly mention in unencrypted mail.
Comment 22 Daniel Veditz [:dveditz] 2007-06-20 20:25:28 PDT
Outlook Express on WinXP SP2 has the same problem as IE, it'll launch these links from an HTML email. I wonder if the programs are simply calling ShellExecute() and trusting that to escape properly. That's probably the level it should be fixed at since that's the API constructing the command-line.

Outlook Express _does_ escape http: and ftp: urls so those are not an attack vector. But it does not escape the unknown firefoxurl: scheme.

rstrong reports that the default "WinMail" on Vista does _not_ have this problem  so maybe they added escaping.

Opera 9.2 is safe, it escapes quotes.
Comment 23 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-21 10:57:27 PDT
Created attachment 269241 [details] [diff] [review]
patch rev 3
Comment 24 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-21 11:57:43 PDT
Comment on attachment 269241 [details] [diff] [review]
patch rev 3

Seth, since this has a couple of installer changes could you review the entire patch? Thanks!
Comment 25 Daniel Veditz [:dveditz] 2007-06-21 17:47:11 PDT
Created attachment 269305 [details]
Extended Billy Rios testcase (command execution)

-chrome does work whether the browser is running or not on WinXP, I must have had a bug in my own testcase, but Billy's works just fine :-(
Comment 26 Daniel Veditz [:dveditz] 2007-06-21 18:02:11 PDT
Comment on attachment 269241 [details] [diff] [review]
patch rev 3

This patch seems incomplete. I'm glad we're blocking firefoxurl: as an obvious sign of evilness, but I strongly feel we should do what I suggest in comment 15. That is (spelled out)

1) reorder the installed handlers so -requestPending is first (so it can't be messed with by injected content)

2) if we find -requestPending then we know we're a registered command-line handler and we MUST ignore all further options other than -url.

caveats: if -requestPending has other uses then we should invent a new option whose sole purpose is to signal this "I'm a registered handler" state.

rationale: IE happens to correctly escape http(s)/ftp/gopher URLS before (presumably) sending them to ShellExecute() so the injection trick isn't going to work, but that's no guarantee all other apps will do it. I really don't want to end up vulnerable later because acrobat/flash/aim/pidgin/<random RSS reader>/some-IRC-client get it wrong and don't escape http: urls, leading to the exact same -chrome exploit through a protocol we can't simply say is evil and not support. Also people could install firefox extentions that register additional protocol handlers that IE does not escape (for example, Chatzilla could register irc: and then here we are again)
Comment 27 Giorgio Maone [:mao] 2007-06-21 18:45:33 PDT
As expected, the exploit works from within Firefox if https://addons.mozilla.org/en-US/firefox/addon/1419 or similar is installed. NetScape should be vulnerable as well for obvious reasons, but I didn't check.

In the meanwhile, both the chrome privileged and the "normal" XSS tricks are prevented by the current NoScript development version, available from http://noscript.net/getit#devel and scheduled to be released in the mainstream tomorrow.
Comment 28 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-21 21:53:56 PDT
(In reply to comment #26)
> (From update of attachment 269241 [details] [diff] [review])
> This patch seems incomplete. I'm glad we're blocking firefoxurl: as an obvious
> sign of evilness, but I strongly feel we should do what I suggest in comment
> 15. That is (spelled out)
> 
> 1) reorder the installed handlers so -requestPending is first (so it can't be
> messed with by injected content)
> 
> 2) if we find -requestPending then we know we're a registered command-line
> handler and we MUST ignore all further options other than -url.
> 
> caveats: if -requestPending has other uses then we should invent a new option
> whose sole purpose is to signal this "I'm a registered handler" state.
> 
> rationale: IE happens to correctly escape http(s)/ftp/gopher URLS before
> (presumably) sending them to ShellExecute() so the injection trick isn't going
> to work, but that's no guarantee all other apps will do it. I really don't want
> to end up vulnerable later because acrobat/flash/aim/pidgin/<random RSS
> reader>/some-IRC-client get it wrong and don't escape http: urls, leading to
> the exact same -chrome exploit through a protocol we can't simply say is evil
> and not support. Also people could install firefox extentions that register
> additional protocol handlers that IE does not escape (for example, Chatzilla
> could register irc: and then here we are again)
I've been considering this as well. The caveat you stated regarding requestPending is entirely true as stated in comment #20 so a new flag will have to be used. On top of that we will most likely need to modify the reg key for DDE in order to prefix it with the new command line flag.
Comment 29 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-21 22:21:03 PDT
The approach I plan on taking for evaluating whether or not the command line is valid is to reorder the flags so they are -requestPending -newFlag -url "%1". The dde request will also include this new flag though I haven't decided yet the best way to implement this. If -newFlag (actual name not yet decided upon) is present only allow the -requestPending flag and the -url flag followed by the url param in the command line. If there are additional flags the entire operation will be a noop. Though we could just remove the additional flags I think this way is safer since the only time we should end up in this state is when an exploit is attempted.

Also, I think we should continue to make firefoxurl as a noop when it is passed in as the url param since this will prevent the app looping when it meets the aforementioned criteria above (e.g. -url "firefoxurl:http://localhost/").
Comment 30 timeless 2007-06-22 00:15:58 PDT
-untrusted ?
Comment 31 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-22 13:32:01 PDT
note: to cover all of the cases when launching the app with arguments we are going to have to do this at startup since several command lines are handled in nsAppRunner.cpp
Comment 32 Daniel Veditz [:dveditz] 2007-06-22 14:41:38 PDT
> -untrusted ?

Or some varation on "OS Integration" since that's what these are for?
   -osint
   -osi
   -os
    ?

If we register anything similar on Mac and Linux we should add the same flag to prevent future issues on those OS's. How does Linux know to open Firefox when I click on a local .html file?
Comment 33 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-27 13:33:55 PDT
Created attachment 270052 [details] [diff] [review]
branch patch ver 4

Benjamin, this prevents invoking command line arguments in nsAppRunner if the -osint command line flag is present except for -help, -version, etc. since those return immediately. It also adds nsAllowedCheckCommandLineHandler to nsBrowserContentHandler.js with a category of a-allowedcheck so it should be called first. Not pretty but it should be safe.

For the trunk I think a new interface for validation should be added (e.g. nsICommandLineValidator) which would allow commandlinehandlers to opt in to be validators. It would be enumerated in the same way nsICommandLineHandler is enumerated and have one method for the validation of the commandline args. This would allow extension provided commandlinehandlers that add os integration to validate their arguments. We would still need the checks for -osint in nsAppRunner since it handles several commandline flags early in startup.

Thoughts?
Comment 34 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-27 14:41:18 PDT
Created attachment 270067 [details] [diff] [review]
branch patch rev 5 (fixed a typo)
Comment 35 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-27 17:44:31 PDT
(In reply to comment #33)
>...
> For the trunk I think a new interface for validation should be added (e.g.
> nsICommandLineValidator) which would allow commandlinehandlers to opt in to be
> validators. It would be enumerated in the same way nsICommandLineHandler is
> enumerated and have one method for the validation of the commandline args. This
> would allow extension provided commandlinehandlers that add os integration to
> validate their arguments. We would still need the checks for -osint in
> nsAppRunner since it handles several commandline flags early in startup.
I've got this working and just need to finish up the code comments... I'm happy to say that the trunk patch may very well be safe enough for the branch.
Comment 36 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-27 22:07:39 PDT
Created attachment 270130 [details] [diff] [review]
trunk patch rev 4

Benjamin, I think this patch is just as safe for the branch as the previous patch.
Comment 37 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-27 22:11:49 PDT
Created attachment 270131 [details] [diff] [review]
trunk patch rev 5 (comment fix)
Comment 38 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-27 22:50:24 PDT
Created attachment 270136 [details] [diff] [review]
patch rev 6 (diff from trunk)

Benjamin, I'd like to go this route for both the trunk and branch
Comment 39 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-27 23:22:19 PDT
Created attachment 270140 [details] [diff] [review]
Thunderbird patch in progress

Scott, the logic in validate in nsMailDefaultHandler.js needs to be beefed up due to having to handle both a mail and compose flag. The MAPI calls will also have to be verified that it sets state to STATE_REMOTE_EXPLICIT.
Comment 40 Robert Strong [:rstrong] (use needinfo to contact me) 2007-06-28 12:54:43 PDT
Created attachment 270213 [details] [diff] [review]
patch rev 6 (diff from branch)

Forgot to include the new idl in the trunk patch.

This has been tested using attachment #269305 [details] and firefoxurl:http://localhost
Comment 41 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2007-07-02 08:02:23 PDT
Comment on attachment 270213 [details] [diff] [review]
patch rev 6 (diff from branch)

Are there any instances where CheckArg should work with "osint"? Can we just change the implementation of CheckArg to always check for "osint" and return ARG_BAD if it is present? That means we'd have to teach a few callers about ARG_BAD, but it would be less confusing than checking for "osint" in each case.
Comment 42 Robert Strong [:rstrong] (use needinfo to contact me) 2007-07-02 08:05:34 PDT
I considered that and went with this so the stderr output would be correct... I'll submit a new patch shortly implementing the check for osint in CheckArg. Thanks!
Comment 43 Robert Strong [:rstrong] (use needinfo to contact me) 2007-07-05 12:29:32 PDT
Created attachment 271103 [details] [diff] [review]
 patch rev 7 (diff from branch) 

Implements the changes requested in comment #41.

Regretfully attachment #270213 [details] [diff] [review] seems to be cleaner especially in regards to STDERR as this patch since there are a couple of one off's in how the arguments are handled.
Comment 44 Robert Strong [:rstrong] (use needinfo to contact me) 2007-07-05 12:57:14 PDT
Created attachment 271110 [details] [diff] [review]
  patch rev 7 (diff from branch)

Missed a one off case in the previous patch.
Comment 45 timeless 2007-07-10 00:34:45 PDT
I'm removing the security sensitive bit, it seems that
http://larholm.com/2007/07/10/internet-explorer-0day-exploit/

exists now. note that I've stuck the URL into the URL field to avoid duplicates. this is not to say that I'm giving larholm any more credit for the find.
Comment 46 Thor Larholm 2007-07-10 01:17:03 PDT
How pervasive is the use of -chrome on Intranets? I can't find any references to general usage of it, so perhaps it could simply be disabled by default with a preferences flag - then Intranets can enable it specifically.

For what it's worth, simply removing the "URL Protocol" value from the registry will prevent IE and any other applications from launching Firefox through FirefoxURL on XPSP2, without breaking additional functionality. I don't know about Vista, but it should still work just fine as the URL handler logic is not used for opening html files and the DefaultIcon is still in place.
Comment 47 Daniel Veditz [:dveditz] 2007-07-10 12:29:56 PDT
timeless: should not have opened the iDefense advisory information without coordinating release with them. They didn't publish when Billy Rios re-invented this a couple weeks ago, for example.
Comment 48 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2007-07-10 12:37:44 PDT
Comment on attachment 271110 [details] [diff] [review]
  patch rev 7 (diff from branch)

>Index: toolkit/xre/nsAppRunner.cpp

>+  ar = CheckArg("a", PR_FALSE, &temp);

For this and -profile, why do you pass PR_FALSE?

>+  /* nsICommandLineValidator */
>+  validate : function bch_validate(cmdLine) {
>+    // Other handlers may use osint so only handle the osint flag if the url
>+    // flag is also present and the command line is valid.
>+    var osintFlagIdx = cmdLine.findFlag("osint", false);
>+    var urlFlagIdx = cmdLine.findFlag("url", false);
>+    if (urlFlagIdx > -1 && (osintFlagIdx > -1 ||
>+        cmdLine.state == nsICommandLine.STATE_REMOTE_EXPLICIT)) {
>+      var urlParam = cmdLine.getArgument(urlFlagIdx + 1);
>+      if (cmdLine.length != urlFlagIdx + 2 || /firefoxurl:/.test(urlParam))
>+        throw NS_ERROR_ABORT;
>+      cmdLine.handleFlag("osint", false)

Does this handle URLs which have correctly-encoded spaces in them? e.g.

in some app: http://example.com/some%20file.html

Does this get passed to Firefox as some%20file.html or some file.html?
Comment 49 Robert Strong [:rstrong] (use needinfo to contact me) 2007-07-10 16:00:54 PDT
Created attachment 271754 [details] [diff] [review]
Thunderbird patch
Comment 50 Robert Strong [:rstrong] (use needinfo to contact me) 2007-07-10 16:08:47 PDT
(In reply to comment #48)
> (From update of attachment 271110 [details] [diff] [review])
> >Index: toolkit/xre/nsAppRunner.cpp
> 
> >+  ar = CheckArg("a", PR_FALSE, &temp);
> 
> For this and -profile, why do you pass PR_FALSE?
Actually, all the CheckArg calls in HandleRemoteArgument should pass PR_FALSE since the command line check that calls this passes PR_TRUE.

> >+  /* nsICommandLineValidator */
> >+  validate : function bch_validate(cmdLine) {
> >+    // Other handlers may use osint so only handle the osint flag if the url
> >+    // flag is also present and the command line is valid.
> >+    var osintFlagIdx = cmdLine.findFlag("osint", false);
> >+    var urlFlagIdx = cmdLine.findFlag("url", false);
> >+    if (urlFlagIdx > -1 && (osintFlagIdx > -1 ||
> >+        cmdLine.state == nsICommandLine.STATE_REMOTE_EXPLICIT)) {
> >+      var urlParam = cmdLine.getArgument(urlFlagIdx + 1);
> >+      if (cmdLine.length != urlFlagIdx + 2 || /firefoxurl:/.test(urlParam))
> >+        throw NS_ERROR_ABORT;
> >+      cmdLine.handleFlag("osint", false)
> 
> Does this handle URLs which have correctly-encoded spaces in them? e.g.
> 
> in some app: http://example.com/some%20file.html
> 
> Does this get passed to Firefox as some%20file.html or some file.html?
It gets passed quoted via the reg entry value of:
<path to firefox.exe> -requestPending -osint -url "%1"
where %1 is replaced by the url.
Comment 51 Scott MacGregor 2007-07-10 22:19:22 PDT
Comment on attachment 271754 [details] [diff] [review]
Thunderbird patch

r/sr=mscott. I'm still working on putting together a build with the patches but the code looks good to me. Thanks Rob.
Comment 52 Robert Strong [:rstrong] (use needinfo to contact me) 2007-07-10 23:11:35 PDT
Checked in to Trunk and MOZILLA_1_8_BRANCH

Trunk

Checking in mozilla/toolkit/components/commandlines/public/Makefile.in;
/cvsroot/mozilla/toolkit/components/commandlines/public/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/toolkit/components/commandlines/public/nsICommandLineValidator.idl,v
done
Checking in mozilla/toolkit/components/commandlines/public/nsICommandLineValidator.idl;
/cvsroot/mozilla/toolkit/components/commandlines/public/nsICommandLineValidator.idl,v  <--  nsICommandLineValidator.idl
initial revision: 1.1
done
Checking in mozilla/toolkit/components/commandlines/src/nsCommandLine.cpp;
/cvsroot/mozilla/toolkit/components/commandlines/src/nsCommandLine.cpp,v  <--  nsCommandLine.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in mozilla/toolkit/xre/nsAppRunner.cpp;
/cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v  <--  nsAppRunner.cpp
new revision: 1.178; previous revision: 1.177
done
Checking in mozilla/browser/components/nsBrowserContentHandler.js;
/cvsroot/mozilla/browser/components/nsBrowserContentHandler.js,v  <--  nsBrowserContentHandler.js
new revision: 1.41; previous revision: 1.40
done
Checking in mozilla/browser/components/shell/src/nsWindowsShellService.cpp;
/cvsroot/mozilla/browser/components/shell/src/nsWindowsShellService.cpp,v  <--  nsWindowsShellService.cpp
new revision: 1.47; previous revision: 1.46
done
Checking in mozilla/browser/installer/windows/nsis/shared.nsh;
/cvsroot/mozilla/browser/installer/windows/nsis/shared.nsh,v  <--  shared.nsh
new revision: 1.9; previous revision: 1.8
done
Checking in mozilla/mail/components/nsMailDefaultHandler.js;
/cvsroot/mozilla/mail/components/nsMailDefaultHandler.js,v  <--  nsMailDefaultHandler.js
new revision: 1.8; previous revision: 1.7
done
Checking in mozilla/mail/installer/windows/nsis/shared.nsh;
/cvsroot/mozilla/mail/installer/windows/nsis/shared.nsh,v  <--  shared.nsh
new revision: 1.6; previous revision: 1.5
done
Checking in mozilla/mail/components/shell/nsMailWinIntegration.cpp;
/cvsroot/mozilla/mail/components/shell/nsMailWinIntegration.cpp,v  <--  nsMailWinIntegration.cpp
new revision: 1.9; previous revision: 1.8
done

MOZILLA_1_8_BRANCH

Checking in mozilla/toolkit/components/commandlines/public/Makefile.in;
/cvsroot/mozilla/toolkit/components/commandlines/public/Makefile.in,v  <--  Makefile.in
new revision: 1.2.12.1; previous revision: 1.2
done
Checking in mozilla/toolkit/components/commandlines/public/nsICommandLineValidator.idl;
/cvsroot/mozilla/toolkit/components/commandlines/public/nsICommandLineValidator.idl,v  <--  nsICommandLineValidator.idl
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in mozilla/toolkit/components/commandlines/src/nsCommandLine.cpp;
/cvsroot/mozilla/toolkit/components/commandlines/src/nsCommandLine.cpp,v  <--  nsCommandLine.cpp
new revision: 1.13.2.1; previous revision: 1.13
done
Checking in mozilla/toolkit/xre/nsAppRunner.cpp;
/cvsroot/mozilla/toolkit/xre/nsAppRunner.cpp,v  <--  nsAppRunner.cpp
new revision: 1.113.2.21; previous revision: 1.113.2.20
done
Checking in mozilla/browser/components/nsBrowserContentHandler.js;
/cvsroot/mozilla/browser/components/nsBrowserContentHandler.js,v  <--  nsBrowserContentHandler.js
new revision: 1.12.2.22; previous revision: 1.12.2.21
done
Checking in mozilla/browser/components/shell/src/nsWindowsShellService.cpp;
/cvsroot/mozilla/browser/components/shell/src/nsWindowsShellService.cpp,v  <--  nsWindowsShellService.cpp
new revision: 1.21.2.6; previous revision: 1.21.2.5
done
Checking in mozilla/browser/installer/windows/nsis/shared.nsh;
/cvsroot/mozilla/browser/installer/windows/nsis/shared.nsh,v  <--  shared.nsh
new revision: 1.3.2.8; previous revision: 1.3.2.7
done
Checking in mozilla/mail/components/nsMailDefaultHandler.js;
/cvsroot/mozilla/mail/components/nsMailDefaultHandler.js,v  <--  nsMailDefaultHandler.js
new revision: 1.4.4.4; previous revision: 1.4.4.3
done
Checking in mozilla/mail/installer/windows/nsis/shared.nsh;
/cvsroot/mozilla/mail/installer/windows/nsis/shared.nsh,v  <--  shared.nsh
new revision: 1.2.2.5; previous revision: 1.2.2.4
done
Checking in mozilla/mail/components/shell/nsMailWinIntegration.cpp;
/cvsroot/mozilla/mail/components/shell/nsMailWinIntegration.cpp,v  <--  nsMailWinIntegration.cpp
new revision: 1.2.2.4; previous revision: 1.2.2.3
done
Comment 53 Robert Strong [:rstrong] (use needinfo to contact me) 2007-07-10 23:14:11 PDT
Created attachment 271794 [details] [diff] [review]
Branch as checked in
Comment 54 Robert Strong [:rstrong] (use needinfo to contact me) 2007-07-10 23:14:59 PDT
Created attachment 271795 [details] [diff] [review]
Trunk as checked in
Comment 55 juan becerra [:juanb] 2007-07-17 14:47:38 PDT
Verified using test cases in comment #14, with installation of Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5) Gecko/2007071317 Firefox/2.0.0.5

Also Thunderbird version 2.0.0.5 (20070716) with modified test cases in comment #14
Comment 56 Robert Strong [:rstrong] (use needinfo to contact me) 2007-07-23 11:55:26 PDT
Created attachment 273430 [details] [diff]
Remove the handler added by the MS shim

After reading the following
http://msinfluentials.com/blogs/jesper/archive/2007/07/10/blocking-the-firefox-gt-ie-0-day.aspx

I realized we also need to remove the handler added by the MS shim.
Comment 57 Robert Strong [:rstrong] (use needinfo to contact me) 2007-07-23 12:08:31 PDT
Created attachment 273433 [details] [diff]
Remove the handler added by the MS shim take 2
Comment 58 (not reading, please use seth@sspitzer.org instead) 2007-07-23 12:12:30 PDT
Comment on attachment 273433 [details] [diff]
Remove the handler added by the MS shim take 2

r=sspitzer
Comment 59 Daniel Veditz [:dveditz] 2007-07-23 12:25:19 PDT
Comment 56 needs to be in a new follow-up bug. We've already shipped a release with what we thought was the fix, adding more blocking/fixed flags here would make a big mess.

Filed bug 389287 to track that installer patch.
Comment 60 Robert Strong [:rstrong] (use needinfo to contact me) 2007-07-23 12:30:04 PDT
Created attachment 273435 [details] [diff]
simpler patch - as checked in.

After discussing this with sspitzer we decided this would be a simpler approach. Carrying forward his r+
Comment 61 Ben Bucksch (:BenB) 2007-07-25 09:38:06 PDT
I would really like to open this, as this is discussed widely in the open, and I can't reference the patch and bug.
Comment 62 Ben Bucksch (:BenB) 2007-07-25 09:41:13 PDT
whiteboard says
[sg:critical] please keep bug closed until iDefense announcement (SA25984)
http://secunia.com/advisories/25984/ is public, so I assume this can be opened now.
Comment 63 Daniel Veditz [:dveditz] 2007-07-25 14:00:50 PDT
Yeah, but then this morphed into a new unfixed bug at comment 56 :-(
Comment 64 Daniel Veditz [:dveditz] 2009-05-05 21:26:35 PDT
What does it mean that this is in "Core Graveyard" now? We still have this code, and could conceivably make a similar mistake in the future that would be a security hole we'd have to fix. How is that a "Graveyard"?
Comment 65 Samuel Sidler (old account; do not CC) 2009-05-05 21:36:39 PDT
dveditz: bug 491354

bsmedberg: comment 64 :)
Comment 66 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-05-06 06:02:41 PDT
The cmd-line handling component is obsolete. I moved all open bugs in this component to the correct component and had the component moved to graveyard so that new bugs could not be opened in it.

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