CVE-2024-10467
Overview
Background
- `CacheFileIOManager`
- The networking subsystem’s singleton that owns cache file state and whose members like
mHandles,mSpecialHandles, andmMetadataWritesTimerare only safe to touch on the dedicated cache I/O thread. - `SizeOfHandlesRunnable`
- A
Runnablesync-dispatched to the cache I/O thread so that memory-reporting code can measure I/O-thread-owned objects from the correct thread. - `mMetadataWritesTimer`
- An
nsCOMPtr<nsITimer>member ofCacheFileIOManagerthat schedules deferred metadata writes and, like other I/O-thread state, must not be accessed concurrently from another thread. - `SizeOfExcludingThisInternal`
- The memory-reporter entry point that tallies the manager’s retained heap size and previously read some members directly on the calling (non-I/O) thread.
Root Cause Analysis
The memory-reporting path in CacheFileIOManager::SizeOfExcludingThisInternal correctly dispatched a SizeOfHandlesRunnable to the I/O thread to measure mHandles and mSpecialHandles, but it then queried mMetadataWritesTimer via do_QueryInterface and SizeOfIncludingThis directly on the memory-reporter’s own thread. Because mMetadataWritesTimer is I/O-thread-owned state, this violated the invariant that the timer is only read or mutated on the I/O thread, creating a data race: the timer could be concurrently reset, released, or reassigned by the I/O thread while the reporter was dereferencing it, leading to a use-after-free or torn read.
The fix removes the off-thread access from SizeOfExcludingThisInternal and instead passes mMetadataWritesTimer into the SizeOfHandlesRunnable, which measures it inside Run on the I/O thread where every other timer access already happens. This works because all reads of the timer now occur under the same single-threaded discipline, eliminating the concurrent access window.
mMetadataWritesTimer) from the memory-reporter thread instead of inside the already-dispatched I/O-thread Runnable; the fix relocates that measurement into SizeOfHandlesRunnable::Run so the timer is only ever touched on its owning thread.Attack Path
- Trigger a memory report
An attacker (or automated telemetry/about:memory path they can influence) causes
CacheFileIOManager::SizeOfExcludingThisInternalto run on a non-I/O thread. - Race the timer
Concurrent cache activity on the I/O thread reschedules or releases
mMetadataWritesTimer, reassigning or freeing the underlyingnsITimerobject. - Off-thread dereference
The reporter’s
do_QueryInterface(mMetadataWritesTimer)andSizeOfIncludingThiscall reads the timer pointer and dereferences it while it is being mutated or destroyed on the I/O thread. - Corrupt memory access The torn read or use-after-free dereferences a stale or partially-updated pointer, producing memory corruption whose contents an attacker may shape through controlled cache load.
Impact Assessment
nsITimer object inside the parent-process networking subsystem. Successful exploitation could corrupt heap memory and, per Mozilla’s assessment, potentially be leveraged toward arbitrary code execution, though it requires precise timing between the memory-reporter thread and the cache I/O thread. The precondition is that the timer is accessed off the I/O thread while the I/O thread is free to mutate it.Changed Functions
| Function | Change | Notes |
|---|---|---|
mSizenetwerk/cache2/CacheFileIOManager.cpp |
modified | |
ifnetwerk/cache2/CacheFileIOManager.cpp |
modified |
Files Changed
netwerk/cache2/CacheFileIOManager.cpp
Audit Directions
- Off-thread member reads in memory reportersReview other
SizeOfExcludingThis/SizeOfIncludingThisimplementations for members documented as single-thread-owned that are measured directly on the reporter thread instead of via a dispatched runnable. - Thread-affinity annotationsSearch for members guarded by
MOZ_UNANNOTATEDor comments like “must be accessed only on the I/O thread” and confirm every access site, including size and diagnostic code, honors that affinity. - Timer and nsCOMPtr lifetime racesAudit
nsITimerand othernsCOMPtrmembers that can be reset or released on one thread for any read,do_QueryInterface, or dereference from a different thread.
Patch
diff --git a/netwerk/cache2/CacheFileIOManager.cpp b/netwerk/cache2/CacheFileIOManager.cpp
index 85cd017d8b9..b4f986929e3 100644
--- a/netwerk/cache2/CacheFileIOManager.cpp
+++ b/netwerk/cache2/CacheFileIOManager.cpp
@@ -4410,13 +4410,15 @@ class SizeOfHandlesRunnable : public Runnable {
public:
SizeOfHandlesRunnable(mozilla::MallocSizeOf mallocSizeOf,
CacheFileHandles const& handles,
- nsTArray<CacheFileHandle*> const& specialHandles)
+ nsTArray<CacheFileHandle*> const& specialHandles,
+ nsCOMPtr<nsITimer> const& metadataWritesTimer)
: Runnable("net::SizeOfHandlesRunnable"),
mMonitor("SizeOfHandlesRunnable.mMonitor"),
mMonitorNotified(false),
mMallocSizeOf(mallocSizeOf),
mHandles(handles),
mSpecialHandles(specialHandles),
+ mMetadataWritesTimer(metadataWritesTimer),
mSize(0) {}
size_t Get(CacheIOThread* thread) {
@@ -4448,6 +4450,8 @@ class SizeOfHandlesRunnable : public Runnable {
for (uint32_t i = 0; i < mSpecialHandles.Length(); ++i) {
mSize += mSpecialHandles[i]->SizeOfIncludingThis(mMallocSizeOf);
}
+ nsCOMPtr<nsISizeOf> sizeOf = do_QueryInterface(mMetadataWritesTimer);
+ if (sizeOf) mSize += sizeOf->SizeOfIncludingThis(mallocSizeOf);
mMonitorNotified = true;
mon.Notify();
@@ -4455,11 +4459,12 @@ class SizeOfHandlesRunnable : public Runnable {
}
private:
- mozilla::Monitor mMonitor MOZ_UNANNOTATED;
+ mozilla::Monitor mMonitor;
bool mMonitorNotified;
mozilla::MallocSizeOf mMallocSizeOf;
CacheFileHandles const& mHandles;
nsTArray<CacheFileHandle*> const& mSpecialHandles;
+ nsCOMPtr<nsITimer> const& mMetadataWritesTimer;
size_t mSize;
};
@@ -4473,10 +4478,11 @@ size_t CacheFileIOManager::SizeOfExcludingThisInternal(
if (mIOThread) {
n += mIOThread->SizeOfIncludingThis(mallocSizeOf);
- // mHandles and mSpecialHandles must be accessed only on the I/O thread,
- // must sync dispatch.
+ // mHandles, mSpecialHandles and mMetadataWritesTimer must be accessed
+ // only on the I/O thread, must sync dispatch.
RefPtr<SizeOfHandlesRunnable> sizeOfHandlesRunnable =
- new SizeOfHandlesRunnable(mallocSizeOf, mHandles, mSpecialHandles);
+ new SizeOfHandlesRunnable(mallocSizeOf, mHandles, mSpecialHandles,
+ mMetadataWritesTimer);
n += sizeOfHandlesRunnable->Get(mIOThread);
}
@@ -4485,9 +4491,6 @@ size_t CacheFileIOManager::SizeOfExcludingThisInternal(
sizeOf = do_QueryInterface(mCacheDirectory);
if (sizeOf) n += sizeOf->SizeOfIncludingThis(mallocSizeOf);
- sizeOf = do_QueryInterface(mMetadataWritesTimer);
- if (sizeOf) n += sizeOf->SizeOfIncludingThis(mallocSizeOf);
-
sizeOf = do_QueryInterface(mTrashTimer);
if (sizeOf) n += sizeOf->SizeOfIncludingThis(mallocSizeOf);