diff --git a/ipn/ipnserver/server.go b/ipn/ipnserver/server.go index 273500552..5e93ddd7b 100644 --- a/ipn/ipnserver/server.go +++ b/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. diff --git a/util/winutil/winutil.go b/util/winutil/winutil.go index 81b3623a6..ebf053173 100644 --- a/util/winutil/winutil.go +++ b/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 -// +build windows - -// Package winuntil contains misc Windows/win32 helper functions. +// Package winutil contains misc Windows/Win32 helper functions. 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 -} +// RegBase is the registry path inside HKEY_LOCAL_MACHINE where registry settings +// are stored. This constant is a non-empty string only when GOOS=windows. +const RegBase = regBase // 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) - 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 + return getRegString(name, defval) } // 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) - 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 + return getRegInteger(name, defval) } -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) +// 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 isSIDValidPrincipal(uid) } diff --git a/util/winutil/winutil_notwindows.go b/util/winutil/winutil_notwindows.go index 49a46e4ec..3011eebc3 100644 --- a/util/winutil/winutil_notwindows.go +++ b/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 -// 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 } +func getRegString(name, defval string) string { return defval } -// GetRegInteger looks up a registry path in our local machine path, or returns -// 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 GetRegInteger(name string, defval uint64) uint64 { return defval } +func getRegInteger(name string, defval uint64) uint64 { return defval } + +func isSIDValidPrincipal(uid string) bool { return false } diff --git a/util/winutil/winutil_windows.go b/util/winutil/winutil_windows.go new file mode 100644 index 000000000..447d0e926 --- /dev/null +++ b/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 + } +}