From 6f66c7697d3bb6a259bf8d0261490cdee0ef8986 Mon Sep 17 00:00:00 2001 From: "Jason A. Donenfeld" Date: Mon, 16 Sep 2019 23:36:49 -0600 Subject: global: use SECURITY_DESCRIPTOR apis from x/sys/windows Signed-off-by: Jason A. Donenfeld --- conf/migration_windows.go | 27 +++------------- conf/zsyscall_windows.go | 27 ---------------- elevate/privileges.go | 8 ++--- elevate/shellexecute.go | 3 +- main.go | 9 ++---- manager/ipc_server.go | 4 +-- tunnel/firewall/helpers.go | 36 ++++++++++++++------- tunnel/firewall/rules.go | 3 +- tunnel/firewall/syscall_windows.go | 3 -- tunnel/firewall/types_windows.go | 44 -------------------------- tunnel/firewall/zsyscall_windows.go | 10 ------ tunnel/ipcpermissions.go | 62 +++++++++++++++++++++---------------- tunnel/winipcfg/winipcfg_test.go | 3 +- ui/managewindow.go | 4 +-- ui/ui.go | 3 +- updater/msirunner_windows.go | 5 ++- 16 files changed, 81 insertions(+), 170 deletions(-) diff --git a/conf/migration_windows.go b/conf/migration_windows.go index 4b7ffe30..d8d349f5 100644 --- a/conf/migration_windows.go +++ b/conf/migration_windows.go @@ -13,44 +13,27 @@ import ( "golang.org/x/sys/windows" ) -//sys getFileSecurity(fileName *uint16, securityInformation uint32, securityDescriptor *byte, descriptorLen uint32, requestedLen *uint32) (err error) = advapi32.GetFileSecurityW -//sys getSecurityDescriptorOwner(securityDescriptor *byte, sid **windows.SID, ownerDefaulted *bool) (err error) = advapi32.GetSecurityDescriptorOwner -const ownerSecurityInformation = 0x00000001 - func maybeMigrate(c string) { vol := filepath.VolumeName(c) withoutVol := strings.TrimPrefix(c, vol) oldRoot := filepath.Join(vol, "\\windows.old") oldC := filepath.Join(oldRoot, withoutVol) - var err error - var sd []byte - reqLen := uint32(128) - for { - sd = make([]byte, reqLen) - //XXX: Since this takes a file path, it's technically a TOCTOU. - err = getFileSecurity(windows.StringToUTF16Ptr(oldRoot), ownerSecurityInformation, &sd[0], uint32(len(sd)), &reqLen) - if err != windows.ERROR_INSUFFICIENT_BUFFER { - break - } - } + sd, err := windows.GetNamedSecurityInfo(oldRoot, windows.SE_FILE_OBJECT, windows.OWNER_SECURITY_INFORMATION) if err == windows.ERROR_PATH_NOT_FOUND || err == windows.ERROR_FILE_NOT_FOUND { return } if err != nil { - log.Printf("Not migrating configuration from ‘%s’ due to GetFileSecurity error: %v", oldRoot, err) + log.Printf("Not migrating configuration from ‘%s’ due to GetNamedSecurityInfo error: %v", oldRoot, err) return } - var defaulted bool - var sid *windows.SID - err = getSecurityDescriptorOwner(&sd[0], &sid, &defaulted) + owner, defaulted, err := sd.Owner() if err != nil { log.Printf("Not migrating configuration from ‘%s’ due to GetSecurityDescriptorOwner error: %v", oldRoot, err) return } - if defaulted || !sid.IsWellKnown(windows.WinLocalSystemSid) { - sidStr, _ := sid.String() - log.Printf("Not migrating configuration from ‘%s’, as it is not explicitly owned by SYSTEM, but rather ‘%s’", oldRoot, sidStr) + if defaulted || !owner.IsWellKnown(windows.WinLocalSystemSid) { + log.Printf("Not migrating configuration from ‘%s’, as it is not explicitly owned by SYSTEM, but rather ‘%v’", oldRoot, owner) return } err = windows.MoveFileEx(windows.StringToUTF16Ptr(oldC), windows.StringToUTF16Ptr(c), windows.MOVEFILE_COPY_ALLOWED) diff --git a/conf/zsyscall_windows.go b/conf/zsyscall_windows.go index ec63bc3d..9dcf68fe 100644 --- a/conf/zsyscall_windows.go +++ b/conf/zsyscall_windows.go @@ -38,12 +38,9 @@ func errnoErr(e syscall.Errno) error { var ( modwininet = windows.NewLazySystemDLL("wininet.dll") - modadvapi32 = windows.NewLazySystemDLL("advapi32.dll") modkernel32 = windows.NewLazySystemDLL("kernel32.dll") procInternetGetConnectedState = modwininet.NewProc("InternetGetConnectedState") - procGetFileSecurityW = modadvapi32.NewProc("GetFileSecurityW") - procGetSecurityDescriptorOwner = modadvapi32.NewProc("GetSecurityDescriptorOwner") procFindFirstChangeNotificationW = modkernel32.NewProc("FindFirstChangeNotificationW") procFindNextChangeNotification = modkernel32.NewProc("FindNextChangeNotification") ) @@ -54,30 +51,6 @@ func internetGetConnectedState(flags *uint32, reserved uint32) (connected bool) return } -func getFileSecurity(fileName *uint16, securityInformation uint32, securityDescriptor *byte, descriptorLen uint32, requestedLen *uint32) (err error) { - r1, _, e1 := syscall.Syscall6(procGetFileSecurityW.Addr(), 5, uintptr(unsafe.Pointer(fileName)), uintptr(securityInformation), uintptr(unsafe.Pointer(securityDescriptor)), uintptr(descriptorLen), uintptr(unsafe.Pointer(requestedLen)), 0) - if r1 == 0 { - if e1 != 0 { - err = errnoErr(e1) - } else { - err = syscall.EINVAL - } - } - return -} - -func getSecurityDescriptorOwner(securityDescriptor *byte, sid **windows.SID, ownerDefaulted *bool) (err error) { - r1, _, e1 := syscall.Syscall(procGetSecurityDescriptorOwner.Addr(), 3, uintptr(unsafe.Pointer(securityDescriptor)), uintptr(unsafe.Pointer(sid)), uintptr(unsafe.Pointer(ownerDefaulted))) - if r1 == 0 { - if e1 != 0 { - err = errnoErr(e1) - } else { - err = syscall.EINVAL - } - } - return -} - func findFirstChangeNotification(path *uint16, watchSubtree bool, filter uint32) (handle windows.Handle, err error) { var _p0 uint32 if watchSubtree { diff --git a/elevate/privileges.go b/elevate/privileges.go index a02d8a5d..eae0ac3e 100644 --- a/elevate/privileges.go +++ b/elevate/privileges.go @@ -14,19 +14,15 @@ import ( ) func DropAllPrivileges(retainDriverLoading bool) error { - processHandle, err := windows.GetCurrentProcess() - if err != nil { - return err - } var luid windows.LUID if retainDriverLoading { - err = windows.LookupPrivilegeValue(nil, windows.StringToUTF16Ptr("SeLoadDriverPrivilege"), &luid) + err := windows.LookupPrivilegeValue(nil, windows.StringToUTF16Ptr("SeLoadDriverPrivilege"), &luid) if err != nil { return err } } var processToken windows.Token - err = windows.OpenProcessToken(processHandle, windows.TOKEN_READ|windows.TOKEN_WRITE, &processToken) + err := windows.OpenProcessToken(windows.GetCurrentProcess(), windows.TOKEN_READ|windows.TOKEN_WRITE, &processToken) if err != nil { return err } diff --git a/elevate/shellexecute.go b/elevate/shellexecute.go index 2c4190e0..b1dcc155 100644 --- a/elevate/shellexecute.go +++ b/elevate/shellexecute.go @@ -45,7 +45,8 @@ func ShellExecute(program string, arguments string, directory string, show int32 } }() - processToken, err := windows.OpenCurrentProcessToken() + var processToken windows.Token + err = windows.OpenProcessToken(windows.GetCurrentProcess(), windows.TOKEN_QUERY|windows.TOKEN_DUPLICATE, &processToken) if err != nil { return } diff --git a/main.go b/main.go index 8d33b09f..d14b1cd5 100644 --- a/main.go +++ b/main.go @@ -57,11 +57,7 @@ func usage() { func checkForWow64() { var b bool - p, err := windows.GetCurrentProcess() - if err != nil { - fatal(err) - } - err = windows.IsWow64Process(p, &b) + err := windows.IsWow64Process(windows.GetCurrentProcess(), &b) if err != nil { fatalf("Unable to determine whether the process is running under WOW64: %v", err) } @@ -72,7 +68,8 @@ func checkForWow64() { func checkForAdminGroup() { // This is not a security check, but rather a user-confusion one. - processToken, err := windows.OpenCurrentProcessToken() + var processToken windows.Token + err := windows.OpenProcessToken(windows.GetCurrentProcess(), windows.TOKEN_QUERY|windows.TOKEN_DUPLICATE, &processToken) if err != nil { fatalf("Unable to open current process token: %v", err) } diff --git a/manager/ipc_server.go b/manager/ipc_server.go index 9b2aac43..0a3bceae 100644 --- a/manager/ipc_server.go +++ b/manager/ipc_server.go @@ -15,11 +15,11 @@ import ( "os" "sync" "sync/atomic" - "syscall" "time" "golang.org/x/sys/windows" "golang.org/x/sys/windows/svc" + "golang.zx2c4.com/wireguard/ipc/winpipe" "golang.zx2c4.com/wireguard/windows/conf" @@ -59,7 +59,7 @@ func (s *ManagerService) RuntimeConfig(tunnelName string, config *conf.Config) e if err != nil { return err } - pipe, err := winpipe.DialPipe(pipePath, nil, (*syscall.SID)(localSystem)) + pipe, err := winpipe.DialPipe(pipePath, nil, localSystem) if err != nil { return err } diff --git a/tunnel/firewall/helpers.go b/tunnel/firewall/helpers.go index abb2684c..04e5c664 100644 --- a/tunnel/firewall/helpers.go +++ b/tunnel/firewall/helpers.go @@ -71,8 +71,9 @@ func wrapErr(err error) error { return fmt.Errorf("Firewall error at %s:%d: %v", file, line, err) } -func getCurrentProcessSecurityDescriptor() (*wtFwpByteBlob, error) { - processToken, err := windows.OpenCurrentProcessToken() +func getCurrentProcessSecurityDescriptor() (*windows.SECURITY_DESCRIPTOR, error) { + var processToken windows.Token + err := windows.OpenProcessToken(windows.GetCurrentProcess(), windows.TOKEN_QUERY, &processToken) if err != nil { return nil, wrapErr(err) } @@ -99,21 +100,32 @@ func getCurrentProcessSecurityDescriptor() (*wtFwpByteBlob, error) { return nil, wrapErr(windows.ERROR_NO_SUCH_GROUP) } - access := &wtExplicitAccess{ - accessPermissions: cFWP_ACTRL_MATCH_FILTER, - accessMode: cGRANT_ACCESS, - trustee: wtTrustee{ - trusteeForm: cTRUSTEE_IS_SID, - trusteeType: cTRUSTEE_IS_GROUP, - sid: sid, + access := []windows.EXPLICIT_ACCESS{{ + AccessPermissions: cFWP_ACTRL_MATCH_FILTER, + AccessMode: windows.GRANT_ACCESS, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(sid), }, + }} + dacl, err := windows.ACLFromEntries(access, nil) + if err != nil { + return nil, wrapErr(err) + } + sd, err := windows.NewSecurityDescriptor() + if err != nil { + return nil, wrapErr(err) + } + err = sd.SetDACL(dacl, true, false) + if err != nil { + return nil, wrapErr(err) } - blob := &wtFwpByteBlob{} - err = buildSecurityDescriptor(nil, nil, 1, access, 0, nil, nil, &blob.size, &blob.data) + sd, err = sd.ToSelfRelative() if err != nil { return nil, wrapErr(err) } - return blob, nil + return sd, nil } func getCurrentProcessAppID() (*wtFwpByteBlob, error) { diff --git a/tunnel/firewall/rules.go b/tunnel/firewall/rules.go index 0ef64692..7bca508b 100644 --- a/tunnel/firewall/rules.go +++ b/tunnel/firewall/rules.go @@ -154,14 +154,13 @@ func permitWireGuardService(session uintptr, baseObjects *baseObjects, weight ui if err != nil { return wrapErr(err) } - defer windows.LocalFree(windows.Handle(unsafe.Pointer(sd.data))) conditions[1] = wtFwpmFilterCondition0{ fieldKey: cFWPM_CONDITION_ALE_USER_ID, matchType: cFWP_MATCH_EQUAL, conditionValue: wtFwpConditionValue0{ _type: cFWP_SECURITY_DESCRIPTOR_TYPE, - value: uintptr(unsafe.Pointer(sd)), + value: uintptr(unsafe.Pointer(&wtFwpByteBlob{sd.Length(), (*byte)(unsafe.Pointer(sd))})), }, } diff --git a/tunnel/firewall/syscall_windows.go b/tunnel/firewall/syscall_windows.go index ac705b41..1d2696a1 100644 --- a/tunnel/firewall/syscall_windows.go +++ b/tunnel/firewall/syscall_windows.go @@ -34,6 +34,3 @@ package firewall // https://docs.microsoft.com/en-us/windows/desktop/api/fwpmu/nf-fwpmu-fwpmprovideradd0 //sys fwpmProviderAdd0(engineHandle uintptr, provider *wtFwpmProvider0, sd uintptr) (err error) [failretval!=0] = fwpuclnt.FwpmProviderAdd0 - -// https://docs.microsoft.com/en-us/windows/desktop/api/aclapi/nf-aclapi-buildsecuritydescriptorw -//sys buildSecurityDescriptor(owner *wtTrustee, group *wtTrustee, countAccessEntries uint32, accessEntries *wtExplicitAccess, countAuditEntries uint32, auditEntries *wtExplicitAccess, oldSd **byte, sizeNewSd *uint32, newSd **byte) (ret error) = advapi32.BuildSecurityDescriptorW diff --git a/tunnel/firewall/types_windows.go b/tunnel/firewall/types_windows.go index fd313993..d61c6480 100644 --- a/tunnel/firewall/types_windows.go +++ b/tunnel/firewall/types_windows.go @@ -405,50 +405,6 @@ const ( cIPPROTO_UDP wtIPProto = 17 ) -type wtExplicitAccess struct { - accessPermissions uint32 - accessMode uint32 - inheritance uint32 - trustee wtTrustee -} - -type wtTrustee struct { - multipleTrustee *wtTrustee - multipleTrusteeOperation uint32 - trusteeForm uint32 - trusteeType uint32 - sid *windows.SID -} - -const ( - cTRUSTEE_IS_UNKNOWN = iota - cTRUSTEE_IS_USER - cTRUSTEE_IS_GROUP - cTRUSTEE_IS_DOMAIN - cTRUSTEE_IS_ALIAS - cTRUSTEE_IS_WELL_KNOWN_GROUP - cTRUSTEE_IS_DELETED - cTRUSTEE_IS_INVALID - cTRUSTEE_IS_COMPUTER -) -const ( - cTRUSTEE_IS_SID = iota - cTRUSTEE_IS_NAME - cTRUSTEE_BAD_FORM - cTRUSTEE_IS_OBJECTS_AND_SID - cTRUSTEE_IS_OBJECTS_AND_NAME -) - -const ( - cNOT_USED_ACCESS = iota - cGRANT_ACCESS - cSET_ACCESS - cDENY_ACCESS - cREVOKE_ACCESS - cSET_AUDIT_SUCCESS - cSET_AUDIT_FAILURE -) - const ( cFWP_ACTRL_MATCH_FILTER = 1 ) diff --git a/tunnel/firewall/zsyscall_windows.go b/tunnel/firewall/zsyscall_windows.go index 8e83d6e3..846d4ae8 100644 --- a/tunnel/firewall/zsyscall_windows.go +++ b/tunnel/firewall/zsyscall_windows.go @@ -38,7 +38,6 @@ func errnoErr(e syscall.Errno) error { var ( modfwpuclnt = windows.NewLazySystemDLL("fwpuclnt.dll") - modadvapi32 = windows.NewLazySystemDLL("advapi32.dll") procFwpmEngineOpen0 = modfwpuclnt.NewProc("FwpmEngineOpen0") procFwpmEngineClose0 = modfwpuclnt.NewProc("FwpmEngineClose0") @@ -50,7 +49,6 @@ var ( procFwpmTransactionCommit0 = modfwpuclnt.NewProc("FwpmTransactionCommit0") procFwpmTransactionAbort0 = modfwpuclnt.NewProc("FwpmTransactionAbort0") procFwpmProviderAdd0 = modfwpuclnt.NewProc("FwpmProviderAdd0") - procBuildSecurityDescriptorW = modadvapi32.NewProc("BuildSecurityDescriptorW") ) func fwpmEngineOpen0(serverName *uint16, authnService wtRpcCAuthN, authIdentity *uintptr, session *wtFwpmSession0, engineHandle unsafe.Pointer) (err error) { @@ -165,11 +163,3 @@ func fwpmProviderAdd0(engineHandle uintptr, provider *wtFwpmProvider0, sd uintpt } return } - -func buildSecurityDescriptor(owner *wtTrustee, group *wtTrustee, countAccessEntries uint32, accessEntries *wtExplicitAccess, countAuditEntries uint32, auditEntries *wtExplicitAccess, oldSd **byte, sizeNewSd *uint32, newSd **byte) (ret error) { - r0, _, _ := syscall.Syscall9(procBuildSecurityDescriptorW.Addr(), 9, uintptr(unsafe.Pointer(owner)), uintptr(unsafe.Pointer(group)), uintptr(countAccessEntries), uintptr(unsafe.Pointer(accessEntries)), uintptr(countAuditEntries), uintptr(unsafe.Pointer(auditEntries)), uintptr(unsafe.Pointer(oldSd)), uintptr(unsafe.Pointer(sizeNewSd)), uintptr(unsafe.Pointer(newSd))) - if r0 != 0 { - ret = syscall.Errno(r0) - } - return -} diff --git a/tunnel/ipcpermissions.go b/tunnel/ipcpermissions.go index 20af4257..613d0283 100644 --- a/tunnel/ipcpermissions.go +++ b/tunnel/ipcpermissions.go @@ -6,10 +6,8 @@ package tunnel import ( - "fmt" - "unsafe" - "golang.org/x/sys/windows" + "golang.zx2c4.com/wireguard/ipc" "golang.zx2c4.com/wireguard/windows/conf" @@ -19,37 +17,47 @@ func CopyConfigOwnerToIPCSecurityDescriptor(filename string) error { if conf.PathIsEncrypted(filename) { return nil } - handle, err := windows.CreateFile(windows.StringToUTF16Ptr(filename), windows.STANDARD_RIGHTS_READ, windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE, nil, windows.OPEN_EXISTING, 0, 0) + + fileSd, err := windows.GetNamedSecurityInfo(filename, windows.SE_FILE_OBJECT, windows.OWNER_SECURITY_INFORMATION) if err != nil { return err } - defer windows.CloseHandle(handle) - var sid *windows.SID - var sd windows.Handle - //TODO: Move into x/sys/windows - const SE_FILE_OBJECT = 1 - const OWNER_SECURITY_INFORMATION = 1 - r, _, _ := windows.NewLazySystemDLL("advapi32.dll").NewProc("GetSecurityInfo").Call( - uintptr(handle), - SE_FILE_OBJECT, - OWNER_SECURITY_INFORMATION, - uintptr(unsafe.Pointer(&sid)), - 0, - 0, - 0, - uintptr(unsafe.Pointer(&sd)), - ) - if r != uintptr(windows.ERROR_SUCCESS) { - return windows.Errno(r) - } - defer windows.LocalFree(sd) - if sid.IsWellKnown(windows.WinLocalSystemSid) { + fileOwner, _, err := fileSd.Owner() + if err != nil { + return err + } + if fileOwner.IsWellKnown(windows.WinLocalSystemSid) { return nil } - sidString, err := sid.String() + additionalEntries := []windows.EXPLICIT_ACCESS{{ + AccessPermissions: windows.GENERIC_ALL, + AccessMode: windows.GRANT_ACCESS, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_USER, + TrusteeValue: windows.TrusteeValueFromSID(fileOwner), + }, + }} + + sd, err := ipc.UAPISecurityDescriptor.ToAbsolute() + if err != nil { + return err + } + dacl, defaulted, _ := sd.DACL() + + newDacl, err := windows.ACLFromEntries(additionalEntries, dacl) + if err != nil { + return err + } + err = sd.SetDACL(newDacl, true, defaulted) if err != nil { return err } - ipc.UAPISecurityDescriptor += fmt.Sprintf("(A;;GA;;;%s)", sidString) + sd, err = sd.ToSelfRelative() + if err != nil { + return err + } + ipc.UAPISecurityDescriptor = sd + return nil } diff --git a/tunnel/winipcfg/winipcfg_test.go b/tunnel/winipcfg/winipcfg_test.go index 36ef2849..2b0c5ff5 100644 --- a/tunnel/winipcfg/winipcfg_test.go +++ b/tunnel/winipcfg/winipcfg_test.go @@ -56,7 +56,8 @@ var ( ) func runningElevated() bool { - process, err := windows.OpenCurrentProcessToken() + var process windows.Token + err := windows.OpenProcessToken(windows.GetCurrentProcess(), windows.TOKEN_QUERY, &process) if err != nil { return false } diff --git a/ui/managewindow.go b/ui/managewindow.go index 8bf6422c..6ba49e65 100644 --- a/ui/managewindow.go +++ b/ui/managewindow.go @@ -40,8 +40,8 @@ var initedManageTunnels sync.Once func NewManageTunnelsWindow() (*ManageTunnelsWindow, error) { initedManageTunnels.Do(func() { walk.AppendToWalkInit(func() { - walk.MustRegisterWindowClass(manageWindowWindowClass) - taskbarButtonCreatedMsg = win.RegisterWindowMessage(windows.StringToUTF16Ptr("TaskbarButtonCreated")) + walk.MustRegisterWindowClass(manageWindowWindowClass) + taskbarButtonCreatedMsg = win.RegisterWindowMessage(windows.StringToUTF16Ptr("TaskbarButtonCreated")) }) }) diff --git a/ui/ui.go b/ui/ui.go index 42b351e8..179b0d38 100644 --- a/ui/ui.go +++ b/ui/ui.go @@ -25,8 +25,7 @@ var startTime = time.Now() func RunUI() { runtime.LockOSThread() - thisProcess, _ := windows.GetCurrentProcess() - windows.SetProcessPriorityBoost(thisProcess, false) + windows.SetProcessPriorityBoost(windows.GetCurrentProcess(), false) defer func() { if err := recover(); err != nil { showErrorCustom(nil, "Panic", fmt.Sprint(err, "\n\n", string(debug.Stack()))) diff --git a/updater/msirunner_windows.go b/updater/msirunner_windows.go index 6b8ae476..c11d4db9 100644 --- a/updater/msirunner_windows.go +++ b/updater/msirunner_windows.go @@ -17,7 +17,6 @@ import ( "unsafe" "golang.org/x/sys/windows" - "golang.zx2c4.com/wireguard/ipc/winpipe" ) func runMsi(msiPath string, userToken uintptr) error { @@ -61,13 +60,13 @@ func msiTempFile() (*os.File, error) { if n != int(len(randBytes)) { return nil, errors.New("Unable to generate random bytes") } - sd, err := winpipe.SddlToSecurityDescriptor("O:SYD:PAI(A;;FA;;;SY)(A;;FR;;;BA)") + sd, err := windows.SecurityDescriptorFromString("O:SYD:PAI(A;;FA;;;SY)(A;;FR;;;BA)") if err != nil { return nil, err } sa := &windows.SecurityAttributes{ Length: uint32(unsafe.Sizeof(windows.SecurityAttributes{})), - SecurityDescriptor: uintptr(unsafe.Pointer(&sd[0])), + SecurityDescriptor: sd, } // TODO: os.TempDir() returns C:\windows\temp when calling from this context. Supposedly this is mostly secure // against TOCTOU, but who knows! Look into this! -- cgit v1.2.3-59-g8ed1b