Crash [@ js::Nursery::moveToTenured] with use-after-free

VERIFIED FIXED in Firefox 39, Firefox OS master

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: decoder, Assigned: jorendorff)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla40
x86_64
Linux
crash, csectype-uaf, regression, sec-critical, testcase
Points:
---

Firefox Tracking Flags

(firefox37 unaffected, firefox38 unaffected, firefox38.0.5 unaffected, firefox39+ verified, firefox40+ verified, firefox41 verified, firefox-esr31 unaffected, firefox-esr38 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-master fixed)

Details

(Whiteboard: [jsbugmon:update,ignore], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
The following testcase crashes on mozilla-central revision de27ac2ab94f (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --no-threads):

var handler = {
  get: function get(t, pk, r) {},
  has: function has(t = has = parseInt) {}
};
Object.prototype.__proto__ = new Proxy(Object.create(null), handler);
function f() { 
  var desc = { get: function() {} };
  var arr = Object.defineProperty([], "0", desc);
}
schedulegc(24);
evaluate("f()");



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000005d2395 in js::Nursery::moveToTenured (this=0x7ffff693c3a0, trc=0x7fffffffd2f0, src=0x7ffff54001e0) at js/src/jsobj.h:128
#0  0x00000000005d2395 in js::Nursery::moveToTenured (this=0x7ffff693c3a0, trc=0x7fffffffd2f0, src=0x7ffff54001e0) at js/src/jsobj.h:128
#1  0x00000000005d3351 in js::Nursery::MinorGCCallback (jstrc=<optimized out>, thingp=0x7ffff7e5e0d8, kind=<optimized out>) at js/src/gc/Nursery.cpp:789
#2  0x00000000005c0624 in invoke (kind=JSTRACE_OBJECT, thing=0x7ffff7e5e0d8, this=0x7fffffffd2f0) at ../../dist/include/js/TracingAPI.h:216
#3  DoTracing<JSObject*> (i=18446744073709551615, name=0xd3cf95 "getterObj", thingp=0x7ffff7e5e0d8, trc=0x7fffffffd2f0) at js/src/gc/Marking.cpp:567
#4  DispatchToTracer<JSObject*> (trc=0x7fffffffd2f0, thingp=0x7ffff7e5e0d8, name=0xd3cf95 "getterObj", i=18446744073709551615) at js/src/gc/Marking.cpp:471
#5  0x00000000005ec89a in js::TraceManuallyBarrieredEdge<JSObject*> (trc=<optimized out>, thingp=<optimized out>, name=<optimized out>) at js/src/gc/Marking.cpp:388
#6  0x0000000000ad7dfc in js::Shape::fixupGetterSetterForBarrier (this=0x7ffff7e5e0b0, trc=0x7fffffffd2f0) at js/src/jspropertytree.cpp:324
#7  0x000000000078d8e9 in js::gc::StoreBuffer::GenericBuffer::mark (this=this@entry=0x7ffff69426b0, owner=owner@entry=0x7ffff693c4a0, trc=trc@entry=0x7fffffffd2f0) at js/src/gc/StoreBuffer.cpp:112
#8  0x00000000005d37b5 in markGenericEntries (trc=0x7fffffffd2f0, this=0x7ffff693c4a0) at js/src/gc/StoreBuffer.h:455
#9  js::Nursery::collect (this=this@entry=0x7ffff693c3a0, rt=0x7ffff693c000, reason=reason@entry=JS::gcreason::DESTROY_CONTEXT, pretenureGroups=pretenureGroups@entry=0x0) at js/src/gc/Nursery.cpp:855
#10 0x0000000000a82947 in js::gc::GCRuntime::minorGCImpl (this=this@entry=0x7ffff693c348, reason=reason@entry=JS::gcreason::DESTROY_CONTEXT, pretenureGroups=pretenureGroups@entry=0x0) at js/src/jsgc.cpp:6344
#11 0x00000000005e7214 in js::gc::GCRuntime::evictNursery (this=0x7ffff693c348, reason=JS::gcreason::DESTROY_CONTEXT) at js/src/gc/GCRuntime.h:618
#12 0x0000000000ae1828 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff693c348, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:5939
#13 0x0000000000ae1d25 in js::gc::GCRuntime::collect (this=this@entry=0x7ffff693c348, incremental=incremental@entry=false, budget=..., reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6112
#14 0x0000000000ae2015 in js::gc::GCRuntime::gc (this=this@entry=0x7ffff693c348, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::DESTROY_CONTEXT) at js/src/jsgc.cpp:6173
#15 0x0000000000a2b037 in js::DestroyContext (cx=0x7ffff691b4e0, mode=js::DCM_FORCE_GC) at js/src/jscntxt.cpp:186
#16 0x0000000000a2b3ae in JS_DestroyContext (cx=<optimized out>) at js/src/jsapi.cpp:729
#17 0x0000000000470258 in DestroyContext (withGC=true, cx=0x7ffff691b4e0) at js/src/shell/js.cpp:5588
#18 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6379
rax	0x7ffff693c3a0	140737330267040
rbx	0x7fffffffd2f0	140737488343792
rcx	0xbad0bad1	3134241489
rdx	0x7ffff54001e0	140737308000736
rsi	0x198ae80	26783360
rdi	0x2b2b2b2b2b2b2b2b	3110627432037296939
rbp	0x7fffffffceb0	140737488342704
rsp	0x7fffffffce40	140737488342592
r8	0x0	0
r9	0x1	1
r10	0x552fcba7	1429195687
r11	0x7	7
r12	0x7ffff7e5e0d8	140737352425688
r13	0x7ffff54001e0	140737308000736
r14	0x7fffffffd2f0	140737488343792
r15	0x10	16
rip	0x5d2395 <js::Nursery::moveToTenured(js::gc::MinorCollectionTracer*, JSObject*)+37>
=> 0x5d2395 <js::Nursery::moveToTenured(js::gc::MinorCollectionTracer*, JSObject*)+37>:	mov    (%rdi),%rax
   0x5d2398 <js::Nursery::moveToTenured(js::gc::MinorCollectionTracer*, JSObject*)+40>:	cmp    %rsi,%rax


S-s and sec-critical because this looks like a use-after-free to me.
(Reporter)

Updated

3 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
(Reporter)

Comment 1

3 years ago
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/63dbcc4fd0f0
user:        Jason Orendorff
date:        Sat Feb 14 07:37:13 2015 -0600
summary:     Bug 1133081, part 4 - Reimplement the remaining PropDesc methods and delete PropDesc. r=efaust.

This iteration took 2.035 seconds to run.
(Reporter)

Comment 2

3 years ago
Needinfo from jorendorff based on comment 1.
Flags: needinfo?(jorendorff)
Tracking for 38+ since the likely regression range would mean the issue affects 38, and because this is marked as sec-critical.
status-firefox37: --- → unaffected
status-firefox38: --- → affected
status-firefox39: --- → affected
tracking-firefox38: --- → +
tracking-firefox39: --- → +
tracking-firefox40: --- → +
It is a fuzzing bug, no known exploit using it, not tracking.
tracking-firefox38: + → ---
OK. Naveed is this something that your team wants to work on for the 39 timeframe, or is 40 more realistic? I realize there's no exploit but it's still sec-critical.
Flags: needinfo?(nihsanullah)
Assignee: nobody → jorendorff
Flags: needinfo?(nihsanullah)
(Assignee)

Comment 6

3 years ago
This reproduces in the specific revision named, but not in mozilla-inbound. I'm bisecting, in case it's a duplicate.

If not: it crashes at 0x2b2b2b2b2b2b2b2b, the GC-nursery poison pattern, suggesting a rooting bug in my new code. Reproduces under rr.
Flags: needinfo?(jorendorff)
(Assignee)

Comment 7

3 years ago
Hmm.

The first good revision is:
changeset:   245203:00959f6f41b7
user:        Jason Orendorff <jorendorff@mozilla.com>
date:        Thu Apr 09 14:17:38 2015 -0500
summary:     Bug 1148750, part 8 - Implement ValidateAndApplyPropertyDescriptor step 6. r=efaust.

Unclear what fixed it. Yay. I get to play with watchpoints.
Flags: needinfo?(jorendorff)
(Assignee)

Comment 8

3 years ago
Confirmed: this was a real GC hazard, and the "first good revision" from comment 7 really did inadvertently fix it.

The fix: hg diff -c 00959f6f41b7 jsapi.h

Full diagnosis:

- we're in ToPropertyDescriptor
    - it calls desc.setGetterObject(...) without updating desc.attributes()
    - it calls GetPropertyIfPresent, which can GC. In this test case, it triggers proxy traps.
        - JSPropertyDescriptor::trace() uses the attribute bits to determine whether
          the getter field contains a pointer that needs to be traced!
    - gc finishes; we're back in ToPropertyDescriptor
    - it calls desc.setAttributes(attrs)
    - but it's too late, desc contains a pointer to the old location of the getter,
      which was moved during GC

Leaving ni?me because I want to:

- check and see if anyplace else is using setGetter() or setSetter() in dangerous ways
- see if we can fix the API to make this hard to screw up
- see if ToPropertyDescriptor itself should be fixed/clarified
- see if we can assert in some places that a PropertyDescriptor is self-consistent
It looks like the regressing bug landed in 39, and the fix landed in 40, so 39 is still affected. Is there some fix we can make for 39?  The fixing bug looks really huge so maybe that's not really backportable to beta and we can just let it ride the trains or something.
status-firefox38: affected → unaffected
status-firefox40: affected → fixed
(Assignee)

Comment 10

3 years ago
I've filed bug 1164599 in the open for the cleanup stuff mentioned in comment 8.

This bug remains open and assigned to me because we need a patch for 39. We can't take the fixing patch, but it's easy to fix independently.
Flags: needinfo?(jorendorff)
Group: javascript-core-security
(Reporter)

Updated

3 years ago
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
(Reporter)

Comment 11

3 years ago
JSBugMon: The testcase found in this bug no longer reproduces (tried revision f578b845c4b8).
Jason, any luck here? You mention in comment 10 that we should patch something in 39. This could still get into beta 4 next Monday.
Flags: needinfo?(jorendorff)
(Assignee)

Comment 13

3 years ago
Created attachment 8616465 [details] [diff] [review]
In ToPropertyDescriptor, update attributes immediately after storing a getter or setter in a descriptor
Attachment #8616465 - Flags: review?(jdemooij)
(Assignee)

Updated

3 years ago
Flags: needinfo?(jorendorff)
Comment on attachment 8616465 [details] [diff] [review]
In ToPropertyDescriptor, update attributes immediately after storing a getter or setter in a descriptor

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

Looks good, based on the description in comment 8.

::: js/src/jit-test/tests/gc/bug-1155208.js
@@ +1,4 @@
> +var handler = {
> +    get: function get(t, pk, r) {},
> +    has: function has(t = has = parseInt) {}
> +};

I think we should land the test after the fix has made it into a release.
Attachment #8616465 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 16

3 years ago
Comment on attachment 8616465 [details] [diff] [review]
In ToPropertyDescriptor, update attributes immediately after storing a getter or setter in a descriptor

Approval Request Comment
[Feature/regressing bug #]: bug 1133081
[User impact if declined]: Occasional GC crashes.
[Describe test coverage new/current, TreeHerder]:
  test case exists, but will not be checked in immediately
[Risks and why]: no known risks
[String/UUID change made/needed]: none

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
    Hard but possible. It's a GC marking bug.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
    Without the test, no. It doesn't look like a GC bugfix at all.

Which older supported branches are affected by this flaw?
    Only mozilla-beta. (39)

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
    The only target branch is mozilla-beta. The bug was serendipitously
    fixed in m-c by a different patch (which is too big to backport).

How likely is this patch to cause regressions; how much testing does it need?
    Can't think of a way this could cause regressions. Just causing a
    field that wasn't being marked to be marked.
Attachment #8616465 - Flags: sec-approval?
Attachment #8616465 - Flags: approval-mozilla-beta?
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-master: --- → fixed
status-firefox38.0.5: --- → unaffected
status-firefox-esr31: --- → unaffected
status-firefox-esr38: --- → unaffected
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Reporter)

Updated

3 years ago
Status: RESOLVED → VERIFIED
status-firefox41: --- → verified
(Reporter)

Comment 17

3 years ago
JSBugMon: This bug has been automatically verified fixed.
Attachment #8616465 - Flags: sec-approval?
Attachment #8616465 - Flags: sec-approval+
Attachment #8616465 - Flags: approval-mozilla-beta?
Attachment #8616465 - Flags: approval-mozilla-beta+
(Reporter)

Updated

3 years ago
status-firefox39: fixed → verified
(Reporter)

Comment 19

3 years ago
JSBugMon: This bug has been automatically verified fixed on Fx39
Group: javascript-core-security
(Reporter)

Updated

3 years ago
status-firefox40: fixed → verified
(Reporter)

Comment 20

3 years ago
JSBugMon: This bug has been automatically verified fixed on Fx40

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
Keywords: csectype-uaf
You need to log in before you can comment on or make changes to this bug.