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: Windows DLL preloading attack possible for winmm.dll #28978

Closed
sj26 opened this issue Nov 28, 2018 · 15 comments
Closed

runtime: Windows DLL preloading attack possible for winmm.dll #28978

sj26 opened this issue Nov 28, 2018 · 15 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows Security
Milestone

Comments

@sj26
Copy link

sj26 commented Nov 28, 2018

Go 1.11 seems vulnerable to dll preloading on windows with winmm.dll. It looks like #14959 mostly fixed this, kernel32.dll etc are protected, but winmm.dll still seems to be affected. It seems to be loaded implicitly by the go runtime —

#pragma dynimport runtime·timeBeginPeriod timeBeginPeriod "winmm.dll"
— but I notice is not listed with the other safely loaded DLLs — 
modkernel32 = NewLazyDLL("kernel32.dll")
modadvapi32 = NewLazyDLL("advapi32.dll")
modshell32 = NewLazyDLL("shell32.dll")
modmswsock = NewLazyDLL("mswsock.dll")
modcrypt32 = NewLazyDLL("crypt32.dll")
modws2_32 = NewLazyDLL("ws2_32.dll")
moddnsapi = NewLazyDLL("dnsapi.dll")
modiphlpapi = NewLazyDLL("iphlpapi.dll")
modsecur32 = NewLazyDLL("secur32.dll")
modnetapi32 = NewLazyDLL("netapi32.dll")
moduserenv = NewLazyDLL("userenv.dll")

What version of Go are you using (go version)?

$ docker run --rm golang:1.11 go version
go version go1.11.2 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ docker run --rm golang:1.11 go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build782466443=/tmp/go-build -gno-record-gcc-switches"

What did you do?

  1. Create a main.go with:

    package main
    
    import "fmt"
    
    func main() {
    	fmt.Println("Hello world")
    }
    
  2. Cross-compile for windows:

    docker run -v $PWD:/go -e GOARCH=amd64 -e GOOS=windows --rm golang:1.11 go build -o test.exe main.go
    
  3. Copy test.exe to a windows vm

  4. Add a winmm.dll beside test.exe with contents not a dll

  5. Double click test.exe

What did you expect to see?

"Hello world"

What did you see instead?

screen shot 2018-11-28 at 11 48 54 am

@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 28, 2018
@agnivade agnivade added this to the Go1.13 milestone Nov 28, 2018
@odeke-em
Copy link
Member

odeke-em commented Nov 28, 2018

Thank you for filing this bug @sj26 !

I'll page some folks who can help both with Windows and security /cc @alexbrainman @bradfitz @jordanrh1 @FiloSottile @aclements @ianlancetaylor

In the future @sj26, please report security bugs to security@golang.org as recommended at https://golang.org/security

@alexbrainman
Copy link
Member

It looks like #14959 mostly fixed this, kernel32.dll etc are protected, but winmm.dll still seems to be affected. It seems to be loaded implicitly by the go runtime —

kernel32.dll also loaded by runtime - see many lines ending with "kernel32.dll" in os_windows.go file. Going by your logic, kernel32.dll must be as broken as winmm.dll. I suspect, you can do your trick with kernel32.dll, if you make your program more complicated.

I tried your trick with notepad.exe and corrupted kernel32.dll, and notepad.exe does not start. And I do not see much difference between notepad.exe and Go built executable. They both have import table in the executable that specifies which DLL and functions to use. And Windows program loader searches for DLLs as per standard rules. How else do you propose Windows executable access OS?

Leaving this to real security experts.

Alex

@sj26
Copy link
Author

sj26 commented Nov 28, 2018

I confess I don't use Windows and don't understand its dynamic library loading in detail. I'm reporting an issue we were alerted to which seemed odd. It sounds like a bug that some libraries would be protected and some would not. I didn't consider it a high severity security issue, it just seemed like a library search order problem.

kernel32.dll does not seem affected, nor any other dll I could find referenced, only winmm.dll.

The only difference I could find is that unaffected dlls are referenced in that generated syscall source file, and then again referenced in runtime, but winmm is only referenced in runtime. Perhaps this changes when and how they are loaded?

I notice that back in #14959 the effort was to switch these well-known libraries to use LoadLibraryEx with a flag to make sure they were loaded only from the system directory, not the regular search path. I don't understand the codebase deeply enough, but I couldn't find evidence of this any more, I can only find LoadLibrary[A|W]. So I'm not sure how only winmm.dll seems affected.

Perhaps this is because of Windows' underlying "known dlls" functionality?

I'll leave the sleuthing to folks who understand the functionality. Maybe the answer is that this is not a problem, it's just how Windows functions?

@diagprov
Copy link

Hi guys, I've been a Windows dev for some time and I do stuff in security. I'm not affiliated with the project, I just stumbled upon this bug looking for another issue.

So, the default DLL search path starts with the application directory itself by default. This is because Windows considers application directories to be bundles, much like OSX .app folders: https://blogs.msdn.microsoft.com/oldnewthing/20110620-00/?p=10393 and https://blogs.msdn.microsoft.com/oldnewthing/20161013-00/?p=94505

KnownDLLs was a Longhorn (Vista) optimization, which loads the listed DLLs into memory at boot and then every time an application asks for one the DLL is mapped in automatically, without ever searching the disk for the file and running the PE loader again. You can probably see why this is a performance optimization, avoiding these disk hits for every process. This has some security implications, as it means windows ignores the usual search paths, so you won't be able to inject kernel32.dll even if you try really hard. It is not, however, a security boundary or defensive measure (it just has useful consequences).

There's also another method that is rarely discussed, WinSxS. Most PE binaries (especially those that implement GUIs) have an application manifest xml file embedded as a resource. dot net developers will be more familiar with this, I suspect. This XML resource can specify alternative versions of DLLs (assemblies) to be loaded from the WinSxS directory. In fact, kernel32.dll is just hardlinked into system32. This is an aside, but I want to mention it for completeness.

So, now we come to how DLLs actually get loaded. There are two ways: statically, from the import directory table. In this case, the load order is the default supplied by Windows and you can't change it. The other case is dynamically, your LoadLibrary* or dlopen. This you can alter the order at runtime.

The pragma dynimport above is creating a static, embedded in the PE IDT reference to winmm.dll; here's the output from dumpbin (windows objdump):

V:\go\src\test\testapp>dumpbin /imports testapp.exe
Microsoft (R) COFF/PE Dumper Version 14.16.27023.1
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file testapp.exe

File Type: EXECUTABLE IMAGE

  Section contains the following imports:

    winmm.dll
                548000 Import Address Table
                5DF2F6 Import Name Table
                     0 time date stamp
                     0 Index of first forwarder reference

                           0 timeEndPeriod
                           0 timeBeginPeriod

    ws2_32.dll
                548018 Import Address Table
                5DF30E Import Name Table
                     0 time date stamp
                     0 Index of first forwarder reference

                           0 WSAGetOverlappedResult

    kernel32.dll
                548028 Import Address Table
                5DF31E Import Name Table
                     0 time date stamp
                     0 Index of first forwarder reference

                           0 WriteFile
                           0 WriteConsoleW
                           0 WaitForSingleObject
<snip>

As a consequence, there's nothing the go runtime code can do to alter the search path, because no go runtime code can possibly be called before that DLL is resolved and loaded by Windows. The only alternative is to load the library dynamically.

A security issue arises with DLL loading when an application is executed either with higher privileges, or with different privileges, and the application directory is writable. In this case, where such static DLLs are loaded and they're not KnownDLLs, DLL Injection is trivial and you have a local privesg bug (or lateral movement). As I said above, Windows considers directories to be application bundles and for them to be safe locations to load libraries from (installers in the NSIS case are a bit of a special case, because they're not run from trusted locations). The privileges of such a directory should be set appropriately by code installing the binary in question. I suggest using WIX and generating an MSI; Windows Installer will then do this for you.

There are other ways a developer may shoot themselves in the foot in addition to this DLL. I am assuming any cgo C code that did some kind of pragma import would have that honoured and the DLL added to the IDT as well (I have not looked into this). Same problem occurs. Go plugins also produce loadable DLL components out of go code and developers have plenty of opportunity to do something inadvisable with those.

I don't really have strong opinions as to what to do about this. It really depends on if you want to support writing application installers in golang. However, as things stand this is totally expected behaviour and would apply to any non-KnownDLL that is referenced in the import table.

@bradfitz
Copy link
Contributor

@diagprov, thanks for the information!

@sj26
Copy link
Author

sj26 commented Nov 29, 2018

@diagprov incredibly interesting, thank you!

I think a common use case for golang is writing utility programs which are often downloaded directly via a browser to a Downloads directory and then directly executed in that directory. Which creates an opportunity to persuade a user to download a maliciously crafted and appropriately named dll as well in the same way, which would then be in the same directory. But I'm not sure there's anything practical that golang can do here? It sounds like using the import table for kernel32.dll is pretty safe, and that exposes LoadLibrary etc which can be limited to system load paths which could then be used to dynamically load the rest of the required libraries, but this sounds pretty convoluted, and not always desirable.

@diagprov
Copy link

diagprov commented Nov 29, 2018

So just quickly, on linux systems, the "API" linux applications can expect to remain consistent and mostly working is directly at the syscall level. The syscall numbers are stable and if your application calls them directly and statically links (like go does), fine. This is not true of Windows, since NT supports multiple subsystems - the syscall interface is an internal API, as is NTDLL.DLL. The only subsystem of this kind (WSL works differently) still used is "win32"; public API for that is kernel32.dll, user32.dll, advapi32.dll, etc etc. You more or less have to link against kernel32.dll and possibly others from KnownDLLs. This is OK, it's how Windows works.

I don't think there's anything wrong with LoadLibrary/handle check/GetProcAddress/null check/use function as a way to load winmm. It's a bit annoying perhaps, but the NewLazyDLL code in Go I believe already uses this pattern (you can call LoadLibrary on a DLL loaded through the IDT no problem, you'll get a handle like normal, just to head off that question). You can control the search path this way and allow the static imports by the runtime to be only "standard" DLLs.

One possible question is why Golang is raising timer resolution frequency in the first place. I would guess this is to make the GC as performant as possible, but this has global impact, so, for the life of any golang process the timer resolution goes from the default 15.6ms to 1ms for the whole machine. This would be of particular concern from windows services ("daemons") written in golang. Chrome have discussed it: https://bugs.chromium.org/p/chromium/issues/detail?id=153139 and MSDN also discusses the performance considerations, to quote:

This function affects a global Windows setting. Windows uses the lowest value (that is, highest resolution) requested by any process. Setting a higher resolution can improve the accuracy of time-out intervals in wait functions. However, it can also reduce overall system performance, because the thread scheduler switches tasks more often. High resolutions can also prevent the CPU power management system from entering power-saving modes.

@ianlancetaylor
Copy link
Contributor

For background on why the Go runtime fiddles with the timer resolution frequency see #8687 and #20937. It's not because of the garbage collector.

@diagprov
Copy link

diagprov commented Dec 5, 2018

So it seems the calls were removed, but it slows down go's scheduling code in some cases, so the removal was reverted. #7876 seems to be an open issue for removing these calls. More complicated than I thought and unfortunately I don't know go nearly well enough to help on that issue.

@sj26
Copy link
Author

sj26 commented Dec 5, 2018

It sounds like we're stuck with timeBeginPeriod for now, although there might be a better solution in the works.

In the meantime, could this library be imported dynamically with the system-only flags right before the timeBeginPeriod call? That would solve this particular issue with hopefully minimal performance impact?

@sj26
Copy link
Author

sj26 commented Dec 6, 2018

Wow, I was looking at a very old version of the code. Most of the code I posted is very out of date.

I had a go at making winmm load dynamically and safely using LoadLibraryEx with LOAD_LIBRARY_SEARCH_SYSTEM32. It works!

master...sj26:load-winmm-dynamically

This approach could be applied to the other LoadLibraryA calls here, too.

@alexbrainman
Copy link
Member

In the meantime, could this library be imported dynamically with the system-only flags right before the timeBeginPeriod call? That would solve this particular issue with hopefully minimal performance impact?

Personally I don't feel there is a problem here.

Alex

@gopherbot
Copy link

Change https://golang.org/cl/165798 mentions this issue: runtime: safely load DLLs

@gopherbot
Copy link

Change https://golang.org/cl/168339 mentions this issue: [release-branch.go1.12] runtime: safely load DLLs

gopherbot pushed a commit that referenced this issue Mar 24, 2019
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 #30666
Updates #14959
Updates #28978
Updates #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>
(cherry picked from commit 9b6e9f0)
Reviewed-on: https://go-review.googlesource.com/c/go/+/168339
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/175378 mentions this issue: [release-branch.go1.11] runtime: safely load DLLs

gopherbot pushed a commit that referenced this issue May 6, 2019
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 #30989
Updates #14959
Updates #28978
Updates #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>
(cherry picked from commit 9b6e9f0)
Reviewed-on: https://go-review.googlesource.com/c/go/+/175378
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Andrew Bonventre <andybons@golang.org>
@golang golang locked and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows Security
Projects
None yet
Development

No branches or pull requests

8 participants