Last Comment Bug 735073 - Plugins can be fooled by window.location (again)
: Plugins can be fooled by window.location (again)
Status: VERIFIED FIXED
[sg:high (at least)][qa!]
: regression, sec-high
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
:
Mentors:
: 736694 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-12 15:24 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2012-10-16 02:32 PDT (History)
13 users (show)
bobbyholley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
verified
+
verified
+
verified
12+
verified
unaffected


Attachments
testcase (255 bytes, text/html)
2012-03-12 15:24 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details
updated test coverage to expose the issue (2.65 KB, patch)
2012-03-12 16:16 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details | Diff | Splinter Review
aurora fix - v1 (6.95 KB, patch)
2012-04-09 23:04 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
beta/esr10 fix (6.94 KB, patch)
2012-04-09 23:04 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-03-12 15:24:31 PDT
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?
Comment 1 Daniel Veditz [:dveditz] 2012-03-12 15:39:14 PDT
> 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) (busy with Stylo) 2012-03-12 16:16:18 PDT
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.
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-03-13 16:51:39 PDT
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] 2012-03-13 18:00:27 PDT
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 2012-03-20 10:05:19 PDT
[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] 2012-03-22 14:00:56 PDT
Is bug 736694 due to the same problem?
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2012-03-22 18:03:25 PDT
(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] (remove +bmo to email) 2012-03-22 19:06:00 PDT
__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) (busy with Stylo) 2012-03-22 19:26:13 PDT
(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] (remove +bmo to email) 2012-03-22 19:44:25 PDT
Well...see bug 738525.  :-(
Comment 11 Daniel Veditz [:dveditz] 2012-03-23 16:43:25 PDT
*** Bug 736694 has been marked as a duplicate of this bug. ***
Comment 12 Bobby Holley (:bholley) (busy with Stylo) 2012-04-09 23:04:00 PDT
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.
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2012-04-09 23:04:59 PDT
Created attachment 613503 [details] [diff] [review]
beta/esr10 fix

Beta/ESR10 fix. Mostly identical modulo a few merge conflicts. Flagging mrbkap for rubberstamp.
Comment 14 Bobby Holley (:bholley) (busy with Stylo) 2012-04-10 09:42:11 PDT
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] 2012-04-10 12:26:53 PDT
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) (busy with Stylo) 2012-04-10 13:13:32 PDT
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) (busy with Stylo) 2012-04-10 14:20:24 PDT
Pushed to m-b at lsblakk's requests: http://hg.mozilla.org/releases/mozilla-beta/rev/bb3d8a18edd5
Comment 18 Bobby Holley (:bholley) (busy with Stylo) 2012-04-10 14:22:10 PDT
And, for good measure, esr10: http://hg.mozilla.org/releases/mozilla-esr10/rev/f00f7011a218
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2012-05-09 01:18:52 PDT
This landed everywhere, so I think it should be ok to resolve.
Comment 20 Andrew McCreight [:mccr8] 2012-10-15 08:35:23 PDT
Bobby, did the tests here ever land? (There's no evidence of that in this bug, but perhaps elsewhere.)
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2012-10-16 02:32:12 PDT
Yes, this landed with bug 733984.

Note You need to log in before you can comment on or make changes to this bug.