1732421 - Delay loading should use LOAD_WITH_ALTERED_SEARCH_PATH

Status: ASSIGNED

(Core :: Security: Process Sandboxing, defect, P2)

Description

[Masatoshi Kimura [:emk]]

xul.dll is loaded with LOAD_WITH_ALTERED_SEARCH_PATH flag:
https://searchfox.org/mozilla-central/rev/a276135f5459fce19fa710337244d4dcdc4289ee/xpcom/glue/standalone/nsXPCOMGlue.cpp#50

Implicitly-loaded DLLs will transitively use LOAD_WITH_ALTERED_SEARCH_PATH. But delay-loaded DLLs will not use this flag by default. It will make DLL planting attack a bit easier (especially when PreferSystem32Images mitigation is unavailable).

__delayLoadHelper2 can customize delay-loading behavior.

Comment 1

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

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

Comment 2

[Marco Castelluccio [:marco]]

This is a regression due to a win32k-lockdown change, but will affect users even if win32k-lockdown is not enabled.

Comment 3

[Bob Owen (:bobowen)]

Thanks for finding and reporting this.
As a slight aside, I think we might want to start using SetDefaultDllDirectories on the main process as well.

That might not be available on some win7 (it needs KB2533623), so we should look at adding this back in as well.
Hopefully we can do that through our existing hooking, without the need to use a custom delay loader.

I'll try and get to this soon, so we can uplift to 94 in beta.

Comment 4

[Marco Castelluccio [:marco]]

Bob, any news?

Comment 5

[Bob Owen (:bobowen)]

(In reply to Marco Castelluccio [:marco] from comment #2)

This is a regression due to a win32k-lockdown change, but will affect users even if win32k-lockdown is not enabled.

Yeah, I'll look into a patch now.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Too late for Fx94 at this point.

Comment 7

[Yasmin S]

bob is this something we plan on addressing what are the implications if this is not addressed, should this be a P1 ?

Comment 8

[Bob Owen (:bobowen)]

(In reply to Yasmin S from comment #7)

bob is this something we plan on addressing what are the implications if this is not addressed, should this be a P1 ?

Yes we want to address this, but I haven't had time to look into the solution in detail.
I guess this might not be a P1, an attacker would already need to be able to write DLLs to sensitive directories I believe.

dveditz - do you have a view on the priority/severity of this?

Comment 9

[Daniel Veditz [:dveditz]]

The only difference made by LOAD_WITH_ALTERED_SEARCH_PATH is that if the loaded .dll loads more .dlls the first searched directory is the directly-loaded .dll's directory rather than the application's directory, which in our case is the Firefox install directory. Not too worried about that if someone can already write into that location, although if, because of this difference, a library's dependency isn't found right next to it, it's not going to be found in the Firefox install directory either and Windows will keep searching down the path and might in some cases get into directories that an unprivileged local attacker could write into.

I'm not too worried it's an immediate threat to our users in practice, but this is likely to result in bug bounty claims down the road which will be annoying and expensive (because in certain very specific situations, mutually hostile users who cannot modify Firefox directly might be able to hack each other if any of the shared path directories are writable.