735073 - Plugins can be fooled by window.location (again)

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Attachment: 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?

Comment 1

[Daniel Veditz [:dveditz]]

> 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.

Comment 2

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 3

[Bobby Holley (:bholley)]

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.

Comment 4

[Daniel Veditz [:dveditz]]

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.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

[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.

Comment 6

[Daniel Veditz [:dveditz]]

Is bug 736694 due to the same problem?

Comment 7

[Bobby Holley (:bholley)]

(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__.

Comment 8

[Jeff Walden [:Waldo]]

__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.)

Comment 9

[Bobby Holley (:bholley)]

(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.

Comment 10

[Jeff Walden [:Waldo]]

Well...see bug 738525.  :-(

Comment 12

[Bobby Holley (:bholley)]

Attachment: aurora fix - v1

Aurora fix, more or less identical to the one in bug 733984. Flagging mrbkap for rubberstamp.

Comment 13

[Bobby Holley (:bholley)]

Attachment: beta/esr10 fix

Beta/ESR10 fix. Mostly identical modulo a few merge conflicts. Flagging mrbkap for rubberstamp.

Comment 14

[Bobby Holley (:bholley)]

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.

Comment 15

[Alex Keybl [:akeybl]]

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.

Comment 16

[Bobby Holley (:bholley)]

Pushed to m-a: http://hg.mozilla.org/releases/mozilla-aurora/rev/01ffc413b4cc

I'll let this build before pushing to m-b.

Comment 17

[Bobby Holley (:bholley)]

Pushed to m-b at lsblakk's requests: http://hg.mozilla.org/releases/mozilla-beta/rev/bb3d8a18edd5

Comment 18

[Bobby Holley (:bholley)]

And, for good measure, esr10: http://hg.mozilla.org/releases/mozilla-esr10/rev/f00f7011a218

Comment 19

[Bobby Holley (:bholley)]

This landed everywhere, so I think it should be ok to resolve.

Comment 20

[Andrew McCreight [:mccr8]]

Bobby, did the tests here ever land? (There's no evidence of that in this bug, but perhaps elsewhere.)

Comment 21

[Bobby Holley (:bholley)]

Yes, this landed with bug 733984.