Closed Bug 735073 Opened 12 years ago Closed 12 years ago

Plugins can be fooled by window.location (again)


(Core :: XPConnect, defect)

Not set



Tracking Status
firefox11 --- affected
firefox12 + verified
firefox13 + verified
firefox14 + verified
firefox-esr10 12+ verified
status1.9.2 --- unaffected


(Reporter: bholley, Assigned: bholley)



(Keywords: regression, sec-high, Whiteboard: [sg:high (at least)][qa!])


(4 files)

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

We've theoretically got test coverage for it:

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.

> 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.
Assignee: nobody → bobbyholley+bmo
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.
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.  :-(
Attached patch aurora fix - v1Splinter Review
Aurora fix, more or less identical to the one in bug 733984. Flagging mrbkap for rubberstamp.
Attachment #613501 - Flags: review?(mrbkap)
Attached patch beta/esr10 fixSplinter Review
Beta/ESR10 fix. Mostly identical modulo a few merge conflicts. Flagging mrbkap for rubberstamp.
Attachment #613503 - Flags: review?(mrbkap)
Attachment #613501 - Flags: review?(mrbkap) → review+
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+
Attachment #613501 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Pushed to m-a:

I'll let this build before pushing to m-b.
Pushed to m-b at lsblakk's requests:
Keywords: sec-high
This landed everywhere, so I think it should be ok to resolve.
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [sg:high (at least)] → [sg:high (at least)][qa+]
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.