CVE-2024-10468

Medium CVSS 5.3 firefox Race 🔧 Commit mapped

Overview

Medium

Severity

5.3

CVSS

Exploited ITW

Fixed

Fix Status

Impactmoderate

DescriptionPotential race conditions in IndexedDB could have caused memory corruption, leading to a potentially exploitable crash.

ComponentDOM

Bug ClassRace

Tracker1914982

Fix commit043fae320084 (firefox) +47/-32

CWECWE-362, CWE-770 (Race condition)

CVSS vectorCVSS:3.1/AV:N/AC:H/PR:N/UI:R/S:U/C:N/I:N/A:H

CISA KEVNot listed

CreditedTyson Smith

Disclosed2024-10-29

Background

`FileInfoManager`: Template base class in dom/indexedDB/FileInfoManager.h that owns the per-database map of file metadata (mFileInfos) and the counter mLastFileId.
`DatabaseFileManager`: IndexedDB subclass of FileInfoManager that tracks on-disk blob/structured-clone files backing a database and hands out DatabaseFileInfo objects.
`StaticMutex`: Process-wide lock (sMutex) shared by all FileInfoManager instances that is meant to serialize access to the file-info bookkeeping.
`MOZ_GUARDED_BY` / `MOZ_REQUIRES`: Clang thread-safety annotations that make the compiler prove a field is only touched, or a method only called, while the named lock is held.

Root Cause Analysis

The IndexedDB file-tracking bookkeeping in FileInfoManager was reachable from both the background thread and I/O threads, yet several code paths mutated the shared state mFileInfos and mLastFileId without holding sMutex. In DatabaseFileManager::Init, the initialization loop called mFileInfos.InsertOrUpdate and updated mLastFileId directly instead of going through the locked helper, and the Contains checks in AsyncDeleteFile and SyncDeleteFile read the hash map with no lock held, so the invariant “every access to mFileInfos/mLastFileId happens under Mutex()” was violated. Concurrent insertion and lookup on the same nsTHashMap can corrupt the container’s internal buckets or hand back a DatabaseFileInfo* whose refcount is being changed on another thread, producing a use-after-free or heap corruption.

The fix routes all mutation through the locked CreateFileInfo helper (extended to accept a preset id and DB refcount), replaces the bare mFileInfos.Contains calls with the new lock-taking ContainsFileInfo, and annotates mFileInfos, mLastFileId, and the accessor methods with MOZ_GUARDED_BY/MOZ_REQUIRES so the compiler statically enforces that the lock is always held. Renaming Mutex() to MutexInstance() with MOZ_RETURN_CAPABILITY lets the annotation machinery recognize sMutex as the guarding capability everywhere it is used.

Key insight

The core mistake was that shared file-info state (mFileInfos, mLastFileId) had an implicit “hold the mutex” contract that unlocked fast paths silently broke; the fix makes that contract explicit and machine-checked with MOZ_GUARDED_BY/MOZ_REQUIRES annotations so no accessor can compile without taking Mutex().

Attack Path

Open a database with existing files A page opens an IndexedDB database whose on-disk directory already contains file entries, driving DatabaseFileManager::Init to populate mFileInfos and bump mLastFileId off-lock.
Race a concurrent file operation While Init is still inserting entries, the attacker triggers a concurrent operation (a lookup via GetFileInfo/AcquireFileInfo or a delete path) that reads or writes the same mFileInfos hash map on another thread.
Corrupt the shared container Simultaneous insert and read on nsTHashMap without sMutex held tears the map’s buckets or returns a DatabaseFileInfo* whose refcount is mid-update, breaking memory-safety invariants.
Trigger memory corruption The mismatched refcount or dangling entry leads to premature free or a stale pointer dereference of a DatabaseFileInfo, yielding a potentially exploitable heap corruption.

Impact Assessment

An attacker who can run script driving IndexedDB can provoke a race that corrupts heap memory in the parent/background process that services IndexedDB, giving a potentially exploitable crash and a foothold for further memory manipulation. Exploitation requires winning a timing window between Init population and a concurrent file-info access, and the payoff is bounded by the difficulty of controlling the corrupted allocation, which is why the rating is moderate.

Changed Functions

Function	Change	Notes
`if` dom/indexedDB/ActorsParent.cpp	modified
`if` dom/indexedDB/FileInfoManager.h	modified
`if` dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp	modified
`for` dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp	modified

Files Changed

dom/indexedDB/ActorsParent.cpp
dom/indexedDB/DatabaseFileManager.h
dom/indexedDB/FileInfoManager.h
dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp

Audit Directions

Unannotated shared containers
Search for nsTHashMap/nsTArray members touched from multiple threads that lack MOZ_GUARDED_BY, since implicit locking contracts are exactly what broke here.
Off-lock fast paths
Look for direct .Contains/.MaybeGet/.InsertOrUpdate calls on state that a sibling method accesses under a StaticMutex, indicating a locking-path bypass.
Static accessor mismatches
Audit classes that expose a Mutex()-style accessor but omit MOZ_RETURN_CAPABILITY/MOZ_REQUIRES, where the compiler cannot verify that every caller actually holds the lock.

Patch

view commit

diff --git a/dom/indexedDB/ActorsParent.cpp b/dom/indexedDB/ActorsParent.cpp
index 9579a76ffbe..a55770bd003 100644
--- a/dom/indexedDB/ActorsParent.cpp
+++ b/dom/indexedDB/ActorsParent.cpp
@@ -6725,7 +6725,7 @@ RefPtr<mozilla::dom::quota::Client> CreateQuotaClient() {
 
 nsresult DatabaseFileManager::AsyncDeleteFile(int64_t aFileId) {
   AssertIsOnBackgroundThread();
-  MOZ_ASSERT(!mFileInfos.Contains(aFileId));
+  MOZ_ASSERT(!ContainsFileInfo(aFileId));
 
   QuotaClient* quotaClient = QuotaClient::GetInstance();
   if (quotaClient) {
@@ -11703,12 +11703,8 @@ nsresult DatabaseFileManager::Init(nsIFile* aDirectory,
         // be 0, but the dbRefCnt is non-zero, which will keep the
         // DatabaseFileInfo object alive.
         MOZ_ASSERT(dbRefCnt > 0);
-        mFileInfos.InsertOrUpdate(
-            id, MakeNotNull<DatabaseFileInfo*>(
-                    FileInfoManagerGuard{}, SafeRefPtrFromThis(), id,
-                    static_cast<nsrefcnt>(dbRefCnt)));
-
-        mLastFileId = std::max(id, mLastFileId);
+        DebugOnly ok = static_cast<bool>(CreateFileInfo(Some(id), dbRefCnt));
+        MOZ_ASSERT(ok);
 
         return Ok{};
       }));
@@ -11971,7 +11967,7 @@ Result<FileUsageType, nsresult> DatabaseFileManager::GetUsage(
 }
 
 nsresult DatabaseFileManager::SyncDeleteFile(const int64_t aId) {
-  MOZ_ASSERT(!mFileInfos.Contains(aId));
+  MOZ_ASSERT(!ContainsFileInfo(aId));
 
   if (!this->AssertValid()) {
     return NS_ERROR_UNEXPECTED;
diff --git a/dom/indexedDB/DatabaseFileManager.h b/dom/indexedDB/DatabaseFileManager.h
index fdfea06ba11..a368d5f4ee2 100644
--- a/dom/indexedDB/DatabaseFileManager.h
+++ b/dom/indexedDB/DatabaseFileManager.h
@@ -131,7 +131,9 @@ class DatabaseFileManager final
 
   MOZ_DECLARE_REFCOUNTED_TYPENAME(DatabaseFileManager)
 
-  static StaticMutex& Mutex() { return sMutex; }
+  static StaticMutex& MutexInstance() MOZ_RETURN_CAPABILITY(sMutex) {
+    return sMutex;
+  }
 
   ~DatabaseFileManager() = default;
 };
diff --git a/dom/indexedDB/FileInfoManager.h b/dom/indexedDB/FileInfoManager.h
index 75842aa53fc..68f8786ca9e 100644
--- a/dom/indexedDB/FileInfoManager.h
+++ b/dom/indexedDB/FileInfoManager.h
@@ -46,27 +46,43 @@ class FileInfoManager : public FileInfoManagerBase {
   using AutoLockType = mozilla::detail::BaseAutoLock<MutexType&>;
 
   [[nodiscard]] SafeRefPtr<FileInfoType> GetFileInfo(int64_t aId) const {
-    return AcquireFileInfo([this, aId] { return mFileInfos.MaybeGet(aId); });
+    return AcquireFileInfo([this, aId]() MOZ_REQUIRES(Mutex()) {
+      return mFileInfos.MaybeGet(aId);
+    });
+  }
+
+  bool ContainsFileInfo(int64_t aFileId) {
+    AutoLockType lock(Mutex());
+    return mFileInfos.Contains(aFileId);
   }
 
-  [[nodiscard]] SafeRefPtr<FileInfoType> CreateFileInfo() {
-    return AcquireFileInfo([this] {
-      const int64_t id = ++mLastFileId;
+  [[nodiscard]] SafeRefPtr<FileInfoType> CreateFileInfo(
+      const Maybe<int64_t>& aMaybeId = Nothing(),
+      const nsrefcnt aDBRefCnt = 0) {
+    return AcquireFileInfo([this, &aMaybeId,
+                            &aDBRefCnt]() MOZ_REQUIRES(Mutex()) {
+      const int64_t id = aMaybeId.isSome() ? *aMaybeId : ++mLastFileId;
 
       auto fileInfo =
           MakeNotNull<FileInfoType*>(FileInfoManagerGuard{},
                                      SafeRefPtr{static_cast<FileManager*>(this),
                                                 AcquireStrongRefFromRawPtr{}},
-                                     id);
+                                     id, aDBRefCnt);
 
       mFileInfos.InsertOrUpdate(id, fileInfo);
+
+      if (aMaybeId.isSome()) {
+        mLastFileId = std::max(id, mLastFileId);
+      }
+
       return Some(fileInfo);
     });
   }
 
-  void RemoveFileInfo(const int64_t aId, const AutoLockType& aFileMutexLock) {
+  void RemoveFileInfo(const int64_t aId, const AutoLockType& aFileMutexLock)
+      MOZ_REQUIRES(Mutex()) {
 #ifdef DEBUG
-    aFileMutexLock.AssertOwns(FileManager::Mutex());
+    aFileMutexLock.AssertOwns(Mutex());
 #endif
     mFileInfos.Remove(aId);
   }
@@ -74,7 +90,7 @@ class FileInfoManager : public FileInfoManagerBase {
   // After calling this method, callers should not call any more methods on this
   // class.
   virtual nsresult Invalidate() {
-    AutoLockType lock(FileManager::Mutex());
+    AutoLockType lock(Mutex());
 
     FileInfoManagerBase::Invalidate();
 
@@ -92,6 +108,8 @@ class FileInfoManager : public FileInfoManagerBase {
     FileInfoManagerGuard() = default;
   };
 
+  static MutexType& Mutex() { return FileManager::MutexInstance(); }
+
  private:
   // Runs the given aFileInfoTableOp operation, which must return a FileInfo*,
   // under the FileManager lock, acquires a strong reference to the returned
@@ -107,7 +125,7 @@ class FileInfoManager : public FileInfoManagerBase {
     // We cannot simply change this to SafeRefPtr<FileInfo>, because
     // FileInfo::AddRef also acquires the FileManager::Mutex.
     auto fileInfo = [&aFileInfoTableOp]() -> RefPtr<FileInfoType> {
-      AutoLockType lock(FileManager::Mutex());
+      AutoLockType lock(Mutex());
 
       const auto maybeFileInfo = aFileInfoTableOp();
       if (maybeFileInfo) {
@@ -122,17 +140,19 @@ class FileInfoManager : public FileInfoManagerBase {
     return SafeRefPtr{std::move(fileInfo)};
   }
 
+  // Access to the following private fields must be protected by
+  // FileManager::Mutex() which is now enforced by MOZ_GUARDED_BY annotations.
+  nsTHashMap<nsUint64HashKey, NotNull<FileInfoType*>> mFileInfos
+      MOZ_GUARDED_BY(Mutex());
+
+  int64_t mLastFileId MOZ_GUARDED_BY(Mutex()) = 0;
+
  protected:
 #ifdef DEBUG
   ~FileInfoManager() { MOZ_ASSERT(mFileInfos.IsEmpty()); }
 #else
   ~FileInfoManager() = default;
 #endif
-
-  // Access to the following fields must be protected by
-  // FileManager::Mutex()
-  int64_t mLastFileId = 0;
-  nsTHashMap<nsUint64HashKey, NotNull<FileInfoType*>> mFileInfos;
 };
 
 }  // namespace mozilla::dom::indexedDB
diff --git a/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp b/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
index 8971ee54fbe..3239241cde4 100644
--- a/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
+++ b/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
@@ -38,7 +38,7 @@ class SimpleFileManager final : public FileInfoManager<SimpleFileManager>,
   // SimpleFileManager functions that are used by SimpleFileInfo
 
   [[nodiscard]] nsresult AsyncDeleteFile(const int64_t aFileId) {
-    MOZ_RELEASE_ASSERT(!mFileInfos.Contains(aFileId));
+    MOZ_RELEASE_ASSERT(!ContainsFileInfo(aFileId));
 
     if (mStats) {
       ++mStats->mAsyncDeleteFileCalls;
@@ -48,7 +48,7 @@ class SimpleFileManager final : public FileInfoManager<SimpleFileManager>,
   }
 
   [[nodiscard]] nsresult SyncDeleteFile(const int64_t aFileId) {
-    MOZ_RELEASE_ASSERT(!mFileInfos.Contains(aFileId));
+    MOZ_RELEASE_ASSERT(!ContainsFileInfo(aFileId));
 
     if (mStats) {
       ++mStats->mSyncDeleteFileCalls;
@@ -64,16 +64,13 @@ class SimpleFileManager final : public FileInfoManager<SimpleFileManager>,
     for (const auto id : kDBOnlyFileInfoIds) {
       // Copied from within DatabaseFileManager::Init.
 
-      mFileInfos.InsertOrUpdate(
-          id, MakeNotNull<SimpleFileInfo*>(FileInfoManagerGuard{},
-                                           SafeRefPtrFromThis(), id,
-                                           static_cast<nsrefcnt>(1)));
-
-      mLastFileId = std::max(id, mLastFileId);
+      MOZ_RELEASE_ASSERT(CreateFileInfo(Some(id), static_cast<nsrefcnt>(1)));
     }
   }
 
-  static MutexType& Mutex() { return sMutex; }
+  static MutexType& MutexInstance() MOZ_RETURN_CAPABILITY(sMutex) {
+    return sMutex;
+  }
 
   static constexpr auto kDBOnlyFileInfoIds =
       std::array<int64_t, 3>{{10, 20, 30}};

Loading diff…

Regression Test / PoC

shipped with the fix

diff --git a/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp b/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
index 8971ee54fbe..3239241cde4 100644
--- a/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
+++ b/dom/indexedDB/test/gtest/TestSimpleFileInfo.cpp
@@ -38,7 +38,7 @@ class SimpleFileManager final : public FileInfoManager<SimpleFileManager>,
   // SimpleFileManager functions that are used by SimpleFileInfo
 
   [[nodiscard]] nsresult AsyncDeleteFile(const int64_t aFileId) {
-    MOZ_RELEASE_ASSERT(!mFileInfos.Contains(aFileId));
+    MOZ_RELEASE_ASSERT(!ContainsFileInfo(aFileId));
 
     if (mStats) {
       ++mStats->mAsyncDeleteFileCalls;
@@ -48,7 +48,7 @@ class SimpleFileManager final : public FileInfoManager<SimpleFileManager>,
   }
 
   [[nodiscard]] nsresult SyncDeleteFile(const int64_t aFileId) {
-    MOZ_RELEASE_ASSERT(!mFileInfos.Contains(aFileId));
+    MOZ_RELEASE_ASSERT(!ContainsFileInfo(aFileId));
 
     if (mStats) {
       ++mStats->mSyncDeleteFileCalls;
@@ -64,16 +64,13 @@ class SimpleFileManager final : public FileInfoManager<SimpleFileManager>,
     for (const auto id : kDBOnlyFileInfoIds) {
       // Copied from within DatabaseFileManager::Init.
 
-      mFileInfos.InsertOrUpdate(
-          id, MakeNotNull<SimpleFileInfo*>(FileInfoManagerGuard{},
-                                           SafeRefPtrFromThis(), id,
-                                           static_cast<nsrefcnt>(1)));
-
-      mLastFileId = std::max(id, mLastFileId);
+      MOZ_RELEASE_ASSERT(CreateFileInfo(Some(id), static_cast<nsrefcnt>(1)));
     }
   }
 
-  static MutexType& Mutex() { return sMutex; }
+  static MutexType& MutexInstance() MOZ_RETURN_CAPABILITY(sMutex) {
+    return sMutex;
+  }
 
   static constexpr auto kDBOnlyFileInfoIds =
       std::array<int64_t, 3>{{10, 20, 30}};

Loading diff…

References

Vendor advisory Fix commit Bug tracker NVD

On This Page