Last Comment Bug 756719 - (CVE-2012-1956) Object.defineProperty can shadow window.location
: Object.defineProperty can shadow window.location
[js:p1:fx15][advisory-tracking+] fixe...
: sec-high
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: 12 Branch
: x86_64 Windows 7
: -- normal (vote)
: ---
Assigned To: Jason Orendorff [:jorendorff]
: Andrew Overholt [:overholt]
Depends on: 750307
Blocks: CVE-2012-3994
  Show dependency treegraph
Reported: 2012-05-18 20:40 PDT by Mariusz Mlynski
Modified: 2014-06-30 12:26 PDT (History)
17 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

PoC (130 bytes, text/html)
2012-05-18 20:40 PDT, Mariusz Mlynski
no flags Details
shadowing top.location test (300 bytes, text/html)
2012-06-16 02:41 PDT, Mariusz Mlynski
no flags Details
landed in mc (10.11 KB, patch)
2012-06-18 16:50 PDT, Jason Orendorff [:jorendorff]
jorendorff: review+
akeybl: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review
Part 2: test fixup (needed in ff14) (1.47 KB, patch)
2012-07-10 11:41 PDT, Jason Orendorff [:jorendorff]
bobbyholley: review+
Details | Diff | Splinter Review
Extra kludge for ESR10 (1.70 KB, patch)
2012-10-04 04:44 PDT, Jason Orendorff [:jorendorff]
jst: review+
jst: superreview+
bajaj.bhavana: approval‑mozilla‑esr10+
Details | Diff | Splinter Review
Kludge v2 for ESR10 (1.89 KB, patch)
2012-10-05 11:10 PDT, Blake Kaplan (:mrbkap)
jorendorff: review+
Details | Diff | Splinter Review

Description Mariusz Mlynski 2012-05-18 20:40:33 PDT
Created attachment 625346 [details]

Yet another variant of bug 735073, bug 665548, and bug 541530: it is possible to shadow the location object using Object.defineProperty. Verified with the current release build.
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-05-19 01:15:31 PDT
If I ever have a tumor, I'm going to name it Location.

So, it appears that we do crazy things in the |window| resolve hook when setting up the Location property. In particular, we seem to be defining "location" as a property with a value, a null getter, and a setter:

I'm pretty sure that makes no sense in an ES5 world, and that we should be using a getter here instead. Waldo, can you confirm? Blake, do you know anything about this?
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-05-25 10:26:51 PDT
Jorendorff say that this is actually bug 750307, which he's patching today. Hopefully, this will be fixed on monday (though we should still probably backport the fix).
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-05-25 10:27:11 PDT
Also, we should avoid marking the dependency here, since the other bug is not s-s.
Comment 5 Jason Orendorff [:jorendorff] 2012-06-15 05:18:51 PDT
Fixed by the patch in bug 750307.
Comment 6 Daniel Veditz [:dveditz] 2012-06-15 10:35:32 PDT
resolving based on comment 5
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-15 10:52:30 PDT
Apologies for not seeing this earlier, my bugmail reading is way way out of date these days...

(In reply to Bobby Holley (:bholley) from comment #1)
> So, it appears that we do crazy things in the |window| resolve hook when
> setting up the Location property. In particular, we seem to be defining
> "location" as a property with a value, a null getter, and a setter:
> cpp#7112
> I'm pretty sure that makes no sense in an ES5 world, and that we should be
> using a getter here instead. Waldo, can you confirm? Blake, do you know
> anything about this?

You're seeing a slotful accessor property here -- a property with a slot and a value in it, and with a JSStrictPropertyOp setter associated with it.  (You could have a JSPropertyOp getter, too, but you don't here.)  The idea (just like JSPropertyOp and JSStrictPropertyOp more finely) is a concept that doesn't exist in ECMAScript, which only has properties containing a value and properties with a getter/no getter and a setter/no setter.  So, yes, it is mostly nonsensical.

As for what "should" be done, the location property has this WebIDL:

  [PutForwards=href, Unforgeable] readonly attribute Location location;

I believe this corresponds to a non-configurable property with a getter that returns the actual Location object and a setter that (roughly) does |window.location.href = ...|.  Which obviously isn't what we implement, although with enough fakery we could pretend what we have looks like that, if examined closely.  I did a patch like that for bug 560072 once, but it's a giant overly complicated mess.  We should just make this into a normal accessor property.

So basically it's just the usual thing of any use of JS{Strict,}PropertyOp being the wrong thing to do, film at 11, mixed with the also-bad "slotful" concept applied to accessors.  Aren't you glad you asked?
Comment 12 Al Billings [:abillings] 2012-06-18 06:43:27 PDT
Verified fixed in the 6/18 trunk nightly (and confirmed bug with Firefox 13).
Comment 13 Jason Orendorff [:jorendorff] 2012-06-18 16:50:21 PDT
Created attachment 634237 [details] [diff] [review]
landed in mc

Carrying forward review from bug 750307.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  Already sec:high and tracking FF14, FF15, ESR10.
User impact if declined:
  Security regression; bholley or mrbkap can explain further if needed.
Fix Landed on Version:
  landed in m-c for FF16
Risk to taking this patch (and alternatives if risky):
  Low to moderate risk of breaking some really bad addon code somewhere;
  however the patch went into FF16 with no problem AFAIK, and the security
  issue due to this bug is worse.
String or UUID changes made by this patch: 
Comment 14 Alex Keybl [:akeybl] 2012-06-19 20:04:54 PDT
Comment on attachment 634237 [details] [diff] [review]
landed in mc

[Triage Comment]
Let's start by uplifting to Aurora and letting that sit until Monday. We'll approve/land before the next beta if everything looks good.
Comment 15 Alex Keybl [:akeybl] 2012-06-24 12:17:05 PDT
Why hasn't this landed yet? The late landing on Aurora is putting us at risk of landing on Beta 14.
Comment 16 Jason Orendorff [:jorendorff] 2012-06-29 04:33:53 PDT
Landed early yesterday:
Comment 17 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-02 13:34:35 PDT
Comment on attachment 634237 [details] [diff] [review]
landed in mc

Approving for landing on beta/esr10 -- please land before noon PT tomorrow (Tuesday) so it can be in the next beta that will go to build tomorrow afternoon.  Update status flags once landed.
Comment 18 Jason Orendorff [:jorendorff] 2012-07-04 06:15:42 PDT
I didn't see this in time.
Comment 19 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-06 00:50:29 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #18)
> I didn't see this in time.

Adjusting the flag for Aurora (15) - we still have one more beta before release of 14 so there's still time to get this landed on both beta and ESR branches.  Can you let us know if it will be easy to tell if there is fallout after landing without being shipped to user population? (ie: is there a test that might fail if this doesn't land cleanly?).
Comment 20 Jason Orendorff [:jorendorff] 2012-07-09 10:40:40 PDT
The symptom will be that some plug-in or addon with a binary component will mysteriously stop working. It won't crash; it'll just error out, when previously it was silently clobbering purportedly-unclobberable properties.

However it seems clear that we should take this patch in beta, since the alternatives, as I see it, are:
- ship a known sec:high regression in 14
- land some totally untested patch in 14 right before release
Comment 21 Jason Orendorff [:jorendorff] 2012-07-09 16:17:59 PDT
lsblakk clarified on IRC that the approval-beta+ stands.


The bug number in the commit message still refers to the (non-security) bug where this was originally reported and fixed.
Comment 22 Jason Orendorff [:jorendorff] 2012-07-10 07:04:23 PDT
This bounced. It's causing a mochitest to fail (not crash):
and it looks like it's failing because of the intentional change in behavior.

That mochitest has been moved and renamed and otherwise reorganized in tip; I'm looking into it. Probably the fix is just to make it expect Object.defineProperty to throw.

(The first paragraph of comment 20 was inaccurate. This change affects JS Object.defineProperty callers. The symptom will be that some addon, not necessarily one with a binary component, will mysteriously stop working. Second paragraph stands.)
Comment 23 Al Billings [:abillings] 2012-07-10 11:35:24 PDT
This is going in for Firefox 14 and ESR real soon now or is it being punted to Firefox 15 and ESR 15+?
Comment 24 Jason Orendorff [:jorendorff] 2012-07-10 11:41:02 PDT
Created attachment 640700 [details] [diff] [review]
Part 2: test fixup (needed in ff14)

Here is the fix for the test.
Comment 25 Jason Orendorff [:jorendorff] 2012-07-10 11:45:05 PDT
Comment on attachment 640700 [details] [diff] [review]
Part 2: test fixup (needed in ff14)

A test is using Object.defineProperty to redefine It expects that to succeed. But it should not succeed; that's evil and it should throw. The main patch in this bug fixes that.

This patch simply changes the test to expect the right thing, i.e. expect Object.defineProperty(window, 'top', ...) to throw.
Comment 26 Jason Orendorff [:jorendorff] 2012-07-10 13:03:37 PDT
Incidentally: nothing like this test is in the tree anymore. The test was deleted in rev f6f8d92907b5.

It took me a while to figure this out since apparently a completely unrelated test was later created with the same filename, and then it was renamed to something else. (Plus hg is good at "--follow"ing files backwards in time across renames, but not forwards.)
Comment 27 Bobby Holley (:bholley) (busy with Stylo) 2012-07-10 13:27:25 PDT
Comment on attachment 640700 [details] [diff] [review]
Part 2: test fixup (needed in ff14)

This is fine. I think I remember some hubub about this test back in bug 735073. But anyway, go ahead and land it.
Comment 28 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-12 17:17:40 PDT
comment 22 actually means that this is not landed on beta right now and at this stage it's too late to respin a beta with this in - combined with not landing cleanly on esr yet - we've decided to keep this on the radar if there is a 14 chemspill but not to take it for tomorrow morning's go to build.
Comment 29 Al Billings [:abillings] 2012-08-23 11:08:04 PDT
I'm trying to figure out what the security implications are of shadowing the location object using Object.defineProperty. What could an attacker possibly do with this specifically?
Comment 30 Blake Kaplan (:mrbkap) 2012-08-23 11:38:28 PDT
(In reply to Al Billings [:abillings] from comment #29)
> I'm trying to figure out what the security implications are of shadowing the
> location object using Object.defineProperty. What could an attacker possibly
> do with this specifically?

I think the only reason we care about this is that it means that an attacker can confuse Flash (or other plugins) into thinking that we're on one domain when, in reality, we're on another one leading to XSS attacks.
Comment 31 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-28 10:46:32 PDT
Since this has yet to land on ESR, moving tracking back to ? so we can evaluate whether this is urgent enough to have to do an ESR chemspill or get it into 10.0.8
Comment 32 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-28 10:55:13 PDT
Confirmed we'll want this in mozilla-esr10 for our 10.0.8 ESR release that will ship with FF 16.  Please land to mozilla-esr10 branch.
Comment 33 Andrew McCreight [:mccr8] 2012-09-18 09:17:25 PDT
Jason, are you going to land this on ESR-10?
Comment 34 Jason Orendorff [:jorendorff] 2012-10-02 14:20:46 PDT

Commit message points to bug 750307, where the patch was initially developed.

There's nothing extra to commit to ESR10. This is it. As comment 5 says, this bug is a dup.
Comment 35 Andrew McCreight [:mccr8] 2012-10-02 15:12:17 PDT
Comment 36 Jason Orendorff [:jorendorff] 2012-10-03 07:13:13 PDT
This has been backed out. I think it's just problems with tests that are using Object.defineProperty, testing that it won't crash but implicitly assuming that it won't throw. I hit one of those in FF14 as well.

I'll look at it today.
Comment 37 Jason Orendorff [:jorendorff] 2012-10-03 11:35:21 PDT
The problem is not just tests.

I'm having a bit of trouble tracking down where the exception is coming from, but not stuck yet. More in a bit...
Comment 38 Jason Orendorff [:jorendorff] 2012-10-03 12:31:26 PDT
OK, I can see what's happening here now.

In mochitest-4, layout/base/tests/test_bug607529.html, nsJSContext::ClearScope calls JS_DefineProperty on a proxy, which triggers the new CheckDefineProperty stuff.

nsJSContext::ClearScope is designed to catch and squelch exceptions; unfortunately this exception is forwarded to window.onerror first, for bizarre reasons we don't need to get into here.

I'll try hacking ClearScope and we'll see if that fixes both test failures.
Comment 39 Jason Orendorff [:jorendorff] 2012-10-04 04:44:58 PDT
Created attachment 667903 [details] [diff] [review]
Extra kludge for ESR10

This makes the tests pass.

Try Server is apparently no longer capable of building ESR10, but I ran the tests that were failing on tinderbox locally, and all of mochitest-4 ran clean.
Comment 40 Johnny Stenback (:jst, 2012-10-04 15:20:31 PDT
Comment on attachment 667903 [details] [diff] [review]
Extra kludge for ESR10

Comment 42 Andrew McCreight [:mccr8] 2012-10-04 17:15:45 PDT
backed out for Moth orange
Comment 43 Blake Kaplan (:mrbkap) 2012-10-05 11:10:27 PDT
Created attachment 668514 [details] [diff] [review]
Kludge v2 for ESR10

I debugged this a little and found that in the cases where nsJSContext::ClearScope was throwing, we were actually trying to clear the outer window. JS_ClearScope on an outer window is basically a no-op (though it does re-seed the random number generator) so when we went to redefine the window property on it, we'd forward that definition on to the inner window which already had a window property and that'd throw. This patch is as non-invasive as I could make it.
Comment 44 Blake Kaplan (:mrbkap) 2012-10-05 11:24:03 PDT

Should fix this on esr10. Fingers crossed

My kludge v2 patch had a=bajaj over IRC.
Comment 45 Raymond Forbes[:rforbes] 2013-07-19 18:24:19 PDT

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