Closed Bug 1566678 Opened 2 years ago Closed 2 years ago

Assertion failure: cx->compartment() == obj->compartment(), at src/js/src/vm/Shape.cpp:1496

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- verified

People

(Reporter: tsmith, Assigned: bzbarsky)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, sec-moderate, testcase, Whiteboard: [post-critsmash-triage][adv-main70+][adv-main70+r])

Attachments

(4 files)

Attached file test.zip

STR:

  1. unpack test.zip
  2. using a fuzzing build, a clean profile and the included prefs.js launch the browser
  3. open launcher.html
  4. wait 30 - 45 seconds

Marking as s-s to be safe.

I can consistently reproduce the issue with a fuzzing debug build.

Assertion failure: cx->compartment() == obj->compartment(), at src/js/src/vm/Shape.cpp:1496

0|0|libxul.so|JSObject::setFlags(JSContext*, JS::Handle<JSObject*>, js::BaseShape::Flag, JSObject::GenerateShape)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Shape.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|1496|0x49
0|1|libxul.so|js::SetImmutablePrototype(JSContext*, JS::Handle<JSObject*>, bool*)|hg:hg.mozilla.org/mozilla-central:js/src/vm/JSObject.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|2999|0x12
0|2|libxul.so|js::ForwardingProxyHandler::setImmutablePrototype(JSContext*, JS::Handle<JSObject*>, bool*) const|hg:hg.mozilla.org/mozilla-central:js/src/proxy/Wrapper.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|111|0x10
0|3|libxul.so|js::ForwardingProxyHandler::setImmutablePrototype(JSContext*, JS::Handle<JSObject*>, bool*) const|hg:hg.mozilla.org/mozilla-central:js/src/proxy/Wrapper.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|111|0x10
0|4|libxul.so|SetImmutablePrototype|hg:hg.mozilla.org/mozilla-central:js/src/builtin/TestingFunctions.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|4254|0x11
0|5|libxul.so|CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|448|0x16
0|6|libxul.so|js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|540|0x12
0|7|libxul.so|InternalCall|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|595|0xd
0|8|libxul.so|Interpret|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|599|0x13
0|9|libxul.so|js::RunScript(JSContext*, js::RunState&)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|425|0xb
0|10|libxul.so|js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*)|hg:hg.mozilla.org/mozilla-central:js/src/vm/Interpreter.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|787|0x5
0|11|libxul.so|EvalKernel|hg:hg.mozilla.org/mozilla-central:js/src/builtin/Eval.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|340|0x16
0|12|libxul.so|js::DirectEval(JSContext*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>)|hg:hg.mozilla.org/mozilla-central:js/src/builtin/Eval.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|453|0x37
0|13|libxul.so|js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>)|hg:hg.mozilla.org/mozilla-central:js/src/jit/BaselineIC.cpp:0b014ee5cd51250fcdfc7df3914efa3a3d9f86d2|3197|0x2a
...
see attached full log.
Attached file full_log.txt

It's calling the setImmutablePrototype testing function on a proxy with ForwardingProxyHandler, and the target is in a different compartment. Boris, this looks like our friend OuterWindowProxy :)

Flags: needinfo?(bzbarsky)

Yes, it does; verifying that now, but waiting on builds. Assuming it is, what should actually happen in this case? The obvious options are that either the thing ForwardingProxyHandler::setImmutablePrototype calls should not assert anything about compartments or we should enter the compartment of the underlying object, right? Or just have MaybeCrossOriginObject::setImmutablePrototype throw (or nsOuterWindowProxy::setImmutablePrototype).

Web content can't actually trigger this codepath, right?

Flags: needinfo?(bzbarsky) → needinfo?(jdemooij)

OK, when we hit the assert we're definitely doing a setImmutablePrototype on a xpc::CrossOriginObjectWrapper for a nsOuterWindowProxy.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3)

Or just have MaybeCrossOriginObject::setImmutablePrototype throw (or nsOuterWindowProxy::setImmutablePrototype).

That makes sense to me.

Web content can't actually trigger this codepath, right?

Right, as far as I can tell. Proxy::setImmutablePrototype is called by js::SetImmutablePrototype, and the only interesting caller there is JS_SetImmutablePrototype. That's called for globals and WindowNamedPropertiesHandler:

https://searchfox.org/mozilla-central/search?q=symbol:_Z24JS_SetImmutablePrototypeP9JSContextN2JS6HandleIP8JSObjectEEPb&redirect=false

Flags: needinfo?(jdemooij)

So what I can't figure out is how to write a sane case for this. Which makes it hard to test a fix, because the original steps to reproduce takes tens of minutes to run here.

It seems that I need the following:

  1. Two globals that are not same-compartment.
  2. A cross-compartment wrapper between them that is not an Xray.
  3. The JS testing functions available in one of the globals.

So for example, if I do a mochitest-plain and get the JS testing functions via SpecialPowers, then by the time I call the testing function I'm in the chrome compartment and condition 2 is violated if I am passing in a content window.

If I do a mochitest-chrome, then I can make condition 2 work out, but then all my stuff ends up same-compartment.

I don't even know how the fuzzing testcase ends up getting its hands on testing functions. The only places that do that are shell's NewGlobalObject and Components.utils and WorkerScope... Any idea how that's working?

calling sec-moderate on the belief that web code can't trigger this bug.

Keywords: sec-moderate

I don't even know how the fuzzing testcase ends up getting its hands on testing functions.

Ah, now I do. It's this block in FinishObjectClassInit:

#ifdef FUZZING
  if (cx->options().fuzzing()) {
    if (!DefineTestingFunctions(cx, global, /* fuzzingSafe = */ true,
                                /* disableOOMFunctions = */ false)) {
      return false;
    }
  }
#endif

so I think this codepath actually can't be reached at all in non-fuzzing builds... and you have to have the fuzzing.enabled pref set.

Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Priority: -- → P1
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Is this something we should consider backporting or can it ride the trains? If yes to backporting, which branches are affected?

Flags: needinfo?(bzbarsky)

I wouldn't bother backporting this unless it's causing problems for the fuzzing folks. There's no way to trigger this codepath in a non-fuzzing environment.

Flags: needinfo?(bzbarsky)
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]

Mozilla/5.0 (X11; Linux x86_64; rv:71.0) Gecko/20100101 Firefox/71.0
Build ID: 20190903094847

I can confirm that on the latest fuzzing debug build, I do not get the "Assertion failure: cx->compartment() == obj->compartment(), at src/js/src/vm/Shape.cpp:1496" and that the "target page" tab does not crash anymore (it's in a continuum loading) , but I'm seeing another Assertion failure:
Assertion failure: NS_IsMainThread(), at /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ClearOnShutdown.h:127

Boris, could you please tell me if this is expected?

Flags: needinfo?(bzbarsky)

It's not expected by me, but it's not really related to this bug either.... If it's consistently reproducible, please file a bug with steps to reproduce (including the link to the exact build involved)?

Flags: needinfo?(bzbarsky)

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #15)

It's not expected by me, but it's not really related to this bug either.... If it's consistently reproducible, please file a bug with steps to reproduce (including the link to the exact build involved)?

Thanks Boris, logged Bug 1579395 to cover that assertion failure.

Verified that the "Assertion failure: cx->compartment() == obj->compartment(), at src/js/src/vm/Shape.cpp:1496" is not reproducible on the latest Firefox 70 beta fuzzing version (https://tools.taskcluster.net/index/gecko.v2.mozilla-beta.latest.firefox/linux64-fuzzing-debug). Based on this and on Comment 14, marking this bug as VERIFIED FIXED.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main70+][adv-main70-rollup]
Whiteboard: [post-critsmash-triage][adv-main70+][adv-main70-rollup] → [post-critsmash-triage][adv-main70+][adv-main70+r]
Flags: in-testsuite?
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.