Plugins can be fooled by window.location (again)

VERIFIED FIXED

Status

()

Core
XPConnect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({regression, sec-high})

unspecified
regression, sec-high
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox11 affected, firefox12+ verified, firefox13+ verified, firefox14+ verified, firefox-esr1012+ verified, status1.9.2 unaffected)

Details

(Whiteboard: [sg:high (at least)][qa!])

Attachments

(4 attachments)

Created attachment 605161 [details]
testcase

Looks like we regressed CVE-2010-0170 (bug 541530) some time ago. Probably with compartments.

We've theoretically got test coverage for it: http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/bugs/test_bug541530.html

Unfortunately, the test coverage gets fooled when window.location.__defineGetter__ doesn't exist, and incorrectly concludes that the object is immutable. But we can easily do window.__defineGetter__.apply(window.location, ...) and do the same thing. Ruh roh (testcase attached).

I discovered this when I broke this test with my location wrapper changes in compartment-per-global, and started wondering how we currently deal with the problem.

I already need to change this stuff around to work right with my compartment-per-global changes. So I think the safest and most discrete approach is for me to just fix it there (bug 733984), and then backport to branches later if need be.

Thoughts?
> So I think the safest and most discrete approach is for me to just fix it
> there (bug 733984), and then backport to branches later if need be.

I'm fine with the first half of that sentence; the second half needs to be "backport to branches ASAP".

The backport patches can/should live in this bug once you figure out all the places that need fixing in the other bug.
status1.9.2: --- → unaffected
status-firefox-esr10: --- → affected
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
tracking-firefox-esr10: --- → 12+
tracking-firefox12: --- → +
tracking-firefox13: --- → +
tracking-firefox14: --- → +
Keywords: regression
Whiteboard: [sg:high (at least)]
Assignee: nobody → bobbyholley+bmo
Created attachment 605209 [details] [diff] [review]
updated test coverage to expose the issue

I've added the updated test coverage that tests things properly. It's probably best to avoid landing it right now, so I'll take responsibility for landing it in a month or so.
jst and I just talked about this for a little bit.

I'm finding it cumbersome for various reasons to avoid fixing the tests in the same patch as I update the behavior (bug 733984). I'm going to have to update the test anyway for other reasons, so I might as well just fix it. I don't think the test fixes make it at all obvious that we were previously vulnerable.

A bigger concern is backporting this. Backporting any of the new Location object machinery I'm writing will make it crystal clear what the problem is. So if we're trying to keep it a secret, we might need to figure out something clever there.
The "clever" thing is to not leave much of a window between landing an shipping. Depends on how big a change the back-port turns out to be, and how different from what got tested on mozilla-central, but if we can hold off landing on beta and aurora until last minute (two weeks before merge, say) that might be best.
[Triage Comment]
Removing the tracking on this, leaving status-firefox-esr10 affected, we'll track it for the right esr release once it's fixed.
tracking-firefox-esr10: 12+ → ---
Is bug 736694 due to the same problem?
(In reply to Daniel Veditz [:dveditz] from comment #6)
> Is bug 736694 due to the same problem?

Yep. Same issue. Object.defineProperty is just a newfangled mechanism for __defineGetter__/__defineSetter__.
__define{G,S}etter__ just calls the same method used to implement Object.defineProperty, actually.  (There's one case where they're not the same -- if the property in question is quickstubbed -- but we definitely want to kill that.)
(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #8)
> (There's one case where they're not the
> same -- if the property in question is quickstubbed -- but we definitely
> want to kill that.)

Well, we're killing quickstubs entirely once the new DOM bindings are all settled. Hopefully the new bindings will do the right thing here.
Well...see bug 738525.  :-(
Duplicate of this bug: 736694
Created attachment 613501 [details] [diff] [review]
aurora fix - v1

Aurora fix, more or less identical to the one in bug 733984. Flagging mrbkap for rubberstamp.
Attachment #613501 - Flags: review?(mrbkap)
Created attachment 613503 [details] [diff] [review]
beta/esr10 fix

Beta/ESR10 fix. Mostly identical modulo a few merge conflicts. Flagging mrbkap for rubberstamp.
Attachment #613503 - Flags: review?(mrbkap)

Updated

5 years ago
Attachment #613501 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #613503 - Flags: review?(mrbkap) → review+
Comment on attachment 613501 [details] [diff] [review]
aurora fix - v1

[Approval Request Comment]
Regression caused by (bug #): brain transplants (ff4)
User impact if declined: Vulnerable plugins can be fooled into giving local privileges to untrusted content.
Testing completed (on m-c, etc.): baked on m-c for ~2 weeks.
Risk to taking this patch (and alternatives if risky): low-medium. The main risk is that developers may have started depending on shadowing location object expandos in the gap since FF4. It's unlikely though, since webkit disallows shadowing entirely.

String changes made by this patch: An exception string was added ("Permission denied to shadow native property"). I don't believe that these are localized.

dveditz wanted to wait until the last minute to land this on all branches, since the exploit is obvious. As such, I'm flagging for approval on all three branches, rather than waiting for it to bake on m-a.
Attachment #613501 - Flags: approval-mozilla-aurora?
Attachment #613503 - Flags: approval-mozilla-esr10?
Attachment #613503 - Flags: approval-mozilla-beta?
Comment on attachment 613503 [details] [diff] [review]
beta/esr10 fix

[Triage Comment]
Approved for all branches. Please land ASAP (esp. for mozilla-beta), and include any info on what we should be on the lookout for w/r/t where we might expect to find regressions.
Attachment #613503 - Flags: approval-mozilla-esr10?
Attachment #613503 - Flags: approval-mozilla-esr10+
Attachment #613503 - Flags: approval-mozilla-beta?
Attachment #613503 - Flags: approval-mozilla-beta+

Updated

5 years ago
Attachment #613501 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed to m-a: http://hg.mozilla.org/releases/mozilla-aurora/rev/01ffc413b4cc

I'll let this build before pushing to m-b.
Pushed to m-b at lsblakk's requests: http://hg.mozilla.org/releases/mozilla-beta/rev/bb3d8a18edd5
And, for good measure, esr10: http://hg.mozilla.org/releases/mozilla-esr10/rev/f00f7011a218

Updated

5 years ago
status-firefox-esr10: affected → fixed
status-firefox12: affected → fixed
status-firefox13: affected → fixed
tracking-firefox-esr10: --- → 12+
Keywords: sec-high
This landed everywhere, so I think it should be ok to resolve.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox14: affected → fixed
Resolution: --- → FIXED
Whiteboard: [sg:high (at least)] → [sg:high (at least)][qa+]

Updated

5 years ago
status-firefox12: fixed → verified

Updated

5 years ago
status-firefox-esr10: fixed → verified

Updated

5 years ago
status-firefox14: fixed → verified

Updated

5 years ago
Status: RESOLVED → VERIFIED

Updated

5 years ago
status-firefox13: fixed → verified

Updated

5 years ago
Whiteboard: [sg:high (at least)][qa+] → [sg:high (at least)][qa!]
Group: core-security
Bobby, did the tests here ever land? (There's no evidence of that in this bug, but perhaps elsewhere.)
Flags: in-testsuite?
Yes, this landed with bug 733984.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.