Dead zones are sometimes scheduled for GC
Categories
(Core :: XPCOM, defect)
Tracking
()
People
(Reporter: jonco, Assigned: jonco)
References
Details
(Keywords: csectype-uaf, sec-high, Whiteboard: [sec-survey][adv-main85+r][adv-esr78.7+r])
Attachments
(4 files)
47 bytes,
text/x-phabricator-request
|
tjr
:
approval-mozilla-beta+
tjr
:
sec-approval+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
tjr
:
approval-mozilla-beta+
tjr
:
sec-approval+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr78+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-esr78+
|
Details | Review |
While working on bug 1677765 I came across a situation where CycleCollectedJSRuntime would try to schedule a zone that had already been destroyed. This is quite bad because we don't currently do any checks on the zone pointer passed to JS::PrepareZoneForGC and just go ahead and write one of the zone's flags.
I think this problem happens because we can add zones to the waiting set during an incremental GC which can end up destroying the zone.
Assignee | ||
Comment 1•3 years ago
|
||
This adds assertions that zone pointers passed in refer to zones we know about
and adds and API that's called when zones are destroyed. It also adds some
standard assertions for other related APIs.
Assignee | ||
Comment 2•3 years ago
|
||
This takes account of the fact that zones may be added to the waiting set
during an incremental GC, after the set has been cleared in
CycleCollectedJSRuntime::PrepareWaitingZonesForGC. I considered disallowing
zones to be added to the set during a GC but decided it would be better not to
lose track of poked zones in the usual case where they don't end up getting
destoryed by the current GC.
Depends on D99071
Updated•3 years ago
|
Comment 3•3 years ago
|
||
I'll mark this sec-high, so please be sure to get sec-approval, etc. as we're near the end of a release cycle. I don't know how exploitable it will be, but setting bits in random freed memory is bad.
Assignee | ||
Comment 4•3 years ago
|
||
Comment on attachment 9191920 [details]
Bug 1681268 - Remove destroyed zones from the set of zones waiting for GC r?mccr8
Security Approval Request
- How easily could an exploit be constructed based on the patch?: Very difficult; requires causing zones to be queued for GC at just the right time.
- 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?: All
- If not all supported branches, which bug introduced the flaw?: Bug 1052793
- Do you have backports for the affected branches?: No
- If not, how different, hard to create, and risky will they be?: It should be trivial to apply this fix.
- How likely is this patch to cause regressions; how much testing does it need?: Unlikely. The worst that could happen is that some zones would not be collected promptly.
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Comment 5•3 years ago
|
||
Comment on attachment 9191920 [details]
Bug 1681268 - Remove destroyed zones from the set of zones waiting for GC r?mccr8
Approved to land and uplift
Comment 6•3 years ago
|
||
Comment on attachment 9191919 [details]
Bug 1681268 - Check zone pointers passed into the API and add a callback for zone destruction r?sfink
Approved to land and uplift
Assignee | ||
Comment 7•3 years ago
|
||
Comment 8•3 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/87799e10615a
https://hg.mozilla.org/mozilla-central/rev/7c1dcab22610
Comment 9•3 years ago
|
||
uplift |
Comment 10•3 years ago
|
||
As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.
Please visit this google form to reply.
Comment 11•3 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr78/rev/8e949a39a0f2
https://hg.mozilla.org/releases/mozilla-esr78/rev/851810d2f3a5
Comment 12•3 years ago
|
||
backout |
Backed out from ESR78 for bustage. Needs rebasing around bug 1647319 from the looks of it.
https://hg.mozilla.org/releases/mozilla-esr78/rev/007124719215b266826cfea68a20593cd1b9ae17
https://treeherder.mozilla.org/logviewer?job_id=324973034&repo=mozilla-esr78&lineNumber=1766
Updated•3 years ago
|
Assignee | ||
Comment 13•3 years ago
|
||
This adds assertions that zone pointers passed in refer to zones we know about
and adds and API that's called when zones are destroyed. It also adds some
standard assertions for other related APIs.
Assignee | ||
Comment 14•3 years ago
|
||
This takes account of the fact that zones may be added to the waiting set
during an incremental GC, after the set has been cleared in
CycleCollectedJSRuntime::PrepareWaitingZonesForGC. I considered disallowing
zones to be added to the set during a GC but decided it would be better not to
lose track of poked zones in the usual case where they don't end up getting
destoryed by the current GC.
Depends on D100799
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment 15•3 years ago
|
||
uplift |
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Description
•