From 9ebb5d4205d50d54c1874b404b0a7d83cdaa77ca Mon Sep 17 00:00:00 2001 From: Aaron Klotz Date: Tue, 21 Sep 2021 15:00:30 -0600 Subject: [PATCH] ipn, paths: ensure that the state directory for Windows has the correct perms ProgramData has a permissive ACL. For us to safely store machine-wide state information, we must set a more restrictive ACL on our state directory. We set the ACL so that only talescaled's user (ie, LocalSystem) and the Administrators group may access our directory. We must include Administrators to ensure that logs continue to be easily accessible; omitting that group would force users to use special tools to log in interactively as LocalSystem, which is not ideal. (Note that the ACL we apply matches the ACL that was used for LocalSystem's AppData\Local). There are two cases where we need to reset perms: One is during migration from the old location to the new. The second case is for clean installations where we are creating the file store for the first time. Updates #2856 Signed-off-by: Aaron Klotz --- cmd/tailscale/depaware.txt | 2 +- cmd/tailscaled/depaware.txt | 2 +- ipn/store.go | 5 +- paths/migrate.go | 6 +- paths/paths.go | 11 +++ paths/paths_unix.go | 8 ++ paths/paths_windows.go | 145 ++++++++++++++++++++++++++++++++++++ 7 files changed, 175 insertions(+), 4 deletions(-) create mode 100644 paths/paths_windows.go diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index 643040420..fc2a1d061 100644 --- a/cmd/tailscale/depaware.txt +++ b/cmd/tailscale/depaware.txt @@ -44,7 +44,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep tailscale.com/net/tlsdial from tailscale.com/derp/derphttp tailscale.com/net/tsaddr from tailscale.com/net/interfaces+ 💣 tailscale.com/net/tshttpproxy from tailscale.com/derp/derphttp+ - tailscale.com/paths from tailscale.com/cmd/tailscale/cli+ + 💣 tailscale.com/paths from tailscale.com/cmd/tailscale/cli+ tailscale.com/safesocket from tailscale.com/cmd/tailscale/cli+ 💣 tailscale.com/syncs from tailscale.com/net/interfaces+ tailscale.com/tailcfg from tailscale.com/cmd/tailscale/cli+ diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 6e3ed913f..9740a3c9a 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -131,7 +131,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de tailscale.com/net/tsaddr from tailscale.com/ipn/ipnlocal+ 💣 tailscale.com/net/tshttpproxy from tailscale.com/control/controlclient+ tailscale.com/net/tstun from tailscale.com/cmd/tailscaled+ - tailscale.com/paths from tailscale.com/cmd/tailscaled+ + 💣 tailscale.com/paths from tailscale.com/cmd/tailscaled+ tailscale.com/portlist from tailscale.com/ipn/ipnlocal tailscale.com/safesocket from tailscale.com/ipn/ipnserver+ tailscale.com/smallzstd from tailscale.com/ipn/ipnserver+ diff --git a/ipn/store.go b/ipn/store.go index 58f6bbc38..c4f0d2c6d 100644 --- a/ipn/store.go +++ b/ipn/store.go @@ -19,6 +19,7 @@ import ( "tailscale.com/atomicfile" "tailscale.com/kube" + "tailscale.com/paths" ) // ErrStateNotExist is returned by StateStore.ReadState when the @@ -182,7 +183,9 @@ func NewFileStore(path string) (*FileStore, error) { if os.IsNotExist(err) { // Write out an initial file, to verify that we can write // to the path. - os.MkdirAll(filepath.Dir(path), 0755) // best effort + if err := paths.MkStateDir(filepath.Dir(path)); err != nil { + return nil, fmt.Errorf("creating state directory: %w", err) + } if err = atomicfile.WriteFile(path, []byte("{}"), 0600); err != nil { return nil, err } diff --git a/paths/migrate.go b/paths/migrate.go index 7d1c74aae..8e4f3d595 100644 --- a/paths/migrate.go +++ b/paths/migrate.go @@ -35,7 +35,11 @@ func TryConfigFileMigration(logf logger.Logf, oldFile, newFile string) string { return newFile } - os.MkdirAll(filepath.Dir(newFile), 0700) + if err = MkStateDir(filepath.Dir(newFile)); err != nil { + logf("TryConfigFileMigration failed; MkStateDir: %v", err) + return oldFile + } + err = os.WriteFile(newFile, contents, 0600) if err != nil { removeErr := os.Remove(newFile) diff --git a/paths/paths.go b/paths/paths.go index 1215ebb2f..c1f567206 100644 --- a/paths/paths.go +++ b/paths/paths.go @@ -59,3 +59,14 @@ func DefaultTailscaledStateFile() string { } return "" } + +// MkStateDir ensures that dirPath, the daemon's configurtaion directory +// containing machine keys etc, both exists and has the correct permissions. +// We want it to only be accessible to the user the daemon is running under. +func MkStateDir(dirPath string) error { + if err := os.MkdirAll(dirPath, 0700); err != nil { + return err + } + + return ensureStateDirPerms(dirPath) +} diff --git a/paths/paths_unix.go b/paths/paths_unix.go index fb11437c2..0bc413ad1 100644 --- a/paths/paths_unix.go +++ b/paths/paths_unix.go @@ -61,3 +61,11 @@ func xdgDataHome() string { } return filepath.Join(os.Getenv("HOME"), ".local/share") } + +func ensureStateDirPerms(dirPath string) error { + // Unfortunately there are currently numerous tests that set up state files + // right off of /tmp, on which Chmod will of course fail. We should fix our + // test harnesses to not do that, at which point we can return an error. + os.Chmod(dirPath, 0700) + return nil +} diff --git a/paths/paths_windows.go b/paths/paths_windows.go new file mode 100644 index 000000000..75d49100a --- /dev/null +++ b/paths/paths_windows.go @@ -0,0 +1,145 @@ +// 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 paths + +import ( + "os" + "unsafe" + + "golang.org/x/sys/windows" +) + +func getTokenInfo(token windows.Token, infoClass uint32) ([]byte, error) { + var desiredLen uint32 + err := windows.GetTokenInformation(token, infoClass, nil, 0, &desiredLen) + if err != nil && err != windows.ERROR_INSUFFICIENT_BUFFER { + return nil, err + } + + buf := make([]byte, desiredLen) + actualLen := desiredLen + err = windows.GetTokenInformation(token, infoClass, &buf[0], desiredLen, &actualLen) + return buf, err +} + +func getTokenUserInfo(token windows.Token) (*windows.Tokenuser, error) { + buf, err := getTokenInfo(token, windows.TokenUser) + if err != nil { + return nil, err + } + + return (*windows.Tokenuser)(unsafe.Pointer(&buf[0])), nil +} + +func getTokenPrimaryGroupInfo(token windows.Token) (*windows.Tokenprimarygroup, error) { + buf, err := getTokenInfo(token, windows.TokenPrimaryGroup) + if err != nil { + return nil, err + } + + return (*windows.Tokenprimarygroup)(unsafe.Pointer(&buf[0])), nil +} + +type userSids struct { + User *windows.SID + PrimaryGroup *windows.SID +} + +func getCurrentUserSids() (*userSids, error) { + token, err := windows.OpenCurrentProcessToken() + if err != nil { + return nil, err + } + defer token.Close() + + userInfo, err := getTokenUserInfo(token) + if err != nil { + return nil, err + } + + primaryGroup, err := getTokenPrimaryGroupInfo(token) + if err != nil { + return nil, err + } + + return &userSids{userInfo.User.Sid, primaryGroup.PrimaryGroup}, nil +} + +// ensureStateDirPerms applies a restrictive ACL to the directory specified by dirPath. +// It sets the following security attributes on the directory: +// Owner: The user for the current process; +// Primary Group: The primary group for the current process; +// DACL: Full control to the current user and to the Administrators group. +// (We include Administrators so that admin users may still access logs; +// granting access exclusively to LocalSystem would require admins to use +// special tools to access the Log directory) +// Inheritance: The directory does not inherit the ACL from its parent. +// However, any directories and/or files created within this +// directory *do* inherit the ACL that we are setting. +func ensureStateDirPerms(dirPath string) error { + fi, err := os.Stat(dirPath) + if err != nil { + return err + } + if !fi.IsDir() { + return os.ErrInvalid + } + + // We need the info for our current user as SIDs + sids, err := getCurrentUserSids() + if err != nil { + return err + } + + // We also need the SID for the Administrators group so that admins may + // easily access logs. + adminGroupSid, err := windows.CreateWellKnownSid(windows.WinBuiltinAdministratorsSid) + if err != nil { + return err + } + + // Munge the SIDs into the format required by EXPLICIT_ACCESS. + userTrustee := windows.TRUSTEE{nil, windows.NO_MULTIPLE_TRUSTEE, + windows.TRUSTEE_IS_SID, windows.TRUSTEE_IS_USER, + windows.TrusteeValueFromSID(sids.User)} + + adminTrustee := windows.TRUSTEE{nil, windows.NO_MULTIPLE_TRUSTEE, + windows.TRUSTEE_IS_SID, windows.TRUSTEE_IS_WELL_KNOWN_GROUP, + windows.TrusteeValueFromSID(adminGroupSid)} + + // We declare our access rights via this array of EXPLICIT_ACCESS structures. + // We set full access to our user and to Administrators. + // We configure the DACL such that any files or directories created within + // dirPath will also inherit this DACL. + explicitAccess := []windows.EXPLICIT_ACCESS{ + windows.EXPLICIT_ACCESS{ + windows.GENERIC_ALL, + windows.SET_ACCESS, + windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, + userTrustee, + }, + windows.EXPLICIT_ACCESS{ + windows.GENERIC_ALL, + windows.SET_ACCESS, + windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, + adminTrustee, + }, + } + + dacl, err := windows.ACLFromEntries(explicitAccess, nil) + if err != nil { + return err + } + + // We now reset the file's owner, primary group, and DACL. + // We also must pass PROTECTED_DACL_SECURITY_INFORMATION so that our new ACL + // does not inherit any ACL entries from the parent directory. + const flags = windows.OWNER_SECURITY_INFORMATION | + windows.GROUP_SECURITY_INFORMATION | + windows.DACL_SECURITY_INFORMATION | + windows.PROTECTED_DACL_SECURITY_INFORMATION + return windows.SetNamedSecurityInfo(dirPath, windows.SE_FILE_OBJECT, flags, + sids.User, sids.PrimaryGroup, dacl, nil) +}