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

VERIFIED FIXED in Firefox 36, Firefox OS v2.1

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Assigned: efaust)

Tracking

(Blocks: 1 bug, {assertion, sec-high, testcase})

Trunk
mozilla38
x86_64
Mac OS X
assertion, sec-high, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

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

Details

(Whiteboard: [adv-main36+][b2g-adv-main2.2+])

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8495021 [details]
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.
(Reporter)

Comment 1

4 years ago
Created attachment 8495022 [details]
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

Comment 6

4 years ago
(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 }),
                    {});
(Assignee)

Comment 7

4 years ago
Created attachment 8505036 [details] [diff] [review]
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.
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)
(Assignee)

Comment 10

4 years ago
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)
(Assignee)

Comment 11

4 years ago
Created attachment 8520274 [details] [diff] [review]
Fix

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+
(Assignee)

Comment 12

4 years ago
Created attachment 8521067 [details] [diff] [review]
Alternate fix

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)
(Assignee)

Comment 15

4 years ago
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)
(Assignee)

Comment 18

4 years ago
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?
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
status-firefox35: --- → wontfix
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → fixed
status-firefox-esr31: --- → unaffected
status-firefox38: fixed → affected

Comment 19

4 years ago
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+

Updated

4 years ago
tracking-firefox36: --- → +
tracking-firefox37: --- → +
tracking-firefox38: --- → +
(Assignee)

Comment 20

4 years ago
Created attachment 8554803 [details] [diff] [review]
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.
Attachment #8554803 - Flags: review+
Attachment #8554803 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 21

4 years ago
Created attachment 8554806 [details] [diff] [review]
Beta patch

See aurora comment above.
Attachment #8554806 - Flags: review+
Attachment #8554806 - Flags: approval-mozilla-beta?

Updated

4 years ago
Attachment #8554803 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

4 years ago
Attachment #8554806 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 22

4 years ago
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
Last Resolved: 4 years ago
status-firefox38: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
https://hg.mozilla.org/releases/mozilla-aurora/rev/5b6e1cf75e87
https://hg.mozilla.org/releases/mozilla-beta/rev/897f73d9e4f9
status-b2g-master: affected → fixed
status-firefox36: affected → fixed
status-firefox37: affected → fixed
Group: javascript-core-security

Updated

4 years ago
Whiteboard: [adv-main36+]
Confirmed crash on Fx35, verified fixed on Fx36 2015-02-18, debug build.
Status: RESOLVED → VERIFIED
status-firefox36: fixed → verified
Whiteboard: [adv-main36+] → [adv-main36+][b2g-adv-main2.2+]

Updated

3 years ago
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.