1712084 - Crash in [@ RtlAcquireSRWLockExclusive | RtlDeregisterWaitEx | UnregisterWait]

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/8e45781f-677d-4eb2-8bdb-809f30210519

Reason: EXCEPTION_ACCESS_VIOLATION_WRITE

Top 9 frames of crashing thread:

0 ntdll.dll RtlAcquireSRWLockExclusive 
1 ntdll.dll RtlDeregisterWaitEx 
2 kernel32.dll UnregisterWait 
3 xul.dll static `anonymous namespace'::nsAvailableMemoryWatcher::LowMemoryCallback xpcom/base/AvailableMemoryTracker.cpp:153
4 ntdll.dll RtlpTpWaitCallback 
5 ntdll.dll TppExecuteWaitCallback 
6 ntdll.dll TppWorkerThread 
7 kernel32.dll BaseThreadInitThunk 
8 ntdll.dll RtlUserThreadStart 

This is a regression that was accidentally introduced by bug 1586236. If I'm reading the crash report correctly (and in particular the one with PHC traces) what's happening is the following:

1. Application shutdown starts, this makes the available memory tracker call its Shutdown() method
2. We remove the observers we have registered. This is what keeps alive this object, so when the call to the Observe() method is over it will be deallocated.
3. Right after doing that we call UnregisterWait() on the handle we received when we started waiting on the memory resource handle.
4. The methods over and the object gets deallocated.

Now, at point 3 I thought that UnregisterWait() would prevent the LowMemoryCallback() from being invoked again but I think I was wrong. Apparently the callback might have been scheduled for execution already, and it will thus run after the object has been destroyed, invoking UnregisterWait() again on dead memory. This isn't really an UAF because by the time we get there we read a chunk of memory that has been long reused for something else.

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1712084 - Keep alive the available memory tracker during shutdown r=tkikuchi

Comment 2

[Toshihito Kikuchi [:toshi]]

This looks like a classic race between two threads:

On the main thread:
a1. nsObserverList::NotifyObservers
a2. nsAvailableMemoryWatcher::Shutdown
a3. UnregisterWait
a4. nsAvailableMemoryWatcher::Release --> destroying the object

On a wait thread in the thread pool:
b1. nsAvailableMemoryWatcher::LowMemoryCallback
b2. nsAvailableMemoryWatcher::OnLowMemory
b3. UnregisterWait --> hitting a read AV when accessing a member inside the object mWaitHandle

The scenario should be that a low-memory callback (b1) was invoked before unregistering the wait object (a3) and the main thread destroyed the memory watcher before finishing the callback.

The problem is the callback thread does not hold an object. The fix would be to use RefPtr in nsAvailableMemoryWatcher::LowMemoryCallback. What do you think?

This isn't really an UAF because by the time we get there we read a chunk of memory that has been long reused for something else.

I think this is a UAF scenario though I think it's really hard for an attacker to control an object that will be placed at the address of nsAvailableMemoryWatcher::mWaitHandle which is causing this read AV and even if they do, it's hard to weaponize it.
Marking this as a sec assuming the worst.

Comment 3

[Daniel Veditz [:dveditz]]

This isn't really an UAF because by the time we get there we read a chunk of memory that has been long reused for something else.

That is the actual exploit scenario for a UAF. The poison value helps us detect UAF situations in crashes so we can fix them, but to be used in an exploit the attacker needs to get that memory reused and filled with values they can influence. When we see the poison value we know it's a UAF 100%, but only maybe exploitable. An attacker would need to be able to get that chunk reallocated with useful values before we would otherwise hit the poison value and crash. Sometimes they definitely can exploit it--especially if there's a callback into content JS where they can "groom" memory between free and re-use--and sometimes they definitely can't exploit it if the window between the two is small and there's no chance of a race reallocating the memory. But most of the time it falls in the middle where it's easier to assume it's exploitable and fix it than to try to prove it is NOT exploitable. Even if we're wrong we at least made Firefox more stable.

The longer the window between the free and the re-use the more likely an attacker would have the opportunity to trigger a reallocation of useful values. When we see actual reallocation, as in this case, we usually have to rate those odds higher. Shutdown is a little bit special in that we're pretty sure we won't be running web content anymore. At my most paranoid I can't rule out the possibility that a clever hacker booby-trapped some object such that our shutdown code will do something bad with it, but we haven't actually seen that and we do usually rate those lower severity

Overwriting freed memory with the poison value also prevents some UAF exploits by erasing left-over values that might have been useful, but that is tangential to what we should do with the ones we have left.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 5

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

Keep alive the available memory tracker during shutdown r=tkikuchi
https://hg.mozilla.org/integration/autoland/rev/83aee1be773f376920e1ee76875c0b3581131d10
https://hg.mozilla.org/mozilla-central/rev/83aee1be773f