diff options
| -rw-r--r-- | build.bat | 2 | ||||
| -rw-r--r-- | golang-create-environment-block-process-creation-token.patch | 164 | ||||
| -rw-r--r-- | service/ipc_server.go | 13 | ||||
| -rw-r--r-- | service/securityapi.go | 29 | ||||
| -rw-r--r-- | service/service_manager.go | 18 | ||||
| -rw-r--r-- | service/zsyscall_windows.go | 33 | ||||
| -rw-r--r-- | updater/downloader.go | 4 | ||||
| -rw-r--r-- | updater/msirunner_windows.go | 3 |
8 files changed, 178 insertions, 88 deletions
@@ -20,7 +20,7 @@ if exist .deps\prepared goto :render rem Mirror of https://sourceforge.net/projects/gnuwin32/files/patch/2.5.9-7/patch-2.5.9-7-bin.zip with fixed manifest call :download patch.zip https://download.wireguard.com/windows-toolchain/distfiles/patch-2.5.9-7-bin-fixed-manifest.zip 25977006ca9713f2662a5d0a2ed3a5a138225b8be3757035bd7da9dcf985d0a1 "--strip-components 1 bin" || goto :error echo [+] Patching go - .\patch.exe -f -N -r- -d go -p1 --binary < ..\golang-security-attribute-process-creation.patch || goto :error + for %%a in ("..\golang-*.patch") do .\patch.exe -f -N -r- -d go -p1 --binary < "%%a" || goto :error copy /y NUL prepared > NUL || goto :error cd .. || goto :error diff --git a/golang-create-environment-block-process-creation-token.patch b/golang-create-environment-block-process-creation-token.patch new file mode 100644 index 00000000..b5d75f60 --- /dev/null +++ b/golang-create-environment-block-process-creation-token.patch @@ -0,0 +1,164 @@ +From 3746d930467a486fb714a885ad92ebe16cf33c2a Mon Sep 17 00:00:00 2001 +From: Jason A. Donenfeld <Jason@zx2c4.com> +Date: Sun, 12 May 2019 14:34:30 +0200 +Subject: [PATCH] os: pass correct environment when creating Windows processes + +This is CVE-2019-11888. + +Previously, passing a nil environment but a non-nil token would result +in the new potentially unprivileged process inheriting the parent +potentially privileged environment, or would result in the new +potentially privileged process inheriting the parent potentially +unprivileged environment. Either way, it's bad. In the former case, it's +an infoleak. In the latter case, it's a possible EoP, since things like +PATH could be overwritten. + +Not specifying an environment currently means, "use the existing +environment". This commit amends the behavior to be, "use the existing +environment of the token the process is being created for." The behavior +therefore stays the same when creating processes without specifying a +token. And it does the correct thing when creating processes when +specifying a token. + +Change-Id: Ia57f6e89b97bdbaf7274d6a89c1d9948b6d40ef5 +--- + +diff --git a/src/internal/syscall/windows/syscall_windows.go b/src/internal/syscall/windows/syscall_windows.go +index 121132f..099e91e 100644 +--- a/src/internal/syscall/windows/syscall_windows.go ++++ b/src/internal/syscall/windows/syscall_windows.go +@@ -305,3 +305,6 @@ + func LoadGetFinalPathNameByHandle() error { + return procGetFinalPathNameByHandleW.Find() + } ++ ++//sys CreateEnvironmentBlock(block **uint16, token syscall.Token, inheritExisting bool) (err error) = userenv.CreateEnvironmentBlock ++//sys DestroyEnvironmentBlock(block *uint16) (err error) = userenv.DestroyEnvironmentBlock +diff --git a/src/internal/syscall/windows/zsyscall_windows.go b/src/internal/syscall/windows/zsyscall_windows.go +index 9527a37..ca5b4e6 100644 +--- a/src/internal/syscall/windows/zsyscall_windows.go ++++ b/src/internal/syscall/windows/zsyscall_windows.go +@@ -58,6 +58,8 @@ + procNetShareAdd = modnetapi32.NewProc("NetShareAdd") + procNetShareDel = modnetapi32.NewProc("NetShareDel") + procGetFinalPathNameByHandleW = modkernel32.NewProc("GetFinalPathNameByHandleW") ++ procCreateEnvironmentBlock = moduserenv.NewProc("CreateEnvironmentBlock") ++ procDestroyEnvironmentBlock = moduserenv.NewProc("DestroyEnvironmentBlock") + procImpersonateSelf = modadvapi32.NewProc("ImpersonateSelf") + procRevertToSelf = modadvapi32.NewProc("RevertToSelf") + procOpenThreadToken = modadvapi32.NewProc("OpenThreadToken") +@@ -220,6 +222,36 @@ + return + } + ++func CreateEnvironmentBlock(block **uint16, token syscall.Token, inheritExisting bool) (err error) { ++ var _p0 uint32 ++ if inheritExisting { ++ _p0 = 1 ++ } else { ++ _p0 = 0 ++ } ++ r1, _, e1 := syscall.Syscall(procCreateEnvironmentBlock.Addr(), 3, uintptr(unsafe.Pointer(block)), uintptr(token), uintptr(_p0)) ++ if r1 == 0 { ++ if e1 != 0 { ++ err = errnoErr(e1) ++ } else { ++ err = syscall.EINVAL ++ } ++ } ++ return ++} ++ ++func DestroyEnvironmentBlock(block *uint16) (err error) { ++ r1, _, e1 := syscall.Syscall(procDestroyEnvironmentBlock.Addr(), 1, uintptr(unsafe.Pointer(block)), 0, 0) ++ if r1 == 0 { ++ if e1 != 0 { ++ err = errnoErr(e1) ++ } else { ++ err = syscall.EINVAL ++ } ++ } ++ return ++} ++ + func ImpersonateSelf(impersonationlevel uint32) (err error) { + r1, _, e1 := syscall.Syscall(procImpersonateSelf.Addr(), 1, uintptr(impersonationlevel), 0, 0) + if r1 == 0 { +diff --git a/src/os/env_default.go b/src/os/env_default.go +new file mode 100644 +index 0000000..91c5e9a +--- /dev/null ++++ b/src/os/env_default.go +@@ -0,0 +1,13 @@ ++// Copyright 2019 The Go Authors. All rights reserved. ++// Use of this source code is governed by a BSD-style ++// license that can be found in the LICENSE file. ++ ++// +build !windows ++ ++package os ++ ++import "syscall" ++ ++func environForSysProcAttr(sys *syscall.SysProcAttr) (env []string) { ++ return Environ() ++} +diff --git a/src/os/env_windows.go b/src/os/env_windows.go +new file mode 100644 +index 0000000..76cf011 +--- /dev/null ++++ b/src/os/env_windows.go +@@ -0,0 +1,40 @@ ++// Copyright 2019 The Go Authors. All rights reserved. ++// Use of this source code is governed by a BSD-style ++// license that can be found in the LICENSE file. ++ ++package os ++ ++import ( ++ "internal/syscall/windows" ++ "syscall" ++ "unicode/utf16" ++ "unsafe" ++) ++ ++func environForSysProcAttr(sys *syscall.SysProcAttr) (env []string) { ++ if sys == nil || sys.Token == 0 { ++ return Environ() ++ } ++ var block *uint16 ++ err := windows.CreateEnvironmentBlock(&block, sys.Token, false) ++ if err != nil { ++ return nil ++ } ++ blockp := uintptr(unsafe.Pointer(block)) ++ for { ++ entry := (*[(1 << 30) - 1]uint16)(unsafe.Pointer(blockp))[:] ++ for i, v := range entry { ++ if v == 0 { ++ entry = entry[:i] ++ break ++ } ++ } ++ if len(entry) == 0 { ++ break ++ } ++ env = append(env, string(utf16.Decode(entry))) ++ blockp += 2 * (uintptr(len(entry)) + 1) ++ } ++ windows.DestroyEnvironmentBlock(block) ++ return ++} +diff --git a/src/os/exec_posix.go b/src/os/exec_posix.go +index 7b1ef67..651d24d 100644 +--- a/src/os/exec_posix.go ++++ b/src/os/exec_posix.go +@@ -38,7 +38,7 @@ + Sys: attr.Sys, + } + if sysattr.Env == nil { +- sysattr.Env = Environ() ++ sysattr.Env = environForSysProcAttr(sysattr.Sys) + } + sysattr.Files = make([]uintptr, 0, len(attr.Files)) + for _, f := range attr.Files { diff --git a/service/ipc_server.go b/service/ipc_server.go index becad3ee..12e01ce2 100644 --- a/service/ipc_server.go +++ b/service/ipc_server.go @@ -29,14 +29,9 @@ var managerServicesLock sync.RWMutex var haveQuit uint32 var quitManagersChan = make(chan struct{}, 1) -type UserTokenInfo struct { - elevatedToken windows.Token - elevatedEnvironment []string -} - type ManagerService struct { events *os.File - userTokenInfo *UserTokenInfo + elevatedToken windows.Token } func (s *ManagerService) StoredConfig(tunnelName string, config *conf.Config) error { @@ -258,7 +253,7 @@ func (s *ManagerService) UpdateState(_ uintptr, state *UpdateState) error { } func (s *ManagerService) Update(_ uintptr, _ *uintptr) error { - progress := updater.DownloadVerifyAndExecute(uintptr(s.userTokenInfo.elevatedToken), s.userTokenInfo.elevatedEnvironment) + progress := updater.DownloadVerifyAndExecute(uintptr(s.elevatedToken)) go func() { for { dp := <-progress @@ -271,10 +266,10 @@ func (s *ManagerService) Update(_ uintptr, _ *uintptr) error { return nil } -func IPCServerListen(reader *os.File, writer *os.File, events *os.File, userTokenInfo *UserTokenInfo) error { +func IPCServerListen(reader *os.File, writer *os.File, events *os.File, elevatedToken windows.Token) error { service := &ManagerService{ events: events, - userTokenInfo: userTokenInfo, + elevatedToken: elevatedToken, } server := rpc.NewServer() diff --git a/service/securityapi.go b/service/securityapi.go index 37713b32..d89d9404 100644 --- a/service/securityapi.go +++ b/service/securityapi.go @@ -9,7 +9,6 @@ import ( "errors" "golang.org/x/sys/windows" "runtime" - "unicode/utf16" "unsafe" ) @@ -53,34 +52,6 @@ const ( SE_GROUP_USE_FOR_DENY_ONLY = 0x00000010 ) -//sys createEnvironmentBlock(block *uintptr, token windows.Token, inheritExisting bool) (err error) = userenv.CreateEnvironmentBlock -//sys destroyEnvironmentBlock(block uintptr) (err error) = userenv.DestroyEnvironmentBlock - -func userEnviron(token windows.Token) (env []string, err error) { - var block uintptr - err = createEnvironmentBlock(&block, token, false) - if err != nil { - return - } - offset := uintptr(0) - for { - entry := (*[(1 << 30) - 1]uint16)(unsafe.Pointer(block + offset))[:] - for i, v := range entry { - if v == 0 { - entry = entry[:i] - break - } - } - if len(entry) == 0 { - break - } - env = append(env, string(utf16.Decode(entry))) - offset += 2 * (uintptr(len(entry)) + 1) - } - destroyEnvironmentBlock(block) - return -} - func tokenIsElevated(token windows.Token) bool { var isElevated uint32 var outLen uint32 diff --git a/service/service_manager.go b/service/service_manager.go index c61bd517..92508215 100644 --- a/service/service_manager.go +++ b/service/service_manager.go @@ -110,22 +110,17 @@ func (service *managerService) Execute(args []string, r <-chan svc.ChangeRequest userToken.Close() return } - userTokenInfo := &UserTokenInfo{} - userTokenInfo.elevatedToken, err = getElevatedToken(userToken) + //TODO: The environment that Go gets from CreateEnvironmentBlock seems to have the same PATH as the userToken. Aren't there attacks? + elevatedToken, err := getElevatedToken(userToken) if err != nil { log.Printf("Unable to elevate token: %v", err) return } - if userTokenInfo.elevatedToken != userToken { + if elevatedToken != userToken { userToken.Close() } - defer userTokenInfo.elevatedToken.Close() + defer elevatedToken.Close() userToken = 0 - userTokenInfo.elevatedEnvironment, err = userEnviron(userTokenInfo.elevatedToken) //TODO: This seems to have the same PATH as the userToken. Aren't there attacks? - if err != nil { - log.Printf("Unable to determine elevated environment: %v", err) - return - } first := true for { if stoppingManager { @@ -155,7 +150,7 @@ func (service *managerService) Execute(args []string, r <-chan svc.ChangeRequest return } ourEvents, theirEvents, theirEventStr, err := inheritableEvents() - err = IPCServerListen(ourReader, ourWriter, ourEvents, userTokenInfo) + err = IPCServerListen(ourReader, ourWriter, ourEvents, elevatedToken) if err != nil { log.Printf("Unable to listen on IPC pipes: %v", err) return @@ -169,10 +164,9 @@ func (service *managerService) Execute(args []string, r <-chan svc.ChangeRequest log.Printf("Starting UI process for user '%s@%s' for session %d", username, domain, session) attr := &os.ProcAttr{ Sys: &syscall.SysProcAttr{ - Token: syscall.Token(userTokenInfo.elevatedToken), + Token: syscall.Token(elevatedToken), }, Files: []*os.File{devNull, devNull, devNull}, - Env: userTokenInfo.elevatedEnvironment, } procsLock.Lock() var proc *os.Process diff --git a/service/zsyscall_windows.go b/service/zsyscall_windows.go index 20f9753b..bb8d89d4 100644 --- a/service/zsyscall_windows.go +++ b/service/zsyscall_windows.go @@ -38,15 +38,12 @@ func errnoErr(e syscall.Errno) error { var ( modwtsapi32 = windows.NewLazySystemDLL("wtsapi32.dll") - moduserenv = windows.NewLazySystemDLL("userenv.dll") modadvapi32 = windows.NewLazySystemDLL("advapi32.dll") modkernel32 = windows.NewLazySystemDLL("kernel32.dll") procWTSQueryUserToken = modwtsapi32.NewProc("WTSQueryUserToken") procWTSEnumerateSessionsW = modwtsapi32.NewProc("WTSEnumerateSessionsW") procWTSFreeMemory = modwtsapi32.NewProc("WTSFreeMemory") - procCreateEnvironmentBlock = moduserenv.NewProc("CreateEnvironmentBlock") - procDestroyEnvironmentBlock = moduserenv.NewProc("DestroyEnvironmentBlock") procNotifyServiceStatusChangeW = modadvapi32.NewProc("NotifyServiceStatusChangeW") procSleepEx = modkernel32.NewProc("SleepEx") ) @@ -80,36 +77,6 @@ func wtsFreeMemory(ptr uintptr) { return } -func createEnvironmentBlock(block *uintptr, token windows.Token, inheritExisting bool) (err error) { - var _p0 uint32 - if inheritExisting { - _p0 = 1 - } else { - _p0 = 0 - } - r1, _, e1 := syscall.Syscall(procCreateEnvironmentBlock.Addr(), 3, uintptr(unsafe.Pointer(block)), uintptr(token), uintptr(_p0)) - if r1 == 0 { - if e1 != 0 { - err = errnoErr(e1) - } else { - err = syscall.EINVAL - } - } - return -} - -func destroyEnvironmentBlock(block uintptr) (err error) { - r1, _, e1 := syscall.Syscall(procDestroyEnvironmentBlock.Addr(), 1, uintptr(block), 0, 0) - if r1 == 0 { - if e1 != 0 { - err = errnoErr(e1) - } else { - err = syscall.EINVAL - } - } - return -} - func notifyServiceStatusChange(service windows.Handle, notifyMask uint32, notifyBuffer uintptr) (status uint32) { r0, _, _ := syscall.Syscall(procNotifyServiceStatusChangeW.Addr(), 3, uintptr(service), uintptr(notifyMask), uintptr(notifyBuffer)) status = uint32(r0) diff --git a/updater/downloader.go b/updater/downloader.go index 2f83b9b2..88931d61 100644 --- a/updater/downloader.go +++ b/updater/downloader.go @@ -70,7 +70,7 @@ func CheckForUpdate() (*UpdateFound, error) { var updateInProgress = uint32(0) -func DownloadVerifyAndExecute(userToken uintptr, userEnvironment []string) (progress chan DownloadProgress) { +func DownloadVerifyAndExecute(userToken uintptr) (progress chan DownloadProgress) { progress = make(chan DownloadProgress, 128) progress <- DownloadProgress{Activity: "Initializing"} @@ -158,7 +158,7 @@ func DownloadVerifyAndExecute(userToken uintptr, userEnvironment []string) (prog } progress <- DownloadProgress{Activity: "Installing update"} - err = runMsi(name, userToken, userEnvironment) + err = runMsi(name, userToken) os.Remove(name) //TODO: Do we have any sort of TOCTOU here? if err != nil { progress <- DownloadProgress{Error: err} diff --git a/updater/msirunner_windows.go b/updater/msirunner_windows.go index de3fb58e..1ea405f4 100644 --- a/updater/msirunner_windows.go +++ b/updater/msirunner_windows.go @@ -19,7 +19,7 @@ import ( "unsafe" ) -func runMsi(msiPath string, userToken uintptr, env []string) error { +func runMsi(msiPath string, userToken uintptr) error { system32, err := windows.GetSystemDirectory() if err != nil { return err @@ -34,7 +34,6 @@ func runMsi(msiPath string, userToken uintptr, env []string) error { Token: syscall.Token(userToken), }, Files: []*os.File{devNull, devNull, devNull}, - Env: env, Dir: path.Dir(msiPath), } msiexec := path.Join(system32, "msiexec.exe") |