441120 - (CVE-2008-2933) command-line URLs launch multi-tabs if Firefox not running, exploitable with Safari Carpet bombing

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect, P1)

Description

[Daniel Veditz [:dveditz]]

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:
gopher:https://www.google.com|www.test.com|javascript:alert(1)
gopher:https://www.google.com|www.test.com|chrome://browser/content/browser.xul 

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

This becomes sg:critical when combined with bug 441169

Comment 2

[Mike Schroepfer]

Thoughts on a fix?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

Attachment: patch

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:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.1033&mark=689,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]

Note, this is basically bug 221445, except for the security angle.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

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: [e-mail]]]

Attachment: 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]]

Comment on attachment 326448 [details] [diff] [review]
patch

r=me, but this could use a lot more comments throughout

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 326448 [details] [diff] [review]
patch

sr=dveditz

Did we break windows file:///c|/whatever/ uris any more than they are already?

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 326448 [details] [diff] [review]
patch

a=beltzner

Comment 11

[Daniel Veditz [:dveditz]]

Restoring 1.8.1.16 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: [e-mail]]]

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

Marking checkin-needed as Gavin's on vacation. Vacate, Gavin!

Comment 14

[Daniel Holbert [:dholbert]]

cc'ing reed so he can check this in.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

I'm not actually on vacation... :) Working on a branch patch and testing now.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Attachment: 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: [e-mail]]]

Attachment: 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)]

Comment on attachment 327541 [details] [diff] [review]
branch patch

Requesting approval for this patch.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 327541 [details] [diff] [review]
branch patch

Approved for 1.8.1.16, a=dveditz for release-drivers

Comment 20

[:Gavin Sharp [email: [e-mail]]]

CVS Trunk:
mozilla/browser/base/content/browser.js 	1.1034
mozilla/browser/components/nsBrowserContentHandler.js 	1.48

MOZILLA_1_8_BRANCH:
mozilla/browser/base/content/browser.js 	1.479.2.221
mozilla/browser/components/nsBrowserContentHandler.js 	1.12.2.24 

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Attachment: rollup mozilla-central patch

Includes the fix for bug 442970.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/mozilla-central/index.cgi/rev/cdb5ab09bc26

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Is there a good way to test this for the Firefox 2.0.0.16 release? The repro steps are not 100% clear for branch.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

The steps to reproduce for branch should be the same for trunk. Start->Run gopher:https://www.google.com|www.test.com|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 - ex-MoCo]]

I verified the old behavior with 2.0.0.14 on Windows XP and the new correct behavior in the 2.0.0.16 build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.16) Gecko/2008070205 Firefox/2.0.0.16).

Comment 26

[Al Billings [:abillings - ex-MoCo]]

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 2.0.0.16, 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]]

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, http://www.google.com/|http://yahoo.com/|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: [e-mail]]]

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

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:https://www.google.com|www.test.com|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 www.google.com which happens to be www.theedge.com. Not sure if that is still an issue or not.
 

Comment 30

[Josh Bressers]

Please use CVE-2008-2933 for this issue.

Comment 31

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8.0 branch patch

backported patch for 1.8.0 branch. Mike, could you please check it?

Comment 32

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

The 1.8.0 patch looks fine otherwise, for what it's worth - was this just forgotten?

Comment 34

[Alexander Sack]

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

Attachment: 1.8.0 branch patch (2nd)

a=asac for 1.8.0 branch

Approving based on comment 33

Comment 36

[Alexander Sack]

Changes done for 1.8.0 (2nd)

178c178
< +      var URLlist = urilist.filter(shouldLoadURI).map(function (u) u.spec);
---
> +      var URLlist = urilist.filter(shouldLoadURI).map(function (u) { return u.spec; });
253c253
< +    else if ("arguments" in window && window.arguments.length >= 3)
---
> +    else if ("arguments" in window && window.arguments.length >= 3) {

Comment 54

[Benjamin Smedberg]

Because of persistent spam I'm going to restrict comments on this old dead bug.