Bug 441120 (CVE-2008-2933)

command-line URLs launch multi-tabs if Firefox not running, exploitable with Safari Carpet bombing

RESOLVED FIXED in mozilla1.9.1a1

Status

()

Toolkit
Startup and Profile System
P1
normal
RESOLVED FIXED
9 years ago
a month ago

People

(Reporter: dveditz, Assigned: Gavin)

Tracking

({fixed1.9.0.1, fixed1.9.1, verified1.8.1.16})

Trunk
mozilla1.9.1a1
fixed1.9.0.1, fixed1.9.1, verified1.8.1.16
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9.0.1 +
blocking1.8.1.16 +
wanted1.8.1.x +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] when combined w/bug 441169, sg:high with file: URIs)

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

9 years ago
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.
Flags: wanted1.8.1.x+
Flags: blocking1.9.0.1?
Flags: blocking1.8.1.16?
Flags: blocking-firefox3.1?
(Reporter)

Updated

9 years ago
Whiteboard: [sg:high] potentially sg:critical if there's a dangerous chrome: URI
(Reporter)

Comment 1

9 years ago
This becomes sg:critical when combined with bug 441169
Whiteboard: [sg:high] potentially sg:critical if there's a dangerous chrome: URI → [sg:critical] when combined with bug 441169

Comment 2

9 years ago
Thoughts on a fix?
Assignee: mconnor → gavin.sharp
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.
Status: NEW → ASSIGNED
Created attachment 326251 [details] [diff] [review]
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.
Note, this is basically bug 221445, except for the security angle.
Created attachment 326448 [details] [diff] [review]
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...
Attachment #326251 - Attachment is obsolete: true
Attachment #326448 - Flags: review?(mconnor)
Attachment #326448 - Flags: review?(dveditz)
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.)
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1+
Comment on attachment 326448 [details] [diff] [review]
patch

r=me, but this could use a lot more comments throughout
Attachment #326448 - Flags: review?(mconnor) → review+

Updated

9 years ago
Blocks: 221445
(Reporter)

Comment 9

9 years ago
Comment on attachment 326448 [details] [diff] [review]
patch

sr=dveditz

Did we break windows file:///c|/whatever/ uris any more than they are already?
Attachment #326448 - Flags: review?(dveditz) → review+
Comment on attachment 326448 [details] [diff] [review]
patch

a=beltzner
Attachment #326448 - Flags: approval1.9.0.1+
(Reporter)

Comment 11

9 years ago
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
Flags: blocking1.8.1.16?
Whiteboard: [sg:critical] when combined with bug 441169 → [sg:critical] when combined w/bug 441169, sg:high with file: URIs
(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.
Keywords: checkin-needed
Marking checkin-needed as Gavin's on vacation. Vacate, Gavin!
cc'ing reed so he can check this in.
I'm not actually on vacation... :) Working on a branch patch and testing now.
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);
Attachment #326448 - Attachment is obsolete: true
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 on attachment 327541 [details] [diff] [review]
branch patch

Requesting approval for this patch.
Attachment #327541 - Flags: approval1.8.1.16?
(Reporter)

Comment 19

9 years ago
Comment on attachment 327541 [details] [diff] [review]
branch patch

Approved for 1.8.1.16, a=dveditz for release-drivers
Attachment #327541 - Flags: approval1.8.1.16? → approval1.8.1.16+
(Reporter)

Updated

9 years ago
Flags: blocking1.8.1.17?
Flags: blocking1.8.1.16?
Flags: blocking1.8.1.16+
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 
Keywords: checkin-needed → fixed1.8.1.16, fixed1.9.0.1
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → Firefox 3.1a1
(Reporter)

Updated

9 years ago
Depends on: 442970
No longer depends on: 442970
Depends on: 442970
Created attachment 328035 [details] [diff] [review]
rollup mozilla-central patch

Includes the fix for bug 442970.
Attachment #327540 - Attachment is obsolete: true
Attachment #327541 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/index.cgi/rev/cdb5ab09bc26
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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.
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".
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).
Keywords: fixed1.8.1.16 → verified1.8.1.16
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.
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.
(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

9 years ago
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

9 years ago
Please use CVE-2008-2933 for this issue.
Alias: CVE-2008-2933

Comment 31

9 years ago
Created attachment 328852 [details] [diff] [review]
1.8.0 branch patch

backported patch for 1.8.0 branch. Mike, could you please check it?
Attachment #328852 - Flags: review?
Attachment #328852 - Flags: review? → review?(mconnor)
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.
Attachment #328852 - Flags: review?(mconnor) → review-
The 1.8.0 patch looks fine otherwise, for what it's worth - was this just forgotten?
Flags: blocking1.8.0.15?

Comment 34

9 years ago
(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!
Flags: blocking1.8.0.15? → blocking1.8.0.15+

Comment 35

9 years ago
Created attachment 330047 [details] [diff] [review]
1.8.0 branch patch (2nd)

a=asac for 1.8.0 branch

Approving based on comment 33
Attachment #328852 - Attachment is obsolete: true
Attachment #330047 - Flags: approval1.8.0.15+

Comment 36

9 years ago
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) {
Product: Firefox → Toolkit
(Reporter)

Updated

9 years ago
Group: core-security

Updated

9 years ago
Depends on: 446568
Keywords: fixed1.9.1
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Flags: needinfo?(k667328)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Comment hidden (spam)
Because of persistent spam I'm going to restrict comments on this old dead bug.
Restrict Comments: true
You need to log in before you can comment on or make changes to this bug.