Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime, syscall, x/sys/windows: unexpected DLL loading behavior #64411

Open
rolandshoemaker opened this issue Nov 27, 2023 · 4 comments
Open
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. OS-Windows Security
Milestone

Comments

@rolandshoemaker
Copy link
Member

In specific scenarios binaries on Windows can exhibit unexpected behavior when retrieving procedure addresses from system DLLs.

In general we take care to only load system DLLs from controlled locations. In particular we attempt to restrict the DLL search path by using LoadLibraryExW using the LOAD_LIBRARY_SEARCH_SYSTEM32 flag which dictates that "%windows%\system32 is searched for the DLL and its dependencies" (according to the LoadLibraryExW doc). It is expected that when we load a procedure address (using GetProcAddress) for a function which is forwarded to another DLL (such as SystemFunction036, which is loaded from advapi32.dll, but defined in cryptbase.dll), the search path for that DLL is restricted in the same way that the search for the initial DLL is restricted.

However, when the initial DLL is already loaded, e.g. because it has been explicitly linked against the binary, and the Windows loader has already loaded it, the search path for a dependent/forwarded DLL (which has not already been loaded) is not restricted as expected, and seems to fallback to a path similar to that dictated by LOAD_LIBRARY_SEARCH_DEFAULT_DIRS. Notably the new search path contains the application directory (the directory in which the executed binary resides, which, it should be noted, may not be the current directory), which is unexpected. As such if the application directory contains a DLL which has the same name as the system DLL that is expected to be loaded, it will be loaded instead.

This is clearly a security issue, since it can result in loading an unexpected DLL, and violates a number of documented properties (especially around syscall.LoadDLL/LazyDLL and the golang.org/x/sys/windows variants). That said, we do not consider it a PRIVATE track security issue per the security policy as the application directory is unlikely to be an attacker controlled directory, and there is no obvious solution to the problem which does not break a number of valid use cases for loading user DLLs.

While working on this issue we considered a number of possible solutions to the problem, in particular calling SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32) during runtime initialization, before loading the syscalls we need for the runtime, and then calling SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_DEFAULT_DIRS) before passing off to the user. This inherently breaks a number of use cases though, as it changes the default DLL search order precedence for all DLLs, in particular removing the current directory from the search path. It also still doesn't protect usages of syscall.LoadDLL/LazyDLL et al by users, outside of the runtime operations. It is still possible we will take this approach in a future release, but more work needs to be done to figure out the correct way to implement it. Another option may be to provide a mode which severely locks down the DLL search path, to only LOAD_LIBRARY_SEARCH_SYSTEM32, but this would need to be an opt-in solution.

Thanks to Nasreddine Bencherchali (@nas_bench) and Max Altgelt (@secDre4mer) of Nextron Systems for reporting this.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 27, 2023
@rolandshoemaker
Copy link
Member Author

Note: the use of SystemFunction036 in the runtime (RtlGenRandom) caused all binaries which explicitly linked advapi32.dll to be vulnerable to this type of attack, specifically against cryptbase.dll. https://go.dev/issue/53192 fixed this particular vector, but not the entire class of attacks, by switch to using ProcessPrng instead.

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 28, 2023
@mknyszek
Copy link
Contributor

mknyszek commented Dec 6, 2023

@rolandshoemaker This landed in the compiler/runtime triage queue. Is there anything we should do here? Thanks.

@mknyszek mknyszek added this to the Backlog milestone Dec 6, 2023
@rolandshoemaker
Copy link
Member Author

I think its still unclear what the best solution is here, and I suspect there is unlikely to be any headway made before 1.22, so I think its probably fine to defer any real thought on it for now.

@rolandshoemaker
Copy link
Member Author

That said, if anyone on the compiler/runtime side has any thoughts about a possible path forward, I'd love to hear them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. OS-Windows Security
Projects
Development

No branches or pull requests

4 participants