Closed Bug 1096319 Opened 5 years ago Closed 5 years ago

browser.js should validate opener when using it to determine sidebar to load

Categories

(Firefox :: Security, defect)

defect
Not set
Points:
2

Tracking

()

VERIFIED FIXED
Firefox 37
Iteration:
37.3 - 12 Jan
Tracking Status
firefox34 --- wontfix
firefox35 + verified
firefox36 + verified
firefox37 + verified
firefox-esr31 35+ verified

People

(Reporter: bholley, Assigned: Gijs)

Details

(Keywords: sec-high, Whiteboard: [adv-main35-][adv-esr31.4-][embargo][b2g-adv-main2.2-][b2g-unaffected])

Attachments

(3 files)

In bug 1092388 comment 15, moz_bug_r_a4 describes how browser.js behavior allows the second part of a two-part remote code execution attack (the first part, in which content may trigger window.open on browser.xul, is fixed in bug 1092388). It sounds like this was also part of the exploit that nailed us at pwn2own last year (bug 982909).

Gavin says this is Netscape-era code that needs to be fixed. Given that this shares half of the responsibility  for several remote-code execution bugs, I'm marking this sec-high.
I was a bit confused when we chatted about this earlier - different code than I thought.

Not being able to rely on window.opener being what we expect here is a bit weird, but it would be easy to add a bit of extra validation of it (e.g. enforce that it's location is "chrome://browser/content/browser.xul"). Would that help?
Flags: needinfo?(moz_bug_r_a4)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #1)
> Not being able to rely on window.opener being what we expect here is a bit
> weird, but it would be easy to add a bit of extra validation of it (e.g.
> enforce that it's location is "chrome://browser/content/browser.xul"). Would
> that help?

I think so.
(I accidentally did not clear the flag.)
Flags: needinfo?(moz_bug_r_a4)
Points: --- → 2
Flags: qe-verify-
Flags: firefox-backlog+
Summary: browser.js should not use opener to load sidebar → browser.js should validate opener when using it to determine sidebar to load
Yoink.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Attached patch Patch v0.1Splinter Review
Questions:
0) is this the right thing? I think so, but having looked at the testcases, they seem to all have been fixed by other code changes, so I'm not sure how to test this.

1) should we audit other uses of window.opener and/or has someone done that already? This is not the only use in chrome code... I don't know if others are as easy to exploit considering they usually belong to windows that never load arbitrary web content the way browser.xul does, but still...

2) this patch is so trivial it does rather pinpoint the (general) problem - should we crossland for 34rc/35/nightly and the relevant 31 esr iteration?
Attachment #8523923 - Flags: review?(gavin.sharp)
Group: firefox-core-security
Comment on attachment 8523923 [details] [diff] [review]
Patch v0.1

Looks good to me. moz_bug_r_a4@mozilla.com might have other ideas.

I'm not sure offhand whether other .opener uses should be adjusted accordingly. Can you audit them and list them here?

I don't think we need to worry about hiding this patch given that it's just a belt-and-suspenders mitigation with no current known exploit.
Attachment #8523923 - Flags: review?(gavin.sharp)
Attachment #8523923 - Flags: review+
Attachment #8523923 - Flags: feedback?(moz_bug_r_a4)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #6)
> I don't think we need to worry about hiding this patch given that it's just
> a belt-and-suspenders mitigation with no current known exploit.

Well, the known exploit is in bug 1092388, which isn't fixed on beta yet, right?
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #6)
> I'm not sure offhand whether other .opener uses should be adjusted
> accordingly. Can you audit them and list them here?

OK. Reading https://bugzilla.mozilla.org/attachment.cgi?id=8515934&action=edit, it seems like the exploit could arbitrarily open other chrome URLs, making window.opener "some other content window" instead of the chrome window, as we almost universally expect. So any code in any window that accesses window.opener (especially onload) could in theory be suspect.

However... I'm less sure as to how exploitable it would be for other windows - again, inasmuch as I understand what's going on, the fact that the code in browser.js basically takes arbitrary string data from the content window and inserts it in a privileged window+context is what kills us.

AIUI the string insertion is necessary here, that is, just creating some arbitrary getters on the content window that gets passed as window.opener won't lead to privilege escalation because wrappers/compartments. Bobby, please correct me if I'm misunderstanding.

There are quite a number of window.opener uses that dive into the opener without doing much in the way of security checks themselves. I've tried to divvy things up between onload things and other functions and "I am really relatively sure this is OK". Please sanity-check.

onload (or called from onload):
/browser/base/content/pageinfo/pageInfo.js (page info window)
    line 328 -- gWindow = window.opener.gBrowser.selectedBrowser.contentWindowAsCPOW;
/browser/base/content/pageinfo/security.js (page info window, security pane)
    line 104 -- if (window.opener.gBrowser)
    line 105 --   return window.opener.gBrowser.securityUI;
/browser/components/preferences/main.js (prefs - gone in in-content prefs, it looks like)
    line 261 -- win = window.opener;
/toolkit/components/cookie/content/cookieAcceptDialog.js (yeah, we still have a cookie accept dialog...)
    line 134 -- if (window.opener && PrivateBrowsingUtils.isWindowPrivate(window.opener)) {

other methods:
/browser/base/content/sync/utils.js (in _openLink, so would require additional user interaction, I *think*)
    line 27 -- openerDocEl = window.opener && window.opener.document.documentElement;
    line 28 -- if (thisDocEl.id == "accountSetup" && window.opener &&
/browser/components/search/content/engineManager.js (if user clicks "get more search engines" from search egnine dialog)
    line 76 -- window.opener.BrowserSearch.loadAddEngines();

/toolkit/content/finddialog.js (yeah, the old find dialog... which bails in this particular function if the opener thinks it's a private window. I guess the opener could lie about that, potentially maybe, and then have form history persist from a private window? Not super sure)
    line 92 -- window.opener.findDialog = 0;
    line 134 -- if (window.opener.PrivateBrowsingUtils &&
    line 135 -- window.opener.PrivateBrowsingUtils.isWindowPrivate(window.opener) ||



things I think are fine:
https://hg.mozilla.org/mozilla-central/annotate/7d17b594834f/browser/base/content/browser.js?mark=6635#l6626 (!!contentWindow.opener check, nothing else)

/browser/components/downloads/content/allDownloadsViewOverlay.js (library window and such, but only passes to isPrivateWindow and seems to otherwise be OK. Maybe? Assumes isPrivateWindow deals with content windows securely already)
    line 778 -- this._downloadsData = DownloadsCommon.getData(window.opener || window);



Things that actually worry me:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#132

This randomly looks for some preference elements in the parent window if it's in a "child" pref window, and then copies preference values from there. So AFAICT if the web could potentially alter child-window-pref-UI-exposed prefs this way.

The openWindow implementation a little further on looks more innocuous to me, although I'm not sure why it needs to do its own thing there...
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#1024
Flags: needinfo?(bobbyholley)
(submitted prematurely - I still need to look at some of the toolkit/ uses not listed below, but I need to run out right now - I'll get to this either tonight or tomorrow morning)
(In reply to :Gijs Kruitbosch from comment #8)
> AIUI the string insertion is necessary here, that is, just creating some
> arbitrary getters on the content window that gets passed as window.opener
> won't lead to privilege escalation because wrappers/compartments. Bobby,
> please correct me if I'm misunderstanding.

Correct - XrayWrappers will protect us there.

I didn't look in detail at the rest of comment 8, but I very much appreciate the careful and in-depth analysis.
Flags: needinfo?(bobbyholley)
So other funtimes from toolkit/:

http://mxr.mozilla.org/mozilla-central/source/toolkit/profile/content/createProfileWizard.js#213

This would require going through the full profile flow, but once it's done, you get called with a profile object. Which is where I hope and pray xraywrappers will save us from giving away file paths on the users machine including the profile hash, but rather than hoping I'd prefer to be sure...

/toolkit/obsolete/content/dialogOverlay.js reads a bunch of opener position properties, could maybe position a window offscreen. (despite the "obsolete" in the URL, this is actually still used in some places)



Then there's the login manager code (spread over LoginManagerContent/Parent and nsLoginManagerPrompter.js). It's a little messy; It's meant to deal with the case where a popup window does auth, and the "do you want to save this password" thing is supposed to show up in the opener window. It then determines the chrome window for the content opener window. Because it expects a content window and ensures it gets a chrome window before it does anything else, I think this is safe, but stuff gets passed around a lot, so it would probably be good if someone who knows the code better would have a look.
Iteration: 36.3 → 37.1
Comment on attachment 8523923 [details] [diff] [review]
Patch v0.1

I'm sorry for the delay. I mistook the meaning of the feedback request (I had been trying to find other unsafe uses of window.opener).

> Questions:
> 0) is this the right thing? I think so, but having looked at the testcases,
> they seem to all have been fixed by other code changes, so I'm not sure how
> to test this.

I think the patch is good. I'll attach a testcase, which is not affected by bug 1092388.
Attachment #8523923 - Flags: feedback?(moz_bug_r_a4) → feedback+
This works on fx >= 31 with the Browser Console. (This does not work with e10s.)
Iteration: 37.1 → 37.2
OK, so I've looked at all of these again after a quick discussion with bholley. I've tried a couple of the things to verify that wrappers/compartments save our butt here by adapting the testcase moz_bug_r_a4 provided (thanks!), which seems to be the case. There's two cases that I'm wanting to check with you, Bobby:

1) http://mxr.mozilla.org/mozilla-central/source/toolkit/obsolete/content/dialogOverlay.js#66 reads opener positions and uses that to position itself. I'm 99% sure all of these would be values provided by the wrapper (ie the page can't lie about its position) and so the page would need to be offscreen itself in order to position the dialog offscreen, which is a bit of a chicken/egg so I don't think we need to worry about it. AFAICT that code doesn't use the opener in any other way.


2) http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#132

assuming the ability to actually be able to openDialog() the arbitrary prefs XUL files, this would be exploitable (in that you could use it to set arbitrary preferences in the user's profile as long as they're included in type="child" preference pane .xul files)

You'd be doing that by including correctly named <preference> elements (HTML allows arbitrary tags), if it wasn't for line 144, which uses getElementsByAttribute, which is XUL-specific. Web can't create XUL elements, and poly-filling this in JS doesn't help because it gets wrapperized, so that makes this hard to exploit. Furthermore, it relies on node.value, which won't exist for the random <preference> elements we create. Can't use web components to give it a better prototype (with a .value) because that'd require the new element name to have a dash.

However, this last code makes me nervous enough that I'd want to add a safety check because I prefer being safe over being sorry, lest someone refactors this to use querySelector (which would make sense and would probably be faster) and getAttribute, and thereby makes it 'less secure'. Am I too paranoid yet?
Flags: needinfo?(bobbyholley)
Yes, sounds like the first is a non-issue, and we should do a document.nodePrincipal check on the second.
Flags: needinfo?(bobbyholley)
Attachment #8539326 - Flags: review?(gavin.sharp)
Iteration: 37.2 → 37.3
Attachment #8539326 - Flags: review?(jaws)
Comment on attachment 8539326 [details] [diff] [review]
Fix preferences.xml

Annoying that there's no good way to reliably use Services.jsm in XBL bindings.
Attachment #8539326 - Flags: review?(jaws)
Attachment #8539326 - Flags: review?(gavin.sharp)
Attachment #8539326 - Flags: review+
Thinking of more robust long-term fixes, can we somehow make chromeWindow.opener always return null in cases where it would currently return a content window?
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #18)
> Thinking of more robust long-term fixes, can we somehow make
> chromeWindow.opener always return null in cases where it would currently
> return a content window?

Sounds sensible to me. Looks like we should be able to just check for this case in http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#4447 . I can file a separate (non-sec-sensitive) bug for that, if that sounds good?
Comment on attachment 8523923 [details] [diff] [review]
Patch v0.1

NB: requesting sec-approval and branch approval to land both patches in one go.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not trivially. See bug 1092388.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Only sort of, but the window.opener use is only half of the security issue here, and the other half was landed on 35+ already (again, see bug 1092388).

Which older supported branches are affected by this flaw?
release/beta/aurora (34/35/36), 31esr -- so, all of them.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I've not made these yet; I don't really anticipate having to do anything to the patches themselves besides conflicts in the context. So, easy to create, very low risk. I would like to land on aurora/beta as soon as this lands on central without blowing up the tree. To that end, I'm requesting approval immediately.

How likely is this patch to cause regressions; how much testing does it need?
Very very very low risk / unlikely to cause regressions. Doesn't really need much testing.


Approval Request Comment
[Risks and why]: low risk, small security fixes
[String/UUID change made/needed]: no.
Attachment #8523923 - Flags: sec-approval?
Attachment #8523923 - Flags: approval-mozilla-esr31?
Attachment #8523923 - Flags: approval-mozilla-beta?
Attachment #8523923 - Flags: approval-mozilla-aurora?
(In reply to :Gijs Kruitbosch from comment #19)
> Sounds sensible to me. Looks like we should be able to just check for this
> case in
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.
> cpp#4447 . I can file a separate (non-sec-sensitive) bug for that, if that
> sounds good?

Yes please!
Comment on attachment 8523923 [details] [diff] [review]
Patch v0.1

sec-approval+ for trunk. Once it is in, please nominate patches for Beta, Aurora, and ESR31.
Attachment #8523923 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/b6c3d0131066
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment on attachment 8523923 [details] [diff] [review]
Patch v0.1

We'll have to take this for the RC on Mon Jan 5th due to timing.
Attachment #8523923 - Flags: approval-mozilla-esr31?
Attachment #8523923 - Flags: approval-mozilla-esr31+
Attachment #8523923 - Flags: approval-mozilla-beta?
Attachment #8523923 - Flags: approval-mozilla-beta+
Attachment #8523923 - Flags: approval-mozilla-aurora?
Attachment #8523923 - Flags: approval-mozilla-aurora+
Unfortunately only 1 of the two patches landed (see comment #20). I guess in future I should set the flags on both attachments to avoid any confusion. :-(

I've landed the other one on fx-team:

remote:   https://hg.mozilla.org/integration/fx-team/rev/b5128f5b0876

I will land the first patch on branches in a second, and the second one on 36 after it gets merged to m-c, as it's not essential that that patch gets taken for 35, and it has had no trunk baking, so at this point I can't be sure it won't bust the tree.
Whiteboard: [adv-main35+][adv-esr31.4+]
Should this be embargoed because it was found via bug 1092388, which we just embargoed? 

When we do write about it, should we credit moz_bug_r_a4 since he found 1092388.
Flags: needinfo?(bobbyholley)
Flags: needinfo?(bobbyholley) → needinfo?(gijskruitbosch+bugs)
(In reply to Al Billings [:abillings] from comment #30)
> Should this be embargoed because it was found via bug 1092388, which we just
> embargoed? 

I'm not 100% sure what 'embargoed' means here, but logically this suggestion makes sense, yes.

> When we do write about it, should we credit moz_bug_r_a4 since he found
> 1092388.

Yes.
Flags: needinfo?(gijskruitbosch+bugs)
Sorry, I meant to ask Bobby since he asked for bug 1092388 to be embargoed (which means we don't discuss it or write an advisory for it until a later date). Bad needinfo? on my part.
Flags: needinfo?(bobbyholley)
Oh, and now I notice that I did it right and Bobby reassigned it. Doh. 

Bobby, do you have any thoughts here? Otherwise, I'm writing an advisory on this for 35 but it seems odd with the bug that was the vehicle for finding this being embargoed.
(In reply to Al Billings [:abillings] from comment #33)
> Oh, and now I notice that I did it right and Bobby reassigned it. Doh. 

Yes, because I'm busy.

> Bobby, do you have any thoughts here? Otherwise, I'm writing an advisory on
> this for 35 but it seems odd with the bug that was the vehicle for finding
> this being embargoed.

We could probably get away with it - this is basically a bug that happens if you can spoof 'opener', but doesn't tell you _how_ to spoof opener (which is the other bug) - so IIUC it's not really sec-high on its own. I'm more for just embargoing it to be on the safe side.
Flags: needinfo?(bobbyholley)
Group: firefox-core-security
I reproduced the original issue using the following build:
* http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/10/2014-10-11-00-05-02-mozilla-esr31/

Followed the instructions in the POC and received the following prompt in a new fx window:
* "JS Frame::javascript:alert(Components.stack);::<TOP_LEVEL>::line 1"

OS's used:

* Ubuntu 14.04.1 x64 - PASSED
* Windows 8.1 x64 - PASSED

Verified using the following builds:

* http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/31.4.0esr-candidates/build1/
* http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-01-09-03-02-24-mozilla-central/
* http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-01-09-00-40-05-mozilla-aurora/
* http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/35.0-candidates/build3/

With the newer builds mentioned above, I receive a blank window without the prompt message that I listed above. Gijs, does this look sufficient for verifying this bug?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Kamil Jozwiak [:kjozwiak] from comment #35)
> With the newer builds mentioned above, I receive a blank window without the
> prompt message that I listed above. Gijs, does this look sufficient for
> verifying this bug?

Per IRC discussion, yes, this looks good.
Flags: needinfo?(gijskruitbosch+bugs)
Comment 30 is private: false
Whiteboard: [adv-main35+][adv-esr31.4+] → [adv-main35-][adv-esr31.4-][embargo]
Whiteboard: [adv-main35-][adv-esr31.4-][embargo] → [adv-main35-][adv-esr31.4-][embargo][b2g-adv-main2.2-][b2g-unaffected]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.