Christian Holler (:decoder) Reporter
	Description • 8 years ago

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

function runTestCase(testcase) {}
var lfGlobal = newGlobal();
for (lfLocal in this) {
    if (!(lfLocal in lfGlobal)) {
        lfGlobal[lfLocal] = this[lfLocal];
    }
}
lfGlobal.offThreadCompileScript(`
  var p = new Proxy({}, {});
  runTestCase.prototype.__proto__ = p;
  fullcompartmentchecks(true);
`);
lfGlobal.runOffThreadScript();



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x0000000000975cd0 in CompartmentCheckTracer::onChild (this=this@entry=0x7fffffffcfd0, thing=...) at js/src/jsgc.cpp:3743
#0  0x0000000000975cd0 in CompartmentCheckTracer::onChild (this=this@entry=0x7fffffffcfd0, thing=...) at js/src/jsgc.cpp:3743
#1  0x0000000000d57c5c in JS::CallbackTracer::onShapeEdge (shapep=0x7ffff7e74648, this=0x7fffffffcfd0) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/TracingAPI.h:131
#2  JS::CallbackTracer::dispatchToOnEdge (shapep=0x7ffff7e74648, this=0x7fffffffcfd0) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/TracingAPI.h:209
#3  DoCallback<js::Shape*> (trc=0x7fffffffcfd0, thingp=0x7ffff7e74648, name=<optimized out>) at js/src/gc/Tracer.cpp:51
#4  0x0000000000a09fb7 in js::ProxyObject::trace (trc=0x7fffffffcfd8, obj=0x7ffff7e74640) at js/src/proxy/Proxy.cpp:619
#5  0x000000000094546f in js::Class::doTrace (this=<optimized out>, obj=0x7ffff7e74640, trc=0x7fffffffcfd8) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Class.h:815
#6  JSObject::traceChildren (this=0x7ffff7e74640, trc=0x7fffffffcfd8) at js/src/jsobj.cpp:3890
#7  0x0000000000d3b28a in js::TraceChildren (trc=trc@entry=0x7fffffffcfd8, thing=0x7ffff7e74640, kind=kind@entry=JS::TraceKind::Object) at js/src/gc/Tracer.cpp:126
#8  0x000000000093f0ad in js::gc::GCRuntime::checkForCompartmentMismatches (this=this@entry=0x7ffff6965608) at js/src/jsgc.cpp:3769
#9  0x0000000000976608 in js::gc::GCRuntime::checkForCompartmentMismatches (this=0x7ffff6965608) at js/src/jsgc.cpp:3811
#10 js::gc::GCRuntime::beginMarkPhase (this=this@entry=0x7ffff6965608, reason=reason@entry=JS::gcreason::DESTROY_RUNTIME, lock=...) at js/src/jsgc.cpp:3799
#11 0x000000000097b1e6 in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ffff6965608, budget=..., reason=reason@entry=JS::gcreason::DESTROY_RUNTIME, lock=...) at js/src/jsgc.cpp:5919
#12 0x000000000097c6ef in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff6965608, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DESTROY_RUNTIME) at js/src/jsgc.cpp:6204
#13 0x000000000097cdb6 in js::gc::GCRuntime::collect (this=this@entry=0x7ffff6965608, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DESTROY_RUNTIME) at js/src/jsgc.cpp:6312
#14 0x000000000097d1ab in js::gc::GCRuntime::gc (this=this@entry=0x7ffff6965608, gckind=gckind@entry=GC_NORMAL, reason=reason@entry=JS::gcreason::DESTROY_RUNTIME) at js/src/jsgc.cpp:6379
#15 0x0000000000b289cf in JSRuntime::destroyRuntime (this=this@entry=0x7ffff69651c8) at js/src/vm/Runtime.cpp:430
#16 0x0000000000910569 in JSContext::~JSContext (this=0x7ffff6965000, __in_chrg=<optimized out>) at js/src/jscntxt.cpp:892
#17 0x00000000009108ab in js_delete_poison<JSContext> (p=0x7ffff6965000) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Utility.h:392
#18 js::DestroyContext (cx=0x7ffff6965000) at js/src/jscntxt.cpp:133
#19 0x0000000000896bf7 in JS_DestroyRuntime (rt=rt@entry=0x7ffff69651c8) at js/src/jsapi.cpp:476
#20 0x0000000000438578 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:7452
rax	0x0	0
rbx	0x7fffffffcd30	140737488342320
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffcdd0	140737488342480
rsp	0x7fffffffcce0	140737488342240
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fdc740	140737353992000
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7ffff7ed1718	140737352898328
r13	0x7ffff7e74640	140737352517184
r14	0xfffa7fffffffffff	-1548112371908609
r15	0xfffc000000000000	-1125899906842624
rip	0x975cd0 <CompartmentCheckTracer::onChild(JS::GCCellPtr const&)+864>
=> 0x975cd0 <CompartmentCheckTracer::onChild(JS::GCCellPtr const&)+864>:	movl   $0x0,0x0
   0x975cdb <CompartmentCheckTracer::onChild(JS::GCCellPtr const&)+875>:	ud2    


This might be a dup to one of the previous GC bugs on file, but I'm not sure because the assertion here is new. Marking s-s because GC is involved.

	Christian Holler (:decoder) Reporter
	Comment 1 • 8 years ago

Maybe bug 1275080 because it also involves Proxy?

	Fuzzing Team
	Comment 2 • 8 years ago

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160427220852" and the hash "37c815005a7223bb81f947957bd80ae45c26376f".
The "bad" changeset has the timestamp "20160427224854" and the hash "3c4b7e1de6290ef6e21f2f9e17f99ee5a04f47c6".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=37c815005a7223bb81f947957bd80ae45c26376f&tochange=3c4b7e1de6290ef6e21f2f9e17f99ee5a04f47c6

	Andrew McCreight [:mccr8]
	Comment 3 • 8 years ago

Sounds like a compartment mismatch, so I'm marking this sec-high.

The regression window blames Waldo.

	Andrew McCreight [:mccr8]
	Comment 4 • 8 years ago

Maybe you could look at this, Eric? Thanks.

	Eric Faust [:efaust] Assignee
	Comment 5 • 8 years ago

This test is pretty targeted. I suggest we hold off on landing it for a while.

	Jeff Walden [:Waldo]
	Comment 6 • 8 years ago

Comment on attachment 8773051 [details] [diff] [review]
Fix

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

Yes, this makes sense.  Don't know why I wasn't seeing this immediately when looking at this in bug 1280246, and bug 1275080.  :-(

(I do think the proposed fix in bug 1275080, which would have addressed the problem as well, is also something we should do -- but it's not critical if we make this change, which routes around the problem in similar fashion.)

	Jeff Walden [:Waldo]
	Comment 9 • 8 years ago

There's memory-unsafety in them thar hills.

	Marcia Knous [:marcia]
	Comment 10 • 8 years ago

[Tracking Requested - why for this release]: Carrying over tracking flags from dupe Bug 1275080. There was an open question in the other bug as to whether this affects 48 and 45esr.

	Eric Faust [:efaust] Assignee
	Comment 11 • 8 years ago

Comment on attachment 8773051 [details] [diff] [review]
Fix

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

It's not all that complicated to discover the problem, based on the patch. It involves a stray memory write and incorrect object flags, but the particular object being set is probably not actually exploitable? Still, those side effects are pretty scary. The test, which is basically the minimal operations set to hit the bug, will not land, immediately.

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

Nope.

Which older supported branches are affected by this flaw?

47+

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

Bug 888969. The bug was closed for milestone 49, but it was leave-open for a while.

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

This patch should apply cleanly.

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

Very unlikely. This just correctly handles what was an API violation to avoid a semi-random write.

	Fuzzing Team
	Comment 12 • 8 years ago

JSBugMon: Cannot process bug: Error: Unsupported branch "47 Branch" required by bug

	Al Billings [:abillings - ex-MoCo]
	Comment 13 • 8 years ago

This is too late for the current release cycle so it will have to land two weeks into the next. I'm giving it sec-approval+ for checkin on August 16 (not before). We'll want patches for affected branches made and nominated at the same time as well.

	Christian Holler (:decoder) Reporter
	Comment 14 • 8 years ago

Eric, can we land this today? That would greatly help me triaging other bugs with similar signatures. Thanks!

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcfbc324f389

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/fcfbc324f389

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 18 • 8 years ago

Can you also request uplift as Al mentioned, for aurora and beta? We are heading into beta 8 next Monday. Thanks.

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 19 • 8 years ago

This can still make beta 9. Can you request uplift? It's sec-critical and we already landed the fix on m-c two weeks ago.

	Eric Faust [:efaust] Assignee
	Comment 20 • 8 years ago

Comment on attachment 8773051 [details] [diff] [review]
Fix

Approval Request Comment
[Feature/regressing bug #]:
Bug 888969
[User impact if declined]:
sec-crit random write.
[Describe test coverage new/current, TreeHerder]:
Landed on central 2 weeks ago. No negative reports.
[Risks and why]: Small, no new behavior, just fixes an API violation.
[String/UUID change made/needed]: None.

	Gerry Chang [:gchang]
	Comment 21 • 8 years ago

Comment on attachment 8773051 [details] [diff] [review]
Fix

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

This patch fixes a sec-critical issue. Take it in aurora and 49 beta.

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 8 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/20b2e73b19df

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 23 • 8 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/29375a8f7973