GC races involving the self-hosting zone and workers

RESOLVED FIXED in Firefox -esr45

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jandem, Assigned: jonco)

Tracking

({crash, sec-high})

unspecified
mozilla53
crash, sec-high
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr4551+ fixed, firefox50 wontfix, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main51+][adv-esr45.7+])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
I was looking at the ConstraintTypeSet::sweep topcrash in bug 1220385 and noticed we can call ObjectGroup::sweep for the self-hosting zone. This seemed suspicious because it can race with worker threads (the s-h zone is shared).

I wrote some shell tests involving evalInWorker and below is a test case that asserts reliably on OS X and Linux:

$ JS_GC_ZEAL=2 dist/bin/js test.js
Assertion failure: CurrentThreadCanAccessZone(zone), at Heap.h:1268

Opt builds crash in GC trace functions.

function f() {
    for (var i=0; i<100; i++)
        evalInWorker("while (true) { var a = [1, 2, 3]; a.indexOf(1); relazifyFunctions(); }");
    while (true) {
        a = [];
	a.indexOf(a);
        gc();
    }
}
f();
(Reporter)

Updated

2 years ago
Keywords: crash, sec-high
(Reporter)

Comment 1

2 years ago
This is probably related to bug 1324512 and bug 1311060 that decoder found, same signature and these tests also use evalInWorker.
(Assignee)

Comment 2

2 years ago
(In reply to Jan de Mooij [:jandem] from comment #1)
Yes, this assertion failure is the same problem as bug 1324512: JSRuntime::cloneSelfHostedFunctionScript uses a context which has entered the target compartment to root a function which is in a different compartment (which is in the self hosting zone).  The rooting API pushes Rooteds onto a list stored in the context's zone in most cases so what's happening here is that we're pushing a Rooted onto the list for a different zone.  I'm currently trying to figure out a good way to assert that this doesn't happen.
(Assignee)

Updated

2 years ago
Assignee: nobody → jcoppeard
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1324512
(Assignee)

Comment 4

2 years ago
Created attachment 8824479 [details] [diff] [review]
bug1328251-check-traced-runtime

Here's a patch to not trace GC things in other runtimes.  This simplifies the current checks slightly and adds an assertion that the things we're skipping are those that are legitimately shared between runtimes: self hosted things, permanent atoms and well known symbols.

This fixes the assertion with the testcase above and that in bug 1324512.  I'm no sure whether it will fix the problem with calling ObjectGroup::sweep for the self-hosting zone.

It doesn't address the problem I identified in comment 2 which turned out not to be the issue here.  I'll file a separate bug for that.
Attachment #8824479 - Flags: review?(sphink)
(Reporter)

Comment 5

2 years ago
(In reply to Jon Coppeard (:jonco) from comment #4)
> I'm no sure whether it will fix the problem with calling ObjectGroup::sweep
> for the self-hosting zone.

For this, can we assert in ObjectGroup::sweep etc that the Zone belongs to the current runtime?
(Assignee)

Comment 6

2 years ago
Comment on attachment 8824479 [details] [diff] [review]
bug1328251-check-traced-runtime

This causes assertions in CGC tests.  Cancelling review request.
Attachment #8824479 - Flags: review?(sphink)
(Assignee)

Comment 7

2 years ago
Created attachment 8825041 [details] [diff] [review]
bug1328251-check-traced-runtime v2

The check for tracing inter-runtime pointers is needed in CheckTracedThing too, and we also need to check this during compacting.
Attachment #8824479 - Attachment is obsolete: true
Attachment #8825041 - Flags: review?(sphink)
Comment on attachment 8825041 [details] [diff] [review]
bug1328251-check-traced-runtime v2

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

The new code makes more sense to read, too.
Attachment #8825041 - Flags: review?(sphink) → review+
(Assignee)

Comment 9

2 years ago
Comment on attachment 8825041 [details] [diff] [review]
bug1328251-check-traced-runtime v2

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

Very difficult.

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

No.

Which older supported branches are affected by this flaw?

Everything back to 30.

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

Bug 964057.

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

Should be straightforward.

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

Unlikely.
Attachment #8825041 - Flags: sec-approval?
status-firefox50: --- → wontfix
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
tracking-firefox-esr45: --- → ?
sec-high, tracking for 51+
tracking-firefox51: ? → +
tracking-firefox52: ? → +
tracking-firefox53: ? → +
tracking-firefox-esr45: ? → 51+
Comment on attachment 8825041 [details] [diff] [review]
bug1328251-check-traced-runtime v2

sec-approval+ for trunk. We should get branch patches made and nominated as it is tracking+ for affected branches.
Attachment #8825041 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/ce8ca07638c1
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: javascript-core-security → core-security-release
Hi :jonco, 
Do you think this is worth uplifting to Beta51 & Aurora52?
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 16

2 years ago
Comment on attachment 8825041 [details] [diff] [review]
bug1328251-check-traced-runtime v2

Approval Request Comment
[Feature/Bug causing the regression]: Bug 964057.
[User impact if declined]: Possible crash / security vulnerability.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a small change and is covered by tests
[String changes made/needed]: None
Flags: needinfo?(jcoppeard)
Attachment #8825041 - Flags: approval-mozilla-beta?
Attachment #8825041 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 17

2 years ago
Comment on attachment 8825740 [details] [diff] [review]
bug1328251-esr45

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high bug
User impact if declined: Possible crash / security vulnerability
Fix Landed on Version: 53
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8825740 - Flags: approval-mozilla-esr45?
Comment on attachment 8825041 [details] [diff] [review]
bug1328251-check-traced-runtime v2

Fix a sec-high. Beta51+ & Aurora52+. Should be in 51 beta 14.
Attachment #8825041 - Flags: approval-mozilla-beta?
Attachment #8825041 - Flags: approval-mozilla-beta+
Attachment #8825041 - Flags: approval-mozilla-aurora?
Attachment #8825041 - Flags: approval-mozilla-aurora+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Comment on attachment 8825740 [details] [diff] [review]
bug1328251-esr45

Sec-high, meets the ESR45 uplift criteria. Let's include this fix in ESR45.7
Attachment #8825740 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+][adv-esr45.7+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.