Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 441120 - (CVE-2008-2933) command-line URLs launch multi-tabs if Firefox not running, exploitable with Safari Carpet bombing
: command-line URLs launch multi-tabs if Firefox not running, exploitable with ...
[sg:critical] when combined w/bug 441...
: fixed1.9.0.1, fixed1.9.1, verified1.8.1.16
Product: Toolkit
Classification: Components
Component: Startup and Profile System (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9.1a1
Assigned To: :Gavin Sharp [email:]
: Benjamin Smedberg [:bsmedberg]
Depends on: 442970 446568
Blocks: 221445
  Show dependency treegraph
Reported: 2008-06-22 05:16 PDT by Daniel Veditz [:dveditz]
Modified: 2016-10-15 07:07 PDT (History)
21 users (show)
mbeltzner: blocking1.9.1+
mbeltzner: blocking1.9.0.1+
dveditz: blocking1.8.1.16+
dveditz: wanted1.8.1.x+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (6.60 KB, patch)
2008-06-22 23:37 PDT, :Gavin Sharp [email:]
no flags Details | Diff | Splinter Review
patch (9.09 KB, patch)
2008-06-24 02:35 PDT, :Gavin Sharp [email:]
mconnor: review+
dveditz: review+
mbeltzner: approval1.9.0.1+
Details | Diff | Splinter Review
some manual tests I ran (55.93 KB, text/html)
2008-06-24 02:37 PDT, :Gavin Sharp [email:]
no flags Details
updated trunk patch (9.11 KB, patch)
2008-06-30 21:44 PDT, :Gavin Sharp [email:]
no flags Details | Diff | Splinter Review
branch patch (9.54 KB, patch)
2008-06-30 21:46 PDT, :Gavin Sharp [email:]
dveditz: approval1.8.1.16+
Details | Diff | Splinter Review
rollup mozilla-central patch (9.82 KB, patch)
2008-07-03 13:22 PDT, :Gavin Sharp [email:]
no flags Details | Diff | Splinter Review
1.8.0 branch patch (11.01 KB, patch)
2008-07-10 03:25 PDT, Martin Stránský review-
Details | Diff | Splinter Review
1.8.0 branch patch (2nd) (11.03 KB, patch)
2008-07-17 08:25 PDT, Alexander Sack
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2008-06-22 05:16:44 PDT
Billy Rios reported that if Firefox is not already running, passing it a command-line URI with pipe symbols will open multiple tabs (apparently because it's being treated as a home-page). If Firefox is running then it's treated as a single URI with pipe symbols and probably come up with something pretty mangled.

Not only is the inconsistency of behavior depending on state a bug, this allows another application to bypass the fix for bug 305269 and bug 298255 which disallowed apps from loading chrome: uris.

This also potentially puts non-Firefox users at risk from Firefox bugs if a unique protocol handler allows launching Firefox. Someone who tried Firefox for a while and went back to their previous browser might end up with an old vulnerable Firefox, and an attacker might be able to use this trick to send the user to an exploit page.

Examples that work in Safari on Windows:

Although Firefox 3 no longer registers for the gopher: protocol, neither does it appear to delete a pre-existing Firefox 2 registration. A user who has upgraded from an earlier Firefox might still be at risk.

Now combine this with the Safari "Carpet Bombing" attack: Safari drops .html files onto the desktop, uses this trick to have Firefox open them. In Firefox 2 this file can now rummage through your disk and exfiltrate anything interesting (cookies? password file? financial documents?).

Firefox 3 users are protected by the fix for bug 230606 -- the dropped file can only read the contents of known files (because directories can't be read) in the same directory or below. Didn't expect that fix to pay off so quickly, less than a week since launch.

Nate McFeeters also pointed out that even in the "fixed" Safari 3.1.2, data: uris with unknown content-types are downloaded to the c:\tmp directory (even if %TMP% is set to something else) with somewhat predictable names.

It's unclear how bad the bypass of bug 305269 is (loading chrome:). Simply opening most chrome URIs doesn't cause any bad action, but with zillions of addons out there I wouldn't want to guarantee that.
Comment 1 Daniel Veditz [:dveditz] 2008-06-22 11:26:38 PDT
This becomes sg:critical when combined with bug 441169
Comment 2 Mike Schroepfer 2008-06-22 15:10:31 PDT
Thoughts on a fix?
Comment 3 :Gavin Sharp [email:] 2008-06-22 22:55:50 PDT
This bug can actually be reproduced even with Firefox already running, by using the -new-window parameter. Harder to get trigger one of those, though.
Comment 4 :Gavin Sharp [email:] 2008-06-22 23:37:50 PDT
Created attachment 326251 [details] [diff] [review]

Here's a patch that:
1) Makes the command line code always pass at least three arguments to browser.xul, ensuring that we avoid the "|"-splitting behavior:,693#671 (and add a comment to make sure this isn't unintentionally regressed)
2) Creates a new way to launch multiple tabs on window load by passing an explicit array of URLs
3) Makes use of that new method for existing "safe" callers (those that pass default arguments based on home page prefs, e.g. - splitPipesIntoArray callers in the patch)

I initially looked into having shouldLoadURI split("|") the URL and verify each part, but that's fragile because it means keeping the loading and verifying steps in sync, and the string munging in the command line code means that it's hard to be sure that you're checking all the right arguments at the right time. By disabling the pipe-splitting entirely for command line callers, we can be sure that the URIs we're passing in will be loaded as-is.

I considered changing the behavior of the browser in the default case (to always avoid splitting by "|"), but that could potentially break extensions or other code that relies on the behavior.

Changes 2) and 3) are needed to support multiple-URL loading code (e.g. first-run) and for supporting "|"-split homepages (Flock actually used this at one point, not sure if they still do). 

I audited other callers that open browser windows directly and pass in URLs:
	- nsMacCommandLine::OpenURL and XRemoteService::OpenURL probably need to pass the minimum number of arguments to ensure that the "|"-splitting is avoided
	- utilityOverlay.js functions openUILinkIn() and openNewWindowWith() - these are safe, I think, no way for external code to end up calling them
	- browser.js calls to getBrowserURL() - safe, with the exception of nsBrowserAccess's OPEN_NEWWINDOW handler, which explains comment 3. -new-window is fixed by the patch, since it goes through openWindow. "firefox -remote openURL(foo|bar|chrome://baz,new-window)" while firefox is running is still technically vulnerable, but we only parse that without quotes, and most shells won't let you run that without quotes because the pipe is interpreted by the command line otherwise. Fixed by the patch anyways, though.

Needs testing still, but I'll have to do that when I'm less tired.
Comment 5 Benjamin Smedberg [:bsmedberg] 2008-06-23 06:44:05 PDT
Note, this is basically bug 221445, except for the security angle.
Comment 6 :Gavin Sharp [email:] 2008-06-24 02:35:44 PDT
Created attachment 326448 [details] [diff] [review]

I had to change approaches for passing multiple URIs to the browser window - some homepages might not be valid nsIURIs, but still need to be passed. Using strings instead of nsIURI to pass these now.

Not sure who best person to review this is...
Comment 7 :Gavin Sharp [email:] 2008-06-24 02:37:30 PDT
Created attachment 326449 [details]
some manual tests I ran

These are the tests I've run to make sure I didn't break anything. We don't have a harness that's suitable for testing startup code at the moment, so this will have to do. I'm going to make sure to run them again since the last time I ran them was before I had to modify the patch.

(Forgive the horrendous HTML, this is exported from Google docs.)
Comment 8 Mike Connor [:mconnor] 2008-06-26 18:57:17 PDT
Comment on attachment 326448 [details] [diff] [review]

r=me, but this could use a lot more comments throughout
Comment 9 Daniel Veditz [:dveditz] 2008-06-30 16:49:39 PDT
Comment on attachment 326448 [details] [diff] [review]


Did we break windows file:///c|/whatever/ uris any more than they are already?
Comment 10 Mike Beltzner [:beltzner, not reading bugmail] 2008-06-30 17:37:07 PDT
Comment on attachment 326448 [details] [diff] [review]

Comment 11 Daniel Veditz [:dveditz] 2008-06-30 17:56:18 PDT
Restoring blocking request, given the lack of restrictions on file: uris this is important to fix in Firefox 2 even though the older releases are not affected by bug 441169
Comment 12 :Gavin Sharp [email:] 2008-06-30 18:21:32 PDT
(In reply to comment #9)
> Did we break windows file:///c|/whatever/ uris any more than they are already?

If anything this will fix them, since we'll now treat that as a single URI rather than splitting it in two.
Comment 13 Mike Beltzner [:beltzner, not reading bugmail] 2008-06-30 19:31:49 PDT
Marking checkin-needed as Gavin's on vacation. Vacate, Gavin!
Comment 14 Daniel Holbert [:dholbert] (PTO Oct 21-25) 2008-06-30 19:33:55 PDT
cc'ing reed so he can check this in.
Comment 15 :Gavin Sharp [email:] 2008-06-30 20:42:42 PDT
I'm not actually on vacation... :) Working on a branch patch and testing now.
Comment 16 :Gavin Sharp [email:] 2008-06-30 21:44:20 PDT
Created attachment 327540 [details] [diff] [review]
updated trunk patch

Ran through the tests again, found one bug that I'd introduced shortly after running the previous set of tests. Need to pass an array of strings to openWindow, not an array of URIs (I also renamed "URLList" to avoid confusion with "urilist"):

-      urilist = urilist.filter(shouldLoadURI);
+      var URLlist = urilist.filter(shouldLoadURI).map(function (u) u.spec);
Comment 17 :Gavin Sharp [email:] 2008-06-30 21:46:04 PDT
Created attachment 327541 [details] [diff] [review]
branch patch

Branch patch, also tested, with the same change as the previous one, except that we can't use expression closures on the branch:

+      var URLlist = urilist.filter(shouldLoadURI).map(function (u) { return u.spec; });
Comment 18 Samuel Sidler (old account; do not CC) 2008-06-30 21:48:16 PDT
Comment on attachment 327541 [details] [diff] [review]
branch patch

Requesting approval for this patch.
Comment 19 Daniel Veditz [:dveditz] 2008-06-30 22:44:09 PDT
Comment on attachment 327541 [details] [diff] [review]
branch patch

Approved for, a=dveditz for release-drivers
Comment 20 :Gavin Sharp [email:] 2008-06-30 22:58:41 PDT
CVS Trunk:
mozilla/browser/base/content/browser.js 	1.1034
mozilla/browser/components/nsBrowserContentHandler.js 	1.48

mozilla/browser/base/content/browser.js 	1.479.2.221
Comment 21 :Gavin Sharp [email:] 2008-07-03 13:22:15 PDT
Created attachment 328035 [details] [diff] [review]
rollup mozilla-central patch

Includes the fix for bug 442970.
Comment 22 :Gavin Sharp [email:] 2008-07-03 13:38:25 PDT
Comment 23 Al Billings [:abillings] 2008-07-07 16:58:10 PDT
Is there a good way to test this for the Firefox release? The repro steps are not 100% clear for branch.
Comment 24 :Gavin Sharp [email:] 2008-07-08 07:50:27 PDT
The steps to reproduce for branch should be the same for trunk. Start->Run gopher:||javascript:alert(1) with Firefox set as your default browser, but not running, should result in Firefox launching with a single "could not find the server at https" error page rather than multiple tabs loading various sites and an alert box that says "1".
Comment 25 Al Billings [:abillings] 2008-07-08 13:50:52 PDT
I verified the old behavior with on Windows XP and the new correct behavior in the build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2008070205 Firefox/
Comment 26 Al Billings [:abillings] 2008-07-08 13:54:56 PDT
I notice that running the 3.1 nightly (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008070803 Minefield/3.1a1pre) on the same machine, that Firefox doesn't open with the tabs either. The odd thing is that when testing the problem with branch, Firefox starts up and then doesn't open tabs, etc. When testing with the 3.1 build, the error message about it being a bad URL comes from IE, not from Minefield, even though the latter is marked as my default browser.
Comment 27 Tony Chung [:tchung] 2008-07-08 16:02:17 PDT
hasham, can you try out this test as well?  

some other STRs to use:
1) set Fx3.0.1 as default browser
2) close all applications
3) start > Run,||chrome://browser/content/browser.xul
4) Verify a script error appears, but only one one page, instead of 3 separate tabs.
5) Also try this back in a non-fixed build (eg. FF3.0), and make sure that it should break and open multi-tabs.
Comment 28 :Gavin Sharp [email:] 2008-07-08 16:03:55 PDT
(In reply to comment #26)
> When testing with the 3.1 build, the error message about it
> being a bad URL comes from IE, not from Minefield, even though the latter is
> marked as my default browser.

Talked to abillings about this on IRC, turns out this is because Firefox 3 doesn't register the "gopher:" protocol handler the way Firefox 2 did (as mentioned in comment 0).
Comment 29 Hasham 2008-07-08 17:20:43 PDT
Ok, I managed to repro this in Firefox 3 with the following steps:

1) Install FF2 and make it your default browser.
2) Install FF3 in the same directory as FF2 and make it your default browser.
3) Close all applications
4) Start > Run http:||javascript:alert(1)

You should get three tabs. It seems the double http somehow affects this because it won't work with gopher.

BTW this is appears to be fixed on 3.0.1 where it only opens one tab, but opens up the "I'm feeling lucky" link for which happens to be Not sure if that is still an issue or not.
Comment 30 Josh Bressers 2008-07-09 11:02:01 PDT
Please use CVE-2008-2933 for this issue.
Comment 31 Martin Stránský 2008-07-10 03:25:41 PDT
Created attachment 328852 [details] [diff] [review]
1.8.0 branch patch

backported patch for 1.8.0 branch. Mike, could you please check it?
Comment 32 :Gavin Sharp [email:] 2008-07-12 16:09:36 PDT
Comment on attachment 328852 [details] [diff] [review]
1.8.0 branch patch

>diff -up -U10 mozilla/browser/components/nsBrowserContentHandler.js.441120 mozilla/browser/components/nsBrowserContentHandler.js

>+      var URLlist = urilist.filter(shouldLoadURI).map(function (u) u.spec);

This won't work on the branch, expression closures don't exist there. You need "function (u) { return u.spec; }" like in the 1.8 branch patch. Did you not base this patch on the 1.8 branch one? Would be good to run through the tests in attachment 326449 [details] in your 1.8.0 build, too.
Comment 33 :Gavin Sharp [email:] 2008-07-17 02:47:51 PDT
The 1.8.0 patch looks fine otherwise, for what it's worth - was this just forgotten?
Comment 34 Alexander Sack 2008-07-17 08:23:05 PDT
(In reply to comment #33)
> The 1.8.0 patch looks fine otherwise, for what it's worth - was this just
> forgotten?

Yes, we forgot to reattach it here. Thanks for caring!
Comment 35 Alexander Sack 2008-07-17 08:25:53 PDT
Created attachment 330047 [details] [diff] [review]
1.8.0 branch patch (2nd)

a=asac for 1.8.0 branch

Approving based on comment 33
Comment 36 Alexander Sack 2008-07-17 08:26:57 PDT
Changes done for 1.8.0 (2nd)

< +      var URLlist = urilist.filter(shouldLoadURI).map(function (u) u.spec);
> +      var URLlist = urilist.filter(shouldLoadURI).map(function (u) { return u.spec; });
< +    else if ("arguments" in window && window.arguments.length >= 3)
> +    else if ("arguments" in window && window.arguments.length >= 3) {
Comment 37 Khan Saab 2016-10-15 07:07:17 PDT Comment hidden (spam)

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