Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
syscall: guard against Windows DLL preloading attacks #14959
Comments
|
/cc @taruti, too. |
|
Proposed fix: https://golang.org/cl/21140 |
|
I'm not convinced that Go needs to solve this problem because dso hijack is Besides, this is not backward compatible as we silently changed the semantics I'm fine if we only change the runtime to load ntdll.dll, kernel32.dll, etc. with |
|
Your analogy isn't complete. Windows users download binaries and double-click those binaries from their Downloads folder. Windows users can get (malicious) DLLs silently downloaded to their Downloads folder just by browsing the web. Windows users don't need to set LD_PRELOAD to shoot themselves in the foot. The gun is always pointed down towards the foot by default on Windows. |
|
CL https://golang.org/cl/21140 mentions this issue. |
|
FYI, the order of paths for searching dll(s) https://msdn.microsoft.com/en-us/library/7d83bc18.aspx
|
I read the article, and I don't see what the problem is. I went to the page pointed by the article which is suppose to automatically download malicious DLL, but my Chrome actually refused to download the DLL. Chrome told me that DLL looks suspicious and I shouldn't be downloading it. As to CL 21140, I think it breaks existing valid programs. Somehow CL assumes that syscall.LoadDLL is onle used to load "system" DLLs. But that is not true. It is quite common to put application specific DLLs into application directory - directory where executable is located. How do you propose we handle this scenario if CL 21140 is accepted? If you think there is something for us to fix here, perhaps we could create new syscall.LoadSystemDLL (or similar) that does what you want, and change all standard packages to use syscall.LoadSystemDLL. Alex |
Chrome was updated according to the comments. The post also mentions the Edge browser (the default Microsoft browser) and comments suggest it hasn't been updated. I haven't tried.
https://golang.org/doc/go1compat says "A security issue in the specification or implementation may come to light whose resolution requires breaking compatibility. We reserve the right to address such security issues." I believe calling
The proposal is to add that not to the standard library, but to x/sys. (See the original comment at the top of this bug). |
Current directory yes, but that is not what I was talking about. Some apps store DLLs in the directory where EXE is located. How do you propose people load these DLLs? Alex |
See the top comment in this bug: "2) Add a LoadLibraryEx to x/sys/win so that users can still get at the old behavior if they want it (by appropriate passing of flags)." (A flags of 0 would mean the current behavior) |
|
Here is one of many programs I use on my PC. Just to demonstrate that behaviour you're about to break is actually used by some products.
But that will still break my existing code. I don't personally have problem like described in https://textplain.wordpress.com/2015/12/18/dll-hijacking-just-wont-die/. Why should I suffer? Why cannot we do what I suggested above? What is the downside? Alex |
|
I agree with alex, loading DLL from program installation directory
is pretty common.
If we really want to anything here, we can make the runtime and
syscall package only use LOAD_LIBRARY_SEARCH_SYSTEM32
to load known system DLLs (ntdll, kernel32, user32, advapi etc.)
We should not change the behavior of exported syscall functions.
I don't think this security problem is enough to invoke the security
bug clause in the Go 1 guarantee.
|
|
I propose we add the new @minux, @alexbrainman, I consider both of you advanced users who would use |
I don't think this will work for what your broke. Since you changed syscall.LoadDLL (in CL 21140), you would have to provide similar alternative. In syscall package too. Otherwise people would have to change too much code if they need to import x/sys/windows as well as syscall, and things can get messy. I don't use x/sys/windows myself, it is difficult for me to see what can go wrong here. Alex |
|
I checked loading dll from go. #include <windows.h>
BOOL WINAPI DllMain(HINSTANCE hinstDll, DWORD dwReason, LPVOID lpReserved) {
switch (dwReason) {
case DLL_PROCESS_ATTACH:
MessageBox(0, "Evil", "Wow!", MB_OK);
break;
case DLL_THREAD_ATTACH:
break;
case DLL_THREAD_DETACH:
break;
case DLL_PROCESS_DETACH:
break;
}
return TRUE;
}build kernel32.dll loader written in go package main
import (
"os"
"syscall"
)
func main() {
kernel32 := syscall.NewLazyDLL(os.Args[1])
procGetCurrentThreadId := kernel32.NewProc("GetCurrentThreadId")
procGetCurrentThreadId.Call()
}then, pass argument pointed path to kernel32.dll like below
problems seem to not occur unless the program point to kernel32.dll relatively or point to kernel32.dll directly. |
|
@mattn, That is because Windows registry has a list of dlls that will always be loaded from a system directory first. That covers some, but not all of the dlls that are used by Go stdlib. I see at least three possible solutions: |
I think your solution A must include some plan for Go users on how to fix their breakage. I am worried they will just start copying runtime code. And some will make mistakes along the way. And maybe even copying code will not work for some reason or other. As to my preference, I like your B approach (we can do better than syscall.LoadDllSafe name). I can be arm twisted into doing C :-), but B is not much harder, and it will make all standard library code safe for this current issue. And it won't break any user code. We can mention new syscall.LoadDllSafe function in release notes, and let our users decide if it is important for their projects. syscall.LoadDllSafe could be a simple replacement for syscall.LoadDLL, so it should be a trivial change for everyone who decides to follow our advice. Alex |
|
I've updated https://go-review.googlesource.com/21140 to hopefully be non-controversial. It now makes sure that all DLLs loaded by Go itself are safe, but does nothing to protect users using |
|
That seems like a good way forward. Would exposing that to third party code via the sys-repo be fine? I can submit that, if it is ok. |
|
To write a proper test for https://golang.org/cl/21140, I would like a DLL which, when loaded, does something we can check easily in a test. Maybe it can write to stdout, or crash the process with a certain error code. @mattn, could you provide such a minimal DLL & source code? From #14959 (comment) it looks like you know how to make them easily. I only have gomote+trybots for Windows testing. I don't have a Windows development machine at the moment. I found some information online about how to generate Windows DLLs from Linux which I can try otherwise. One DLL is probably fine, but maybe a 32-bit one and a 64-bit one would be better? |
|
@taruti, adding x/sys/windows API for all the possibilities is not controversial. It's only new API in the standard library that we want to avoid. If you'd like to do the x/sys change(s) already, feel free. I'd be happy to review. |
I think we already have TestStdcallAndCDeclCallbacks that does what you want. I wonder why the test didn't fail with your CL 21140 changes are applied. Alex |
|
Oh, nice. Maybe the builders don't have gcc? func TestStdcallAndCDeclCallbacks(t *testing.T) {
if _, err := exec.LookPath("gcc"); err != nil {
t.Skip("skipping test: gcc is missing")
}Will investigate. |
They surely do. misc/cgo tests wouldn't run without gcc. Alex |
|
@alexbrainman, TestStdcallAndCDeclCallbacks didn't fail because it's using an absolute path to the DLL, which is unaffected by https://golang.org/cl/21140: I'll write a new similar test using a base filename ("test_bad.dll") and verify it doesn't search for it in the current directory once "test_bad.dll" is registered as requiring system32. |
|
x/sys counterpart for discussion: https://go-review.googlesource.com/21388 |
|
CL https://golang.org/cl/21388 mentions this issue. |
|
@bradfitz Go should not paranoid when loading DLLs. Search Path behavior should remaining same https://msdn.microsoft.com/en-us/library/7d83bc18.aspx its not Go's fault when loaded dll is fake, it is user fault executing unknown program in their environment. I am Windows user, I consider this as a feature we can try and load newer dlls without the need to replace original system32 dlls. so udating software is much easier.
Win User know it, and they already have their own guards. |
|
@blinksmith, we're only altering the DLL search path for DLLs used by Go. x/sys/windows is getting a new |
Make sure that for any DLL that Go uses itself, we only look for the
DLL in the Windows System32 directory, guarding against DLL preloading
attacks.
(Unless the Windows version is ancient and LoadLibraryEx is
unavailable, in which case the user probably has bigger security
problems anyway.)
This does not change the behavior of syscall.LoadLibrary or NewLazyDLL
if the DLL name is something unused by Go itself.
This change also intentionally does not add any new API surface. Instead,
x/sys is updated with a LoadLibraryEx function and LazyDLL.Flags in:
https://golang.org/cl/21388
Updates #14959
Change-Id: I8d29200559cc19edf8dcf41dbdd39a389cd6aeb9
Reviewed-on: https://go-review.googlesource.com/21140
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Updates golang/go#14959 Change-Id: Ib91c359c3df919df0b30e584d38e56f79f3e3dc9 Reviewed-on: https://go-review.googlesource.com/21388 Reviewed-by: Russ Cox <rsc@golang.org> Reviewed-by: Alex Brainman <alex.brainman@gmail.com> Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
|
CL https://golang.org/cl/21428 mentions this issue. |
Updates the syscall generator for patchset 4 of https://golang.org/cl/21388. Updates #14959 Change-Id: Icbd6df489887d3dcc076dfc73d4feb1376abaf8b Reviewed-on: https://go-review.googlesource.com/21428 Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
|
What's left to fix this? |
|
Please wait bradfitz. Now he's trying to buy Windows XP at Best Buy maybe. |
|
Nothing's left, other than to ship Go1.6.1. @alexbrainman confirmed all.bat passes on his Windows XP machine, and the new test fails (skips) as expected on XP. |
|
CL https://golang.org/cl/21639 mentions this issue. |
|
CL https://golang.org/cl/21680 mentions this issue. |
|
CL https://golang.org/cl/21696 mentions this issue. |
|
CL https://golang.org/cl/21697 mentions this issue. |
Make sure that for any DLL that Go uses itself, we only look for the
DLL in the Windows System32 directory, guarding against DLL preloading
attacks.
(Unless the Windows version is ancient and LoadLibraryEx is
unavailable, in which case the user probably has bigger security
problems anyway.)
This does not change the behavior of syscall.LoadLibrary or NewLazyDLL
if the DLL name is something unused by Go itself.
This change also intentionally does not add any new API surface. Instead,
x/sys is updated with a LoadLibraryEx function and LazyDLL.Flags in:
https://golang.org/cl/21388
Updates #14959
Change-Id: I8d29200559cc19edf8dcf41dbdd39a389cd6aeb9
Reviewed-on: https://go-review.googlesource.com/21140
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/21639
Run-TryBot: Andrew Gerrand <adg@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Updates the syscall generator for patchset 4 of https://golang.org/cl/21388. Updates #14959 Change-Id: Icbd6df489887d3dcc076dfc73d4feb1376abaf8b Reviewed-on: https://go-review.googlesource.com/21428 Reviewed-by: Alex Brainman <alex.brainman@gmail.com> Reviewed-on: https://go-review.googlesource.com/21680 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Make sure that for any DLL that Go uses itself, we only look for the
DLL in the Windows System32 directory, guarding against DLL preloading
attacks.
(Unless the Windows version is ancient and LoadLibraryEx is
unavailable, in which case the user probably has bigger security
problems anyway.)
This does not change the behavior of syscall.LoadLibrary or NewLazyDLL
if the DLL name is something unused by Go itself.
This change also intentionally does not add any new API surface. Instead,
x/sys is updated with a LoadLibraryEx function and LazyDLL.Flags in:
https://golang.org/cl/21388
Updates #14959
Change-Id: I8d29200559cc19edf8dcf41dbdd39a389cd6aeb9
Reviewed-on: https://go-review.googlesource.com/21140
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/21696
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Updates the syscall generator for patchset 4 of https://golang.org/cl/21388. Updates #14959 Change-Id: Icbd6df489887d3dcc076dfc73d4feb1376abaf8b Reviewed-on: https://go-review.googlesource.com/21428 Reviewed-by: Alex Brainman <alex.brainman@gmail.com> Reviewed-on: https://go-review.googlesource.com/21697 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
|
CL https://golang.org/cl/23025 mentions this issue. |
See issue golang/go#14959 for details. Change-Id: I7c06b23b5cb98e4330fb85c025cad2cbd3db37ae Reviewed-on: https://go-review.googlesource.com/23025 Reviewed-by: Nigel Tao <nigeltao@golang.org>
While many other call sites have been moved to using the proper higher-level system loading, these areas were left out. This prevents DLL directory injection attacks. This includes both the runtime load calls (using LoadLibrary prior) and the implicitly linked ones via cgo_import_dynamic, which we move to our LoadLibraryEx. The goal is to only loosely load kernel32.dll and strictly load all others. Meanwhile we make sure that we never fallback to insecure loading on older or unpatched systems. This is CVE-2019-9634. Fixes #14959 Fixes #28978 Fixes #30642 Change-Id: I401a13ed8db248ab1bb5039bf2d31915cac72b93 Reviewed-on: https://go-review.googlesource.com/c/go/+/165798 Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Taru Karttunen noted that Go should be more paranoid by default when loading DLLs.
Background:
https://textplain.wordpress.com/2015/12/18/dll-hijacking-just-wont-die/
Microsoft's guidelines:
https://msdn.microsoft.com/en-us/library/ff919712%28VS.85%29.aspx
LoadLibraryEx docs:
https://msdn.microsoft.com/en-us/library/ms684179(v=vs.85).aspx
@rsc proposed:
CL forthcoming.
/cc @alexbrainman @adg @broady @jbuberel @ianlancetaylor