Closed
Bug 901351
Opened 11 years ago
Closed 11 years ago
Assertion failure: !obj->is<ProxyObject>(), at js/src/json.cpp:676
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
People
(Reporter: anba, Assigned: Waldo)
References
Details
(Keywords: sec-critical, Whiteboard: [adv-main24+][adv-esr1709+])
Attachments
(4 files, 3 obsolete files)
9.06 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
2.50 KB,
patch
|
Waldo
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
3.50 KB,
patch
|
Waldo
:
review+
lsblakk
:
approval-mozilla-esr17+
akeybl
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
2.48 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
The following test case asserts with: !obj->is<ProxyObject>(), at js/src/json.cpp:676 JSON.parse('{"a":0, "b":1}', function(n, v) { if (n == "a") { Object.defineProperty(this, "b", {value: new Proxy({c:0}, {})}) } return v })
Assignee | ||
Comment 1•11 years ago
|
||
The assertion makes a bad assumption, and it should be removed. However, looking at the code slightly, I think a little bit more fixing is necessary: the subsequent code is along these lines: if (obj->is<ArrayObject>()) { ...array-specific code... } else { ...non-array code... } And given the vector demonstrated here, you could encounter a cross-compartment wrapper around an array, that would end up in the else-path when it should be in the first path. So that also needs a fix. The testcase here depends on property enumeration order, but I'm pretty sure you could make one that didn't, if the string contained an array instead of an object and did its definition for n == 0 and defined over index 1 or so.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 2•11 years ago
|
||
Simplified test case with arrays as mentioned in comment 1: JSON.parse('[0, 1]', function(n, v) { if (n == "0") { this[1] = new Proxy({}, {}); } return v; }) Just removing the assertion in line 676 will then trigger bug 901380.
Assignee | ||
Comment 3•11 years ago
|
||
Actually...I'm going to play it safe. It doesn't take huge leaps to get to bug 901380 from this, I think. Patch and tests in a second.
Group: core-security
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•11 years ago
|
||
Our MOP again gets in the way of spec compliance here, by preventing us from having those [[DefineOwnProperty]] calls not actually throw exceptions when conflicts are encountered. :-( Bleh. I find myself wondering if this it's possibly a spec oversight that deletion/redefinition in this patch (and in context) and in the Walk spec ignore errors. I suspect the spec does this because whoever wrote the spec thought (as I did when writing this code, or when reordering/reimplementing/re-something-ing it in the past) that only JSON.parse-created objects are visible, nothing that's arbitrarily script-controllable. But that's not actually the case, as the examples here and in bug 901380 show. In that light, the spec should perhaps make these deletions/redefinitions throw a TypeError on error. But that's tangential -- worth bringing up after we've fixed this in a stable release, but nothing we need to address now.
Assignee: general → jwalden+bmo
Attachment #785990 -
Flags: review?(jorendorff)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #785991 -
Flags: review?(jorendorff)
Assignee | ||
Comment 6•11 years ago
|
||
On second thought, we might as well mark bug 901380 as a dup of this one. So we might as well dump these tests here.
Assignee | ||
Comment 7•11 years ago
|
||
Stepping through a few failing cases with the is<ProxyObject> and isNative assertions gone/commented out, I can get all the way into JSObject::ensureDenseElements before I hit another isNative() assertion. By that point we're querying the capacity and (I think) initialized length fields in a punned ObjectElements header, and we're so far off the rails it is inconceivable (yes, that word means *exactly* what I think it means) that we can't get dangling pointers (the ArrayBuffer link field looks ripe for attack), memory mismanagement, or something equally bad that can be leveraged to a full exploit. This appears to date back to bug 653888 in 2011, i.e. the dawn of time. :-\
status-b2g18:
--- → affected
status-firefox22:
--- → affected
status-firefox23:
--- → affected
status-firefox24:
--- → affected
status-firefox25:
--- → affected
status-firefox26:
--- → affected
status-firefox-esr17:
--- → affected
Keywords: sec-critical
Assignee | ||
Comment 9•11 years ago
|
||
Jesse, your fuzzers probably should have been able to find this. Do they possibly not know enough about the reviver argument to JSON.parse to write fuzzer-coherent revivers that actually terminate when used?
Updated•11 years ago
|
Assignee | ||
Updated•11 years ago
|
Attachment #785999 -
Flags: review?(jorendorff)
Updated•11 years ago
|
Comment 10•11 years ago
|
||
Comment on attachment 785990 [details] [diff] [review] Don't assume values being walked are non-proxy objects, don't assume non-arrays are native Review of attachment 785990 [details] [diff] [review]: ----------------------------------------------------------------- Want to move this file into the builtin dir? rs=me. :D Needs tests: the test case in comment 0, plus one involving cross-compartment wrappers, please. ::: js/src/json.cpp @@ +697,5 @@ > } else { > /* Step 2a(iii)(3). */ > + // XXX This define shouldn't throw if this definition > + // conflicts, but we can't implement that behavior > + // right now. :-( Clarify what "shouldn't" means in this comment. (I think it means "the spec says do a non-strict define".) @@ +726,5 @@ > } else { > /* Step 2b(ii)(3). */ > + // XXX This define shouldn't throw if this definition > + // conflicts, but we can't implement that behavior > + // right now. :-( Same.
Attachment #785990 -
Flags: review?(jorendorff) → review+
Comment 11•11 years ago
|
||
Oh, I see the tests. Now I think I need to go back and read ES5.1 again though.
Comment 12•11 years ago
|
||
Comment on attachment 785991 [details] [diff] [review] Tests for the is<ProxyObject> assertion failures Review of attachment 785991 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/ecma_5/extensions/reviver-mutates-holder-object.js @@ +16,5 @@ > + **************/ > + > +// A little trickiness to account for the undefined-ness of property > +// enumeration order. > +var first = "unset"; Consider: first = prop; second = (prop === "a" ? "b" : "a"); ... Object.defineProperty(this, second, ...); ... assertEq(obj[first], 0); assertEq(obj[second], proxyObj); such that even if it's opposite day we still test the property we aimed to test. More importantly, please add a test where it actually has to Walk some properties on the proxy. Also one where the proxy is a cross-compartment wrapper, to test that the invocation of reviver handles that case (I'm quite sure it's correct, but for future hackers let's have the test checked in).
Attachment #785991 -
Flags: review?(jorendorff) → review+
Comment 13•11 years ago
|
||
Comment on attachment 785999 [details] [diff] [review] Tests for isNative() assertion failures Review of attachment 785999 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/tests/ecma_5/extensions/reviver-mutates-holder-object-nonnative.js @@ +15,5 @@ > + * BEGIN TEST * > + **************/ > + > +// A little trickiness to account for the undefined-ness of property > +// enumeration order. Same halfhearted comment. :) Can you also assert that we're calling the reviver function on the TypedArray's elements, as I think the spec for Walk requires?
Attachment #785999 -
Flags: review?(jorendorff) → review+
Comment 14•11 years ago
|
||
Waldo discovered a remarkable bug, illustrated by the following test case: function thaw(obj) { var done = true; JSON.parse("[0, 1]", function (k, v) { if (this !== obj && k !== "" && done) { done = false; this[1 - k] = obj; } return v; }); } var obj = Object.freeze({a: 1, b: 2, c: 3}); obj.a = false; // does nothing, obj.a is non-writable assertEq(obj.a, 1); thaw(obj); // bug: the properties become writable, which shouldn't be possible obj.a = false; assertEq(obj.a, 1); // FAILS, obj.a is now false
Assignee | ||
Comment 15•11 years ago
|
||
Comment 14 basically comes down to JSObject::define* not respecting any of the configurability/writability/etc. invariants. (It preserves non-configurability in the overwritten properties, tho -- CheckCanChangeAttrs's /* A permanent property must stay permanent. */ does this, if I read right, but it doesn't respect the bit when initially set.) Object.defineProperty has custom code that *does* respect it. Proxy property-definition proxying code has more custom code to respect it. These central APIs don't. We need them to, at some point, if we're going to avoid a whack-a-mole game spot-hacking each individual API that we think needs the change -- the reality is that they all do. A spot-fix for Walk only might have us cons up a property descriptor object and pass it to js_DefineOwnProperty, or whatever it's called. But it's a spot-fix that doesn't address the underlying problem, and it'd probably make the underlying problem more obvious, to an intelligent observer who noticed the patch. So we shouldn't do that *unless* someone somehow manages to 0-day us specifically using Walk, and in that case we'll have to recognize that our "fix" is only a partial one, and other vectors likely remain.
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #785991 -
Attachment is obsolete: true
Attachment #785999 -
Attachment is obsolete: true
Attachment #789840 -
Flags: review+
Assignee | ||
Comment 17•11 years ago
|
||
[Security approval request comment] How easily could an exploit be constructed based on the patch? The issue leads to a whole bunch of invariants being violated. How easy it is to parlay those violations into an exploit, I don't know -- I don't have exploit-writing experience to know how trivial this is. But I'd bet a skilled person, with some knowledge of engine internals, could write one fairly quickly. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The removal of the !is<ProxyObject>() assertion probably wouldn't look very good; whether someone would pick up on the change as being dangerous, I'm not sure. Which older supported branches are affected by this flaw? Everything. If not all supported branches, which bug introduced the flaw? Bug 653888. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? They should be easy to create. This code hasn't substantially changed in years. How likely is this patch to cause regressions; how much testing does it need? The relevant changes are pretty simple, so this is unlikely to cause regressions and shouldn't need large amounts of testing. I think we probably want to land this late in a cycle so there's as little exposure of it as possible prior to release.
Attachment #785990 -
Attachment is obsolete: true
Attachment #789846 -
Flags: sec-approval?
Attachment #789846 -
Flags: review+
Comment 18•11 years ago
|
||
Release management, we need input on when is the latest we can land this on beta and aurora.
Flags: needinfo?(release-mgmt)
Assignee | ||
Comment 19•11 years ago
|
||
A moderately straightforward backport, with the only interesting change being to switch an array_deleteElement to JSObject::deleteElement. (It was wrong for the exact same reasons unconditional DefineNativeProperty is wrong. The reason trunk didn't need that change is that bug 827490 made arrays native and reverted the array_deleteElement call to a safe and generic JSObject::deleteElement.)
Attachment #789931 -
Flags: review+
Assignee | ||
Comment 20•11 years ago
|
||
The extra changes needed for beta were even fewer than for esr17/b2g18.
Attachment #789934 -
Flags: review+
Comment 21•11 years ago
|
||
Comment on attachment 789846 [details] [diff] [review] Patch for checkin sec-approval+ for trunk. I expect we'll take this on Aurora and Beta.
Attachment #789846 -
Flags: sec-approval? → sec-approval+
Comment 22•11 years ago
|
||
Jeff, can you check this in now?
Flags: needinfo?(release-mgmt) → needinfo?(jwalden+bmo)
Assignee | ||
Comment 23•11 years ago
|
||
Queued up for checkin, awaiting an open tree. Oh, the trunk patch applies to aurora, which I seem to have forgotten to mention before here.
Flags: needinfo?(jwalden+bmo)
Comment 24•11 years ago
|
||
Comment on attachment 789846 [details] [diff] [review] Patch for checkin [Triage Comment]
Attachment #789846 -
Flags: approval-mozilla-beta+
Attachment #789846 -
Flags: approval-mozilla-aurora+
Updated•11 years ago
|
tracking-b2g18:
? → ---
tracking-firefox-esr17:
--- → 24+
Assignee | ||
Comment 26•11 years ago
|
||
Trunk: https://hg.mozilla.org/integration/mozilla-inbound/rev/bc984c603e79
Assignee | ||
Comment 27•11 years ago
|
||
Comment on attachment 789931 [details] [diff] [review] esr17, b2g18 patch [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 653888. User impact if declined: sec-critical vulnerabilities Testing completed: Just landed on trunk, tests in bug (but not landed, because security) pass Risk to taking this patch (and alternatives if risky): Pretty low, this is straightforward code String or UUID changes made by this patch: N/A
Attachment #789931 -
Flags: approval-mozilla-b2g18?
Comment 28•11 years ago
|
||
Comment on attachment 789931 [details] [diff] [review] esr17, b2g18 patch Marking ESR17 approval as ? since we probably need to take this there.
Attachment #789931 -
Flags: approval-mozilla-esr17?
Comment 29•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bc984c603e79
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 30•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/abe44f1c9fcb https://hg.mozilla.org/releases/mozilla-beta/rev/1cef98f03401
Comment 31•11 years ago
|
||
Comment on attachment 789931 [details] [diff] [review] esr17, b2g18 patch Approving for ESR17 as we're tracking this for ESR 24+
Attachment #789931 -
Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
Updated•11 years ago
|
Whiteboard: [adv-main24+][adv-esr1709+]
Comment 33•11 years ago
|
||
b2g18 is frozen for non-leo+ blockers. Please request blocking status if you feel that this must land there.
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-b2g-v1.1hd:
--- → wontfix
Updated•11 years ago
|
Attachment #789931 -
Flags: approval-mozilla-b2g18?
Comment 34•11 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #33) > b2g18 is frozen for non-leo+ blockers. Please request blocking status if you > feel that this must land there. I'm sorry, I fucked this up. I fed RyanVM bad information. The v1.1 branch will be open until 3/3/2014, as it's being maintained for security until that time.
Updated•11 years ago
|
Attachment #789931 -
Flags: approval-mozilla-b2g18+
Assignee | ||
Comment 35•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b1dfb3281146
Comment 36•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/b1dfb3281146
Flags: in-testsuite?
Comment 37•11 years ago
|
||
Confirmed assert and crash on FF23. Verified fixed on 17esr, 24, 25 and 26, 2013-09-11.
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•