1706041 - Crash in [@ mozilla::glue::LoaderObserver::OnEndDllLoad]

Status: RESOLVED FIXED

(Firefox :: Launcher Process, defect)

Description

[Gabriele Svelto [:gsvelto] (PTO)]

Crash report: https://crash-stats.mozilla.org/report/index/72c44512-c27f-44a5-85e6-94d5a0210417

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 mozglue.dll mozilla::glue::LoaderObserver::OnEndDllLoad mozglue/dllservices/LoaderObserver.cpp:88
1 firefox.exe mozilla::freestanding::LoaderPrivateAPIImp::NotifyEndDllLoad browser/app/winlauncher/freestanding/LoaderPrivateAPI.cpp:196
2 firefox.exe mozilla::freestanding::ModuleLoadFrame::~ModuleLoadFrame browser/app/winlauncher/freestanding/ModuleLoadFrame.cpp:40
3 firefox.exe mozilla::freestanding::patched_LdrLoadDll browser/app/winlauncher/freestanding/DllBlocklist.cpp:359
4 kernelbase.dll LoadLibraryExW 
5 user32.dll _ClientLoadLibrary 
6 ntdll.dll KiUserCallbackDispatch 
7 win32u.dll NtUserMessageCall 
8 user32.dll int64_t RealDefWindowProcWorker 
9 user32.dll DefWindowProcW 

Tentatively filing this in the launcher component because the launcher seems involved though I don't know exactly how. I'm trying to get a better stack trace using Visual Studio.

This is a use-after-free crash, rax contains the poison value in every crash I looked at.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

I opened a random minidump with Visual Studio but couldn't get a better trace. The DLL that was being loaded is C:\WINDOWS\system32\apphelp.dll. It would be interesting to know if it's the same in other crashes or not.

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

There are different DLLs in other crashes from what I can tell.

Comment 3

[Toshihito Kikuchi [:toshi]]

It seems that gDllServices was already freed. And in my understanding, we freeing it only on shutdown, which is registered here. The fix would be to make sure DisableFull() is called when destroying DllServices.

Comment 4

[Toshihito Kikuchi [:toshi]]

I understood the scenario. Every crash was running ShowProfileManager. In that case, gDllServices was freed here, at the end of the scope for ScopedXPCOMStartup. After that line, gDllServices becomes a dangling pointer because destroying DllServices does not update gDllServices. If any module is loaded in that situation, this crash happens.

I'm not sure this is exploitable because I think there is no chance to load external data when we show the profile manager.

Comment 5

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1706041 - Reset gDllServices when DllServices is destroyed. r=aklotz

Comment 6

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

Reset gDllServices when DllServices is destroyed. r=aklotz
https://hg.mozilla.org/integration/autoland/rev/68af0a79c75edd1ebe42ff4bb7b04dda66817766
https://hg.mozilla.org/mozilla-central/rev/68af0a79c75e

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR78 approval when you get a chance.

Comment 8

[Toshihito Kikuchi [:toshi]]

Comment on attachment 9218657 [details]
Bug 1706041 - Reset gDllServices when DllServices is destroyed. r=aklotz

Beta/Release Uplift Approval Request

• User impact if declined: Firefox hits shutdown crash due to UAF if it launched in the Profiler Manager mode and a module was loaded during shutdown.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The crashing scenario is clear, and the fix is one liner and easily understood.
• String changes made/needed: None

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is still a UAF even though this scenario is not likely to be exploitable. We should fix the bad code to avoid introducing a new exploitable scenario in the future.
• User impact if declined: Firefox hits shutdown crash due to UAF if it launched in the Profiler Manager mode and a module was loaded during shutdown.
• Fix Landed on Version: 90
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The crashing scenario is clear, and the fix is one liner and easily understood.
• String or UUID changes made by this patch: None

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9218657 [details]
Bug 1706041 - Reset gDllServices when DllServices is destroyed. r=aklotz

Approved for 89.0b12 and 78.11esr.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/71040f2fadb1

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/b527fc161019