1105914 - (CVE-2015-4478) JSON.parse with reviver allows redefining non-configurable properties

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase: look at your web console

Credit for this one also to André Bargull.

The implementation of Walk() in json.cpp (used when there is a reviver) uses JSObject::defineGeneric to define the property.  This is not what the spec says to do, and in particular it's missing the various sanity checks that [[DefineOwnProperty]] is supposed to perform.  Compare to what Object.defineProperty does (which lands in DefinePropertyOnObject, which does those checks).

Anyway, the upshot is that you can redefine non-configurable properties.  André's testcase does this with Location, redefining things like toString.

This particular testcase is "fixed" by my patch in bug 1101123, in that before you get to toString you try to redefine "href", which is an accessor property so you get an exception.  Except per spec failures to define a prop during Walk shouldn't throw afaict, and I'm sure the testcase could be adjusted to avoid the href issue.... and in any case, this can be used to redefine non-configurable stuff on objects which only have value props no matter what I do in bug 1101123.

Comment 1

[André Bargull [:anba]]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #0)
> This particular testcase is "fixed" by my patch in bug 1101123, in that
> before you get to toString you try to redefine "href", which is an accessor
> property so you get an exception. Except per spec failures to define a prop
> during Walk shouldn't throw afaict, and I'm sure the testcase could be
> adjusted to avoid the href issue....

You could try to change the last return statement `return propertyValue;` with this code, maybe that helps to workaround the fix from bug 1101123:
---
switch (propertyName) {
case "":
case "a1":
case "a2":
  return propertyValue;
default:
  // Return undefined to trigger [[Delete]] instead of [[DefineOwnProperty]]
  return;
}
---

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: With that change

Yep, that makes the redefinitions happen even with the bug 1101123 patch.

Comment 3

[Andrew McCreight [:mccr8]]

This sounds pretty bad, so I'm marking it sec-critical. Adjust as needed.

Comment 4

[Andrew McCreight [:mccr8]]

Jeff, is this something you could fix?  Thanks.

Comment 6

[Jeff Walden [:Waldo]]

According to Jason elsewhere, it's his Q1 goal to clean up a ton of stuff in jsobj.cpp and vm/NativeObject.cpp including this bug.

Comment 8

[Andrew McCreight [:mccr8]]

Ok, thanks.  I'll assign this to Jason then.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

This sec-critical issue has been sitting for a while. Do you think you are likely to have time to work on this one soon or is there someone else that could be working on it?

Comment 10

[Andrew McCreight [:mccr8]]

I emailed Jason a week or two ago, and he said he's on schedule to fix this.  There's a bunch of background work happening in other bugs that has started landing, despite the lack of activity in this bug.

Comment 11

[Andrew McCreight [:mccr8]]

My understanding is that bug 1125624 will fix this.

Comment 12

[Tom S]

After Bug 1148568 this should be fixed. The testcase doesn't reproduce anymore as well. Looking at the patch we might even be able to uplift it, but I am not sure that is useful.

Comment 13

[Andrew McCreight [:mccr8]]

Marking fixed per comment 12.

Jason, can this work be backported?

Comment 14

[Jason Orendorff [:jorendorff]]

I think it can be backported. I'll try it tomorrow. Leaving the ni?me bit set.

Comment 15

[Jason Orendorff [:jorendorff]]

On reflection, I don't think this is worth backporting. There are too many other paths to DefineProperty. See e.g. bug 1073808, which was filed in the open.

The *real* fix, changing DefineProperty to enforce all the invariants (bug 904886) is a huge stack of patches -- really the product of about 3 months of work, and not yet fully debugged.

I think we basically can't do anything useful about the general problem, and narrowly fixing JSON.parse does not seem worthwhile. Open to suggestions.

Comment 16

[Jason Orendorff [:jorendorff]]

As far as I know, this is sec-high, not sec-critical.

Comment 18

[Andrew McCreight [:mccr8]]

Closing as fixed per comment 12.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Redefining Location.toString is totally worthy of a bug bounty.

Comment 22

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed issue in Fx39, verified fixed in Fx40.0b8.

Comment 23

[Daniel Veditz [:dveditz]]

(In reply to On vacation August 4-25.  Please mail manually if really needed. from comment #21)
> Redefining Location.toString is totally worthy of a bug bounty.

If you redefine Location.toString and chrome code accesses it, would the overwritten method be run with chrome privilege? If so that would most definitely be sec-critical rather than sec-high.

Comment 24

[Jeff Walden [:Waldo]]

(In reply to Daniel Veditz [:dveditz] from comment #23)
> (In reply to bz from comment #21)
> > Redefining Location.toString is totally worthy of a bug bounty.
> 
> If you redefine Location.toString and chrome code accesses it, would the
> overwritten method be run with chrome privilege?

I don't believe it would be -- I'd have thought behind xrays or whatever chrome code would see the initial, safe behavior.  But I'm not super-familiar with the alleged failure modes here, so redirecting.

It's also still entirely unclear to me why Window.location or Location.toString must be non-configurable any more.  (If they don't have to be, the only vulnerability is wrt things like Caja, which is "only" sec-high.)  Flash hasn't depended on these properties for security in a very long time.  Years-old unfixed Flash is vulnerable all sorts of other ways.  Other plugin usage is pretty negligible.  Even so, some of them probably have adopted the new security API, and the others, if they haven't been updated for this, probably have other unfixed vulnerabilities.  It's completely unclear to me that this [Unforgeable] baggage is needed in specs any more.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> It's also still entirely unclear to me why Window.location or Location.toString must be
> non-configurable any more.

That's a question for Bobby, but I believe the main motivation there are things like Flash used to be that do security checks based on location.toString() (e.g. scripts in web pages).  For chrome code, unless it's waived Xrays it wouldn't even see the overridden property, so not an issue there.

Comment 26

[Bobby Holley (:bholley)]

I don't know what the current state of dependencies on location unforgeability are. There was some time ago that we thought that Flash had moved on, but then someone instrumented NPAPI and found that they were still querying window.location all over the place. That may have changed, but I don't know exactly how much it's worth trying to find out.

We have other unforgeable properties too, like Event.isTrusted and various ServiceWorker things.