ipn/ipnserver, util/winutil: update workaround for os/user.LookupId failures on Windows to reject SIDs from deleted/invalid security principals.

Our current workaround made the user check too lax, thus allowing deleted
users. This patch adds a helper function to winutil that checks that the
uid's SID represents a valid Windows security principal.

Now if `lookupUserFromID` determines that the SID is invalid, we simply
propagate the error.

Updates https://github.com/tailscale/tailscale/issues/869

Signed-off-by: Aaron Klotz <aaron@tailscale.com>

ipn/ipnserver/server.go

@@ -47,6 +47,7 @@
 	"tailscale.com/util/groupmember"
 	"tailscale.com/util/pidowner"
 	"tailscale.com/util/systemd"
+	"tailscale.com/util/winutil"
 	"tailscale.com/version"
 	"tailscale.com/version/distro"
 	"tailscale.com/wgengine"
@@ -182,6 +183,13 @@ func (s *Server) getConnIdentity(c net.Conn) (ci connIdentity, err error) {
 func lookupUserFromID(logf logger.Logf, uid string) (*user.User, error) {
 	u, err := user.LookupId(uid)
 	if err != nil && runtime.GOOS == "windows" && errors.Is(err, syscall.Errno(0x534)) {
+		// The below workaround is only applicable when uid represents a
+		// valid security principal. Omitting this check causes us to succeed
+		// even when uid represents a deleted user.
+		if !winutil.IsSIDValidPrincipal(uid) {
+			return nil, err
+		}
+
 		logf("[warning] issue 869: os/user.LookupId failed; ignoring")
 		// Work around https://github.com/tailscale/tailscale/issues/869 for
 		// now. We don't strictly need the username. It's just a nice-to-have.

util/winutil/winutil.go

@@ -2,33 +2,12 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-//go:build windows
+// Package winutil contains misc Windows/Win32 helper functions.
-// +build windows
-
-// Package winuntil contains misc Windows/win32 helper functions.
 package winutil
 
-import (
+// RegBase is the registry path inside HKEY_LOCAL_MACHINE where registry settings
-	"log"
+// are stored. This constant is a non-empty string only when GOOS=windows.
-	"syscall"
+const RegBase = regBase
-
-	"golang.org/x/sys/windows"
-	"golang.org/x/sys/windows/registry"
-)
-
-const RegBase = `SOFTWARE\Tailscale IPN`
-
-// GetDesktopPID searches the PID of the process that's running the
-// currently active desktop and whether it was found.
-// Usually the PID will be for explorer.exe.
-func GetDesktopPID() (pid uint32, ok bool) {
-	hwnd := windows.GetShellWindow()
-	if hwnd == 0 {
-		return 0, false
-	}
-	windows.GetWindowThreadProcessId(hwnd, &pid)
-	return pid, pid != 0
-}
 
 // GetRegString looks up a registry path in our local machine path, or returns
 // the given default if it can't.
@@ -36,21 +15,7 @@ func GetDesktopPID() (pid uint32, ok bool) {
 // This function will only work on GOOS=windows. Trying to run it on any other
 // OS will always return the default value.
 func GetRegString(name, defval string) string {
-	key, err := registry.OpenKey(registry.LOCAL_MACHINE, RegBase, registry.READ)
+	return getRegString(name, defval)
-	if err != nil {
-		log.Printf("registry.OpenKey(%v): %v", RegBase, err)
-		return defval
-	}
-	defer key.Close()
-
-	val, _, err := key.GetStringValue(name)
-	if err != nil {
-		if err != registry.ErrNotExist {
-			log.Printf("registry.GetStringValue(%v): %v", name, err)
-		}
-		return defval
-	}
-	return val
 }
 
 // GetRegInteger looks up a registry path in our local machine path, or returns
@@ -59,31 +24,17 @@ func GetRegString(name, defval string) string {
 // This function will only work on GOOS=windows. Trying to run it on any other
 // OS will always return the default value.
 func GetRegInteger(name string, defval uint64) uint64 {
-	key, err := registry.OpenKey(registry.LOCAL_MACHINE, RegBase, registry.READ)
+	return getRegInteger(name, defval)
-	if err != nil {
-		log.Printf("registry.OpenKey(%v): %v", RegBase, err)
-		return defval
-	}
-	defer key.Close()
-
-	val, _, err := key.GetIntegerValue(name)
-	if err != nil {
-		if err != registry.ErrNotExist {
-			log.Printf("registry.GetIntegerValue(%v): %v", name, err)
-		}
-		return defval
-	}
-	return val
 }
 
-var (
+// IsSIDValidPrincipal determines whether the SID contained in uid represents a
-	kernel32                         = syscall.NewLazyDLL("kernel32.dll")
+// type that is a valid security principal under Windows. This check helps us
-	procWTSGetActiveConsoleSessionId = kernel32.NewProc("WTSGetActiveConsoleSessionId")
+// work around a bug in the standard library's Windows implementation of
-)
+// LookupId in os/user.
-
+// See https://github.com/tailscale/tailscale/issues/869
-// TODO(crawshaw): replace with x/sys/windows... one day.
+//
-// https://go-review.googlesource.com/c/sys/+/331909
+// This function will only work on GOOS=windows. Trying to run it on any other
-func WTSGetActiveConsoleSessionId() uint32 {
+// OS will always return false.
-	r1, _, _ := procWTSGetActiveConsoleSessionId.Call()
+func IsSIDValidPrincipal(uid string) bool {
-	return uint32(r1)
+	return isSIDValidPrincipal(uid)
 }

util/winutil/winutil_notwindows.go

@@ -2,23 +2,12 @@
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
-//go:build !windows
-// +build !windows
-
 package winutil
 
-const RegBase = ``
+const regBase = ``
 
-// GetRegString looks up a registry path in our local machine path, or returns
+func getRegString(name, defval string) string { return defval }
-// the given default if it can't.
-//
-// This function will only work on GOOS=windows. Trying to run it on any other
-// OS will always return the default value.
-func GetRegString(name, defval string) string { return defval }
 
-// GetRegInteger looks up a registry path in our local machine path, or returns
+func getRegInteger(name string, defval uint64) uint64 { return defval }
-// the given default if it can't.
+
-//
+func isSIDValidPrincipal(uid string) bool { return false }
-// This function will only work on GOOS=windows. Trying to run it on any other
-// OS will always return the default value.
-func GetRegInteger(name string, defval uint64) uint64 { return defval }

util/winutil/winutil_windows.go

@@ -0,0 +1,95 @@
+// Copyright (c) 2021 Tailscale Inc & AUTHORS All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package winutil
+
+import (
+	"log"
+	"syscall"
+
+	"golang.org/x/sys/windows"
+	"golang.org/x/sys/windows/registry"
+)
+
+const regBase = `SOFTWARE\Tailscale IPN`
+
+// GetDesktopPID searches the PID of the process that's running the
+// currently active desktop and whether it was found.
+// Usually the PID will be for explorer.exe.
+func GetDesktopPID() (pid uint32, ok bool) {
+	hwnd := windows.GetShellWindow()
+	if hwnd == 0 {
+		return 0, false
+	}
+	windows.GetWindowThreadProcessId(hwnd, &pid)
+	return pid, pid != 0
+}
+
+func getRegString(name, defval string) string {
+	key, err := registry.OpenKey(registry.LOCAL_MACHINE, RegBase, registry.READ)
+	if err != nil {
+		log.Printf("registry.OpenKey(%v): %v", RegBase, err)
+		return defval
+	}
+	defer key.Close()
+
+	val, _, err := key.GetStringValue(name)
+	if err != nil {
+		if err != registry.ErrNotExist {
+			log.Printf("registry.GetStringValue(%v): %v", name, err)
+		}
+		return defval
+	}
+	return val
+}
+
+func getRegInteger(name string, defval uint64) uint64 {
+	key, err := registry.OpenKey(registry.LOCAL_MACHINE, RegBase, registry.READ)
+	if err != nil {
+		log.Printf("registry.OpenKey(%v): %v", RegBase, err)
+		return defval
+	}
+	defer key.Close()
+
+	val, _, err := key.GetIntegerValue(name)
+	if err != nil {
+		if err != registry.ErrNotExist {
+			log.Printf("registry.GetIntegerValue(%v): %v", name, err)
+		}
+		return defval
+	}
+	return val
+}
+
+var (
+	kernel32                         = syscall.NewLazyDLL("kernel32.dll")
+	procWTSGetActiveConsoleSessionId = kernel32.NewProc("WTSGetActiveConsoleSessionId")
+)
+
+// TODO(crawshaw): replace with x/sys/windows... one day.
+// https://go-review.googlesource.com/c/sys/+/331909
+func WTSGetActiveConsoleSessionId() uint32 {
+	r1, _, _ := procWTSGetActiveConsoleSessionId.Call()
+	return uint32(r1)
+}
+
+func isSIDValidPrincipal(uid string) bool {
+	usid, err := syscall.StringToSid(uid)
+	if err != nil {
+		return false
+	}
+
+	_, _, accType, err := usid.LookupAccount("")
+	if err != nil {
+		return false
+	}
+
+	switch accType {
+	case syscall.SidTypeUser, syscall.SidTypeGroup, syscall.SidTypeDomain, syscall.SidTypeAlias, syscall.SidTypeWellKnownGroup, syscall.SidTypeComputer:
+		return true
+	default:
+		// Reject deleted users, invalid SIDs, unknown SIDs, mandatory label SIDs, etc.
+		return false
+	}
+}