2022367 - Data race on non-atomic reference count in `JS::TimeZoneString` leads to double-free / use-after-free

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jason Kratzer [:jkratzer]]

Summary

Data race on non-atomic reference count in JS::TimeZoneString leads to double-free / use-after-free. Root cause is at /firefox/js/src/vm/Realm.cpp:572 where Realm::getDateTimeInfo() performs non-atomic AddRef/Release on every call, combined with cross-thread object sharing via /firefox/dom/workers/WorkerPrivate.h:738 (CopyJSSettings).

Affected Code

File: /firefox/js/src/vm/Realm.cpp, line 572

js::DateTimeInfo* Realm::getDateTimeInfo() {
#if JS_HAS_INTL_API
  if (RefPtr<TimeZoneString> timeZone = behaviors_.timeZoneOverride()) {   // Line 572: AddRef+Release every call
    if (!dateTimeInfo_) {
      ...
      dateTimeInfo_ = js::MakeUnique<js::DateTimeInfo>(timeZone);
      ...
    } else {
      dateTimeInfo_->updateTimeZoneOverride(timeZone);                     // Another AddRef+Release (by-value arg)
    }
    return dateTimeInfo_.get();
  }
#endif
  return nullptr;
}

File: /firefox/js/public/RealmOptions.h, lines 63 and 314

struct TimeZoneString : js::RefCounted<TimeZoneString> {    // Non-atomic refcount
  const char* chars_;
  ...
};
...
RefPtr<TimeZoneString> timeZoneOverride() const { return timeZoneOverride_; }   // Return by value = copy ctor = AddRef

File: /firefox/js/public/RefCounted.h, lines 30-48

template <typename T>
class RefCounted {
  ...
  void AddRef() const {
    MOZ_ASSERT(int32_t(mRefCnt) >= 0);
    ++mRefCnt;                                              // Non-atomic increment
  }

  void Release() const {
    MOZ_ASSERT(int32_t(mRefCnt) > 0);
    MozRefCountType cnt = --mRefCnt;                        // Non-atomic decrement
    if (0 == cnt) {
      js_delete(const_cast<T*>(static_cast<const T*>(this)));  // Line 42: free when reaches 0
    }
  }
 private:
  mutable MozRefCountType mRefCnt;   // MozRefCountType = uintptr_t (plain integer)
};

File: /firefox/dom/workers/WorkerPrivate.h, line 736-739

void CopyJSSettings(workerinternals::JSSettings& aSettings) {
  mozilla::MutexAutoLock lock(mMutex);
  aSettings = mJSSettings;    // Struct copy => RealmOptions copy => RealmBehaviors copy => RefPtr<TimeZoneString> copy
}                             // Mutex only protects READ of mJSSettings, NOT the shared refcount

Why it's vulnerable: behaviors_.timeZoneOverride() returns RefPtr<TimeZoneString> by value. Each invocation of getDateTimeInfo() therefore performs at minimum 2 AddRefs + 2 Releases (one pair for the local timeZone, one pair for the by-value arg to updateTimeZoneOverride). Disassembly of /firefox/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so at symbol _ZN2JS5Realm15getDateTimeInfoEv confirms these are unlocked incq (%rdi) / decq (%rdi) instructions — non-atomic read-modify-write on the shared counter.

The TimeZoneString object becomes cross-thread shared via nested worker creation: /firefox/dom/workers/WorkerPrivate.cpp:2851 calls aParent->CopyJSSettings(mJSSettings) which copy-constructs JSSettings → RealmOptions → RealmBehaviors → RefPtr<TimeZoneString>, sharing the same underlying object. Each worker's Realm::behaviors_ (initialized at /firefox/js/src/vm/Realm.cpp:51 via behaviors_(options.behaviors())) also holds a RefPtr to the same object.

When two worker threads concurrently execute getDateTimeInfo(), lost updates on ++mRefCnt/--mRefCnt cause the refcount to drift downward. Once it prematurely reaches 0, one thread frees the object; subsequent operations on other threads produce double-free or use-after-free.

An identical parallel bug exists for LocaleString at /firefox/js/src/vm/Realm.cpp:556 (Realm::getLocale()), since LocaleString at /firefox/js/public/RealmOptions.h:54 also inherits js::RefCounted.

Exploit Chain

1. Precondition: privacy.resistFingerprinting=true (user-settable pref; default in Tor Browser) causes /firefox/js/xpconnect/src/nsXPConnect.cpp:552 to call setTimeZoneOverride(nsRFPService::GetSpoofedJSTimeZone().get()) with "Atlantic/Reykjavik".
2. Web content creates a Worker. Since aParent == nullptr, /firefox/dom/workers/WorkerPrivate.cpp:2889 calls InitGlobalObjectOptions() → setTimeZoneOverride() → CopyStringZ<TimeZoneString>() at /firefox/js/src/jsapi.cpp:1784 allocates a 35-byte TimeZoneString (16 bytes struct + 19 bytes "Atlantic/Reykjavik\0").
3. The worker's JS calls new Worker(...) creating nested workers. /firefox/dom/workers/WorkerPrivate.cpp:2851 calls CopyJSSettings() → struct copy → RefPtr copy constructor → same TimeZoneString now shared across threads.
4. Each nested worker's Realm is constructed at /firefox/js/src/vm/Realm.cpp:51 with behaviors_(options.behaviors()) → another RefPtr copy → more shared references.
5. All worker threads concurrently call Date.prototype.getHours() → date_getHours → fillLocalTimeSlots() → dateTimeInfo() → Realm::getDateTimeInfo() at /firefox/js/src/vm/Realm.cpp:572.
6. Each call executes ~4 unlocked incq/decq on mRefCnt. Concurrent lost updates across physical cores cause the refcount to underflow its true value.
7. One thread's Release() observes --mRefCnt == 0 → js_delete() frees the 35-byte region.
8. Another thread's Release() also reaches 0 → double-free; or its AddRef() / timeZone->chars() dereferences freed memory → use-after-free.

Security Impact

Severity: High

Attacker capability: Remote web content can trigger a double-free / use-after-free of a 35-byte heap object in the content process. The 35-byte allocation size is amenable to heap grooming (e.g. ArrayBuffer, strings), enabling type confusion and potential arbitrary read/write.

Preconditions:

• privacy.resistFingerprinting=true (default in Tor Browser; user-enabled in Firefox for privacy)
• Alternatively: WebDriver BiDi emulation.setTimezoneOverride
• Multi-core CPU (race needs cache-coherency latency between physical cores)

Suggested Fix

Change TimeZoneString and LocaleString to use js::AtomicRefCounted:

// /firefox/js/public/RealmOptions.h
struct LocaleString : js::AtomicRefCounted<LocaleString> {      // was: js::RefCounted
  const char* chars_;
  explicit LocaleString(const char* chars) : chars_(chars) {}
  auto* chars() const { return chars_; }
};

struct TimeZoneString : js::AtomicRefCounted<TimeZoneString> {  // was: js::RefCounted
  const char* chars_;
  explicit TimeZoneString(const char* chars) : chars_(chars) {}
  auto* chars() const { return chars_; }
};

Additionally, getDateTimeInfo() and getLocale() should avoid hot-path refcount churn since behaviors_ is owned by the Realm and its RefPtr<TimeZoneString> member cannot be modified while the function runs on the owning thread.

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: crash_stack.txt

Comment 2

[Jason Kratzer [:jkratzer]]

Attachment: prefs.json

Comment 3

[Jason Kratzer [:jkratzer]]

Attachment: test.html

Comment 4

[Jason Kratzer [:jkratzer]]

Attachment: ASan Log

Comment 5

[Andrew McCreight [:mccr8]]

How reproducible is this? Also maybe a TSan log would be useful. Thanks.

Comment 6

[Jon Coppeard (:jonco) (PTO until 14th September)]

Suggested Fix: Change TimeZoneString and LocaleString to use js::AtomicRefCounted

The JS API is not intended to be used across different threads like this. Arguably this is a bug in the worker code that does this, although it's not at all clear that you shouldn't just copy RealmOptions.

Comment 7

[Jason Kratzer [:jkratzer]]

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

How reproducible is this? Also maybe a TSan log would be useful. Thanks.

This reproduces consistently for me on ASan. I'll attach the TSan log shortly.

Comment 8

[Jason Kratzer [:jkratzer]]

Attachment: TSan Log

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Jason Kratzer [:jkratzer] from comment #7)

This reproduces consistently for me on ASan. I'll attach the TSan log shortly.

Thank you! I'll call this sec-high then.

(In reply to Jon Coppeard (:jonco) from comment #6)

The JS API is not intended to be used across different threads like this. Arguably this is a bug in the worker code that does this, although it's not at all clear that you shouldn't just copy RealmOptions.

We could move this to DOM workers if you think the fix should lie there. It would probably be good to add some thread safety assertions if possible. We do that with refcounted objects in the DOM, where it saves a thread affinity and then checks it on use.

Comment 10

[Gustavo Tonietto (he/him) [:gtonietto] [:tonietto]]

Even tho a meta was assigned it can of course be changed after investigation if ownership is determined to be on DOM workers.

Comment 11

[Jan de Mooij [:jandem]]

The simplest fix is to copy the override strings when copying JS settings for a nested worker.

I also tried a bigger change to disable the normal copy constructors for RealmOptions/RealmBehaviors and require using either move constructors or an explicit copy. This works but the patch is much larger because it affects all JS_NewGlobalObject callers. I'm also not sure that change is worth it just to deal with this case.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: (secure)

Comment 13

[Jan de Mooij [:jandem]]

Attachment: (secure)

Comment 14

[Jan de Mooij [:jandem]]

Bug 1980211 added the locale override string to realm options. After that, bug 1984126 added the time zone string.

Comment 15

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1980211

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 9553769 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It's possible but requires privacy.resistFingerprinting (used by Tor)
• 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?: beta, release
• If not all supported branches, which bug introduced the flaw?: Bug 1980211
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Patch should apply or be easy to backport.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely; very small patch.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 17

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1980211

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 9553769 [details]
(secure)

sec-approval+ to land and request uplifts; please hold off on landing tests until late May (I'll set a reminder)

Comment 19

[Daniel Veditz [:dveditz]]

Since ESR is unaffected we might be able to get this into the 149 mid-cycle point release. If so we could land the tests a couple of weeks earlier.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/005857b3bb02
https://hg.mozilla.org/integration/autoland/rev/f5ad88f29e9e
Copy override strings in CopyJSSettings. r=asuth,jonco,dom-worker-reviewers

Comment 21

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Potential crashes or security bugs
• 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: Small low-risk patch to copy locale/timezone override strings when copying JS settings for a child worker.
• String changes made/needed?: N/A
• Is Android affected?: yes

Comment 22

[Jan de Mooij [:jandem]]

Attachment: (secure)

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

Comment 23

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/mozilla-central/rev/f5ad88f29e9e

Comment 24

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/6f23c1d24f8f
https://hg.mozilla.org/releases/mozilla-beta/rev/c97c2a16dc33

Comment 25

[BugBot [:suhaib / :marco]]

2 months ago, dveditz placed a reminder on the bug using the whiteboard tag [reminder-test 2026-05-20] .

jandem, please refer to the original comment to better understand the reason for the reminder.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7af4c228baf1
https://hg.mozilla.org/integration/autoland/rev/70de025abcb6
Add test and comments. r=asuth,jonco,dom-worker-reviewers

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/70de025abcb6