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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: 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.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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");

Comment 4

[Bobby Holley (:bholley)]

I think this is probably efaust's territory at this point.

Comment 5

[Daniel Veditz [:dveditz]]

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.

Comment 6

[Jeff Walden [:Waldo]]

(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 }),
                    {});

Comment 7

[Eric Faust [:efaust]]

Attachment: Fix

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.

Comment 8

[Jason Orendorff [:jorendorff]]

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 9

[Jason Orendorff [:jorendorff]]

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?

Comment 10

[Eric Faust [:efaust]]

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

Comment 11

[Eric Faust [:efaust]]

Attachment: Fix

Rewrite parts of the fix, as well as adding a better test, and fix a bogus comment.

Comment 12

[Eric Faust [:efaust]]

Attachment: Alternate fix

Now without changeProperty

Comment 13

[Jason Orendorff [: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?

Comment 14

[Andrew McCreight [:mccr8]]

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.

Comment 15

[Eric Faust [:efaust]]

Uh, it should go in wherever it can. It's ridiculous, frankly, that it's missed trains.

Comment 16

[Andrew McCreight [:mccr8]]

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.

Comment 17

[Andrew McCreight [:mccr8]]

ni for sec-approval for that patch.  Thanks.

Comment 18

[Eric Faust [:efaust]]

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.

Comment 19

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

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).

Comment 20

[Eric Faust [:efaust]]

Attachment: Aurora patch

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.

Comment 21

[Eric Faust [:efaust]]

Attachment: Beta patch

See aurora comment above.

Comment 22

[Eric Faust [:efaust]]

The Aurora patch applies cleanly to inbound as well, and should be landed there.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca9cc2f0195

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ca9cc2f0195

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5b6e1cf75e87
https://hg.mozilla.org/releases/mozilla-beta/rev/897f73d9e4f9

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5b6e1cf75e87
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ceed888f5784

Comment 27

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

Confirmed crash on Fx35, verified fixed on Fx36 2015-02-18, debug build.