Use LoadLibraryExW in LoadNativeLibrary on Windows |
||||
Issue descriptionChrome Version: All OS: Windows Today in LoadNativeLibrary, we use LoadLibraryW [1] to load a DLL. But to be able to find dependencies in the same folder, we SetCurrentDirectory() to the DLL directory, and sets it back after the DLL is loaded [2]. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory [3]. However, there are several issues with this approach: 1. SetCurrentDirectory() is not recommended in a multithreaded application: "Multithreaded applications and shared library code should not use the SetCurrentDirectory function ..." due to potential race conditions of SetCurrentDirectory and GetCurrentDirectory [4]. 2. Even with SetCurrentDirectory(), the "current directory" is searched after a bunch of other directories [3]. This could pose a security risk: "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." [5]. 3. It seems the SetCurrentDirectory() approach could fail. I am not sure about this yet, but it's likely given the evidences in issue 700208. To fix this, we could use LoadLibraryExW [6], where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR (the directory that contains the DLL is temporarily added to the beginning of the list of directories that are searched for the DLL's dependencies). With this, we can eliminate the need of doing SetCurrentDirectory(). LoadLibraryExW requires Windows XP or higher, which is not a problem. LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR requires Windows 7 or higher, which is also fine. But on some systems it requires a security update which I am not sure how we could check: "Windows 7, Windows Server 2008 R2, Windows Vista and Windows Server 2008: This value requires KB2533623 to be installed." [6] [1] https://cs.chromium.org/chromium/src/base/native_library_win.cc?rcl=6fd7349089bc194ac6a08af6f94298987629dd9b&l=61 [2] https://cs.chromium.org/chromium/src/base/native_library_win.cc?rcl=6fd7349089bc194ac6a08af6f94298987629dd9b&l=27 [3] https://msdn.microsoft.com/en-us/library/windows/desktop/ms682586(v=vs.85).aspx#standard_search_order_for_desktop_applications [4] https://msdn.microsoft.com/en-us/library/windows/desktop/aa365530(v=vs.85).aspx [5] https://msdn.microsoft.com/en-us/library/windows/desktop/ff919712(v=vs.85).aspx [6] https://msdn.microsoft.com/en-us/library/windows/desktop/ms684179(v=vs.85).aspx
,
Mar 10 2017
I agree that changing the current directory is not recommended. LoadLibraryExW seems like the correct fix. Changing the current directory is worrisome, but not necessarily incorrect - it depends on what other code in Chrome is doing. Do you know whether it is actually causing problems? Evidence of misbehavior or actual security problems (as opposed to security risk) would raise the priority. Either way I would guess that this is worth fixing, but I'd have to find somebody to investigate and to create and test a patch. Do you have any interest in creating such a patch, or would you just like to see this fixed?
,
Mar 10 2017
Please check issue 700208 to see whether it's a reasonable "evidence of misbehavior". I don't have time to do it this quarter but I do like to see it fixed, which should help fix issue 700208 I think. So if you can find anyone to do it it'll be great.
,
Mar 11 2017
,
Mar 16 2017
,
Mar 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5946c9233dc393f2e3e275323e66659fe5fb35cd commit 5946c9233dc393f2e3e275323e66659fe5fb35cd Author: chengx <chengx@chromium.org> Date: Thu Mar 16 05:49:21 2017 Use LoadLibraryExW if applicable in LoadNativeLibrary on Windows In the current LoadNativeLibrary implementation, LoadLibraryW Windows API is used to load a DLL. To be able to find dependencies in the same folder, SetCurrentDirectory() is needed to search for the DLL directory, and sets it back after the DLL is loaded. This is required because on Windows, it'll search for dependencies in a search list, which includes the system "current directory", but not the DLL directory. However, SetCurrentDirectory() can be potentially problematic. It is not recommended in a multithreaded application, and could pose a security risk as "If an attacker gains control of one of the directories on the DLL search path, it can place a malicious copy of the DLL in that directory. This is sometimes called a DLL preloading attack or a binary planting attack." The right thing to do is to use LoadLibraryExW, where we can specify additional flags like LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR to enable searching in the DLL directory. With this, we can eliminate the need of doing SetCurrentDirectory(). Using these additional flags requires KB2533623 to be installed and the method is "To determine whether the flags are available, use GetProcAddress to get the address of the AddDllDirectory, RemoveDllDirectory, or SetDefaultDllDirectories function. If GetProcAddress succeeds, the LOAD_LIBRARY_SEARCH_* flags can be used with LoadLibraryEx." Therefore, we can dynamically call LoadLibraryExW if the API and the flags are available. If not or its call fails, we should use the LoadLibraryW API. This CL also adds UMA histogram to record the calling status of both LoadLibraryExW and LoadLibraryW APIs. Besides, this CL removes the LoadNativeLibraryDynamically method as it is not used anywhere. Running Chromium built with this CL locally shows that LoadLibraryExW call were successful for kernel32.dll and widevinecdm.dll (which caused crbug.com/700208), but failed when loading MDMRegistration.dll. LoadLibraryW succeeds in loading MDMRegistration.dll though. BUG= 700503 ,700208 Review-Url: https://codereview.chromium.org/2744043003 Cr-Commit-Position: refs/heads/master@{#457359} [modify] https://crrev.com/5946c9233dc393f2e3e275323e66659fe5fb35cd/base/native_library.h [modify] https://crrev.com/5946c9233dc393f2e3e275323e66659fe5fb35cd/base/native_library_win.cc [modify] https://crrev.com/5946c9233dc393f2e3e275323e66659fe5fb35cd/tools/metrics/histograms/histograms.xml
,
Mar 21 2017
Verified in UMA and the CDM DLL load failures are gone. |
||||
►
Sign in to add a comment |
||||
Comment 1 by xhw...@chromium.org
, Mar 10 2017