VULN-143514: Windows svc: use --token-file instead of --token

As with recent changes with Linux and MacOS, use --token-file instead of --token when installing the service for Windows.

The secret token was viewable by an unprivileged user by looking at the registry entry HKLM\\SYSTEM\\CurrentControlSet\\Services\\Cloudflared\\ImagePath, which stores the full command-line invocation of cloudflared (complete with --token ).

We fix this by storing the token in a file and restricting access to it.

The canonical way of protecting a secret token on Windows is to use the CryptProtectData and CryptUnprotectData functions in dpapi.h, which encrypt/unencrypt data using an OS-managed secret key. See here:

https://learn.microsoft.com/en-us/windows/win32/api/dpapi/

While we could use the DPAPI to encrypt/decrypt the token before writing it out to disk, this has two problems:

1) We would break existing Windows installs using --token-file with an unencrypted token file
2) We would introduce an inconsistency between how --token-file works on Linux/MacOS and Windows

Because of this, we keep things consistent and just add logic to cloudflared to protect the token file by modifying the permissions of the token file.

Windows's permission model differs completely from Linux and MacOS, so a Windows-specific function is used to restrict the token file's permissions. We strip ACLs from the file such that it's only readable by SYSTEM and Administrators.

Also done by this MR:

* Improve "service install --help" output on Windows to be in-line with Linux and MacOS
* Change uses of path.Join that work with file paths to be filepath.Join instead, which will use the correct platform-specific path separator (\\ on windows or / on \*nix) instead of only forward slashes
* Pull out constant string in MacOS service

cmd/cloudflared/common_service.go

@@ -4,7 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"path"
+	"path/filepath"
 
 	"github.com/rs/zerolog"
 	"github.com/urfave/cli/v2"
@@ -15,7 +15,6 @@ import (
 
 const (
 	defaultTokenFile = "token"
-	tokenPerms       os.FileMode = 0o600
 )
 
 func ensureConfigDirExists(configDir string) error {
@@ -23,24 +22,44 @@ func ensureConfigDirExists(configDir string) error {
 		if errors.Is(err, os.ErrExist) {
 			return nil
 		}
-		return fmt.Errorf("failed to create config dir at %s: %w", configDir, err)
+		return fmt.Errorf("create config dir at %s: %w", configDir, err)
 	}
 	return nil
 }
 
+func createTokenFileUnix(path string) error {
+	const tokenPerms os.FileMode = 0o600
+	f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, tokenPerms) //nolint:gosec // All callers of this function construct path from constant strings or well-known env vars (e.g., $HOME)
+	if err != nil {
+		return fmt.Errorf("create token file at %s: %w", path, err)
+	}
+	defer func() { _ = f.Close() }()
+
+	// If the file already existed with unrestrictive permissions, os.OpenFile
+	// will not update its permissions, so perform an extra os.Chmod
+	if err := os.Chmod(path, tokenPerms); err != nil {
+		return fmt.Errorf("chmod token file at %s: %w", path, err)
+	}
+
+	return nil
+}
+
+// Write out the token file to the configuration directory with the correct
+// permissions. Since the method used to restrict the permissions is platform
+// dependent, make the function used to restrict the permissions an injectable
+// dependency
 func writeTokenToFile(path string, token string) error {
 	if _, err := tunnel.ParseToken(token); err != nil {
 		return cliutil.UsageError("Provided tunnel token is not valid (%s).", err)
 	}
 
-	if err := os.WriteFile(path, []byte(token), tokenPerms); err != nil {
-		return fmt.Errorf("failed to write token to %s: %w", path, err)
+	if err := createTokenFile(path); err != nil {
+		return fmt.Errorf("create token file at %s: %w", path, err)
 	}
 
-	// If the token file already existed with unrestrictive perms, os.WriteFile
-	// above will not update them
-	if err := os.Chmod(path, tokenPerms); err != nil {
-		return fmt.Errorf("failed to restrict permissions on token file %s: %w", path, err)
+	// Won't update permissions as file already exists
+	if err := os.WriteFile(path, []byte(token), 0o600); err != nil {
+		return fmt.Errorf("write token to %s: %w", path, err)
 	}
 
 	return nil
@@ -62,7 +81,7 @@ func buildArgsForTokenFile(configDir string) []string {
 }
 
 func tokenPath(configDir string) string {
-	return path.Join(configDir, defaultTokenFile)
+	return filepath.Join(configDir, defaultTokenFile)
 }
 
 func writeTokenToConfigDir(c *cli.Context, configDir string) error {
@@ -76,26 +95,3 @@ func writeTokenToConfigDir(c *cli.Context, configDir string) error {
 
 	return nil
 }
-
-// nolint:unused // This function is used by the Windows build, the unused warning when building for Linux and MacOS is spurious
-func buildArgsForToken(c *cli.Context, log *zerolog.Logger) ([]string, error) {
-	token := c.Args().First()
-	if _, err := tunnel.ParseToken(token); err != nil {
-		return nil, cliutil.UsageError("Provided tunnel token is not valid (%s).", err)
-	}
-
-	return []string{
-		"tunnel", "run", "--token", token,
-	}, nil
-}
-
-// nolint:unused // This function is used by the Windows build, the unused warning when building for Linux and MacOS is spurious
-func getServiceExtraArgsFromCliArgs(c *cli.Context, log *zerolog.Logger) ([]string, error) {
-	if c.NArg() > 0 {
-		// currently, we only support extra args for token
-		return buildArgsForToken(c, log)
-	} else {
-		// empty extra args
-		return make([]string, 0), nil
-	}
-}

cmd/cloudflared/linux_service.go

@@ -67,6 +67,9 @@ const (
 	cloudflaredOpenRCService = "cloudflared"
 )
 
+// OS-specific function for token file creation
+var createTokenFile = createTokenFileUnix
+
 var systemdAllTemplates = map[string]ServiceTemplate{
 	cloudflaredService: {
 		Path: fmt.Sprintf("/etc/systemd/system/%s", cloudflaredService),

cmd/cloudflared/macos_service.go

@@ -5,7 +5,7 @@ package main
 import (
 	"fmt"
 	"os"
-	"path"
+	"path/filepath"
 
 	homedir "github.com/mitchellh/go-homedir"
 	"github.com/pkg/errors"
@@ -19,6 +19,9 @@ const (
 	launchdIdentifier = "com.cloudflare.cloudflared"
 )
 
+// OS-specific function for token file creation
+var createTokenFile = createTokenFileUnix
+
 func runApp(app *cli.App, _ chan struct{}) {
 	app.Commands = append(app.Commands, &cli.Command{
 		Name:  "service",
@@ -89,9 +92,11 @@ func isRootUser() bool {
 }
 
 func resolveLibraryPath(subPath, fileName string) (string, error) {
+	const libraryDirName = "Library"
+
 	// We use the system-wide /Library/... instead of ~/Library/... if the user is root
 	if isRootUser() {
-		return path.Join("/Library", subPath, fileName), nil
+		return filepath.Join("/", libraryDirName, subPath, fileName), nil
 	}
 
 	// This returns the home dir of the executing user using OS-specific method
@@ -102,7 +107,7 @@ func resolveLibraryPath(subPath, fileName string) (string, error) {
 	if err != nil {
 		return "", errors.Wrap(err, "Cannot determine home directory for the user")
 	}
-	return path.Join(userHomeDir, "Library", subPath, fileName), nil
+	return filepath.Join(userHomeDir, libraryDirName, subPath, fileName), nil
 }
 
 // For docs on these subdirectories, see:

cmd/cloudflared/windows_service.go

@@ -8,6 +8,7 @@ package main
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"syscall"
 	"time"
 	"unsafe"
@@ -28,6 +29,12 @@ const (
 	windowsServiceDescription = "Cloudflared agent"
 	windowsServiceUrl         = "https://developers.cloudflare.com/cloudflare-one/connections/connect-apps/configure-tunnels/local-management/as-a-service/windows/"
 
+	// Env var that points to a directory for storing application-specific
+	// configuration and data (analogous to /etc/). Normally this points to
+	// C:\ProgramData.
+	programDataEnvVar = "PROGRAMDATA"
+	configDirName     = "cloudflared"
+
 	recoverActionDelay      = time.Second * 20
 	failureCountResetPeriod = time.Hour * 24
 
@@ -50,6 +57,16 @@ func runApp(app *cli.App, graceShutdownC chan struct{}) {
 			{
 				Name:      "install",
 				Usage:     "Install cloudflared as a Windows service",
+				ArgsUsage: "[TOKEN]",
+				Description: `
+Installs cloudflared as a Windows service
+
+A token may optionally be provided. If a token is provided, it will be written
+to disk in the service configuration directory and the cloudflared service
+configured to use it via the --token-file argument.
+
+If no token is provided, cloudflared will run without the --token-file argument,
+causing it to look for credentials in a configuration file upon startup.`,
 				Action: cliutil.ConfiguredAction(installWindowsService),
 			},
 			{
@@ -96,6 +113,105 @@ func runApp(app *cli.App, graceShutdownC chan struct{}) {
 	}
 }
 
+// Creates the token file at the given path, restricting its permissions by
+// modifying its Windows ACLs. We change the ACLs on the token file such that
+// the Administrators group and SYSTEM account (which is what cloudflared runs
+// as) have full access and all others are denied, with the Administrator group
+// owning the file.
+func createTokenFile(path string) error {
+	// This is a Windows Security Descriptor string describing the permissions
+	// we apply to the token file. This is the domain-specific language Windows
+	// uses for representing access rights.
+	//
+	// - O:BA         -> Set the owner to the builtin administrators group (BA)
+	// - D:           -> Start of discretionary access control list describing access rights
+	// - P            -> Set the SE_DACL_PROTECTED flag, which prevents the file from
+	//                   inheriting the (usually permissive) ACEs from its parent directory
+	// - (A;;FA;;;BA) -> ACE #1: Allow (A) Full access (FA) to the Builtin Administrators group (BA)
+	// - (A;;FA;;;SY) -> ACE #2: Ditto but for the Local System user (SY)
+	//
+	// Relevant Docs:
+	//
+	// - SecurityDescriptor string as a whole:
+	//     https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-string-format
+	// - SID Strings such as BA/SY
+	//     https://learn.microsoft.com/en-us/windows/win32/secauthz/sid-strings
+	// - ACE Strings such as (A;;FA;;BA)
+	//     https://learn.microsoft.com/en-us/windows/win32/secauthz/ace-strings
+	const sdString = "O:BAD:P(A;;FA;;;BA)(A;;FA;;;SY)"
+	sd, err := windows.SecurityDescriptorFromString(sdString)
+	if err != nil {
+		return fmt.Errorf("create token security descriptor: %w", err)
+	}
+
+	pathRaw, err := windows.UTF16PtrFromString(path)
+	if err != nil {
+		return fmt.Errorf("convert path to UTF-16: %w", err)
+	}
+
+	f, err := windows.CreateFile(
+		pathRaw,
+		windows.GENERIC_WRITE,
+		0,
+		&windows.SecurityAttributes{
+			Length:             uint32(unsafe.Sizeof(windows.SecurityAttributes{})),
+			SecurityDescriptor: sd,
+			InheritHandle:      0,
+		},
+		windows.CREATE_ALWAYS, // Will truncate the file if it exists
+		windows.FILE_ATTRIBUTE_NORMAL,
+		0,
+	)
+
+	if err != nil {
+		return fmt.Errorf("create token file: %w", err)
+	}
+
+	if err := windows.CloseHandle(f); err != nil {
+		return fmt.Errorf("close token file: %w", err)
+	}
+
+	// As with os.CreateFile / os.OpenFile on Unix, if the file already exists
+	// windows.CreateFile will not update the permission information, so we do
+	// that explicitly after creating the file.
+
+	owner, _, err := sd.Owner()
+	if err != nil {
+		return fmt.Errorf("get token file owner: %w", err)
+	}
+
+	dacl, _, err := sd.DACL()
+	if err != nil {
+		return fmt.Errorf("get token file DACL: %w", err)
+	}
+
+	// Bitmask indicating which security info we want to set on the file:
+	//
+	// OWNER_SECURITY_INFORMATION
+	//	-> Set file owner
+	// DACL_SECURITY_INFORMATION
+	// 	-> Set ACEs
+	// PROTECTED_DACL_SECURITY_INFORMATION
+	//  -> Update DACL to be "protected' such that it cannot inherit entries from its parent
+	const securityInfo = windows.OWNER_SECURITY_INFORMATION |
+		windows.DACL_SECURITY_INFORMATION |
+		windows.PROTECTED_DACL_SECURITY_INFORMATION
+
+	if err := windows.SetNamedSecurityInfo(
+		path,
+		windows.SE_FILE_OBJECT,
+		securityInfo,
+		owner,
+		nil,
+		dacl,
+		nil,
+	); err != nil {
+		return fmt.Errorf("set token file security info: %w", err)
+	}
+
+	return nil
+}
+
 type windowsService struct {
 	app            *cli.App
 	graceShutdownC chan struct{}
@@ -173,6 +289,15 @@ func (s *windowsService) Execute(serviceArgs []string, r <-chan svc.ChangeReques
 	}
 }
 
+func getConfigDir() (string, error) {
+	progDat, progDatSet := os.LookupEnv(programDataEnvVar)
+	if !progDatSet {
+		return "", fmt.Errorf("could not find program data directory, %s env var must be set", programDataEnvVar)
+	}
+
+	return filepath.Join(progDat, configDirName), nil
+}
+
 func installWindowsService(c *cli.Context) error {
 	zeroLogger := logger.CreateLoggerFromContext(c, logger.EnableTerminalLog)
 
@@ -192,11 +317,35 @@ func installWindowsService(c *cli.Context) error {
 		s.Close()
 		return errors.New(serviceAlreadyExistsWarn(windowsServiceName))
 	}
-	extraArgs, err := getServiceExtraArgsFromCliArgs(c, &log)
+	var extraArgs []string
+	if c.NArg() > 0 {
+		// The service has been installed using a token e.g.,
+		// $ cloudflared service install <token>
+		//
+		// Write the token file to a config directory so we can start the
+		// service with --token-file.
+
+		// Don't use :=, if we did so we would create a new err variable and
+		// shadow the outer one, causing the defer below to not have access to
+		// the outer err
+		var configDir string
+		configDir, err = getConfigDir()
 		if err != nil {
-		errMsg := "Unable to determine extra arguments for windows service"
-		log.Err(err).Msg(errMsg)
-		return errors.Wrap(err, errMsg)
+			return fmt.Errorf("locate config dir: %w", err)
+		}
+
+		// Remove token file if service install fails any point onwards from here
+		defer func() {
+			if err != nil {
+				removeTokenFile(configDir, zeroLogger)
+			}
+		}()
+
+		if err = writeTokenToConfigDir(c, configDir); err != nil {
+			return fmt.Errorf("write token to configuration directory at %s: %w", configDir, err)
+		}
+
+		extraArgs = buildArgsForTokenFile(configDir)
 	}
 
 	config := mgr.Config{StartType: mgr.StartAutomatic, DisplayName: windowsServiceDescription}
@@ -219,10 +368,13 @@ func installWindowsService(c *cli.Context) error {
 	}
 
 	err = s.Start()
-	if err == nil {
-		log.Info().Msg("Agent service for cloudflared installed successfully")
+	if err != nil {
+		s.Delete()
+		return errors.Wrap(err, "Cannot start service")
 	}
-	return err
+
+	log.Info().Msg("Agent service for cloudflared installed successfully")
+	return nil
 }
 
 func uninstallWindowsService(c *cli.Context) error {
@@ -258,6 +410,15 @@ func uninstallWindowsService(c *cli.Context) error {
 	if err != nil {
 		return errors.Wrap(err, "Cannot remove event logger")
 	}
+
+	configDir, err := getConfigDir()
+	if err != nil {
+		// We don't need to hard-error out here, this isn't critical, but we should log it
+		log.Warn().Err(err).Msgf("Failed to find configuration directory, not removing secret token file")
+	} else {
+		removeTokenFile(configDir, &log)
+	}
+
 	return nil
 }