ipn/ipnserver, util/winutil: update workaround for os/user.LookupId f…

…ailures 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 #869

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

ipn/ipnserver/server.go

@@ -47,6 +47,7 @@ import (
 	"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

@@ -87,3 +87,23 @@ 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
+	}
+}

util/winutil/winutil_notwindows.go

@@ -22,3 +22,13 @@ func GetRegString(name, defval string) string { return defval }
 // 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 }
+
+// IsSIDValidPrincipal determines whether the SID contained in uid represents a
+// type that is a valid security principal under Windows. This check helps us
+// work around a bug in the standard library's Windows implementation of
+// LookupId in os/user.
+// See https://github.com/tailscale/tailscale/issues/869
+//
+// This function will only work on GOOS=windows. Trying to run it on any other
+// OS will always return false.
+func IsSIDValidPrincipal(uid string) bool { return false }