service: local system's token is a bit more locked down than elevated

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

2 files changed, 7 insertions, 6 deletions

diff --git a/attacksurface.md b/attacksurface.md
index 49fffa2c..4a4ab37a 100644
--- a/attacksurface.md
+++ b/attacksurface.md
@@ -29,15 +29,15 @@ The manager service is a userspace service running as Local System, responsible
   - A writable `CreateFileMapping` handle to a binary ringlog shared by all services, inherited by the unprivileged UI process. It's unclear if this brings with it surprising hidden attack surface in the mm system.
   - It listens for service changes in tunnel services according to the string prefix "WireGuardTunnel$".
   - It manages DPAPI-encrypted configuration files in Local System's local appdata directory, and makes some effort to enforce good configuration filenames.
-  - It uses `wtsEnumerateSessions` and `WTSSESSION_NOTIFICATION` to walk through each available session. It then uses `wtfQueryUserToken`, and then calls `GetTokenInformation(TokenGroups)` on it. If one of the returned group's SIDs matches `CreateWellKnownSid(WinBuiltinAdministratorsSid)`, then it spawns the unprivileged UI process as that user token, passing it three unnamed pipe handles for IPC and the log mapping handle, as descried above.
+  - It uses `wtsEnumerateSessions` and `WTSSESSION_NOTIFICATION` to walk through each available session. It then uses `wtfQueryUserToken`, and then calls `GetTokenInformation(TokenGroups)` on it. If one of the returned group's SIDs matches `CreateWellKnownSid(WinBuiltinAdministratorsSid)`, and has attributes of either `SE_GROUP_ENABLED` or `SE_GROUP_USE_FOR_DENY_ONLY` and calling `GetTokenInformation(TokenElevation)` on it or its `TokenLinkedToken` indicates that either is elevated, then it spawns the unprivileged UI process as that (unelevated) user token, passing it three unnamed pipe handles for IPC and the log mapping handle, as descried above.
 
 ### UI
 
 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:
 
-  - <s>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.</s> This is currently disabled, as it is not high integrity and also seems to cause a few problems for the shell / notification mechanism. **This must be revisited.**
-  - 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.
+  - The manager service (above) calls `GetSecurityInfo(ATTRIBUTE_SECURITY_INFORMATION|LABEL_SECURITY_INFORMATION|SCOPE_SECURITY_INFORMATION|OWNER_SECURITY_INFORMATION|GROUP_SECURITY_INFORMATION|DACL_SECURITY_INFORMATION)` on itself. Then it examines all of the groups associated with the users token, and finds the first one that is of type `SE_GROUP_LOGON_ID`. It adds to the prior DACL one for this SID for only `PROCESS_QUERY_LIMITED_INFORMATION` permission. Then it 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.
+  - Right now, we're removing the SACL/integrity level from the above, which means the UI process only requires medium integrity to access 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. On the other hand, the permissions of the above are fairly strict (`O:BAG:SYD:(A;;0x1fffff;;;SY)(A;;0x121411;;;BA)(A;;0x1000;;;S-logonsid)`).
   - 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/service_manager.go b/service/service_manager.go
index 2862c729..722f954d 100644
--- a/service/service_manager.go
+++ b/service/service_manager.go
@@ -118,9 +118,10 @@ func (service *managerService) Execute(args []string, r <-chan svc.ChangeRequest
 			log.Printf("Unable to determine elevated environment: %v", err)
 			return
 		}
-		securityAttributes, err := getSecurityAttributes(userTokenInfo.elevatedToken, userToken)
+		currentProcess, _ := windows.GetCurrentProcess()
+		securityAttributes, err := getSecurityAttributes(windows.Token(currentProcess), userToken)
 		if err != nil {
-			log.Printf("Unable to extract security attributes from elevated token and combine them with SID from user token: %v", err)
+			log.Printf("Unable to extract security attributes from manager token and combine them with SID from user token: %v", err)
 			return
 		}
 		for {