Moderate firefox Logic Error 🔧 Commit mapped

Overview

Moderate
Severity
—
CVSS
No
Exploited ITW
Fixed
Fix Status
Impactmoderate
DescriptionSandbox escape due to use-after-free in the Preferences: Backend component
ComponentCore
Bug ClassLogic Error
Tracker2058647
Fix commitf35e75640c47 (firefox) +69/-14
CISA KEVNot listed
CreditedMozilla
Disclosed2026-09-29

Changed Functions

FunctionChangeNotes
for
modules/libpref/Preferences.cpp
modified
if
modules/libpref/Preferences.cpp
modified

Files Changed

  • modules/libpref/Preferences.cpp
  • modules/libpref/Preferences.h
  • modules/libpref/test/gtest/Basics.cpp
diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
index 7fe64921944..aeb7e7bc9ce 100644
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -1421,6 +1421,21 @@ static nsCString CopyStrippingTrailingDot(const nsACString& aDomain) {
   return nsCString(aDomain);
 }
 
+// A single trailing dot is normalized away; more than one is rejected since
+// only one would be stripped, leaving an empty trailing segment.
+static bool DomainEndsWithMultipleDots(const nsACString& aDomain) {
+  return StringEndsWith(aDomain, ".."_ns);
+}
+
+static bool DomainEndsWithMultipleDots(const char* const* aDomains) {
+  for (const char* const* p = aDomains; *p; ++p) {
+    if (DomainEndsWithMultipleDots(nsDependentCString(*p))) {
+      return true;
+    }
+  }
+  return false;
+}
+
 // The fire-time payload of a pref-change callback: the function and its
 // closure. Shared base of the trie's refcounted CallbackNode and the mirror
 // list's inline entries (see MirrorCallbackList), so the notify, sweep and
@@ -3484,30 +3499,37 @@ nsPrefBranch::AddObserverImpl(const nsACString& aDomain, nsIObserver* aObserver,
       mozilla::UniquePtr<PrefCallback> existing;
       mObservers.Remove(&weakKey, &existing);
       if (existing) {
-        Preferences::UnregisterCallback(NotifyObserver, prefName,
-                                        existing.get(),
-                                        /* aPrefixMatch */ true);
+        nsresult rv = Preferences::UnregisterCallback(NotifyObserver, prefName,
+                                                      existing.get(),
+                                                      /* aPrefixMatch */ true);
+        if (NS_WARN_IF(NS_FAILED(rv))) {
+          // The trie still points at the weak PrefCallback; keep it alive and
+          // leave the weak registration in place.
+          mObservers.InsertOrUpdate(&weakKey, std::move(existing));
+          return rv;
+        }
       }
     }
   }
 
+  nsresult rv = NS_OK;
   mObservers.WithEntryHandle(pCallback.get(), [&](auto&& p) {
     if (p) {
       NS_WARNING(
           nsPrintfCString("Ignoring duplicate observer: %s", prefName.get())
               .get());
     } else {
-      // We must pass a fully qualified preference name to the callback
-      // aDomain == nullptr is the only possible failure, and we trapped it with
-      // NS_ENSURE_ARG above.
-      Preferences::RegisterCallback(NotifyObserver, prefName, pCallback.get(),
-                                    /* aPrefixMatch */ true);
-
-      p.Insert(std::move(pCallback));
+      // We must pass a fully qualified preference name to the callback.
+      rv = Preferences::RegisterCallback(NotifyObserver, prefName,
+                                         pCallback.get(),
+                                         /* aPrefixMatch */ true);
+      if (NS_SUCCEEDED(rv)) {
+        p.Insert(std::move(pCallback));
+      }
     }
   });
 
-  return NS_OK;
+  return rv;
 }
 
 NS_IMETHODIMP
@@ -3529,8 +3551,9 @@ nsPrefBranch::RemoveObserverImpl(const nsACString& aDomain,
   }
 
   // Remove the relevant PrefCallback from mObservers and get an owning pointer
-  // to it. Unregister the callback first, and then let the owning pointer go
-  // out of scope and destroy the callback.
+  // to it. If unregistering the trie callback fails (e.g. because mObservers
+  // and the trie have drifted out of sync), the CallbackNode in the trie still
+  // holds Data() == this PrefCallback, so put it back.
   const nsCString& prefName = GetPrefName(aDomain);
   PrefCallback key(prefName, aObserver, this);
   mozilla::UniquePtr<PrefCallback> pCallback;
@@ -3551,6 +3574,10 @@ nsPrefBranch::RemoveObserverImpl(const nsACString& aDomain,
     rv = Preferences::UnregisterCallback(NotifyObserver, prefName,
                                          pCallback.get(),
                                          /* aPrefixMatch */ true);
+    if (NS_FAILED(rv)) {
+      PrefCallback* raw = pCallback.get();
+      mObservers.InsertOrUpdate(raw, std::move(pCallback));
+    }
   }
 
   return rv;
@@ -5940,6 +5967,7 @@ nsresult PreferencesImpl::RegisterCallbackImpl(PrefChangedFunc aCallback,
                                                bool aIsPrefix) {
   MOZ_ASSERT(NS_IsMainThread());
   NS_ENSURE_ARG(aCallback);
+  NS_ENSURE_FALSE(DomainEndsWithMultipleDots(aPrefNode), NS_ERROR_INVALID_ARG);
   NS_ENSURE_TRUE(Preferences::InitStaticMembers(), NS_ERROR_NOT_AVAILABLE);
 
   RefPtr<CallbackNode> node =
@@ -5969,6 +5997,7 @@ nsresult PreferencesImpl::UnregisterCallbackImpl(PrefChangedFunc aCallback,
                                                  bool aIsPrefix) {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aCallback);
+  NS_ENSURE_FALSE(DomainEndsWithMultipleDots(aPrefNode), NS_ERROR_INVALID_ARG);
   if (Preferences::sShutdown) {
     MOZ_ASSERT(!Preferences::sPreferences);
     return NS_OK;
diff --git a/modules/libpref/Preferences.h b/modules/libpref/Preferences.h
index 188bea22c42..8e199ba1e9a 100644
--- a/modules/libpref/Preferences.h
+++ b/modules/libpref/Preferences.h
@@ -294,7 +294,9 @@ class Preferences final : public nsIPrefService,
   // boundaries: the callback fires for aPref itself and for any pref that
   // extends it by one or more whole '.'-delimited segments, but not for a pref
   // that merely shares a leading substring (e.g. a "foo.bar" prefix does not
-  // match "foo.barbaz"). A trailing '.' is optional and is normalized away.
+  // match "foo.barbaz"). A trailing '.' is optional and is normalized away;
+  // a domain ending in more than one '.' is rejected with
+  // NS_ERROR_INVALID_ARG.
   template <typename T = void>
   static nsresult RegisterPrefixCallback(PrefChangedFunc aCallback,
                                          const nsACString& aPref,
diff --git a/modules/libpref/test/gtest/Basics.cpp b/modules/libpref/test/gtest/Basics.cpp
index db3341e4857..72abb9ec786 100644
--- a/modules/libpref/test/gtest/Basics.cpp
+++ b/modules/libpref/test/gtest/Basics.cpp
@@ -400,6 +400,30 @@ TEST(PrefsCallbackTrie, TrailingDotEquivalence)
   EXPECT_EQ(count, 1);
 }
 
+// A domain ending in more than one dot is rejected rather than partially
+// normalized.
+TEST(PrefsCallbackTrie, MultipleTrailingDotsRejected)
+{
+  int count = 0;
+  EXPECT_EQ(Preferences::RegisterCallback(IncrementCount,
+                                          "test.trie.dots.a.."_ns, &count),
+            NS_ERROR_INVALID_ARG);
+  EXPECT_EQ(Preferences::RegisterPrefixCallback(
+                IncrementCount, "test.trie.dots.a.."_ns, &count),
+            NS_ERROR_INVALID_ARG);
+  EXPECT_EQ(Preferences::UnregisterCallback(IncrementCount,
+                                            "test.trie.dots.a.."_ns, &count),
+            NS_ERROR_INVALID_ARG);
+
+  nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
+  ASSERT_TRUE(prefs);
+  RefPtr<TestWeakPrefObserver> observer = new TestWeakPrefObserver();
+  EXPECT_EQ(prefs->AddObserver("test.trie.dots.d.."_ns, observer, false),
+            NS_ERROR_INVALID_ARG);
+  Preferences::SetBool("test.trie.dots.d.e", true);
+  EXPECT_EQ(observer->mNotifyCount, 0);
+}
+
 // ---------------------------------------------------------------------------
 // Compact / MarkDead tests — verify lazy-unregister behavior.
 // ---------------------------------------------------------------------------
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/modules/libpref/test/gtest/Basics.cpp b/modules/libpref/test/gtest/Basics.cpp
index db3341e4857..72abb9ec786 100644
--- a/modules/libpref/test/gtest/Basics.cpp
+++ b/modules/libpref/test/gtest/Basics.cpp
@@ -400,6 +400,30 @@ TEST(PrefsCallbackTrie, TrailingDotEquivalence)
   EXPECT_EQ(count, 1);
 }
 
+// A domain ending in more than one dot is rejected rather than partially
+// normalized.
+TEST(PrefsCallbackTrie, MultipleTrailingDotsRejected)
+{
+  int count = 0;
+  EXPECT_EQ(Preferences::RegisterCallback(IncrementCount,
+                                          "test.trie.dots.a.."_ns, &count),
+            NS_ERROR_INVALID_ARG);
+  EXPECT_EQ(Preferences::RegisterPrefixCallback(
+                IncrementCount, "test.trie.dots.a.."_ns, &count),
+            NS_ERROR_INVALID_ARG);
+  EXPECT_EQ(Preferences::UnregisterCallback(IncrementCount,
+                                            "test.trie.dots.a.."_ns, &count),
+            NS_ERROR_INVALID_ARG);
+
+  nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
+  ASSERT_TRUE(prefs);
+  RefPtr<TestWeakPrefObserver> observer = new TestWeakPrefObserver();
+  EXPECT_EQ(prefs->AddObserver("test.trie.dots.d.."_ns, observer, false),
+            NS_ERROR_INVALID_ARG);
+  Preferences::SetBool("test.trie.dots.d.e", true);
+  EXPECT_EQ(observer->mNotifyCount, 0);
+}
+
 // ---------------------------------------------------------------------------
 // Compact / MarkDead tests — verify lazy-unregister behavior.
 // ---------------------------------------------------------------------------
Loading diff…