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 {