Closed Bug 1105914 (CVE-2015-4478) Opened 9 years ago Closed 9 years ago

JSON.parse with reviver allows redefining non-configurable properties

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla40
Tracking Status
firefox35 --- wontfix
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- wontfix
firefox40 --- verified
firefox-esr31 --- wontfix
firefox-esr38 --- wontfix
b2g-v1.4 --- wontfix
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- wontfix
b2g-v2.1S --- wontfix
b2g-v2.2 --- wontfix
b2g-master --- fixed

People

(Reporter: bzbarsky, Assigned: jorendorff)

References

Details

(Keywords: sec-high, Whiteboard: [reporter-external][adv-main40+])

Attachments

(2 files)

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.
Flags: sec-bounty?
(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;
}
---
Attached file With that change —
Yep, that makes the redefinitions happen even with the bug 1101123 patch.
Whiteboard: [reporter-external]
This sounds pretty bad, so I'm marking it sec-critical. Adjust as needed.
Keywords: sec-critical
Jeff, is this something you could fix?  Thanks.
Flags: needinfo?(jwalden+bmo)
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.
Flags: needinfo?(jwalden+bmo)
Ok, thanks.  I'll assign this to Jason then.
Assignee: nobody → jorendorff
Group: javascript-core-security
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?
Flags: needinfo?(jorendorff)
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.
Flags: needinfo?(jorendorff)
Depends on: 1125624
My understanding is that bug 1125624 will fix this.
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.
Marking fixed per comment 12.

Jason, can this work be backported?
Depends on: 1148568
No longer depends on: 1125624
Flags: needinfo?(jorendorff)
I think it can be backported. I'll try it tomorrow. Leaving the ni?me bit set.
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.
Flags: needinfo?(jorendorff)
As far as I know, this is sec-high, not sec-critical.
Closing as fixed per comment 12.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Flags: needinfo?(jorendorff)
Flags: needinfo?(bzbarsky)
Redefining Location.toString is totally worthy of a bug bounty.
Flags: needinfo?(bzbarsky)
Group: javascript-core-security
Flags: sec-bounty? → sec-bounty+
QA Contact: mwobensmith
Confirmed issue in Fx39, verified fixed in Fx40.0b8.
Status: RESOLVED → VERIFIED
Whiteboard: [reporter-external] → [reporter-external][adv-main40+]
Alias: CVE-2015-4478
(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.
Flags: needinfo?(jwalden+bmo)
(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.
Flags: needinfo?(jwalden+bmo) → needinfo?(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.
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley)
Flags: needinfo?(jorendorff)
Group: core-security → core-security-release
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.
Flags: needinfo?(bobbyholley)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.