diff options
author | Jason A. Donenfeld <Jason@zx2c4.com> | 2021-01-31 23:38:46 +0100 |
---|---|---|
committer | Jason A. Donenfeld <Jason@zx2c4.com> | 2021-02-02 21:44:58 +0100 |
commit | a2216958d90e4af64c6d130f46fa75b666a2a7b7 (patch) | |
tree | 38aab04b68597cb02a2c241f4deca80d9a2065a9 | |
parent | winipcfg: move to undocumented DNS function (diff) | |
download | wireguard-windows-a2216958d90e4af64c6d130f46fa75b666a2a7b7.tar.xz wireguard-windows-a2216958d90e4af64c6d130f46fa75b666a2a7b7.zip |
manager: use stricter handle inheritability
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
9 files changed, 530 insertions, 87 deletions
diff --git a/go-patches/0001-runtime-allow-builtin-write-function-to-be-redirecte.patch b/go-patches/0001-runtime-allow-builtin-write-function-to-be-redirecte.patch index 26894971..d02eeb2d 100644 --- a/go-patches/0001-runtime-allow-builtin-write-function-to-be-redirecte.patch +++ b/go-patches/0001-runtime-allow-builtin-write-function-to-be-redirecte.patch @@ -1,8 +1,8 @@ From 7b5c04be2cd231d33a8d12db1542055318767c81 Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" <Jason@zx2c4.com> Date: Thu, 3 Dec 2020 13:29:58 +0100 -Subject: [PATCH] runtime: allow builtin write function to be redirected with - function pointer +Subject: [PATCH 1/6] runtime: allow builtin write function to be redirected + with function pointer The x/sys/windows package currently uses go:linkname for other facilities inside of runtime that are not suitable to be exposed as a public API diff --git a/go-patches/0002-syscall-add-support-for-proc-thread-attribute-lists.patch b/go-patches/0002-syscall-add-support-for-proc-thread-attribute-lists.patch new file mode 100644 index 00000000..f3e3a959 --- /dev/null +++ b/go-patches/0002-syscall-add-support-for-proc-thread-attribute-lists.patch @@ -0,0 +1,158 @@ +From ed7ed5c7b3ce4db8abd0219b12ff17ad9074f1cb Mon Sep 17 00:00:00 2001 +From: "Jason A. Donenfeld" <Jason@zx2c4.com> +Date: Sun, 31 Jan 2021 17:37:20 +0100 +Subject: [PATCH 2/6] syscall: add support for proc thread attribute lists + +This will allow us to pass additional attributes when starting +processes. + +Updates #44011. + +Change-Id: I4af365c5544a6d421830f247593ec970200e5e03 +--- + src/syscall/syscall_windows.go | 28 ++++++++++++++++++++++++++++ + src/syscall/types_windows.go | 14 ++++++++++++++ + src/syscall/zsyscall_windows.go | 24 ++++++++++++++++++++++++ + 3 files changed, 66 insertions(+) + +diff --git a/src/syscall/syscall_windows.go b/src/syscall/syscall_windows.go +index ba69133d81..d892cc110d 100644 +--- a/src/syscall/syscall_windows.go ++++ b/src/syscall/syscall_windows.go +@@ -284,6 +284,9 @@ func NewCallbackCDecl(fn interface{}) uintptr { + // This function returns 1 byte BOOLEAN rather than the 4 byte BOOL. + //sys CreateSymbolicLink(symlinkfilename *uint16, targetfilename *uint16, flags uint32) (err error) [failretval&0xff==0] = CreateSymbolicLinkW + //sys CreateHardLink(filename *uint16, existingfilename *uint16, reserved uintptr) (err error) [failretval&0xff==0] = CreateHardLinkW ++//sys initializeProcThreadAttributeList(attrlist *_PROC_THREAD_ATTRIBUTE_LIST, attrcount uint32, flags uint32, size *uintptr) (err error) = InitializeProcThreadAttributeList ++//sys deleteProcThreadAttributeList(attrlist *_PROC_THREAD_ATTRIBUTE_LIST) = DeleteProcThreadAttributeList ++//sys updateProcThreadAttribute(attrlist *_PROC_THREAD_ATTRIBUTE_LIST, flags uint32, attr uintptr, value uintptr, size uintptr, prevvalue uintptr, returnedsize *uintptr) (err error) = UpdateProcThreadAttribute + + // syscall interface implementation for other packages + +@@ -1209,3 +1212,28 @@ func Readlink(path string, buf []byte) (n int, err error) { + + return n, nil + } ++ ++// newProcThreadAttributeList allocates new PROC_THREAD_ATTRIBUTE_LIST, with a have maximum of maxattrcount attributes. ++func newProcThreadAttributeList(maxattrcount uint32) (*_PROC_THREAD_ATTRIBUTE_LIST, error) { ++ var size uintptr ++ err := initializeProcThreadAttributeList(nil, maxattrcount, 0, &size) ++ if err != ERROR_INSUFFICIENT_BUFFER { ++ if err == nil { ++ return nil, errorspkg.New("InitializeProcThreadAttributeList returned no error") ++ } ++ return nil, err ++ } ++ al := (*_PROC_THREAD_ATTRIBUTE_LIST)(unsafe.Pointer(&make([]byte, size)[0])) ++ runtime.SetFinalizer(al, func(al *_PROC_THREAD_ATTRIBUTE_LIST) { ++ deleteProcThreadAttributeList(al) ++ }) ++ err = initializeProcThreadAttributeList(al, maxattrcount, 0, &size) ++ if err != nil { ++ return nil, err ++ } ++ return al, err ++} ++ ++func (al *_PROC_THREAD_ATTRIBUTE_LIST) Add(attribute uintptr, value unsafe.Pointer, size uintptr) error { ++ return updateProcThreadAttribute(al, 0, attribute, uintptr(value), size, 0, nil) ++} +diff --git a/src/syscall/types_windows.go b/src/syscall/types_windows.go +index 0349f3b180..59231bdb83 100644 +--- a/src/syscall/types_windows.go ++++ b/src/syscall/types_windows.go +@@ -482,6 +482,20 @@ type StartupInfo struct { + StdErr Handle + } + ++type _PROC_THREAD_ATTRIBUTE_LIST [1]byte ++ ++const ( ++ _PROC_THREAD_ATTRIBUTE_PARENT_PROCESS = 0x00020000 ++ _PROC_THREAD_ATTRIBUTE_HANDLE_LIST = 0x00020002 ++) ++ ++type _STARTUPINFOEXW struct { ++ StartupInfo ++ ProcThreadAttributeList *_PROC_THREAD_ATTRIBUTE_LIST ++} ++ ++const _EXTENDED_STARTUPINFO_PRESENT = 0x00080000 ++ + type ProcessInformation struct { + Process Handle + Thread Handle +diff --git a/src/syscall/zsyscall_windows.go b/src/syscall/zsyscall_windows.go +index 2166be595b..db9e6e10ea 100644 +--- a/src/syscall/zsyscall_windows.go ++++ b/src/syscall/zsyscall_windows.go +@@ -93,6 +93,7 @@ var ( + procCreateSymbolicLinkW = modkernel32.NewProc("CreateSymbolicLinkW") + procCreateToolhelp32Snapshot = modkernel32.NewProc("CreateToolhelp32Snapshot") + procDeleteFileW = modkernel32.NewProc("DeleteFileW") ++ procDeleteProcThreadAttributeList = modkernel32.NewProc("DeleteProcThreadAttributeList") + procDeviceIoControl = modkernel32.NewProc("DeviceIoControl") + procDuplicateHandle = modkernel32.NewProc("DuplicateHandle") + procExitProcess = modkernel32.NewProc("ExitProcess") +@@ -131,6 +132,7 @@ var ( + procGetTempPathW = modkernel32.NewProc("GetTempPathW") + procGetTimeZoneInformation = modkernel32.NewProc("GetTimeZoneInformation") + procGetVersion = modkernel32.NewProc("GetVersion") ++ procInitializeProcThreadAttributeList = modkernel32.NewProc("InitializeProcThreadAttributeList") + procLoadLibraryW = modkernel32.NewProc("LoadLibraryW") + procLocalFree = modkernel32.NewProc("LocalFree") + procMapViewOfFile = modkernel32.NewProc("MapViewOfFile") +@@ -153,6 +155,7 @@ var ( + procSetHandleInformation = modkernel32.NewProc("SetHandleInformation") + procTerminateProcess = modkernel32.NewProc("TerminateProcess") + procUnmapViewOfFile = modkernel32.NewProc("UnmapViewOfFile") ++ procUpdateProcThreadAttribute = modkernel32.NewProc("UpdateProcThreadAttribute") + procVirtualLock = modkernel32.NewProc("VirtualLock") + procVirtualUnlock = modkernel32.NewProc("VirtualUnlock") + procWaitForSingleObject = modkernel32.NewProc("WaitForSingleObject") +@@ -569,6 +572,11 @@ func DeleteFile(path *uint16) (err error) { + return + } + ++func deleteProcThreadAttributeList(attrlist *_PROC_THREAD_ATTRIBUTE_LIST) { ++ Syscall(procDeleteProcThreadAttributeList.Addr(), 1, uintptr(unsafe.Pointer(attrlist)), 0, 0) ++ return ++} ++ + func DeviceIoControl(handle Handle, ioControlCode uint32, inBuffer *byte, inBufferSize uint32, outBuffer *byte, outBufferSize uint32, bytesReturned *uint32, overlapped *Overlapped) (err error) { + r1, _, e1 := Syscall9(procDeviceIoControl.Addr(), 8, uintptr(handle), uintptr(ioControlCode), uintptr(unsafe.Pointer(inBuffer)), uintptr(inBufferSize), uintptr(unsafe.Pointer(outBuffer)), uintptr(outBufferSize), uintptr(unsafe.Pointer(bytesReturned)), uintptr(unsafe.Pointer(overlapped)), 0) + if r1 == 0 { +@@ -897,6 +905,14 @@ func GetVersion() (ver uint32, err error) { + return + } + ++func initializeProcThreadAttributeList(attrlist *_PROC_THREAD_ATTRIBUTE_LIST, attrcount uint32, flags uint32, size *uintptr) (err error) { ++ r1, _, e1 := Syscall6(procInitializeProcThreadAttributeList.Addr(), 4, uintptr(unsafe.Pointer(attrlist)), uintptr(attrcount), uintptr(flags), uintptr(unsafe.Pointer(size)), 0, 0) ++ if r1 == 0 { ++ err = errnoErr(e1) ++ } ++ return ++} ++ + func LoadLibrary(libname string) (handle Handle, err error) { + var _p0 *uint16 + _p0, err = UTF16PtrFromString(libname) +@@ -1099,6 +1115,14 @@ func UnmapViewOfFile(addr uintptr) (err error) { + return + } + ++func updateProcThreadAttribute(attrlist *_PROC_THREAD_ATTRIBUTE_LIST, flags uint32, attr uintptr, value uintptr, size uintptr, prevvalue uintptr, returnedsize *uintptr) (err error) { ++ r1, _, e1 := Syscall9(procUpdateProcThreadAttribute.Addr(), 7, uintptr(unsafe.Pointer(attrlist)), uintptr(flags), uintptr(attr), uintptr(value), uintptr(size), uintptr(prevvalue), uintptr(unsafe.Pointer(returnedsize)), 0, 0) ++ if r1 == 0 { ++ err = errnoErr(e1) ++ } ++ return ++} ++ + func VirtualLock(addr uintptr, length uintptr) (err error) { + r1, _, e1 := Syscall(procVirtualLock.Addr(), 2, uintptr(addr), uintptr(length), 0) + if r1 == 0 { +-- +2.30.0 + diff --git a/go-patches/0003-syscall-restrict-inherited-handles-on-Windows.patch b/go-patches/0003-syscall-restrict-inherited-handles-on-Windows.patch new file mode 100644 index 00000000..13372c4c --- /dev/null +++ b/go-patches/0003-syscall-restrict-inherited-handles-on-Windows.patch @@ -0,0 +1,79 @@ +From fa5c848c8db44c4e40ace3ca6a0c630500b323e3 Mon Sep 17 00:00:00 2001 +From: "Jason A. Donenfeld" <Jason@zx2c4.com> +Date: Sun, 31 Jan 2021 17:54:27 +0100 +Subject: [PATCH 3/6] syscall: restrict inherited handles on Windows + +Windows does not have CLOEXEC, but rather handles are marked explicitly +for being inherited by new processes. This isn't threadsafe, and various +win32 functions can wind up creating a new process. syscall.StartProcess +has traditionally used a mutex to prevent races with itself, but this +hasn't handled races elsewhere very well. + +Fortunately there's a solution: PROC_THREAD_ATTRIBUTE_HANDLE_LIST allows +us to pass the entire list of handles that we want to be inherited. This +lets us get rid of the mutex and also makes process creation safe across +the Go runtime. + +Updates #44011. + +Change-Id: Ia3424cd2ec64868849cbd6cbb5b0d765224bf4ab +--- + src/syscall/exec_windows.go | 24 ++++++++++++++---------- + 1 file changed, 14 insertions(+), 10 deletions(-) + +diff --git a/src/syscall/exec_windows.go b/src/syscall/exec_windows.go +index 46cbd7567d..6aea91e718 100644 +--- a/src/syscall/exec_windows.go ++++ b/src/syscall/exec_windows.go +@@ -310,12 +310,6 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid int, handle + } + } + +- // Acquire the fork lock so that no other threads +- // create new fds that are not yet close-on-exec +- // before we fork. +- ForkLock.Lock() +- defer ForkLock.Unlock() +- + p, _ := GetCurrentProcess() + fd := make([]Handle, len(attr.Files)) + for i := range attr.Files { +@@ -327,7 +321,11 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid int, handle + defer CloseHandle(Handle(fd[i])) + } + } +- si := new(StartupInfo) ++ si := new(_STARTUPINFOEXW) ++ si.ProcThreadAttributeList, err = newProcThreadAttributeList(1) ++ if err != nil { ++ return 0, 0, err ++ } + si.Cb = uint32(unsafe.Sizeof(*si)) + si.Flags = STARTF_USESTDHANDLES + if sys.HideWindow { +@@ -338,13 +336,19 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid int, handle + si.StdOutput = fd[1] + si.StdErr = fd[2] + ++ // Do not accidentally inherit more than these handles. ++ err = si.ProcThreadAttributeList.Add(_PROC_THREAD_ATTRIBUTE_HANDLE_LIST, unsafe.Pointer(&fd[0]), uintptr(len(fd))*unsafe.Sizeof(fd[0])) ++ if err != nil { ++ return 0, 0, err ++ } ++ + pi := new(ProcessInformation) + +- flags := sys.CreationFlags | CREATE_UNICODE_ENVIRONMENT ++ flags := sys.CreationFlags | CREATE_UNICODE_ENVIRONMENT | _EXTENDED_STARTUPINFO_PRESENT + if sys.Token != 0 { +- err = CreateProcessAsUser(sys.Token, argv0p, argvp, sys.ProcessAttributes, sys.ThreadAttributes, !sys.NoInheritHandles, flags, createEnvBlock(attr.Env), dirp, si, pi) ++ err = CreateProcessAsUser(sys.Token, argv0p, argvp, sys.ProcessAttributes, sys.ThreadAttributes, !sys.NoInheritHandles, flags, createEnvBlock(attr.Env), dirp, &si.StartupInfo, pi) + } else { +- err = CreateProcess(argv0p, argvp, sys.ProcessAttributes, sys.ThreadAttributes, !sys.NoInheritHandles, flags, createEnvBlock(attr.Env), dirp, si, pi) ++ err = CreateProcess(argv0p, argvp, sys.ProcessAttributes, sys.ThreadAttributes, !sys.NoInheritHandles, flags, createEnvBlock(attr.Env), dirp, &si.StartupInfo, pi) + } + if err != nil { + return 0, 0, err +-- +2.30.0 + diff --git a/go-patches/0004-syscall-introduce-SysProcAttr.AdditionalInheritedHan.patch b/go-patches/0004-syscall-introduce-SysProcAttr.AdditionalInheritedHan.patch new file mode 100644 index 00000000..74f2b235 --- /dev/null +++ b/go-patches/0004-syscall-introduce-SysProcAttr.AdditionalInheritedHan.patch @@ -0,0 +1,55 @@ +From cfd236cd9a5122d12047751f0ba561c81779a337 Mon Sep 17 00:00:00 2001 +From: "Jason A. Donenfeld" <Jason@zx2c4.com> +Date: Sun, 31 Jan 2021 18:07:43 +0100 +Subject: [PATCH 4/6] syscall: introduce SysProcAttr.AdditionalInheritedHandles + on Windows + +This allows users to specify handles that they explicitly want to be +inherited by the new process. These handles must already be marked as +inheritable. + +Updates #44011. +Updates #21085. + +Change-Id: Ib18322e7dc2909e68c4209e80385492804fa15d3 +--- + src/syscall/exec_windows.go | 16 +++++++++------- + 1 file changed, 9 insertions(+), 7 deletions(-) + +diff --git a/src/syscall/exec_windows.go b/src/syscall/exec_windows.go +index 6aea91e718..958b11260f 100644 +--- a/src/syscall/exec_windows.go ++++ b/src/syscall/exec_windows.go +@@ -235,13 +235,14 @@ type ProcAttr struct { + } + + type SysProcAttr struct { +- HideWindow bool +- CmdLine string // used if non-empty, else the windows command line is built by escaping the arguments passed to StartProcess +- CreationFlags uint32 +- Token Token // if set, runs new process in the security context represented by the token +- ProcessAttributes *SecurityAttributes // if set, applies these security attributes as the descriptor for the new process +- ThreadAttributes *SecurityAttributes // if set, applies these security attributes as the descriptor for the main thread of the new process +- NoInheritHandles bool // if set, each inheritable handle in the calling process is not inherited by the new process ++ HideWindow bool ++ CmdLine string // used if non-empty, else the windows command line is built by escaping the arguments passed to StartProcess ++ CreationFlags uint32 ++ Token Token // if set, runs new process in the security context represented by the token ++ ProcessAttributes *SecurityAttributes // if set, applies these security attributes as the descriptor for the new process ++ ThreadAttributes *SecurityAttributes // if set, applies these security attributes as the descriptor for the main thread of the new process ++ NoInheritHandles bool // if set, each inheritable handle in the calling process is not inherited by the new process ++ AdditionalInheritedHandles []Handle // a list of additional handles, already marked as inheritable, that will be inherited by the new process + } + + var zeroProcAttr ProcAttr +@@ -336,6 +337,7 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid int, handle + si.StdOutput = fd[1] + si.StdErr = fd[2] + ++ fd = append(fd, sys.AdditionalInheritedHandles...) + // Do not accidentally inherit more than these handles. + err = si.ProcThreadAttributeList.Add(_PROC_THREAD_ATTRIBUTE_HANDLE_LIST, unsafe.Pointer(&fd[0]), uintptr(len(fd))*unsafe.Sizeof(fd[0])) + if err != nil { +-- +2.30.0 + diff --git a/go-patches/0005-os-mark-pipes-returned-by-os.Pipe-as-inheritable-by-.patch b/go-patches/0005-os-mark-pipes-returned-by-os.Pipe-as-inheritable-by-.patch new file mode 100644 index 00000000..a20f91f4 --- /dev/null +++ b/go-patches/0005-os-mark-pipes-returned-by-os.Pipe-as-inheritable-by-.patch @@ -0,0 +1,39 @@ +From 39d70b6441f914a41cf8568d0930c299b926c139 Mon Sep 17 00:00:00 2001 +From: "Jason A. Donenfeld" <Jason@zx2c4.com> +Date: Sun, 31 Jan 2021 19:51:45 +0100 +Subject: [PATCH 5/6] os: mark pipes returned by os.Pipe() as inheritable by + default + +Now that we don't automatically pass all inheritable handles to new +processes, we can make pipes returned by os.Pipe() inheritable, just +like they are on Unix. This then allows them to be passed through the +SysProcAttr.AdditionalInheritedHandles parameter simply. + +Updates #44011. +Fixes #21085. + +Change-Id: I8eae329fbc74f9dc7962136fa9aae8fb66879751 +--- + src/os/file_windows.go | 4 ++-- + 1 file changed, 2 insertions(+), 2 deletions(-) + +diff --git a/src/os/file_windows.go b/src/os/file_windows.go +index dfc5fc6ce6..239690ce43 100644 +--- a/src/os/file_windows.go ++++ b/src/os/file_windows.go +@@ -279,10 +279,10 @@ func rename(oldname, newname string) error { + } + + // Pipe returns a connected pair of Files; reads from r return bytes written to w. +-// It returns the files and an error, if any. ++// It returns the files and an error, if any. This handle is inheritable by default. + func Pipe() (r *File, w *File, err error) { + var p [2]syscall.Handle +- e := syscall.CreatePipe(&p[0], &p[1], nil, 0) ++ e := syscall.Pipe(p[:]) + if e != nil { + return nil, nil, NewSyscallError("pipe", e) + } +-- +2.30.0 + diff --git a/go-patches/0006-syscall-introduce-SysProcAttr.ParentProcess-on-Windo.patch b/go-patches/0006-syscall-introduce-SysProcAttr.ParentProcess-on-Windo.patch new file mode 100644 index 00000000..b7453be0 --- /dev/null +++ b/go-patches/0006-syscall-introduce-SysProcAttr.ParentProcess-on-Windo.patch @@ -0,0 +1,171 @@ +From fd146509dda08eca30b2145deddb669b789da6e0 Mon Sep 17 00:00:00 2001 +From: "Jason A. Donenfeld" <Jason@zx2c4.com> +Date: Sun, 31 Jan 2021 18:14:56 +0100 +Subject: [PATCH 6/6] syscall: introduce SysProcAttr.ParentProcess on Windows + +This allows users to specify which process should be used as the parent +process when creating a new process. + +Note that this doesn't just trivially pass the handle onward to +PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, because inherited handles must be +valid in the parent process, so if we're changing the destination +process, then we must also change the origin of the parent handles. And, +the StartProcess function must clean up these handles successfully when +exiting, regardless of where the duplication happened. So, we take care +in this commit to use DuplicateHandle for both duplicating and for +closing the inherited handles. + +The test was taken originally from CL 288272 and adjusted for use here. + +Fixes #44011. + +Change-Id: Ib3b132028dcab1aded3dc0e65126c8abebfa35eb +--- + src/syscall/exec_windows.go | 17 ++++++-- + src/syscall/exec_windows_test.go | 73 ++++++++++++++++++++++++++++++++ + 2 files changed, 87 insertions(+), 3 deletions(-) + +diff --git a/src/syscall/exec_windows.go b/src/syscall/exec_windows.go +index 958b11260f..89257be0a4 100644 +--- a/src/syscall/exec_windows.go ++++ b/src/syscall/exec_windows.go +@@ -243,6 +243,7 @@ type SysProcAttr struct { + ThreadAttributes *SecurityAttributes // if set, applies these security attributes as the descriptor for the main thread of the new process + NoInheritHandles bool // if set, each inheritable handle in the calling process is not inherited by the new process + AdditionalInheritedHandles []Handle // a list of additional handles, already marked as inheritable, that will be inherited by the new process ++ ParentProcess Handle // if non-zero, the new process regards the process given by this handle as its parent process + } + + var zeroProcAttr ProcAttr +@@ -312,18 +313,22 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid int, handle + } + + p, _ := GetCurrentProcess() ++ parentProcess := p ++ if sys.ParentProcess != 0 { ++ parentProcess = sys.ParentProcess ++ } + fd := make([]Handle, len(attr.Files)) + for i := range attr.Files { + if attr.Files[i] > 0 { +- err := DuplicateHandle(p, Handle(attr.Files[i]), p, &fd[i], 0, true, DUPLICATE_SAME_ACCESS) ++ err := DuplicateHandle(p, Handle(attr.Files[i]), parentProcess, &fd[i], 0, true, DUPLICATE_SAME_ACCESS) + if err != nil { + return 0, 0, err + } +- defer CloseHandle(Handle(fd[i])) ++ defer DuplicateHandle(parentProcess, fd[i], 0, nil, 0, false, DUPLICATE_CLOSE_SOURCE) + } + } + si := new(_STARTUPINFOEXW) +- si.ProcThreadAttributeList, err = newProcThreadAttributeList(1) ++ si.ProcThreadAttributeList, err = newProcThreadAttributeList(2) + if err != nil { + return 0, 0, err + } +@@ -333,6 +338,12 @@ func StartProcess(argv0 string, argv []string, attr *ProcAttr) (pid int, handle + si.Flags |= STARTF_USESHOWWINDOW + si.ShowWindow = SW_HIDE + } ++ if sys.ParentProcess != 0 { ++ err = si.ProcThreadAttributeList.Add(_PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, unsafe.Pointer(&sys.ParentProcess), unsafe.Sizeof(sys.ParentProcess)) ++ if err != nil { ++ return 0, 0, err ++ } ++ } + si.StdInput = fd[0] + si.StdOutput = fd[1] + si.StdErr = fd[2] +diff --git a/src/syscall/exec_windows_test.go b/src/syscall/exec_windows_test.go +index eda1d36877..8a1c2ceaae 100644 +--- a/src/syscall/exec_windows_test.go ++++ b/src/syscall/exec_windows_test.go +@@ -5,8 +5,14 @@ + package syscall_test + + import ( ++ "fmt" ++ "io/ioutil" ++ "os" ++ "os/exec" ++ "path/filepath" + "syscall" + "testing" ++ "time" + ) + + func TestEscapeArg(t *testing.T) { +@@ -41,3 +47,70 @@ func TestEscapeArg(t *testing.T) { + } + } + } ++ ++func TestChangingProcessParent(t *testing.T) { ++ if os.Getenv("GO_WANT_HELPER_PROCESS") == "parent" { ++ // in parent process ++ ++ // Parent does nothign. It is just used as a parent of a child process. ++ time.Sleep(time.Minute) ++ os.Exit(0) ++ } ++ ++ if os.Getenv("GO_WANT_HELPER_PROCESS") == "child" { ++ // in child process ++ dumpPath := os.Getenv("GO_WANT_HELPER_PROCESS_FILE") ++ if dumpPath == "" { ++ fmt.Fprintf(os.Stderr, "Dump file path cannot be blank.") ++ os.Exit(1) ++ } ++ err := os.WriteFile(dumpPath, []byte(fmt.Sprintf("%d", os.Getppid())), 0644) ++ if err != nil { ++ fmt.Fprintf(os.Stderr, "Error writing dump file: %v", err) ++ os.Exit(2) ++ } ++ os.Exit(0) ++ } ++ ++ // run parent process ++ ++ parent := exec.Command(os.Args[0], "-test.run=TestChangingProcessParent") ++ parent.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=parent") ++ err := parent.Start() ++ if err != nil { ++ t.Fatal(err) ++ } ++ defer func() { ++ parent.Process.Kill() ++ parent.Wait() ++ }() ++ ++ // run child process ++ ++ const _PROCESS_CREATE_PROCESS = 0x0080 ++ const _PROCESS_DUP_HANDLE = 0x0040 ++ childDumpPath := filepath.Join(t.TempDir(), "ppid.txt") ++ ph, err := syscall.OpenProcess(_PROCESS_CREATE_PROCESS|_PROCESS_DUP_HANDLE|syscall.PROCESS_QUERY_INFORMATION, ++ false, uint32(parent.Process.Pid)) ++ if err != nil { ++ t.Fatal(err) ++ } ++ defer syscall.CloseHandle(ph) ++ ++ child := exec.Command(os.Args[0], "-test.run=TestChangingProcessParent") ++ child.Env = append(os.Environ(), ++ "GO_WANT_HELPER_PROCESS=child", ++ "GO_WANT_HELPER_PROCESS_FILE="+childDumpPath) ++ child.SysProcAttr = &syscall.SysProcAttr{ParentProcess: ph} ++ childOutput, err := child.CombinedOutput() ++ if err != nil { ++ t.Errorf("child failed: %v: %v", err, string(childOutput)) ++ } ++ childOutput, err = ioutil.ReadFile(childDumpPath) ++ if err != nil { ++ t.Fatalf("reading child ouput failed: %v", err) ++ } ++ if got, want := string(childOutput), fmt.Sprintf("%d", parent.Process.Pid); got != want { ++ t.Fatalf("child output: want %q, got %q", want, got) ++ } ++} +-- +2.30.0 + diff --git a/manager/ipc_pipe.go b/manager/ipc_pipe.go deleted file mode 100644 index e1723fbd..00000000 --- a/manager/ipc_pipe.go +++ /dev/null @@ -1,71 +0,0 @@ -/* SPDX-License-Identifier: MIT - * - * Copyright (C) 2019-2021 WireGuard LLC. All Rights Reserved. - */ - -package manager - -import ( - "os" - "strconv" - - "golang.org/x/sys/windows" -) - -func makeInheritableAndGetStr(f *os.File) (str string, err error) { - sc, err := f.SyscallConn() - if err != nil { - return - } - err2 := sc.Control(func(fd uintptr) { - err = windows.SetHandleInformation(windows.Handle(fd), windows.HANDLE_FLAG_INHERIT, windows.HANDLE_FLAG_INHERIT) - str = strconv.FormatUint(uint64(fd), 10) - }) - if err2 != nil { - err = err2 - } - return -} - -func inheritableEvents() (ourEvents *os.File, theirEvents *os.File, theirEventStr string, err error) { - theirEvents, ourEvents, err = os.Pipe() - if err != nil { - return - } - theirEventStr, err = makeInheritableAndGetStr(theirEvents) - if err != nil { - theirEvents.Close() - ourEvents.Close() - return - } - return -} - -func inheritableSocketpairEmulation() (ourReader *os.File, theirReader *os.File, theirReaderStr string, ourWriter *os.File, theirWriter *os.File, theirWriterStr string, err error) { - ourReader, theirWriter, err = os.Pipe() - if err != nil { - return - } - theirWriterStr, err = makeInheritableAndGetStr(theirWriter) - if err != nil { - ourReader.Close() - theirWriter.Close() - return - } - - theirReader, ourWriter, err = os.Pipe() - if err != nil { - ourReader.Close() - theirWriter.Close() - return - } - theirReaderStr, err = makeInheritableAndGetStr(theirReader) - if err != nil { - ourReader.Close() - theirWriter.Close() - theirReader.Close() - ourWriter.Close() - return - } - return -} diff --git a/manager/service.go b/manager/service.go index eda96817..b13870fd 100644 --- a/manager/service.go +++ b/manager/service.go @@ -10,6 +10,7 @@ import ( "log" "os" "runtime" + "strconv" "sync" "syscall" "time" @@ -172,22 +173,23 @@ func (service *managerService) Execute(args []string, r <-chan svc.ChangeRequest first = false } - // TODO: we lock the OS thread so that these inheritable handles don't escape into other processes that - // might be running in parallel Go routines. But the Go runtime is strange and who knows what's really - // happening with these or what is inherited. We need to do some analysis to be certain of what's going on. - runtime.LockOSThread() - ourReader, theirReader, theirReaderStr, ourWriter, theirWriter, theirWriterStr, err := inheritableSocketpairEmulation() + ourReader, theirWriter, err := os.Pipe() if err != nil { - log.Printf("Unable to create two inheritable RPC pipes: %v", err) + log.Printf("Unable to create pipe: %v", err) return } - ourEvents, theirEvents, theirEventStr, err := inheritableEvents() + theirReader, ourWriter, err := os.Pipe() if err != nil { - log.Printf("Unable to create one inheritable events pipe: %v", err) + log.Printf("Unable to create pipe: %v", err) + return + } + theirEvents, ourEvents, err := os.Pipe() + if err != nil { + log.Printf("Unable to create pipe: %v", err) return } IPCServerListen(ourReader, ourWriter, ourEvents, elevatedToken) - theirLogMapping, theirLogMappingHandle, err := ringlogger.Global.ExportInheritableMappingHandleStr() + theirLogMapping, err := ringlogger.Global.ExportInheritableMappingHandle() if err != nil { log.Printf("Unable to export inheritable mapping handle for logging: %v", err) return @@ -197,6 +199,11 @@ func (service *managerService) Execute(args []string, r <-chan svc.ChangeRequest attr := &os.ProcAttr{ Sys: &syscall.SysProcAttr{ Token: syscall.Token(runToken), + AdditionalInheritedHandles: []syscall.Handle{ + syscall.Handle(theirReader.Fd()), + syscall.Handle(theirWriter.Fd()), + syscall.Handle(theirEvents.Fd()), + syscall.Handle(theirLogMapping)}, }, Files: []*os.File{devNull, devNull, devNull}, Dir: userProfileDirectory, @@ -204,7 +211,14 @@ func (service *managerService) Execute(args []string, r <-chan svc.ChangeRequest procsLock.Lock() var proc *os.Process if alive := aliveSessions[session]; alive { - proc, err = os.StartProcess(path, []string{path, "/ui", theirReaderStr, theirWriterStr, theirEventStr, theirLogMapping}, attr) + proc, err = os.StartProcess(path, []string{ + path, + "/ui", + strconv.FormatUint(uint64(theirReader.Fd()), 10), + strconv.FormatUint(uint64(theirWriter.Fd()), 10), + strconv.FormatUint(uint64(theirEvents.Fd()), 10), + strconv.FormatUint(uint64(theirLogMapping), 10), + }, attr) } else { err = errors.New("Session has logged out") } @@ -212,8 +226,7 @@ func (service *managerService) Execute(args []string, r <-chan svc.ChangeRequest theirReader.Close() theirWriter.Close() theirEvents.Close() - windows.Close(theirLogMappingHandle) - runtime.UnlockOSThread() + windows.Close(theirLogMapping) if err != nil { ourReader.Close() ourWriter.Close() diff --git a/ringlogger/ringlogger.go b/ringlogger/ringlogger.go index 66487c08..fa3d8c76 100644 --- a/ringlogger/ringlogger.go +++ b/ringlogger/ringlogger.go @@ -234,7 +234,7 @@ func (rl *Ringlogger) Close() error { return nil } -func (rl *Ringlogger) ExportInheritableMappingHandleStr() (str string, handleToClose windows.Handle, err error) { +func (rl *Ringlogger) ExportInheritableMappingHandle() (handleToClose windows.Handle, err error) { handleToClose, err = windows.CreateFileMapping(windows.Handle(rl.file.Fd()), nil, windows.PAGE_READONLY, 0, 0, nil) if err != nil { return @@ -245,6 +245,5 @@ func (rl *Ringlogger) ExportInheritableMappingHandleStr() (str string, handleToC handleToClose = 0 return } - str = strconv.FormatUint(uint64(handleToClose), 10) return } |