Closed Bug 756719 (CVE-2012-1956) Opened 12 years ago Closed 12 years ago

Object.defineProperty can shadow window.location

Categories

(Core :: XPConnect, defect)

12 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
firefox14 + wontfix
firefox15 + fixed
firefox16 + fixed
firefox-esr10 16+ fixed

People

(Reporter: marius.mlynski, Assigned: jorendorff)

References

Details

(Keywords: sec-high, Whiteboard: [js:p1:fx15][advisory-tracking+] fixed by bug 750307)

Attachments

(5 files, 1 obsolete file)

Attached file PoC
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.
Attachment #625346 - Attachment mime type: text/plain → text/html
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:

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
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).
Also, we should avoid marking the dependency here, since the other bug is not s-s.
Assignee: nobody → jorendorff
Whiteboard: [js:p1:fx15]
Fixed by the patch in bug 750307.
resolving based on comment 5
Status: NEW → RESOLVED
Closed: 12 years ago
Depends on: 750307
Resolution: --- → FIXED
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:
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.
> 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?
Attachment #633785 - Attachment mime type: text/plain → text/html
Verified fixed in the 6/18 trunk nightly (and confirmed bug with Firefox 13).
Status: RESOLVED → VERIFIED
Attached patch landed in mcSplinter Review
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: 
  None.
Attachment #634237 - Flags: review+
Attachment #634237 - Flags: approval-mozilla-esr10?
Attachment #634237 - Flags: approval-mozilla-beta?
Attachment #634237 - Flags: approval-mozilla-aurora?
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.
Attachment #634237 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #633785 - Attachment is obsolete: true
Why hasn't this landed yet? The late landing on Aurora is putting us at risk of landing on Beta 14.
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.
Attachment #634237 - Flags: approval-mozilla-esr10?
Attachment #634237 - Flags: approval-mozilla-esr10+
Attachment #634237 - Flags: approval-mozilla-beta?
Attachment #634237 - Flags: approval-mozilla-beta+
I didn't see this in time.
(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?).
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
lsblakk clarified on IRC that the approval-beta+ stands.

Pushed.

https://hg.mozilla.org/releases/mozilla-beta/rev/acb98d65fa59

The bug number in the commit message still refers to the (non-security) bug where this was originally reported and fixed.
This bounced. It's causing a mochitest to fail (not crash):
  dom/tests/mochitest/browser-frame/test_browserFrame8.html
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.)
This is going in for Firefox 14 and ESR real soon now or is it being punted to Firefox 15 and ESR 15+?
Here is the fix for the test.
Attachment #640700 - Flags: review?
Comment on attachment 640700 [details] [diff] [review]
Part 2: test fixup (needed in ff14)

A test is using Object.defineProperty to redefine window.top. 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.
Attachment #640700 - Flags: review? → review?(bzbarsky)
Whiteboard: [js:p1:fx15] → [js:p1:fx15][advisory-tracking+]
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 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.
Attachment #640700 - Flags: review?(bzbarsky) → review+
Alias: CVE-2012-1956
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.
Whiteboard: [js:p1:fx15][advisory-tracking+] → [js:p1:fx15]
Whiteboard: [js:p1:fx15] → [js:p1:fx15][advisory-tracking+]
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?
(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.
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
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.
Jason, are you going to land this on ESR-10?
https://hg.mozilla.org/releases/mozilla-esr10/rev/6b471843f125

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.
Thanks!
Whiteboard: [js:p1:fx15][advisory-tracking+] → [js:p1:fx15][advisory-tracking+] fixed by bug 750307
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.
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...
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.
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.
Attachment #667903 - Flags: superreview?(jst)
Attachment #667903 - Flags: review?(jst)
Comment on attachment 667903 [details] [diff] [review]
Extra kludge for ESR10

r=jst
Attachment #667903 - Flags: superreview?(jst)
Attachment #667903 - Flags: superreview+
Attachment #667903 - Flags: review?(jst)
Attachment #667903 - Flags: review+
Attachment #667903 - Flags: approval-mozilla-esr10?
Attachment #667903 - Flags: approval-mozilla-esr10? → approval-mozilla-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.
Attachment #668514 - Flags: review?(jorendorff)
Attachment #668514 - Flags: review?(jorendorff) → review+
remote:   https://hg.mozilla.org/releases/mozilla-esr10/rev/f19e6807b8bb
remote:   https://hg.mozilla.org/releases/mozilla-esr10/rev/89d8746d16c5

Should fix this on esr10. Fingers crossed

My kludge v2 patch had a=bajaj over IRC.
Group: core-security
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.