Closed Bug 1667912 Opened 3 years ago Closed 3 years ago

Nonincremental weakmap marking incorrectly splits up Zones


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




84 Branch
Tracking Status
firefox-esr78 83+ fixed
firefox81 --- wontfix
firefox82 --- wontfix
firefox83 + fixed
firefox84 + fixed


(Reporter: sfink, Assigned: sfink, NeedInfo)




(Keywords: csectype-uaf, regression, sec-high, Whiteboard: [sec-survey][adv-main83+r][adv-esr78.5+r])


(1 file)

When I made the incremental weakmap marking preference, I kept the nonincremental code path in place in case we needed to switch back. But it didn't do exactly what it did before. In particular, nonincremental mode builds the whole gcWeakKeys table at one time when switching the weak marking mode. It should be doing that for all collecting zones at once, because the weakmaps in a key zone may add entries to gcWeakKeys in a delegate's zone. Instead, it's does it one zone at a time: clear out gcWeakKeys, then scan through the weakmaps in that zone. If you process a key's zone first, then the delegate's entry in gcWeakKeys will be missing.

This might be exploitable as a UAF.

Assignee: nobody → sphink
Group: javascript-core-security, core-security
Group: core-security
Regressed by: 1633176
Has Regression Range: --- → yes

Comment on attachment 9178337 [details]
Bug 1667912 - Fix nonincremental weakmap marking

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Pretty hard. The patch itself just reorders things in a way that doesn't suggest why it might be important. If you understood the flaw being, then it's still pretty hard.
  • 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?: esr78
  • If not all supported branches, which bug introduced the flaw?: Bug 1633176
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: I expect it would apply pretty easily.
  • How likely is this patch to cause regressions; how much testing does it need?: There is a good chance that there will still be even harder to trigger bugs after this, but the chances of this patch causing a regression is very low. It simplifies the non-incremental version of weakmap marking (which is what is currently enabled.)
Attachment #9178337 - Flags: sec-approval?

Comment on attachment 9178337 [details]
Bug 1667912 - Fix nonincremental weakmap marking

Beta/Release Uplift Approval Request

  • User impact if declined: Rare intermittent in CI, potentially exploitable UAF in production. Fairly hard to trigger/exploit.
  • Is this code covered by automated tests?: Unknown
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce: (I don't have a way to trigger this reliably.)
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It makes the behavior simpler, and matches earlier behavior that was mistakenly changed.
  • String changes made/needed: none
Attachment #9178337 - Flags: approval-mozilla-beta?

It's too late for 82 now.

Comment on attachment 9178337 [details]
Bug 1667912 - Fix nonincremental weakmap marking

Approved to land and uplift

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

This landed:

And got backed out for Build bustage and reftest failure in builds/worker/checkouts/gecko/js/src/gc/GC.cpp:

Push with failures:

Assertion failure: zone->gcWeakKeys().count() == 0, at /builds/worker/checkouts/gecko/js/src/gc/GC.cpp:4350

Flags: needinfo?(sphink)

Ugh. I fear I may have messed up in determining which patches should land now and which can be later. This may require the first patch in bug 1667913 after all. But I have an rr recording of this happening locally, so I should be able to figure it out soon.

Crud. No, it's nothing to do with bug 1667913. It's that I left a diagnostic assertion in the latest patch that I uploaded -- I wasn't sure whether it would hold or not, so I was using to identify any remaining sources of gcWeakKeys entries that I hadn't yet stamped out. But that's just for efficiency and simplifying the proof of correctness; it's not actually needed for correctness. (It's pretty clear from the code -- I assert that a vector is empty immediately before clearing the vector.)

I may just remove the assert, but I'll first check to see if there isn't a way to weaken the assertion to make it hold. (The reason for a nonempty gcWeakKeys that it's identifying right now is a valid one: we mark twice, with different colors. The assert should only be made at the beginning of the first mark; it's ok if the first mark adds some entries before the second one. No barriers run in between.)

Group: javascript-core-security → core-security-release
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 84 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?(sphink)
Whiteboard: [sec-survey]
Whiteboard: [sec-survey] → [sec-survey][adv-main83+r]
Whiteboard: [sec-survey][adv-main83+r] → [sec-survey][adv-main83+r][adv-esr78.5+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.