Use modern, secure, Windows API for dlopen

mkitti · November 12, 2022, 9:27pm

Changing dlopen to use LOAD_LIBRARY_SEARCH_DEFAULT_DIRS rather than LOAD_WITH_ALTERED_SEARCH_PATH

Currently, we use the windows “Alternate Search Order” to find DLLs. I propose that we use a new mechanism that is extensible via the Windows API.

Currently we use LOAD_WITH_ALTERED_SEARCH_PATH

On Windows, we currently use LOAD_WITH_ALTERED_SEARCH_PATH with LoadLibraryExW:

github.com

JuliaLang/julia/blob/3510eeb4c99b6daa42a27d0f7d96be8af21067ac/src/dlload.c#L163


      
          }
          #endif
          
          #if defined(_OS_WINDOWS_)
          JL_DLLEXPORT void *jl_dlopen(const char *filename, unsigned flags) JL_NOTSAFEPOINT
          {
              size_t len = MultiByteToWideChar(CP_UTF8, 0, filename, -1, NULL, 0);
              if (!len) return NULL;
              WCHAR *wfilename = (WCHAR*)alloca(len * sizeof(WCHAR));
              if (!MultiByteToWideChar(CP_UTF8, 0, filename, -1, wfilename, len)) return NULL;
              HANDLE lib = LoadLibraryExW(wfilename, NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
              if (lib)
                  needsSymRefreshModuleList = 1;
              return lib;
          }
          #else
          JL_DLLEXPORT JL_NO_SANITIZE void *jl_dlopen(const char *filename, unsigned flags) JL_NOTSAFEPOINT
          {
              /* The sanitizers break RUNPATH use in dlopen for annoying reasons that are
                 are hard to fix. Specifically, libc will use the return address of the
                 caller to determine certain paths and flags that affect .so location lookup.

The Alternate Search Order for Desktop Applications is only extensible by appending the PATH environment variable.

Proposal: Use LOAD_LIBRARY_SEARCH_DEFAULT_DIRS | LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR

After reading through the documentation on LoadLibraryExW when researching an issue GR.jl, I think we should use LOAD_LIBRARY_SEARCH_DEFAULT_DIRS | LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR.

If we use LOAD_LIBRARY_SEARCH_DEFAULT_DIRS then we could use AddDllDirectory to add directories to the search path. LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR also allows us to use the directory where the DLL is located.

The minimum version supported is Windows 7 with KB2533623: Insecure library loading could allow remote code execution.

Background reading: Cpython bpo36085, github 80266

Since Python 3.8, ctypes uses this approach:

https://bugs.python.org/issue36085

github.com/python/cpython

Enable better DLL resolution

opened 12:28AM - 23 Feb 19 UTC

closed 06:18PM - 01 Apr 19 UTC

zooba

type-feature OS-windows 3.8

BPO | [36085](https://bugs.python.org/issue36085) --- | :--- Nosy | @pfmoore, @d…b3l, @ncoghlan, @tjguk, @jkloth, @ambv, @ericsnowcurrently, @zware, @mattip, @eryksun, @zooba, @pablogsal, @carandraug PRs | <li>python/cpython#12302</li><li>python/cpython#12633</li><li>python/cpython#12636</li><li>python/cpython#19167</li> <sup>*Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.*</sup> <details><summary>Show more details</summary><p> GitHub fields: ```python assignee = 'https://github.com/zooba' closed_at = <Date 2019-04-01.18:18:30.116> created_at = <Date 2019-02-23.00:28:57.358> labels = ['type-feature', '3.8', 'OS-windows'] title = 'Enable better DLL resolution' updated_at = <Date 2020-03-28.00:39:54.810> user = 'https://github.com/zooba' ``` bugs.python.org fields: ```python activity = <Date 2020-03-28.00:39:54.810> actor = 'vstinner' assignee = 'steve.dower' closed = True closed_date = <Date 2019-04-01.18:18:30.116> closer = 'steve.dower' components = ['Windows'] creation = <Date 2019-02-23.00:28:57.358> creator = 'steve.dower' dependencies = [] files = [] hgrepos = [] issue_num = 36085 keywords = ['patch'] message_count = 61.0 messages = ['336349', '336350', '336353', '336355', '336371', '336380', '336391', '336398', '336408', '336416', '336665', '337139', '337160', '337175', '337223', '337705', '337722', '337723', '337728', '337749', '337760', '337765', '337790', '337793', '337795', '337796', '337801', '337805', '337807', '337809', '337826', '337838', '337856', '338213', '338515', '338544', '338562', '338908', '338913', '338994', '339001', '339158', '339162', '339163', '339202', '339208', '339213', '339216', '339219', '339220', '339222', '339231', '339233', '339238', '339239', '339240', '339241', '339242', '347421', '347485', '365191'] nosy_count = 14.0 nosy_names = ['paul.moore', 'db3l', 'ncoghlan', 'tim.golden', 'jkloth', 'lukasz.langa', 'eric.snow', 'zach.ware', 'mattip', 'eryksun', 'steve.dower', 'ralf.gommers', 'pablogsal', 'carandraug'] pr_nums = ['12302', '12633', '12636', '19167'] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue36085' versions = ['Python 3.8'] ``` </p></details>

StefanKarpinski · November 12, 2022, 9:48pm

Have you pinged @jameson about this?

mkitti · November 12, 2022, 9:56pm

No, I have not pinged Jameson. I mentioned it in Slack #internals. I thought he might see it from there. But you’ve pinged him now. Hi Jameson!

jameson · November 13, 2022, 4:57am

That cpython thread was an odd read. They repeatedly noted this was a concerning breaking change in behavior, but then just gave up and merged it anyways. They also made no citation for their repeated claim that it is “safer”. It even ended up in the documentation that this is now made “safer” by this breaking change. They also claimed that Windows deprecated the current API, so switching is required (the linked documentation from Microsoft does not mention this), to this “safer” version. But this change is not safer—you cannot make things safer by disabling a capability the user already proved they have. If the OS fails to correctly check permissions on the user, there is little point to worry about whether the application might be exploited to also abuse the hole already required to exploit the application.

A few small takeaways is that Microsoft may have added a bug in Win 7sp1 with relative directory handling (while fixing an actual secure issue), so GetFullPathNameW may be required now

Also, we could disable searching the current directory entirely if we like. We could also attempt to make any AddDllDirectory calls affect all LoadLibrary calls. It is unclear if either of these are desirable.

In Julia, most of this has been effectively deprecated by JLL files though

mkitti · November 13, 2022, 8:30am

Part of the reason I started to investigate this mechanism is because we are currently reliant on setting environment variables to communicate with GR:

github.com/jheinen/GR.jl

Using GR plugins without setting PATH on Windows

opened 12:56AM - 04 Nov 22 UTC

mkitti

Currently, we set `ENV["PATH"]` on Windows since we need to communicate the DLL …search path to GR so that it can dynamically load plugins. https://github.com/jheinen/GR.jl/blob/8b18b19a29e1a8fd0977e8000daa135939680f2e/src/funcptrs.jl#L38 The normal Windows search order is specified in the documentation with `PATH` being sixth in the priority list. https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order#search-order-for-desktop-applications If SafeDllSearchMode is disabled, the search order is as follows: The directory from which the application loaded. 1. The current directory. 2. The system directory. Use the [GetSystemDirectory](https://learn.microsoft.com/en-us/windows/desktop/api/sysinfoapi/nf-sysinfoapi-getsystemdirectorya) function to get the path of this directory. 3. The 16-bit system directory. There is no function that obtains the path of this directory, but it is searched. 4. The Windows directory. Use the [GetWindowsDirectory](https://learn.microsoft.com/en-us/windows/desktop/api/sysinfoapi/nf-sysinfoapi-getwindowsdirectorya) function to get the path of this directory. 5. The directories that are listed in the PATH environment variable. Note that this does not include the per-application path specified 6. by the App Paths registry key. The App Paths key is not used when computing the DLL search path. As @giordano pointed out, this ["looks like a bad idea"](https://github.com/JuliaPackaging/Yggdrasil/pull/5784#issuecomment-1301183099). The GR plugin loading code is located here: https://github.com/sciapp/gr/blob/9aa4f55f75101b76ba8e4406b78601e410642cc6/lib/gks/plugin.c#L46-L55 ```C #ifdef _WIN32 handle = LoadLibrary(pathname); if (handle == NULL) { grdir = gks_getenv("GRDIR"); if (grdir == NULL) grdir = GRDIR; snprintf(grbin, MAXPATHLEN, "%s/bin", grdir); SetDllDirectory(grbin); handle = LoadLibrary(pathname); } ``` Noticeably, this uses `SetDllDirectory` which maps to either [`SetDllDirectoryA`](https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-setdlldirectorya) or [`SetDllDirectoryW`](https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-setdlldirectoryw) based on the value of the environment variable `GRDIR`. Setting `GRDIR` works for a single directory. The following works in VSCode after commenting out line 38 of funcptrs.jl above. ```julia julia> using GR julia> x = 0:2π/360:2π; y = sin.(x); julia> ENV["GRDIR"] = dirname(dirname(GR.GRPreferences.GR_jll.libGR_path)) "~\\.julia\\artifacts\\e40afe22fcfc9fe1caea4af17a4016e7ef36cd66" julia> plot(x,y) # No Error julia> contains(ENV["PATH"], ENV["GRDIR"]) false ``` If we did not set `GRDIR`, this error occurs: `GKS: svgplugin.dll: can't load library, error 126 (0x7e)`. While setting `GRDIR` for a single directory works, there may be many directories to include in the DLL search path: ```julia julia> length(GR.GRPreferences.GR_jll.LIBPATH_list) 35 ``` One strategy then would be for us to call `SetDllDirectoryA` directly. This works in VSCode: ```julia julia> using GR julia> kernel32 = :kernel32 :kernel32 julia> @ccall kernel32.SetDllDirectoryA(dirname(GR.GRPreferences.GR_jll.libGR_path)::Cstring)::UInt8 0x01 julia> x = 0:2π/360:2π; y = sin.(x); julia> plot(x,y) # No error julia> haskey(ENV, "GRDIR") false julia> contains(ENV["PATH"], dirname(dirname(GR.GRPreferences.GR_jll.libGR_path))) false ``` We could of course do this for all directories in `LIBPATH_list`. This also works in VSCode. ```julia julia> using GR julia> kernel32 = :kernel32 :kernel32 julia> all([@ccall kernel32.SetDllDirectoryA(d::Cstring)::Bool for d in GR.GRPreferences.GR_jll.LIBPATH_list]) true julia> x = 0:2π/360:2π; y = sin.(x); julia> plot(x,y) ``` cc:@t-bltg

GR uses its own plugin system and loads dynamic libraries itself:

github.com

sciapp/gr/blob/e10c07240198239d5491ff45047ae1b40812ea0f/lib/gks/plugin.c#L48-L54


      
          handle = LoadLibrary(pathname);
          if (handle == NULL)
            {
              GetEnvironmentVariableW(L"GRDIR", grdir, MAX_PATH);
              StringCbPrintfW(w_pathname, MAX_PATH, L"%ws\\bin\\%S.%S", grdir, name, EXTENSION);
              handle = LoadLibraryExW(w_pathname, NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
            }

Here we communicate the location of the GR binaries via the GRDIR.

My thought is that perhaps we could use this new API to communicate multiple directories to GR.

In trying out this API, I found something unpleasant. If someone calls SetDefaultDllDirectories with
LOAD_LIBRARY_SEARCH_DEFAULT_DIRS, then it causes LoadLibraryExW to fail with LOAD_WITH_ALTERED_SEARCH_PATH.

julia> begin
           wchar = append!(transcode(UInt16, raw"libdSFMT.dll"), 0x0000)
           LOAD_LIBRARY_SEARCH_DEFAULT_DIRS = 0x1000
           LOAD_WITH_ALTERED_SEARCH_PATH = 0x8
           status = @ccall "kernel32".SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_DEFAULT_DIRS::UInt32)::Bool # GR.jl v0.70.1
           handle = @ccall "kernel32".LoadLibraryExW(wchar::Ptr{UInt16}, C_NULL::Ptr{Nothing}, LOAD_WITH_ALTERED_SEARCH_PATH::UInt32)::Ptr{Nothing} # Julia
           if handle == C_NULL
               println(Libc.FormatMessage())
           end
           handle
       end
The parameter is incorrect. 
Ptr{Nothing} @0x0000000000000000

Everything loads fine if we use LOAD_LIBRARY_SEARCH_DEFAULT_DIRS.

julia> begin
           wchar = append!(transcode(UInt16, raw"libdSFMT.dll"), 0x0000)
           LOAD_LIBRARY_SEARCH_DEFAULT_DIRS = 0x1000
           LOAD_WITH_ALTERED_SEARCH_PATH = 0x8
           status = @ccall "kernel32".SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_DEFAULT_DIRS::UInt32)::Bool
           handle = @ccall "kernel32".LoadLibraryExW(wchar::Ptr{UInt16}, C_NULL::Ptr{Nothing}, LOAD_LIBRARY_SEARCH_DEFAULT_DIRS::UInt32)::Ptr{Nothing}        
           if handle == C_NULL
               println(Libc.FormatMessage())
           end
           handle
       end
Ptr{Nothing} @0x0000000000e20000

The main lesson from this is that we should not use SetDefaultDllDirectories since this causes Julia’s current use of LoadLibraryExW to fail when used with LOAD_WITH_ALTERED_SEARCH_PATH. The other thought is that using LOAD_WITH_ALTERED_SEARCH_PATH is fragile. Loading some plugin that invokes SetDefaultDllDirectories will cause Julia to be unable to dynamically load other dynamic libraries.

To make dynamic library loading more robust and and to take advantage of AddDllDirectory perhaps we could try to use LOAD_LIBRARY_SEARCH_DEFAULT_DIRS with LoadLibraryExW if loading with LOAD_WITH_ALTERED_SEARCH_PATH fails. That might look as follows.

    HANDLE lib = LoadLibraryExW(wfilename, NULL, LOAD_WITH_ALTERED_SEARCH_PATH);
    if(lib == NULL)
    {
        lib = LoadLibraryExW(wfilename, NULL, LOAD_LIBRARY_SEARCH_DEFAULT_DIRS);
    }

Essentially we try to load using LOAD_WITH_ALTERED_SEARCH_PATH. If that fails, then we try LOAD_LIBRARY_SEARCH_DEFAULT_DIRS.

LOAD_LIBRARY_SEARCH_DEFAULT_DIRS is a combination of the following:

LOAD_LIBRARY_SEARCH_APPLICATION_DIR
LOAD_LIBRARY_SEARCH_USER_DIRS (e.g. directories set with AddDllDirectory)
LOAD_LIBRARY_SEARCH_SYSTEM32

Alternatively, the contingency loading could be restricted to LOAD_LIBRARY_SEARCH_USER_DIRS.

mkitti · December 2, 2022, 4:48am

I created a reduced pull request here:

github.com/JuliaLang/julia

Add LOAD_LIBRARY_SEARCH_USER_DIRS for DLL loading

JuliaLang:master ← mkitti:mk/load_library_search_user_dirs

opened 04:09AM - 02 Dec 22 UTC

mkitti

+12 -0

Currently, we use `LOAD_WITH_ALTERED_SEARCH_PATH` to load DLL libraries on Windo…ws. With this pull request, if loading with the above fails, we use `LOAD_LIBRARY_SEARCH_USER_DIRS`. This is a minimalist version of the idea proposed on [Discourse](https://discourse.julialang.org/t/use-modern-secure-windows-api-for-dlopen/90171/5). Rather than replacing `LOAD_WITH_ALTERED_SEARCH_PATH` with `LOAD_LIBRARY_SEARCH_DEFAULT_DIRS`, we only use `LOAD_LIBRARY_SEARCH_USER_DIRS` if loading with ``LOAD_WITH_ALTERED_SEARCH_PATH` fails. This allows the user to manually build a search list of directories via `AddDllDirectory`. ```julia @ccall "kernel32".AddDllDirectory(library::Cwstring)::Ptr{Nothing} ``` https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibraryexw

The pull request makes use of LOAD_LIBRARY_SEARCH_USER_DIRS if loading with LOAD_WITH_ALTERED_SEARCH_PATH fails.