Closed Bug 1852729 (CVE-2023-5728) Opened 10 months ago Closed 9 months ago

Assertion failure: !JS_IsDeadWrapper(obj), at gecko-dev/js/src/gc/FinalizationObservers.cpp:409


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




120 Branch
Tracking Status
firefox-esr115 119+ fixed
firefox118 --- wontfix
firefox119 + fixed
firefox120 + fixed


(Reporter:, Assigned: sfink)


(Blocks 1 open bug)


(Keywords: reporter-external, sec-moderate, Whiteboard: [adv-main119+][adv-ESR115.4+])


(2 files, 1 obsolete file)

Steps to reproduce:

commit: 09f7e00da9eaef55c80620c17e8575ddd4532451


/bin/sh gecko-dev/js/src/configure --enable-debug --disable-optimize --disable-shared-js --disable-tests

Test case:

for (let i = 0, j = 16; i < j; ) 

    Uint32Array.newCompartment = true;
    const foo = this.newGlobal(Uint32Array).WeakRef;
    for (let a = 0, b = 16; a < b; b--) 
        const bar = new foo(this.transplantableObject().object);


Actual results:


Assertion failure: !JS_IsDeadWrapper(obj), at gecko-dev/js/src/gc/FinalizationObservers.cpp:409
JS::NonIncrementalGC(JSContext*, JS::GCOptions, JS::GCReason)[gecko-dev-build/debug_09f7e0/dist/bin/js +0x30e26ff]
Segmentation fault (core dumped)
Group: core-security → javascript-core-security

Steve, would you be willing to take a look at this new bug?

Blocks: GC.stability
Severity: -- → S3
Flags: needinfo?(sphink)
Priority: -- → P2
Ever confirmed: true

Roughly what is going on:

  • make a WeakRef to a target TransplantableDOMObject in another Zone
  • enter it into that Zone's FinalizationObservers' weakRefMap: target -> list of WeakRef CCWs
  • while deref'ing the WeakRef, call the read barrier
  • in the read barrier, the target isDOMClass() and the DOM wrapper is considered to be "released", and the WeakRef target is cleared. I don't really know what this is all about, but I reviewed it when it was added so I'll pretend like I do.
  • nuke all cross-compartment wrappers. Because the WeakRef no longer has a target, it doesn't get cleared out of the weakRefMap
  • crash when a validity check notices that weakRefMap contains a dead wrapper

I'm kind of out of my depth here, but I'll attach a preliminary patch that at least fixes the test case.

Flags: needinfo?(sphink)
Assignee: nobody → sphink
Assignee: nobody → sphink

What are the security implications of this? I'm trying to figure out a rating.

Flags: needinfo?(sphink)

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

What are the security implications of this? I'm trying to figure out a rating.

Yeah, I just went to land this, and realized that I needed to come up with a rating.

I haven't thought through it very carefully, but this doesn't seem very worrisome to me. It is sort of like a use after free, because we could do some finalization work on an object that no longer needs finalization. But the object is a dead object proxy, so I don't think it would really do very much. In particular, I haven't traced through but I can't think of a way for it to access freed memory. There might also be a trivial leak due to the failure to remove an entry from the cross-compartment weakref table.

It's still complicated and weird enough that I would probably keep it sec-something. I find it hard to predict exactly what could happen.

Flags: needinfo?(sphink)

I guess I'll mark it sec-moderate, though we're not entirely sure what the implications might be.

Keywords: sec-moderate
Pushed by
Update weakRefMap when a WeakRef target is cleared r=jonco
Group: javascript-core-security → core-security-release
Closed: 9 months ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 120 Branch

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

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox119 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(sphink)

Comment on attachment 9353030 [details]
Bug 1852729 - Update weakRefMap when a WeakRef target is cleared

Beta/Release Uplift Approval Request

  • User impact if declined: Crashes, maybe some weird behavior. Possibly security problems, though I haven't come up with a scenario.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): The risk is low to medium just because this is complicated code and I guess I wouldn't be horribly surprised if it revealed some other problem. But: it only kicks in for unusual circumstances, the current behavior is clearly wrong, and the updated behavior is removing things from consideration when they get weird, resulting in less overall weirdness. While I appreciate weirdness in the wider world, I prefer to avoid it in Firefox's behavior.
  • String changes made/needed: none
  • Is Android affected?: Yes
Flags: needinfo?(sphink)
Attachment #9353030 - Flags: approval-mozilla-beta?

Comment on attachment 9353030 [details]
Bug 1852729 - Update weakRefMap when a WeakRef target is cleared

Approved for 119.0b6

Attachment #9353030 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Attachment #9353030 - Flags: approval-mozilla-esr115?

Comment on attachment 9353030 [details]
Bug 1852729 - Update weakRefMap when a WeakRef target is cleared

Approved for 115.4esr.

Attachment #9353030 - Flags: approval-mozilla-esr115? → approval-mozilla-esr115+
Whiteboard: [adv-main119+]
Whiteboard: [adv-main119+] → [adv-main119+][adv-ESR115.4+]
Attached file advisory.txt (obsolete) —
Attached file advisory.txt
Attachment #9359577 - Attachment is obsolete: true
Alias: CVE-2023-5728

Bulk-unhiding security bugs fixed in Firefox 119-121 (Fall 2023). Use "moo-doctrine-subsidy" to filter

Group: core-security-release

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons

You need to log in before you can comment on or make changes to this bug.