From 7712ad6e0f741eca84a9d46ecc8f77b93aabf808 Mon Sep 17 00:00:00 2001 From: Austin Wise Date: Sat, 11 Dec 2021 16:57:26 -0800 Subject: [PATCH 1/2] Fix vunerablity to DLL Search Order Hijacking by delay loading version.dll Hypothetically if the user's current directory contains a malicious DLL named version.dll, this DLL will be loaded instead of the one in System32. I checked the list of DLLs referenced by rufus.exe against the KnownDLLs list visible in Sysinternal's winobj.exe. On both Windows 7 and Windows 11, version.dll was the only DLL not on the KnownDLLs list and thus vunerable to this attack. To confirm that this work, I used dumpbin.exe /IMPORTS to make sure version.dll is delay loaded. I then put a breakpoint in the delay load hook and confirmed that the hook is used. This can be triggered by loading a Windows installation ISO file. --- .vs/rufus.vcxproj | 8 ++++++++ src/rufus.c | 35 ++++++++++++++++++++++------------- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/.vs/rufus.vcxproj b/.vs/rufus.vcxproj index 60a14a0f023..1f3eb5b6bb4 100644 --- a/.vs/rufus.vcxproj +++ b/.vs/rufus.vcxproj @@ -138,6 +138,7 @@ true Windows MachineX86 + version.dll;%(DelayLoadDLLs) _UNICODE;UNICODE;RUFUS_LOC;%(PreprocessorDefinitions) @@ -162,6 +163,7 @@ true Windows C:\Program Files (x86)\Windows Kits\10\Lib\10.0.15063.0\um\arm + version.dll;%(DelayLoadDLLs) _UNICODE;UNICODE;RUFUS_LOC;%(PreprocessorDefinitions) @@ -188,6 +190,7 @@ true Windows C:\Program Files (x86)\Windows Kits\10\Lib\10.0.16299.0\um\arm64 + version.dll;%(DelayLoadDLLs) _UNICODE;UNICODE;RUFUS_LOC;%(PreprocessorDefinitions) @@ -219,6 +222,7 @@ true Windows MachineX64 + version.dll;%(DelayLoadDLLs) _UNICODE;UNICODE;RUFUS_LOC;%(PreprocessorDefinitions) @@ -246,6 +250,7 @@ Windows MachineX86 /BREPRO %(AdditionalOptions) + version.dll;%(DelayLoadDLLs) _UNICODE;UNICODE;RUFUS_LOC;%(PreprocessorDefinitions) @@ -273,6 +278,7 @@ Windows C:\Program Files (x86)\Windows Kits\10\Lib\10.0.15063.0\um\arm /BREPRO %(AdditionalOptions) + version.dll;%(DelayLoadDLLs) _UNICODE;UNICODE;RUFUS_LOC;%(PreprocessorDefinitions) @@ -302,6 +308,7 @@ Windows C:\Program Files (x86)\Windows Kits\10\Lib\10.0.16299.0\um\arm64 /BREPRO %(AdditionalOptions) + version.dll;%(DelayLoadDLLs) _UNICODE;UNICODE;RUFUS_LOC;%(PreprocessorDefinitions) @@ -334,6 +341,7 @@ Windows MachineX64 /BREPRO %(AdditionalOptions) + version.dll;%(DelayLoadDLLs) _UNICODE;UNICODE;RUFUS_LOC;%(PreprocessorDefinitions) diff --git a/src/rufus.c b/src/rufus.c index e3aac859db2..fb1fc5e34ae 100755 --- a/src/rufus.c +++ b/src/rufus.c @@ -36,6 +36,7 @@ #include #include #include +#include #include "rufus.h" #include "missing.h" @@ -3193,6 +3194,21 @@ static HANDLE SetHogger(void) return hogmutex; } +// For delay-loaded DLLs, +// use LOAD_LIBRARY_SEARCH_SYSTEM32 to avoid DLL search order hijacking. +FARPROC WINAPI dllDelayLoadHook(unsigned dliNotify, PDelayLoadInfo pdli) +{ + if (dliNotify == dliNotePreLoadLibrary) + { + // Windows 7 without KB2533623 does not support the LOAD_LIBRARY_SEARCH_SYSTEM32 flag. + // That is is OK, because the delay load handler will interrupt the NULL return value + // to mean that it should perform a normal LoadLibrary. + return (FARPROC)LoadLibraryExA(pdli->szDll, NULL, LOAD_LIBRARY_SEARCH_SYSTEM32); + } + + return NULL; +} +const PfnDliHook __pfnDliNotifyHook2 = dllDelayLoadHook; /* * Application Entrypoint @@ -3204,7 +3220,6 @@ int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine #endif { const char* rufus_loc = "rufus.loc"; - wchar_t kernel32_path[MAX_PATH]; int i, opt, option_index = 0, argc = 0, si = 0, lcid = GetUserDefaultUILanguage(); int wait_for_mutex = 0; FILE* fd; @@ -3240,22 +3255,16 @@ int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine // Still, we invoke it, for platforms where the following call might actually work... SetDllDirectoryA(""); - // Also, even if you use SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32), you're - // still going to be brought down if you link to wininet.lib or dwmapi.lib, as these two - // perform their DLL invocations before you've had a chance to execute anything. - // Of course, this is not something that security "researchers" will bother looking into - // to try to help fellow developers, when they can get an ego fix by simply throwing - // generic URLs around and deliberately refusing to practice *responsible disclosure*... + // For libraries on the KnownDLLs list, the system will always load them from System32. + // For other DLLs we link directly to, like version.dll, we delay load the DLL and use + // a delay load hook to load them from System32. + // For all other DLLs, use SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32). // Finally, we need to perform the whole gymkhana below, where we can't call on // SetDefaultDllDirectories() directly, because Windows 7 doesn't have the API exposed. - GetSystemDirectoryW(kernel32_path, ARRAYSIZE(kernel32_path)); - wcsncat(kernel32_path, L"\\kernel32.dll", ARRAYSIZE(kernel32_path) - wcslen(kernel32_path) - 1); - // NB: Because kernel32 should already be loaded, what we do above to ensure that we - // (re)pick the system one is mostly unnecessary. But since for a hammer everything is a - // nail... Also, no, Coverity, we never need to care about freeing kernel32 as a library. + // Also, no, Coverity, we never need to care about freeing kernel32 as a library. // coverity[leaked_storage] pfSetDefaultDllDirectories = (SetDefaultDllDirectories_t) - GetProcAddress(LoadLibraryW(kernel32_path), "SetDefaultDllDirectories"); + GetProcAddress(LoadLibraryW(L"kernel32.dll"), "SetDefaultDllDirectories"); if (pfSetDefaultDllDirectories != NULL) pfSetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32); From 71b8d01f4f8cbb3b64363ea7ae5b9580fb7028c4 Mon Sep 17 00:00:00 2001 From: Austin Wise Date: Sun, 12 Dec 2021 22:24:32 -0800 Subject: [PATCH 2/2] Fix compile error on MinGW. I'm not sure this is actually the correct thing to do. Why is there an extra const? --- src/rufus.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/rufus.c b/src/rufus.c index fb1fc5e34ae..a9d76c2b90b 100755 --- a/src/rufus.c +++ b/src/rufus.c @@ -3208,7 +3208,12 @@ FARPROC WINAPI dllDelayLoadHook(unsigned dliNotify, PDelayLoadInfo pdli) return NULL; } -const PfnDliHook __pfnDliNotifyHook2 = dllDelayLoadHook; + +#ifdef _MSC_VER +// For some reason the Windows SDK headers have a `const` while MinGW does not. +const +#endif +PfnDliHook __pfnDliNotifyHook2 = dllDelayLoadHook; /* * Application Entrypoint