Closed Bug 1072760 Opened 10 years ago Closed 9 years ago

Failed JS_ASSERT_IF(attrs & JSPROP_READONLY, !(attrs & (JSPROP_GETTER | JSPROP_SETTER)));

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla38
Tracking Status
firefox35 --- wontfix
firefox36 + verified
firefox37 + fixed
firefox38 + fixed
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: jruderman, Assigned: efaust)

Details

(Keywords: assertion, sec-high, testcase, Whiteboard: [adv-main36+][b2g-adv-main2.2+])

Attachments

(6 files, 1 obsolete file)

Attached file testcase
Assertion failure: !(attrs & (0x10 | 0x20)), at /Users/jruderman/trees/mozilla-central/js/src/jsobj.cpp:281

This is a macro expansion of:

JS_ASSERT_IF(attrs & JSPROP_READONLY, !(attrs & (JSPROP_GETTER | JSPROP_SETTER)));

This assertion was added in https://hg.mozilla.org/mozilla-central/rev/49bd0c9665e5 (bug 702491) by bholley and waldo.

Security-sensitive out of caution: many JS engine invariants are safety-critical, but I don't know which ones.
Attached file stack
So this testcase starts out with an accessor property and then tries to redefine it with this descriptor:

    Object.defineProperty(window, "onclick", {writable: false});

Since it's an accessor, shouldn't the "writable" just be ignored?
Flags: needinfo?(jwalden+bmo)
This has nothing to do with the DOM, other than the fact that "window" is a proxy.  Pure shell testcase that triggers the same assert:

var obj = new Proxy({}, {});
Object.defineProperty(obj, "foo",
		      { configurable: true, get: function() {}, set: function() {} });
Object.defineProperty(obj, "foo", {writable: false});
Object.getOwnPropertyDescriptor(obj, "foo");
Component: DOM → JavaScript Engine
I think this is probably efaust's territory at this point.
Flags: needinfo?(jwalden+bmo) → needinfo?(efaustbmo)
Marking it sec-high out of paranoia (pending efaust's needinfo) that if we have the state wrong the JIT might optimize things in an unsafe way.
Keywords: sec-high
(In reply to Boris Zbarsky [:bz] from comment #2)
> So this testcase starts out with an accessor property and then tries to
> redefine it with this descriptor:
> 
>     Object.defineProperty(window, "onclick", {writable: false});
> 
> Since it's an accessor, shouldn't the "writable" just be ignored?

No.  The existing accessor (assuming it's configurable) will have its [[Get]]/[[Set]] blown away.  [[Enumerable]] and [[Configurable]] will be preserved.  Because we only have half the data-descriptor info, the missing part's default is imputed.

So the end state here *should* be a data descriptor property like { value: undefined, writable: false, enumerable: <whatever the prior one was>, configurable: <whatever the prior one was> }.

(In reply to Boris Zbarsky [:bz] from comment #3)
> var obj = new Proxy({}, {});
> Object.defineProperty(obj, "foo",
> 		      { configurable: true, get: function() {}, set: function() {} });
> Object.defineProperty(obj, "foo", {writable: false});
> Object.getOwnPropertyDescriptor(obj, "foo");

This should end with this state:

var obj = new Proxy(Object.defineProperty({}, "foo",
                                          { configurable: true, enumerable: false,
                                            writable: false, value: undefined }),
                    {});
Attached patch Fix (obsolete) — Splinter Review
Waldo is right. We need to only do the direct change and not just slam in the new thing if we agree in type with the old descriptor. This was already handled for the accessor descriptor case, so add in the data descriptor one.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8505036 - Flags: review?(jorendorff)
Flags: needinfo?(efaustbmo)
Am I correct in thinking that if you took Waldo's cross-product-of-property-flavors "redefinition" tests, and aimed them at a scripted direct Proxy instead of a plain Object, that would find this bug?

Could you do that, please, just to make sure nothing else shakes loose?
Comment on attachment 8505036 [details] [diff] [review]
Fix

Review of attachment 8505036 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review flag for now.

::: js/src/vm/NativeObject.cpp
@@ +1461,5 @@
>                  shape = obj->lookup(cx, id);
>              }
>  
> +            if (shape->isDataDescriptor()) {
> +                attrs = ApplyOrDefaultAttributes(attrs, shape);

This now makes the code read like this is an optimization. The corresponding chunk above, where we're defining an accessor property, is necessary because we don't have JSPROP_IGNORE_GETTER/SETTER (i.e. full-on standard [[DefineOwnProperty]] MOP); I don't see why this part is necessary. Is it?

If it's not needed, we can fix the bug by deleting this block of code, right? Is there some reason this case warrants an optimization?
Attachment #8505036 - Flags: review?(jorendorff)
Flags: needinfo?(efaustbmo)
A quick run of Waldo's suite with new Proxy({}, {}) reveals more than one issue. I am slowly in the process of sorting them out. The first, most obvious one, is that CheckDefineProperty is way too restrictive. I will file and post other regressions as I identify them
Flags: needinfo?(efaustbmo)
Attached patch FixSplinter Review
Rewrite parts of the fix, as well as adding a better test, and fix a bogus comment.
Attachment #8505036 - Attachment is obsolete: true
Attachment #8520274 - Flags: review?(jorendorff)
Attachment #8520274 - Flags: review?(jorendorff) → review+
Attached patch Alternate fixSplinter Review
Now without changeProperty
Attachment #8521067 - Flags: review?(jorendorff)
Comment on attachment 8521067 [details] [diff] [review]
Alternate fix

Review of attachment 8521067 [details] [diff] [review]:
-----------------------------------------------------------------

This is so much better than the other thing. Don't you think? So many weird possibilities just don't arise when the two branches are at least following the same basic plan.

I didn't try to verify that all this is correct (whatever "correct" even means for a partial fix like this) in every possible path, so please do re-run your Proxy-or-wrap() variation of Waldo's definition and redefinition tests and just sanity-check that this doesn't regress anything. Also... think about adding some tests to pin this improvement in place. Even if they're just tests that accept all outcomes except crashing/asserting.

(In the end, we'll unify Waldo's Object.defineProperty implementation with this code. If we were doing that today, Waldo's existing tests would be sufficient. But, obviously, we're not there yet.)

::: js/src/vm/NativeObject.cpp
@@ +1482,5 @@
>                  shape = obj->lookup(cx, id);
>              }
>  
> +            /*
> +             * ES6 OCT142014 9.1.6.3 7c requires us to change the property from an accessor property

Woof! Please don't use this date format. "2014-OCT-14" would be fine.

Please remember to wrap comments to 79 columns. Only code gets to stretch out to 99.

@@ +1492,3 @@
>              attrs = ApplyOrDefaultAttributes(attrs, shape);
> +            bool justChange = shape->isDataDescriptor() || (attrs & JSPROP_IGNORE_READONLY);
> +            if (justChange) {

I would delete the big comment, flip the if-statement, and add comments like this:

    if (shape->isAccessorDescriptor() && !(attrs & JSPROP_IGNORE_READONLY)) {
        // ES6 draft 2014-10-14 9.1.6.3 step 7.c: Since [[Writable]] is present, change the
        // existing accessor property to a data property.
        ...
    } else {
        // We are at most applying some new attributes to an existing property, and not changing
        // it from data to accessor or vice versa. Retain everything from the existing shape
        // except the few things we're changing.
        ...
    }

I dropped the boolean variable; the name "justChange" doesn't make sense anymore.

@@ +1500,5 @@
> +                if (shape->hasSlot())
> +                    updateValue = obj->getSlot(shape->slot());
> +            } else {
> +                getter = nullptr;
> +                setter = nullptr;

Hmm. I'm not sure we should be touching getter and setter at all. These are the values passed in by the caller, right? And they're ops? Leaving them alone seems safer. That's what we do elsewhere, right?

Also -- do we need to mask away JSPROP_GETTER/SETTER/SHARED from attrs in this branch?
Attachment #8521067 - Flags: review?(jorendorff) → review+
Group: javascript-core-security
What's the status of this, Eric?  Note that at this point it is too late to get on beta, so if it affects that branch it will likely need to wait a number of weeks before landing.
Flags: needinfo?(efaustbmo)
Uh, it should go in wherever it can. It's ridiculous, frankly, that it's missed trains.
Flags: needinfo?(efaustbmo)
Ok, if this applies to more than trunk, please get sec-approval for you patch, and Al can figure out when it should land based on that.
ni for sec-approval for that patch.  Thanks.
Flags: needinfo?(efaustbmo)
Comment on attachment 8521067 [details] [diff] [review]
Alternate fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It would be much easier with the assertion the patch fixes. I will not land a test with it, but the code that triggers the assertion is trivial and will make the engine do "strange things". It's not clear to me that the most obvious paths towards ACE will be met with success.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

We should not land a test with this, probably, as the fixed beahvior is too simple. Tests should sneak in elsewhere, but otherwise, it's not a massive problem.

Which older supported branches are affected by this flaw?

I believe the causing bug (bug 1037770) landed in 34, so we are bad clear

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Not risky. The code has been pretty stable for a while.

How likely is this patch to cause regressions; how much testing does it need?

It shouldn't. If anything it removes an unsafe optimization.
Flags: needinfo?(efaustbmo)
Attachment #8521067 - Flags: sec-approval?
Comment on attachment 8521067 [details] [diff] [review]
Alternate fix

sec-approval+ for trunk. We should get branch patches for this as well so we can check them in when it is in trunk. I agree that we should hold off on landing a test (until we ship the fix).
Attachment #8521067 - Flags: sec-approval? → sec-approval+
Attached patch Aurora patchSplinter Review
Approval Request Comment
[Feature/regressing bug #]: bug 1037770
[User impact if declined]: Potentially exploitable UB.
[Describe test coverage new/current, TreeHerder]: per sec-approval no tests to land, but tested locally with test included in original patch.
[Risks and why]: Low, as it removes an unsafe optimization and shunts more code through more relevant checks and safer code.
[String/UUID change made/needed]: None.
Attachment #8554803 - Flags: review+
Attachment #8554803 - Flags: approval-mozilla-aurora?
Attached patch Beta patchSplinter Review
See aurora comment above.
Attachment #8554806 - Flags: review+
Attachment #8554806 - Flags: approval-mozilla-beta?
Attachment #8554803 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8554806 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
The Aurora patch applies cleanly to inbound as well, and should be landed there.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1ca9cc2f0195
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Group: javascript-core-security
Whiteboard: [adv-main36+]
Confirmed crash on Fx35, verified fixed on Fx36 2015-02-18, debug build.
Status: RESOLVED → VERIFIED
Whiteboard: [adv-main36+] → [adv-main36+][b2g-adv-main2.2+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: