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: dll injection vulnerabilities on Windows (CVE-2019-9634) #30642

Closed
zx2c4 opened this Issue Mar 6, 2019 · 20 comments

Comments

Projects
None yet
5 participants
@zx2c4
Copy link
Contributor

zx2c4 commented Mar 6, 2019

@bradfitz suggested I open an issue for this rather than merely pushing fixes up to gerrit, so that we can track this for a 1.12 point release.

This runtime PR cleans up some LoadLibrary usage: https://go-review.googlesource.com/c/go/+/165798
And this x/sys PR makes the fallback there more reliable: https://go-review.googlesource.com/c/sys/+/165759

The goal is that everywhere LoadLibraryEx preferred, but when not possible, LoadLibrary is called only with either an absolute path computed properly with GetSystemDirectory() or with the exact string kernel32.dll.

I haven't yet dynamically traced the exes yet to verify I've whacked them all now, but hopefully I or someone else can get that done before this issue is closed.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Mar 6, 2019

Previously: #14959

I assume this is a continuation of that effort?

@bradfitz bradfitz added the OS-Windows label Mar 6, 2019

@bradfitz bradfitz added this to the Go1.13 milestone Mar 6, 2019

@bradfitz bradfitz added the NeedsFix label Mar 6, 2019

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Mar 6, 2019

@diagprov

This comment has been minimized.

Copy link

diagprov commented Mar 6, 2019

There's also #28978 regarding winmm.dll. This patchset was posted to those comments: master...sj26:load-winmm-dynamically and would fix winmm.dll loading to use LoadLibrarxEx as well, if this has not already been merged in.

@zx2c4

This comment has been minimized.

Copy link
Contributor Author

zx2c4 commented Mar 6, 2019

Thanks for directing me toward it. https://go-review.googlesource.com/c/go/+/165798 now fixes the winmm.dll issue too.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 6, 2019

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

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 6, 2019

Change https://golang.org/cl/165759 mentions this issue: windows: use proper system directory path in fallback loader

@zx2c4

This comment has been minimized.

Copy link
Contributor Author

zx2c4 commented Mar 6, 2019

I can confirm that the vulnerability is there and appears to be at least mitigated by the CL:

dll-hijack

(Rumor has it, Microsoft will soon fix all security vulnerabilities by removing calc.exe from the default install. Time to look for a new career I guess.)

Boring code:

#include <windows.h>

BOOL WINAPI DllMain(HINSTANCE hinstDLL, DWORD fdwReason, LPVOID lpvReserved)
{
	if (fdwReason != DLL_PROCESS_ATTACH)
		return TRUE;
	STARTUPINFOW si = { .cb = sizeof(STARTUPINFO) };
	PROCESS_INFORMATION pi = { 0 };
	CreateProcessW(L"c:\\windows\\system32\\calc.exe", NULL, NULL, NULL, FALSE, 0, NULL, NULL, &si, &pi);
	TerminateProcess(GetCurrentProcess(), 0);
	return TRUE;
}

__declspec(dllexport) __stdcall MMRESULT timeBeginPeriod(UINT uPeriod) { }
__declspec(dllexport) __stdcall MMRESULT timeEndPeriod(UINT uPeriod) { }
@zx2c4

This comment has been minimized.

Copy link
Contributor Author

zx2c4 commented Mar 7, 2019

The dynamic tracing situation after applying the CLs linked herein is starting to look decent:

image

We now only ever LoadLibrary on kernel32.dll, and the rest get LoadLibraryEx with SEARCH_SYSTEM32.

Meanwhile statically the situation is looking better too:

image

All of those DLLs have been in the KnownDLLs entry going back to Windows XP SP3 at the latest, which is what we're going for. But more to the point, if you compile a more "pure" Go program that doesn't use cgo, the result is totally clean:

image

I'll continue to play with this and trace things further, like undocumented lower level loading functions like LdrLoadDll and such.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Mar 7, 2019

@zx2c4, thanks for your work on this. (And I'm a huge fan of WireGuard!)

Any idea on how we might write a test to catch regressions in this space in the future? Can we do that tracing on a child process ala strace/dtruss without using some GUI app manually?

@zx2c4

This comment has been minimized.

Copy link
Contributor Author

zx2c4 commented Mar 7, 2019

AFAIK, it's not as easy as just apt-get install ltrace && ltrace -e LoadLibrary .\program.exe There are tools like drltrace, and you can script cbd to an extent, but it might take a bit of work to get this automated enough that you could fold it into a Go test or something.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Mar 7, 2019

What about a test trying to attack it instead? Try to run testprog.exe in a directory with a handful of malicious DLLs (that just os.Exit(1)) with all the common names. Then see if testprog.exe exited successfully or not. Then the question is how to build that malicious DLL within the Go build system. We don't have a C compiler available in our builder images, IIRC. Perhaps we just check in the malicious exit-with-failure DLL in as a testdata file? Kinda sketchy to check in random binaries, though, even if we name it sketchytest.dll

@zx2c4

This comment has been minimized.

Copy link
Contributor Author

zx2c4 commented Mar 7, 2019

That's a nice idea. We could write it in Go assembly, I suppose. Or if you know a way of creating a Go shared object with some exported symbols that doesn't pull in the Go runtime, but that sounds impossible. In addition to "all the common names", we could strings the binary looking for anything that ends in ".dll".

@zx2c4

This comment has been minimized.

Copy link
Contributor Author

zx2c4 commented Mar 8, 2019

bradfitz added this to the Go1.13 milestone a day ago

Can we add the 1.12.1 and security marker for this as well? And NeedsFix-->HasFix.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Mar 8, 2019

@gopherbot, please backport to Go 1.12.

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 8, 2019

Backport issue(s) opened: #30666 (for 1.12).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Mar 8, 2019

@zx2c4, we don't have a HasFix label. https://dev.golang.org/ cross-references our open bugs against open code reviews.

@alexbrainman

This comment has been minimized.

Copy link
Member

alexbrainman commented Mar 8, 2019

I can confirm that the vulnerability is there and appears to be at least mitigated by the CL:

Do your changes also mitigate use of kernel32.dll file instead of winmm.dll ? See my comment here #28978 (comment)

Alex

@zx2c4

This comment has been minimized.

Copy link
Contributor Author

zx2c4 commented Mar 8, 2019

Do your changes also mitigate use of kernel32.dll file instead of winmm.dll ? See my comment here #28978 (comment)

kernel32.dll (and ntdll.dll) are generally loaded into every win32 subsystem process regardless of anything else and I'm not so sure those can be injected at all. (We could probably even get the address of kernel32 by following the breadcrumbs from gs:[60h], but nobody does that.)

Could you verify your findings about notepad in that comment? I realize that if you move notepad outside of system32 to a different directory, you might have general problems starting it. But can you actually inject something malicious into it by dropping a trojan'd kernel32.dll? Note that if your findings are correct, you should probably witness either your payload being executed (e.g. calc.exe), or if you didn't bother to make a real dll, at least the error box from the linker.

@alexbrainman

This comment has been minimized.

Copy link
Member

alexbrainman commented Mar 8, 2019

Could you verify your findings about notepad in that comment? I realize that if you move notepad outside of system32 to a different directory, you might have general problems starting it.

What I did last time it this:

C:\Users\Alex\AppData\Local\Temp>mkdir z

C:\Users\Alex\AppData\Local\Temp>cd z

C:\Users\Alex\AppData\Local\Temp\z>copy c:\Windows\notepad.exe .
        1 file(s) copied.

C:\Users\Alex\AppData\Local\Temp\z>echo abc > kernel32.dll

C:\Users\Alex\AppData\Local\Temp\z>notepad.exe

C:\Users\Alex\AppData\Local\Temp\z>

and notepad.exe refused to start.

But you are correct, notepad.exe refused to run even after I deleted kernel32.dll file. So notepad.exe is special.

I tried running other programs (including one compiled with Go) in C:\Users\Alex\AppData\Local\Temp\z directory, and they seems to ignore corrupted kernel32.dll there. And corrupted ntdll.dll too.

So, you are correct about kernel32.dll and ntdll.dll (unlike winmm.dll) are treated differently by Windows.

Alex

@zx2c4 zx2c4 changed the title runtime: dll injection vulnerabilities on Windows runtime: dll injection vulnerabilities on Windows (CVE-2019-9634) Mar 8, 2019

@gopherbot gopherbot closed this in 9b6e9f0 Mar 10, 2019

gopherbot pushed a commit to golang/sys that referenced this issue Mar 10, 2019

windows: use proper system directory path in fallback loader
The %WINDIR% variable is an odd choice and not even entirely reliable.
Since Windows 2000, there has been a specific function for determining
this information, so let's use it. It's also a useful function in its
own right for folks who want to launch system tools in a somewhat safe
way, like netsh.exe.

Updates golang/go#14959
Updates golang/go#30642

Change-Id: Ic24baf37d14f2daced0c1db2771b5a673d2c8852
Reviewed-on: https://go-review.googlesource.com/c/sys/+/165759
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Mar 19, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.