1362596 - Crash reporter is vulnerable to DLL hijacking on Windows

Status: NEW

(Toolkit :: Crash Reporting, defect)

Description

[Molly Howell (she/her) (mostly inactive)]

Creating a DLL file with the same name as any of several Windows DLL's and placing it in the Firefox installation directory causes the crash reporter to load it instead of the real Windows DLL, because of how the default DLL search procedure works.

Which exact DLL's can be used depends on the system (they're all indirect dependencies of Windows DLL's, and what those dependencies are can change), but on my Windows 10 system I've been able to use winmm.dll, wsock32.dll, dbghelp.dll, version.dll, winmmbase.dll, wininet.dll, and msftedit.dll. Additionally imageres.dll is searched for in the install directory, but since it only contains resources I'm not sure it's exploitable.

This isn't hugely serious because the default install location on Windows does not allow non-administrator users to write to it, so an attacker would have to already have admin-level privileges to set up the exploit, and therefore they probably don't need such an exploit. That default is sometimes but not commonly overridden; 5-8% of installations change the default location according to the stub installer's telemetry, and we don't know how many of those are changing it to another protected location.

I'm filing this on behalf of the reporter of bug 1361326, who actually found it and deserves the credit.

Comment 1

[Benjamin Smedberg]

I don't think this is a bug at all. If you can write to the Firefox install directory, you can do much more serious things than this.

We protect against this in the installer because it is typically written to an shared/untrusted location (the Downloads directory or temp directory). And in the updater because it can escalate privileges. I'd like to WONTFIX this, but checking against to make sure there isn't anything I missed.

Comment 2

[Molly Howell (she/her) (mostly inactive)]

I think this is a pretty minor bug, not a high priority, but I don't think it's a WONTFIX. It could be useful to an attacker as part of an exploit chain where they have a way of writing files but need to step up to code execution. This bug allows for taking that step without potentially raising suspicion by altering any signed files.

Comment 3

[Benjamin Smedberg]

ok I'm not the triage owner for this component so I'll let Ted make the final call, but doesn't Firefox itself have the same basic issue?

Comment 4

[Molly Howell (she/her) (mostly inactive)]

firefox.exe mitigates this by delay-loading USER32.DLL (the main culprit) and calling SetDllDirectory early in wmain.

Comment 5

[(not currently active) Ted Mielczarek]

I doubt there's much value in anyone attacking the crashreporter, but we ought to be able to fix this by similarly delay-loading any system DLLs that can be used and calling SetDllDirectory like Firefox does:
https://dxr.mozilla.org/mozilla-central/rev/81977c96c6ff49e4b70f88a55f38d47f5e54a08b/toolkit/xre/nsWindowsWMain.cpp#87

Comment 6

[Jayati Shrivastava]

Hello, May I work on this ?

Comment 7

[Jayati Shrivastava]

Could you point me to the files where we are loading system DLLs?

Comment 8

[Molly Howell (she/her) (mostly inactive)]

.(In reply to Jayati Shrivastava from comment #6)

Hello, May I work on this ?

Of course! This one requires some C++ knowledge and also a Windows build environment, so make sure you have that ready.

(In reply to Jayati Shrivastava from comment #7)

Could you point me to the files where we are loading system DLLs?

The thing that this bug is about is the crash reporter client, which is crashreporter.exe and whose code lives in /toolkit/crashreporter/client.

What it needs to do is delay load the DLL's that it statically depends on the way that firefox.exe does, and then ideally as early in its startup as possible it should call SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32) on systems where that function is available, with a fallback to SetDllDirectoryW(L"") if not.

Comment 9

[Jayati Shrivastava]

Attachment: Bug 1362596 - Prevent DLL hijacking in crashreporter.exe on Windows. r=mhowell

Comment 10

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

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 11

[Alex Franchuk [:afranchuk]]

This may be a bit more complex to apply now that the crash reporter client is written in rust (specifically the delay loading, I have to look into that), though it still ought to be possible.