Closed Bug 1282746 Opened 3 years ago Closed 3 years ago

Assertion failure: comp == compartment || runtime()->isAtomsCompartment(comp) || (srcKind == JS::TraceKind::Object && InCrossCompartmentMap(static_cast<JSObject*>(src), thing)), at js/src/jsgc.cpp:3745

Categories

(Core :: JavaScript Engine, defect, critical)

47 Branch
x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 + fixed
firefox-esr45 --- unaffected
firefox50 + fixed
firefox51 + fixed

People

(Reporter: decoder, Assigned: efaust)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:][post-critsmash-triage][adv-main49+])

Attachments

(1 file)

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.
Maybe bug 1275080 because it also involves Proxy?
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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
Sounds like a compartment mismatch, so I'm marking this sec-high.

The regression window blames Waldo.
Flags: needinfo?(jwalden+bmo)
Keywords: sec-high
Maybe you could look at this, Eric? Thanks.
Flags: needinfo?(efaustbmo)
Attached patch FixSplinter Review
This test is pretty targeted. I suggest we hold off on landing it for a while.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(efaustbmo)
Attachment #8773051 - Flags: review?(jwalden+bmo)
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.)
Attachment #8773051 - Flags: review?(jwalden+bmo) → review+
Duplicate of this bug: 1280246
Duplicate of this bug: 1275080
There's memory-unsafety in them thar hills.
Keywords: sec-highsec-critical
[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.
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.
Attachment #8773051 - Flags: sec-approval?
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Error: Unsupported branch "47 Branch" required by bug
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.
Whiteboard: [jsbugmon:] → [jsbugmon:][checkin on 8/16]
Attachment #8773051 - Flags: sec-approval? → sec-approval+
Eric, can we land this today? That would greatly help me triaging other bugs with similar signatures. Thanks!
Flags: needinfo?(efaustbmo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcfbc324f389
Flags: needinfo?(efaustbmo)
Whiteboard: [jsbugmon:][checkin on 8/16] → [jsbugmon:]
https://hg.mozilla.org/mozilla-central/rev/fcfbc324f389
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Group: javascript-core-security → core-security-release
Duplicate of this bug: 1295172
Can you also request uplift as Al mentioned, for aurora and beta? We are heading into beta 8 next Monday. Thanks.
Flags: needinfo?(efaustbmo)
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.
Flags: needinfo?(nihsanullah)
Flags: needinfo?(jwalden+bmo)
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.
Flags: needinfo?(nihsanullah)
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(efaustbmo)
Attachment #8773051 - Flags: approval-mozilla-beta?
Attachment #8773051 - Flags: approval-mozilla-aurora?
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.
Attachment #8773051 - Flags: approval-mozilla-beta?
Attachment #8773051 - Flags: approval-mozilla-beta+
Attachment #8773051 - Flags: approval-mozilla-aurora?
Attachment #8773051 - Flags: approval-mozilla-aurora+
Whiteboard: [jsbugmon:] → [jsbugmon:][post-critsmash-triage]
Whiteboard: [jsbugmon:][post-critsmash-triage] → [jsbugmon:][post-critsmash-triage][adv-main49+]
Blocks: 888969
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.