2028231 - UAF when WeakRef target is Unlinked by CC

Status: RESOLVED FIXED

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

Description

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

Attachment: reproduction case (requires click to trigger it)

Cloned from bug 2025953, where Clauditor found a way to have a WeakRef to an object that is unlinked as part of a cycle, and the WeakRef allows posthumously reaching that object and things it references.

Comment 1

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

bug 2025953 comment 13 from mccr8:

I don't remember how WeakRef integration into the CC works. There are some tests for it, so I'm not sure why this case doesn't work when the other ones do. I assume there's some place we tell the CC about these weak references? I vaguely recall it being hacked into the WeakMap API for that, but if we make a special one for WeakRefs then the CC could track which JS objects are WeakRefs and then drop the referents during CollectWhite (eg the sweeping phase).

bug 2025953 comment 15 from mccr8:

Is there something like we only clear WeakRefs when we return to the event loop and therefore this test case that synchronously runs a CC is mildly bogus? How does that interact with nested event loops? I guess the inner event loop will also trigger a clear?

bug 2025953 comment 16 from jonco:

(In reply to Andrew McCreight [:mccr8] from comment #13)

How does this work in the GC? I assume during sweeping you clear out the referent somehow?

Yes, FinalizationObservers::traceWeakWeakRefEdges checks the list of weak ref targets per zone and clears the referent for any that are dying: https://searchfox.org/firefox-main/source/js/src/gc/FinalizationObservers.cpp#534

I don't remember how WeakRef integration into the CC works. There are some tests for it, so I'm not sure why this case doesn't work when the other ones do. I assume there's some place we tell the CC about these weak references?

I'm not sure there's anything beyond preserving wrappers that are direct targets of WeakRefs. We have WeakMapTracer but that only traces weak maps. We originally used to use weak maps in the weak ref implementation which might have covered this.

We can add an API to check if an object is the target of a weak ref. Or we could add an API to iterate all WeakRef targets. What would work best?

Maybe we should split the weak map part off into a separate bug.

Not relevant to the weak ref issue but I'm confused as to what cycle the CC sees in the test case here. It seems to me it should be AuthenticatorAttestationResponse -> ArrayBuffer -> reponse wrapper -> reponse, but it's implied that PublicKeyCredential is part of it as it is failing to clear the response pointer when it's unlinked.

Comment 2

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

Attachment: user_prefs to run with

Comment 3

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

I believe what is happening is that in the reproduction case, the WeakRef target is a plain ArrayBuffer, the attestationObject from:

[SecureContext, Pref="security.webauth.webauthn",
 Exposed=Window]
interface AuthenticatorAttestationResponse : AuthenticatorResponse {
    [SameObject, Throws] readonly attribute ArrayBuffer attestationObject;
    sequence<DOMString>                                 getTransports();
    [Throws] ArrayBuffer                                getAuthenticatorData();
    [Throws] ArrayBuffer?                               getPublicKey();
    [Throws] COSEAlgorithmIdentifier                    getPublicKeyAlgorithm();
};

The way this normally works is that bug 1642974 made it so that when retrieving a WeakRef target, unlinked DOM objects are detected and return undefined. In this case, the target object is a plain ArrayBuffer object, so the relevant code is not able to see that it is part of an unlinked cycle.

Comment 4

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

Note that this is exactly the situation mccr8 brought up in bug 1642974 comment 2.

Comment 5

[Andrew McCreight [:mccr8]]

There's also bug 2028061, which jonco filed. Maybe it could get duped to here?

(In reply to Steve Fink [:sfink] [:s:] from comment #4)

Note that this is exactly the situation mccr8 brought up in bug 1642974 comment 2.

What a bold visionary! Thanks for digging up the history here, Steve, I'd totally forgotten any of it.

How are these weak ref edges presented to the cycle collector? Maybe it just doesn't see them at all?

Kind of feels like there's going to have be some elaborate special case integration in the cycle collector, like during CollectWhite it iterates over all grey weak referents, checks if it thinks they are garbage, and clear the target if so.

Comment 7

[Andrew McCreight [:mccr8]]

I think this would mostly manifest as weird null deref crashes in DOM nodes, but I don't remember seeing this in great volume. (We had a similar problem with weak references XPCWrappedJS which did show up fairly often at least in our automated tests.)

Comment 8

[Andrew McCreight [:mccr8]]

I think this would mostly manifest as weird null deref crashes in DOM nodes, but I don't remember seeing this in great volume. (We had a similar problem with weak references XPCWrappedJS which did show up fairly often at least in our automated tests.)

Comment 9

[Jon Coppeard (:jonco)]

Attachment: (secure)

This fixes the problem by clearing the targets of WeakRefs when those tagets
are found to be part of a gray cycle by the cycle collector. This is the same
thing that happens for nsWeakReferences which should be cleared when their
targets are unlinked.

This patch does add a lookup for each gray JS thing found to be part of the
cycle. This might do a hash table lookup but there are a couple of checks that
happen first that can short circuit that. Do you think this will add
unreasonable overhead?

This doesn't add any new tests but I confirmed that the new code is exercised
by existing tests.

Comment 10

[Jon Coppeard (:jonco)]

Attachment: (secure)

This fixes the problem by clearing the targets of WeakRefs when those tagets
are found to be part of a gray cycle by the cycle collector. This is the same
thing that happens for nsWeakReferences which should be cleared when their
targets are unlinked.

This version looks up all WeakRef targets in the cycle collector table to check
whether they are white. If so their corresponding WeakRefs are cleared. This
requires a hash table lookup for every weak ref rather than for every white GC
thing found by the cycle collector.

Comment 11

[Jon Coppeard (:jonco)]

Attachment: Bug 2028231 - Add testcase r?mccr8

Comment 12

[Jon Coppeard (:jonco)]

Comment on attachment 9568219 [details]
(secure)

Security Approval Request

• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: All supportd branches
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Should be simple.
• How likely is this patch to cause regressions; how much testing does it need?: This is a straightforward fix and is unlikely to cause regressions.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/806ccbefee77
https://hg.mozilla.org/integration/autoland/rev/ff7d7cd6ef98
Cycle collector integration for JS WeakRefs r=sfink,mccr8

Comment 14

[Bugmon [:jkratzer for issues]]

Unable to reproduce bug 2028231 using build mozilla-central 20260331210827-1369941f425d. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg-edge.mozilla.org/mozilla-central/rev/ff7d7cd6ef98

Please nominate this for Beta, ESR140, and ESR115 uplift.

Comment 16

[Jon Coppeard (:jonco)]

Attachment: (secure)

Comment 17

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined/Reason for urgency: Possible crash / security vulnerability
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: This is a straightfoward fix to clear the targets of WeakRefs when those targets are found to be part of a cycle that the CC will unlink. This has been on central for three days without issues.
• String changes made/needed?: None
• Is Android affected?: yes

Comment 18

[Jon Coppeard (:jonco)]

Attachment: (secure)

Comment 19

[Phabricator Automation]

firefox-esr115 Uplift Approval Request

• User impact if declined/Reason for urgency: Possible crash / security vulnerability.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: This is a straightfoward fix to clear the targets of WeakRefs when those targets are found to be part of a cycle that the CC will unlink. This has been on central for three days without issues.
• String changes made/needed?: None
• Is Android affected?: yes

Comment 20

[Dianna Smith [:diannaS]]

:jonco still needs a beta one if we want this in todays ESR build

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/f35ac725814b

Comment 22

[Dianna Smith [:diannaS]]

:Jonco this actually had some bustage on esr115
failure log

It appears the issue might occur on both esr patches.

Comment 23

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/292156911ac5

Comment 24

[Jon Coppeard (:jonco)]

Attachment: (secure)

This fixes the problem by clearing the targets of WeakRefs when those tagets
are found to be part of a gray cycle by the cycle collector. This is the same
thing that happens for nsWeakReferences which should be cleared when their
targets are unlinked.

This patch looks up all WeakRef targets in the cycle collector table to check
whether they are white. If so their corresponding WeakRefs are cleared. This
requires a hash table lookup for every weak ref.

Original Revision: https://phabricator.services.mozilla.com/D293099

Comment 25

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Possible crash / security vulnerability.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: This is a straightforward fix to clear the targets of WeakRefs when those targets are found to be part of a cycle that the CC will unlink. This has been on central for four days without issues.
• String changes made/needed?: None
• Is Android affected?: yes

Comment 26

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:jonco, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox150 to wontfix.

For more information, please visit BugBot documentation.

Comment 27

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/ae612fa87a9b

Comment 28

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/1b0f8ee623d2

Comment 29

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/f89b56f26f55
https://hg.mozilla.org/releases/mozilla-release/rev/001460a39f38

Comment 30

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/d8017e376756

Comment 31

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/41748eaadcc1

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/38f931d9a059
https://hg.mozilla.org/integration/autoland/rev/34a0274a973a
Add testcase r=mccr8

Comment 33

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/34a0274a973a