service: set security attributes on new process

4 files changed, 63 insertions, 10 deletions

diff --git a/attacksurface.md b/attacksurface.md
index e655e9fc..f8b16bf1 100644
--- a/attacksurface.md
+++ b/attacksurface.md
@@ -35,7 +35,9 @@ The manager service is a userspace service running as Local System, responsible
 
 The UI is an unprivileged process running as the ordinary user for each user who is in the Administrators group (per the above). It exposes:
 
-  - There currently are no anti-debugging mechanisms, which means any process that can, debug, introspect, inject, send messages, or interact with the UI, may be able to take control of the above handles passed to it by the manager service. In theory that shouldn't be too horrible for the security model, but rather than risk it, we might consider importing all of the insane things VirtualBox does in this domain. Alternatively, since the anti-debugging stuff is pretty ugly and probably doesn't even work properly everywhere, we may be better off with starting the process at some heightened security level, such that only other processes at that level can introspect; however, we'll need to take special care that doing so doesn't give the UI process any special accesses it wouldn't otherwise have.
+  - The manager service (above) calls `GetSecurityInfo(DACL_SECURITY_INFORMATION)` on itself, and passes the returned security attributes as the UI's process and thread attributes in the call to `CreateProcessAsUser`. This means that debugging the UI process (in order for another process to steal its handles and exfiltrate private keys) is only possible by processes that can debug the manager service.
+  - We do not add `ATTRIBUTE_SECURITY_INFORMATION` into the requested information of `GetSecurityInfo`, which means the UI process only requires medium integrity to run it. Requiring greater integrity level access prevents the process from running properly, unfortunately (`ERROR_PRIVILEGE_NOT_HELD`). It would be nice to require high integrity to open the process, without having to increase the privileges that the process has in its own token.
+  - Perhaps due to the above and other reasons, it appears that it is possible for other processes to write into the UI process's message loop, which is bad, and might defeat the purpose of the above.
   - It renders highlighted config files to a msftedit.dll control, which typically is capable of all sorts of OLE and RTF nastiness that we make some attempt to avoid.
 
 ### Updates
diff --git a/service/errors.go b/service/errors.go
index d3f02f54..e456818b 100644
--- a/service/errors.go
+++ b/service/errors.go
@@ -26,6 +26,7 @@ const (
 	ErrorSetNetConfig
 	ErrorDetermineExecutablePath
 	ErrorFindAdministratorsSID
+	ErrorCreateSecurityDescriptor
 	ErrorOpenNULFile
 	ErrorTrackTunnels
 	ErrorEnumerateSessions
@@ -58,6 +59,8 @@ func (e Error) Error() string {
 		return "Unable to set interface addresses, routes, dns, and/or adapter settings"
 	case ErrorFindAdministratorsSID:
 		return "Unable to find Administrators SID"
+	case ErrorCreateSecurityDescriptor:
+		return "Unable to determine security descriptor"
 	case ErrorOpenNULFile:
 		return "Unable to open NUL file"
 	case ErrorTrackTunnels:
diff --git a/service/service_manager.go b/service/service_manager.go
index 4d287c6f..9596671b 100644
--- a/service/service_manager.go
+++ b/service/service_manager.go
@@ -56,6 +56,15 @@ type wellKnownSidType uint32
 //sys wtsEnumerateSessions(handle windows.Handle, reserved uint32, version uint32, sessions **wtsSessionInfo, count *uint32) (err error) = wtsapi32.WTSEnumerateSessionsW
 //sys wtsFreeMemory(ptr uintptr) = wtsapi32.WTSFreeMemory
 
+const (
+	SE_KERNEL_OBJECT               = 6
+	DACL_SECURITY_INFORMATION      = 4
+	ATTRIBUTE_SECURITY_INFORMATION = 16
+)
+
+//sys getSecurityInfo(handle windows.Handle, objectType uint32, si uint32, sidOwner *windows.SID, sidGroup *windows.SID, dacl *uintptr, sacl *uintptr, securityDescriptor *uintptr) (err error) [failretval!=0] = advapi32.GetSecurityInfo
+//sys getSecurityDescriptorLength(securityDescriptor uintptr) (len uint32) = advapi32.GetSecurityDescriptorLength
+
 //sys createEnvironmentBlock(block *uintptr, token windows.Token, inheritExisting bool) (err error) = userenv.CreateEnvironmentBlock
 //sys destroyEnvironmentBlock(block uintptr) (err error) = userenv.DestroyEnvironmentBlock
 
@@ -125,6 +134,23 @@ func (service *managerService) Execute(args []string, r <-chan svc.ChangeRequest
 		return
 	}
 
+	currentProcess, err := windows.GetCurrentProcess()
+	if err != nil {
+		panic(err)
+	}
+	var securityAttributes syscall.SecurityAttributes
+	err = getSecurityInfo(currentProcess, SE_KERNEL_OBJECT, DACL_SECURITY_INFORMATION, nil, nil, nil, nil, &securityAttributes.SecurityDescriptor)
+	if err != nil {
+		serviceError = ErrorCreateSecurityDescriptor
+		return
+	}
+	defer windows.LocalFree(windows.Handle(securityAttributes.SecurityDescriptor))
+	securityAttributes.Length = getSecurityDescriptorLength(securityAttributes.SecurityDescriptor)
+	if securityAttributes.Length == 0 {
+		serviceError = ErrorCreateSecurityDescriptor
+		return
+	}
+
 	devNull, err := os.OpenFile(os.DevNull, os.O_RDWR, 0)
 	if err != nil {
 		serviceError = ErrorOpenNULFile
@@ -216,7 +242,9 @@ func (service *managerService) Execute(args []string, r <-chan svc.ChangeRequest
 			log.Printf("Starting UI process for user: '%s@%s'", username, domain)
 			attr := &os.ProcAttr{
 				Sys: &syscall.SysProcAttr{
-					Token: syscall.Token(userToken),
+					Token:             syscall.Token(userToken),
+					ProcessAttributes: &securityAttributes,
+					ThreadAttributes:  &securityAttributes,
 				},
 				Files: []*os.File{devNull, devNull, devNull},
 				Env:   env,
diff --git a/service/zsyscall_windows.go b/service/zsyscall_windows.go
index 40bac8ff..caa7c91d 100644
--- a/service/zsyscall_windows.go
+++ b/service/zsyscall_windows.go
@@ -38,17 +38,19 @@ func errnoErr(e syscall.Errno) error {
 
 var (
 	modwtsapi32 = windows.NewLazySystemDLL("wtsapi32.dll")
-	moduserenv  = windows.NewLazySystemDLL("userenv.dll")
 	modadvapi32 = windows.NewLazySystemDLL("advapi32.dll")
+	moduserenv  = windows.NewLazySystemDLL("userenv.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")
+	procWTSQueryUserToken           = modwtsapi32.NewProc("WTSQueryUserToken")
+	procWTSEnumerateSessionsW       = modwtsapi32.NewProc("WTSEnumerateSessionsW")
+	procWTSFreeMemory               = modwtsapi32.NewProc("WTSFreeMemory")
+	procGetSecurityInfo             = modadvapi32.NewProc("GetSecurityInfo")
+	procGetSecurityDescriptorLength = modadvapi32.NewProc("GetSecurityDescriptorLength")
+	procCreateEnvironmentBlock      = moduserenv.NewProc("CreateEnvironmentBlock")
+	procDestroyEnvironmentBlock     = moduserenv.NewProc("DestroyEnvironmentBlock")
+	procNotifyServiceStatusChangeW  = modadvapi32.NewProc("NotifyServiceStatusChangeW")
+	procSleepEx                     = modkernel32.NewProc("SleepEx")
 )
 
 func wtfQueryUserToken(session uint32, token *windows.Token) (err error) {
@@ -80,6 +82,24 @@ func wtsFreeMemory(ptr uintptr) {
 	return
 }
 
+func getSecurityInfo(handle windows.Handle, objectType uint32, si uint32, sidOwner *windows.SID, sidGroup *windows.SID, dacl *uintptr, sacl *uintptr, securityDescriptor *uintptr) (err error) {
+	r1, _, e1 := syscall.Syscall9(procGetSecurityInfo.Addr(), 8, uintptr(handle), uintptr(objectType), uintptr(si), uintptr(unsafe.Pointer(sidOwner)), uintptr(unsafe.Pointer(sidGroup)), uintptr(unsafe.Pointer(dacl)), uintptr(unsafe.Pointer(sacl)), uintptr(unsafe.Pointer(securityDescriptor)), 0)
+	if r1 != 0 {
+		if e1 != 0 {
+			err = errnoErr(e1)
+		} else {
+			err = syscall.EINVAL
+		}
+	}
+	return
+}
+
+func getSecurityDescriptorLength(securityDescriptor uintptr) (len uint32) {
+	r0, _, _ := syscall.Syscall(procGetSecurityDescriptorLength.Addr(), 1, uintptr(securityDescriptor), 0, 0)
+	len = uint32(r0)
+	return
+}
+
 func createEnvironmentBlock(block *uintptr, token windows.Token, inheritExisting bool) (err error) {
 	var _p0 uint32
 	if inheritExisting {