Closed Bug 1681268 Opened 3 years ago Closed 3 years ago

Dead zones are sometimes scheduled for GC

Categories

(Core :: XPCOM, defect)

defect

Tracking

()

RESOLVED FIXED
86 Branch
Tracking Status
firefox-esr78 85+ fixed
firefox83 --- wontfix
firefox84 --- wontfix
firefox85 + fixed
firefox86 + fixed

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)

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.

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.

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

Group: javascript-core-security → dom-core-security

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.

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.
Attachment #9191920 - Flags: sec-approval?
Attachment #9191919 - Flags: sec-approval?

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

Attachment #9191920 - Flags: sec-approval?
Attachment #9191920 - Flags: sec-approval+
Attachment #9191920 - Flags: approval-mozilla-esr78+
Attachment #9191920 - Flags: approval-mozilla-beta+

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

Attachment #9191919 - Flags: sec-approval?
Attachment #9191919 - Flags: sec-approval+
Attachment #9191919 - Flags: approval-mozilla-esr78+
Attachment #9191919 - Flags: approval-mozilla-beta+
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch

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.

Flags: needinfo?(jcoppeard)
Whiteboard: [sec-survey]
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-

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.

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

Attachment #9191919 - Flags: approval-mozilla-esr78+
Attachment #9191920 - Flags: approval-mozilla-esr78+
Attachment #9195436 - Flags: approval-mozilla-esr78+
Flags: needinfo?(jcoppeard)
Attachment #9195437 - Flags: approval-mozilla-esr78+
Whiteboard: [sec-survey] → [sec-survey][adv-main85+r]
Whiteboard: [sec-survey][adv-main85+r] → [sec-survey][adv-main85+r][adv-esr78.7+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: