Closed
Bug 1096319
Opened 10 years ago
Closed 9 years ago
browser.js should validate opener when using it to determine sidebar to load
Categories
(Firefox :: Security, defect)
Firefox
Security
Tracking
()
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)
1.21 KB,
patch
|
Gavin
:
review+
moz_bug_r_a4
:
feedback+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
lsblakk
:
approval-mozilla-esr31+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
535 bytes,
text/html
|
Details | |
2.89 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
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)
Comment 2•10 years ago
|
||
(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.
Updated•10 years ago
|
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
Assignee | ||
Comment 4•10 years ago
|
||
Yoink.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Assignee | ||
Comment 5•10 years ago
|
||
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)
Updated•10 years ago
|
Group: firefox-core-security
Comment 6•10 years ago
|
||
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)
Reporter | ||
Comment 7•10 years ago
|
||
(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?
Assignee | ||
Comment 8•10 years ago
|
||
(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)
Assignee | ||
Comment 9•10 years ago
|
||
(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)
Reporter | ||
Comment 10•10 years ago
|
||
(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.
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 11•10 years ago
|
||
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.
Updated•9 years ago
|
Iteration: 36.3 → 37.1
Comment 12•9 years ago
|
||
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+
Comment 13•9 years ago
|
||
This works on fx >= 31 with the Browser Console. (This does not work with e10s.)
Updated•9 years ago
|
Iteration: 37.1 → 37.2
Assignee | ||
Comment 14•9 years ago
|
||
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)
Reporter | ||
Comment 15•9 years ago
|
||
Yes, sounds like the first is a non-issue, and we should do a document.nodePrincipal check on the second.
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8539326 -
Flags: review?(gavin.sharp)
Updated•9 years ago
|
Iteration: 37.2 → 37.3
Assignee | ||
Updated•9 years ago
|
Attachment #8539326 -
Flags: review?(jaws)
Comment 17•9 years ago
|
||
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+
Comment 18•9 years ago
|
||
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?
Assignee | ||
Comment 19•9 years ago
|
||
(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?
Assignee | ||
Comment 20•9 years ago
|
||
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?
Comment 21•9 years ago
|
||
(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!
Updated•9 years ago
|
status-firefox34:
--- → affected
status-firefox35:
--- → affected
status-firefox36:
--- → affected
status-firefox37:
--- → affected
status-firefox-esr31:
--- → affected
tracking-firefox35:
--- → +
tracking-firefox36:
--- → +
tracking-firefox37:
--- → +
tracking-firefox-esr31:
--- → ?
Comment 22•9 years ago
|
||
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+
Updated•9 years ago
|
Updated•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b6c3d0131066
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment 25•9 years ago
|
||
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+
Assignee | ||
Comment 26•9 years ago
|
||
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.
Assignee | ||
Comment 27•9 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/5deadff8cf18 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/c061a8d7d529 remote: https://hg.mozilla.org/releases/mozilla-esr31/rev/ec4f6e86bedb
Updated•9 years ago
|
Whiteboard: [adv-main35+][adv-esr31.4+]
Assignee | ||
Comment 29•9 years ago
|
||
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/66553bdbe522
Comment 30•9 years ago
|
||
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)
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(bobbyholley) → needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 31•9 years ago
|
||
(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)
Comment 32•9 years ago
|
||
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)
Comment 33•9 years ago
|
||
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.
Reporter | ||
Comment 34•9 years ago
|
||
(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)
Updated•9 years ago
|
Group: firefox-core-security
Comment 35•9 years ago
|
||
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)
Assignee | ||
Comment 36•9 years ago
|
||
(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)
Updated•9 years ago
|
Comment 30 is private:
false
Comment 31 is private:
false
Whiteboard: [adv-main35+][adv-esr31.4+] → [adv-main35-][adv-esr31.4-][embargo]
Updated•9 years ago
|
Whiteboard: [adv-main35-][adv-esr31.4-][embargo] → [adv-main35-][adv-esr31.4-][embargo][b2g-adv-main2.2-][b2g-unaffected]
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•