From 03aa0b5f6827af9a6017ded74142961777f49e64 Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Mon, 13 Dec 2021 22:07:04 +0100 Subject: [PATCH] windows: allocate attribute list with LocalAlloc, not individual items We didn't want to wind up with Go pointers mangled by win32, so we were previously copying the contents into a LocalAlloc'd blob, and then adding that to the attribute list. The problem is that recent changes to the API broke this design convention, to the point where it expects 0x18 byte objects to be added using size 0x8. This seems like an unfortunate oversight from Microsoft, but there's nothing we can do about it. So we can work around it by instead LocalAlloc'ing the actual container, and then using the exact pointer value that's passed into Update. This commit also adds a test that both makes sure that these functions actually work, and provokes a UaF that's successfully caught, were you to comment out the line of this commit that reads `al.pointers = append(al.pointers, value)`. Fixes golang/go#50134. Change-Id: Ib73346d2d6ca3db601cd236596cefb564d9dc8f1 Reviewed-on: https://go-review.googlesource.com/c/sys/+/371276 Trust: Jason Donenfeld Run-TryBot: Jason Donenfeld TryBot-Result: Gopher Robot Reviewed-by: Patrik Nyblom Trust: Patrik Nyblom Run-TryBot: Patrik Nyblom --- windows/exec_windows.go | 37 ++++++---------------- windows/syscall_windows_test.go | 55 +++++++++++++++++++++++++++++++++ windows/types_windows.go | 4 +-- 3 files changed, 67 insertions(+), 29 deletions(-) diff --git a/windows/exec_windows.go b/windows/exec_windows.go index 7a11e83b..855698bb 100644 --- a/windows/exec_windows.go +++ b/windows/exec_windows.go @@ -9,8 +9,6 @@ package windows import ( errorspkg "errors" "unsafe" - - "golang.org/x/sys/internal/unsafeheader" ) // EscapeArg rewrites command line argument s as prescribed @@ -147,8 +145,12 @@ func NewProcThreadAttributeList(maxAttrCount uint32) (*ProcThreadAttributeListCo } return nil, err } + alloc, err := LocalAlloc(LMEM_FIXED, uint32(size)) + if err != nil { + return nil, err + } // size is guaranteed to be ≥1 by InitializeProcThreadAttributeList. - al := &ProcThreadAttributeListContainer{data: (*ProcThreadAttributeList)(unsafe.Pointer(&make([]byte, size)[0]))} + al := &ProcThreadAttributeListContainer{data: (*ProcThreadAttributeList)(unsafe.Pointer(alloc))} err = initializeProcThreadAttributeList(al.data, maxAttrCount, 0, &size) if err != nil { return nil, err @@ -157,36 +159,17 @@ func NewProcThreadAttributeList(maxAttrCount uint32) (*ProcThreadAttributeListCo } // Update modifies the ProcThreadAttributeList using UpdateProcThreadAttribute. -// Note that the value passed to this function will be copied into memory -// allocated by LocalAlloc, the contents of which should not contain any -// Go-managed pointers, even if the passed value itself is a Go-managed -// pointer. func (al *ProcThreadAttributeListContainer) Update(attribute uintptr, value unsafe.Pointer, size uintptr) error { - alloc, err := LocalAlloc(LMEM_FIXED, uint32(size)) - if err != nil { - return err - } - var src, dst []byte - hdr := (*unsafeheader.Slice)(unsafe.Pointer(&src)) - hdr.Data = value - hdr.Cap = int(size) - hdr.Len = int(size) - hdr = (*unsafeheader.Slice)(unsafe.Pointer(&dst)) - hdr.Data = unsafe.Pointer(alloc) - hdr.Cap = int(size) - hdr.Len = int(size) - copy(dst, src) - al.heapAllocations = append(al.heapAllocations, alloc) - return updateProcThreadAttribute(al.data, 0, attribute, unsafe.Pointer(alloc), size, nil, nil) + al.pointers = append(al.pointers, value) + return updateProcThreadAttribute(al.data, 0, attribute, value, size, nil, nil) } // Delete frees ProcThreadAttributeList's resources. func (al *ProcThreadAttributeListContainer) Delete() { deleteProcThreadAttributeList(al.data) - for i := range al.heapAllocations { - LocalFree(Handle(al.heapAllocations[i])) - } - al.heapAllocations = nil + LocalFree(Handle(unsafe.Pointer(al.data))) + al.data = nil + al.pointers = nil } // List returns the actual ProcThreadAttributeList to be passed to StartupInfoEx. diff --git a/windows/syscall_windows_test.go b/windows/syscall_windows_test.go index 1c1a3eae..e4fe8463 100644 --- a/windows/syscall_windows_test.go +++ b/windows/syscall_windows_test.go @@ -5,6 +5,7 @@ package windows_test import ( + "bufio" "bytes" "debug/pe" "errors" @@ -18,6 +19,7 @@ import ( "strings" "syscall" "testing" + "time" "unsafe" "golang.org/x/sys/internal/unsafeheader" @@ -1002,3 +1004,56 @@ func TestListWireGuardDrivers(t *testing.T) { t.Logf("%s - %s", drvInfoData.Description(), drvInfoDetailData.InfFileName()) } } + +func TestProcThreadAttributeHandleList(t *testing.T) { + const sentinel = "the gopher dance" + system32, err := windows.GetSystemDirectory() + if err != nil { + t.Fatal(err) + } + executable16, err := windows.UTF16PtrFromString(filepath.Join(system32, "cmd.exe")) + if err != nil { + t.Fatal(err) + } + args16, err := windows.UTF16PtrFromString(windows.ComposeCommandLine([]string{"/c", "echo " + sentinel})) + if err != nil { + t.Fatal(err) + } + attributeList, err := windows.NewProcThreadAttributeList(1) + if err != nil { + t.Fatal(err) + } + defer attributeList.Delete() + si := &windows.StartupInfoEx{ + StartupInfo: windows.StartupInfo{Cb: uint32(unsafe.Sizeof(windows.StartupInfoEx{}))}, + ProcThreadAttributeList: attributeList.List(), + } + pipeR, pipeW, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + defer pipeR.Close() + defer pipeW.Close() + func() { + // We allocate handles in a closure to provoke a UaF in the case of attributeList.Update being buggy. + handles := []windows.Handle{windows.Handle(pipeW.Fd())} + attributeList.Update(windows.PROC_THREAD_ATTRIBUTE_HANDLE_LIST, unsafe.Pointer(&handles[0]), uintptr(len(handles))*unsafe.Sizeof(handles[0])) + si.Flags |= windows.STARTF_USESTDHANDLES + si.StdOutput = handles[0] + }() + pi := new(windows.ProcessInformation) + err = windows.CreateProcess(executable16, args16, nil, nil, true, windows.CREATE_DEFAULT_ERROR_MODE|windows.CREATE_UNICODE_ENVIRONMENT|windows.EXTENDED_STARTUPINFO_PRESENT, nil, nil, &si.StartupInfo, pi) + if err != nil { + t.Fatal(err) + } + defer windows.CloseHandle(pi.Thread) + defer windows.CloseHandle(pi.Process) + pipeR.SetReadDeadline(time.Now().Add(time.Minute)) + out, _, err := bufio.NewReader(pipeR).ReadLine() + if err != nil { + t.Fatal(err) + } + if string(out) != sentinel { + t.Fatalf("got %q; want %q", out, sentinel) + } +} diff --git a/windows/types_windows.go b/windows/types_windows.go index 73087bf5..655447e8 100644 --- a/windows/types_windows.go +++ b/windows/types_windows.go @@ -938,8 +938,8 @@ type StartupInfoEx struct { type ProcThreadAttributeList struct{} type ProcThreadAttributeListContainer struct { - data *ProcThreadAttributeList - heapAllocations []uintptr + data *ProcThreadAttributeList + pointers []unsafe.Pointer } type ProcessInformation struct {