1711413 - Crash [@ js::GCMarker::markUntilBudgetExhausted] or Assertion failure: color != White, at gc/Cell.h:76

Status: VERIFIED FIXED

(Core :: JavaScript: GC, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20210516-3210b5354d3e (opt build, run with --fuzzing-safe --no-threads --more-compartments):

a0=[];
enableShellAllocationMetadataBuilder();
a0.toString=nukeAllCCWs;
g = newGlobal({c:false});
void gcslice(2);
v=new Number(a0);

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000555555e0c052 in js::GCMarker::markUntilBudgetExhausted(js::SliceBudget&, js::GCMarker::ShouldReportMarkTime) ()
#0  0x0000555555e0c052 in js::GCMarker::markUntilBudgetExhausted(js::SliceBudget&, js::GCMarker::ShouldReportMarkTime) ()
#1  0x000055555613b384 in js::gc::GCRuntime::markUntilBudgetExhausted(js::SliceBudget&, js::GCMarker::ShouldReportMarkTime) ()
#2  0x0000555555dfde46 in js::gc::GCRuntime::incrementalSlice(js::SliceBudget&, mozilla::Maybe<JS::GCOptions> const&, JS::GCReason) ()
#3  0x000055555613e5dd in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget const&, mozilla::Maybe<JS::GCOptions> const&, JS::GCReason) ()
#4  0x000055555613eca4 in js::gc::GCRuntime::collect(bool, js::SliceBudget const&, mozilla::Maybe<JS::GCOptions> const&, JS::GCReason) ()
#5  0x00005555558ec4fd in js::gc::GCRuntime::finishGC(JS::GCReason) ()
#6  0x0000555555fb30cb in main ()
rax	0x55555648aecd	93825008185037
rbx	0x7fffffffca60	140737488341600
rcx	0x5555574b7ce0	93825025146080
rdx	0x7ffff6048cd8	140737320881368
rsi	0x0	0
rdi	0x7ffff6048560	140737320879456
rbp	0x7fffffffc6c0	140737488340672
rsp	0x7fffffffc680	140737488340608
r8	0x7ffff5dfe002	140737318477826
r9	0x317c3600000	3400596979712
r10	0x7ffff6048de8	140737320881640
r11	0x7fffffffca60	140737488341600
r12	0x7ffff6048e30	140737320881712
r13	0x7ffff6048560	140737320879456
r14	0x7ffff6048e58	140737320881752
r15	0x7ffff6048de8	140737320881640
rip	0x555555e0c052 <js::GCMarker::markUntilBudgetExhausted(js::SliceBudget&, js::GCMarker::ShouldReportMarkTime)+754>
=> 0x555555e0c052 <_ZN2js8GCMarker24markUntilBudgetExhaustedERNS_11SliceBudgetENS0_20ShouldReportMarkTimeE+754>:	movl   $0x680,0x0
   0x555555e0c05d <_ZN2js8GCMarker24markUntilBudgetExhaustedERNS_11SliceBudgetENS0_20ShouldReportMarkTimeE+765>:	callq  0x5555556d0d30 <abort>

GC issue, marking s-s until investigated.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20210516091748-3210b5354d3e.
The bug appears to have been introduced in the following build range:

Start: 89f7028049edb2eca8e5f8731450278b33f84c27 (20210511140347)
End: f7b553bd9c0b77b0130d377f9a9c696c8c611a01 (20210511142103)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=89f7028049edb2eca8e5f8731450278b33f84c27&tochange=f7b553bd9c0b77b0130d377f9a9c696c8c611a01

Comment 3

[Jon Coppeard (:jonco)]

NI Steve based on comment 2.

Comment 4

[Daniel Veditz [:dveditz]]

"nukeAllCCWs" doesn't sound like something we'd let a web page do? :-)

Comment 5

[Christian Holler (:decoder)]

(In reply to Daniel Veditz [:dveditz] from comment #4)

"nukeAllCCWs" doesn't sound like something we'd let a web page do? :-)

It is something that can happen or get provoked in the browser though. The problem is more likely enableShellAllocationMetadataBuilder and there were other shell-only bugs before that involved it.

Comment 6

[Steve Fink [:sfink] [:s:]]

Yeah, this fuzz bug is great. It's at the problematic intersection of CCW nuking and weakmaps, which has historically been a rich source of low-frequency intermittents in the browser. A web page might not be able to trigger this directly (I'm not sure; perhaps closing an iframe would do it?), but the browser can when it's closing tabs.

As decoder said, enableShellAllocationMetadataBuilder makes it a lot more unlikely. That might get used when profiling native allocations, but I'm not sure. It won't be used in normal circumstances.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1711413 - Fix marking of nuked weakmap key delegates

Comment 8

[Steve Fink [:sfink] [:s:]]

I don't think enableShellAllocationMetadataBuilder is likely to be necessary to trigger this, though I haven't come up with a reproduction without it. (I haven't tried very hard, either.) My current version of this test is:

enableShellAllocationMetadataBuilder();
newGlobal({newCompartment:true});
gcslice(2);
nukeAllCCWs();

which no longer requires the --more-compartments flag.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Fix marking of nuked weakmap key delegates r=jonco
https://hg.mozilla.org/integration/autoland/rev/676659d41d31a9ea253b4bedc0331053c264995d
https://hg.mozilla.org/mozilla-central/rev/676659d41d31

Comment 10

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20210601213358-83f4bfe5ea71.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 11

[Julien Cristau [:jcristau]]

Please request beta uplift when you get a chance.

Comment 12

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9223070 [details]
Bug 1711413 - Fix marking of nuked weakmap key delegates

Beta/Release Uplift Approval Request

• User impact if declined: Crashes, theoretically exploitable though it would be difficult.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change itself is a straightforward change to something that is more conservative. The biggest risk is that it might over-mark part of the GC graph and temporarily increase memory usage (until the next GC).
• String changes made/needed: none

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Very difficult. You would have to

1. trigger the cross-compartment wrapper at a time when a target object is unmarked. That would involve changing domains or navigating away or something? I haven't figured out a way to do it for testing other than using enableAllocationMetadata as the fuzz bug did, and I think that would only be done in the browser if you were profiling with memory allocation tracking. But I can't promise there isn't another way.

2. That will cause the marker to switch to potentially the wrong color. Black instead of gray is safe, so this would be gray instead of black.

3. That color will be used for marking a single object, the delegate. So you'd need it to be part of a garbage cycle that shouldn't be a garbage cycle, which means it is the only remaining incoming edge to that cycle.

4. Then you need the cycle collector to run and release some stuff that it shouldn't, and get a UAF out of that.

But that target cannot be reachable in any other way, since that would mark it black.

Also, it may be that the only possible target here is the inner window proxy, in which case I think its outer window would only be nuked if it's being navigated away from? And in that case, nothing it's keeping alive should be alive. But that logic is shaky.

But it could be slightly worse than the above, because although the code appears as if GCMarker::getStack will merely return the wrong stack, the buggy code is actually using an invalid value for the js::gc::MarkColor variable and so the compiler may optimize it differently than the code reads, which would give an out of range pointer to the mark stack that could be used to scramble internal data. (I haven't looked at the optimized code to see.)

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: beta
• If not all supported branches, which bug introduced the flaw?: Bug 1694538
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It should apply cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely, it's just switching to a more conservative color for marking. See the beta approval request.

The mistake I made here was that I messed up the landing for long enough for this to make it into beta. I didn't need sec-approval to fix it in Nightly, but I should have requested it after it was backed out and the regressing bug uplifted.

Comment 13

[Tom Ritter [:tjr]]

Comment on attachment 9223070 [details]
Bug 1711413 - Fix marking of nuked weakmap key delegates

Approved to land and uplift.

Comment 14

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/04b9b38d80c1