utils.RecursiveChown: Fix for Privilege Escalation due to following symlinks (#33244)

* utils.RecursiveChown: Harden against user access race conditions

Prior to this change a user could exploit Teleports privileged access to `chown` arbitrary files on the system.

This is due to the directory being changed first, allowing a small time window where a user can remove or rename the still `root` owned files with a symlink.  The added tests help show this issue in a more controlled way.

A switch to `os.Lchown` avoids the risk in following symlinks to files.  In addition, in order to remove the risk for hardlinks (notably on OSX with reduced hardlink protections), as well as risks with directory symlinks, the folder structure is inspected before any `chown` operation.  And then the files are updated before their parent directories.

* Update other instances of `os.Chown` to `os.Lchown`

None of these cases should expect a symlink that would need to be followed.

lib/events/auditlog.go

@@ -296,15 +296,15 @@ func NewAuditLog(cfg AuditLogConfig) (*AuditLog, error) {
 		return nil, trace.ConvertSystemError(err)
 	}
 	if cfg.UID != nil && cfg.GID != nil {
-		err := os.Chown(cfg.DataDir, *cfg.UID, *cfg.GID)
+		err := os.Lchown(cfg.DataDir, *cfg.UID, *cfg.GID)
 		if err != nil {
 			return nil, trace.ConvertSystemError(err)
 		}
-		err = os.Chown(sessionDir, *cfg.UID, *cfg.GID)
+		err = os.Lchown(sessionDir, *cfg.UID, *cfg.GID)
 		if err != nil {
 			return nil, trace.ConvertSystemError(err)
 		}
-		err = os.Chown(al.playbackDir, *cfg.UID, *cfg.GID)
+		err = os.Lchown(al.playbackDir, *cfg.UID, *cfg.GID)
 		if err != nil {
 			return nil, trace.ConvertSystemError(err)
 		}

lib/service/service.go

@@ -2878,7 +2878,7 @@ func (process *TeleportProcess) initUploaderService() error {
 			}
 			if uid != nil && gid != nil {
 				log.Infof("Setting directory %v owner to %v:%v.", dir, *uid, *gid)
-				err := os.Chown(dir, *uid, *gid)
+				err := os.Lchown(dir, *uid, *gid)
 				if err != nil {
 					return trace.ConvertSystemError(err)
 				}

lib/srv/reexec.go

@@ -403,7 +403,7 @@ func RunCommand() (errw io.Writer, code int, err error) {
 		if err != nil {
 			return errorWriter, teleport.RemoteCommandFailure, trace.Wrap(err)
 		}
-		if err := os.Chown(c.X11Config.XServerUnixSocket, uid, gid); err != nil {
+		if err := os.Lchown(c.X11Config.XServerUnixSocket, uid, gid); err != nil {
 			return errorWriter, teleport.RemoteCommandFailure, trace.Wrap(err)
 		}
 

lib/srv/term.go

@@ -472,7 +472,7 @@ func (t *terminal) setOwner() error {
 		return trace.Wrap(err)
 	}
 
-	err = os.Chown(t.tty.Name(), uid, gid)
+	err = os.Lchown(t.tty.Name(), uid, gid)
 	if err != nil {
 		return trace.Wrap(err)
 	}

lib/teleagent/agent.go

@@ -131,7 +131,7 @@ func (a *AgentServer) updatePermissions(user *user.User) error {
 
 	testPermissions()
 
-	if err := os.Chown(a.Path, uid, gid); err != nil {
+	if err := os.Lchown(a.Path, uid, gid); err != nil {
 		return trace.ConvertSystemError(err)
 	}
 
@@ -139,7 +139,7 @@ func (a *AgentServer) updatePermissions(user *user.User) error {
 
 	// To prevent a privilege escalation attack, this must occur
 	// after the socket permissions are updated.
-	if err := os.Chown(a.Dir, uid, gid); err != nil {
+	if err := os.Lchown(a.Dir, uid, gid); err != nil {
 		return trace.ConvertSystemError(err)
 	}
 

lib/utils/fs.go

@@ -372,19 +372,32 @@ func RemoveFileIfExist(filePath string) error {
 }
 
 func RecursiveChown(dir string, uid, gid int) error {
-	if err := os.Chown(dir, uid, gid); err != nil {
-		return trace.Wrap(err)
-	}
-	return trace.Wrap(filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error {
+	// First, walk the directory to gather a list of files and directories to update before we open up to modifications
+	var pathsToUpdate []string
+	err := filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error {
 		if err != nil {
 			return trace.Wrap(err)
 		}
-		err = os.Chown(path, uid, gid)
-		if os.IsNotExist(err) { // empty symlinks cause an error here
-			return nil
-		}
+		pathsToUpdate = append(pathsToUpdate, path)
+		return nil
+	})
+	if err != nil {
 		return trace.Wrap(err)
-	}))
+	}
+
+	// filepath.WalkDir is documented to walk the paths in lexical order, iterating
+	// in the reverse order ensures that files are always Lchowned before their parent directory
+	for i := len(pathsToUpdate) - 1; i >= 0; i-- {
+		path := pathsToUpdate[i]
+		if err := os.Lchown(path, uid, gid); err != nil {
+			if errors.Is(err, os.ErrNotExist) {
+				// Unexpected condition where file was removed after discovery.
+				continue
+			}
+			return trace.Wrap(err)
+		}
+	}
+	return nil
 }
 
 func CopyFile(src, dest string, perm os.FileMode) error {

lib/utils/fs_unix_test.go

@@ -0,0 +1,121 @@
+//go:build !windows
+// +build !windows
+
+/*
+Copyright 2023 Gravitational, Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package utils
+
+import (
+	"os"
+	"os/user"
+	"path/filepath"
+	"strconv"
+	"syscall"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// The tests contained here only function on unix systems.
+
+func setupRecursiveChownFiles(t *testing.T) (string, string, string, string) {
+	// Setup will produce the following structure under the temp directory created below:
+	// dir1/
+	// dir1/file
+	// dir2/
+	// dir2/file-s -> dir1/file
+	rootDir := t.TempDir()
+
+	dir1Path := filepath.Join(rootDir, "dir1")
+	require.NoError(t, os.Mkdir(dir1Path, 0755))
+
+	dir1FilePath := filepath.Join(dir1Path, "file")
+	f, err := os.Create(dir1FilePath)
+	require.NoError(t, err)
+	f.Close()
+
+	dir2Path := filepath.Join(rootDir, "dir2")
+	require.NoError(t, os.Mkdir(dir2Path, 0755))
+
+	dir2SymlinkToFile := filepath.Join(dir2Path, "file-s")
+	err = os.Symlink(dir1FilePath, dir2SymlinkToFile)
+	require.NoError(t, err)
+
+	return dir1Path, dir1FilePath, dir2Path, dir2SymlinkToFile
+}
+
+func setupRecursiveChownUser(t *testing.T) (int, int, int, int, bool) {
+	currentUser, err := user.Current()
+	require.NoError(t, err)
+
+	currentUID, err := strconv.Atoi(currentUser.Uid)
+	require.NoError(t, err)
+	currentGID, err := strconv.Atoi(currentUser.Gid)
+	require.NoError(t, err)
+
+	root := os.Geteuid() == 0
+	newUid := currentUID + 1
+	newGid := currentGID + 1
+	if !root {
+		// `root` is required to actually change ownership, if running under a normal user we will reduce the validation
+		newUid = currentUID
+		newGid = currentGID
+	}
+
+	return currentUID, currentGID, newUid, newGid, root
+}
+
+func verifyOwnership(t *testing.T, path string, uid, gid int) {
+	fi, err := os.Lstat(path)
+	require.NoError(t, err)
+	fiCast := fi.Sys().(*syscall.Stat_t)
+	require.Equal(t, uint32(uid), fiCast.Uid)
+	require.Equal(t, uint32(gid), fiCast.Gid)
+}
+
+func TestRecursiveChown(t *testing.T) {
+	t.Run("notFoundError", func(t *testing.T) {
+		t.Parallel()
+
+		require.Error(t, RecursiveChown("/invalid/path/to/nowhere", 1000, 1000))
+	})
+	t.Run("simpleChown", func(t *testing.T) {
+		t.Parallel()
+		_, _, newUid, newGid, _ := setupRecursiveChownUser(t)
+		dir1Path, dir1FilePath, _, _ := setupRecursiveChownFiles(t)
+
+		require.NoError(t, RecursiveChown(dir1Path, newUid, newGid))
+		// validate ownership matches expected ids
+		verifyOwnership(t, dir1Path, newUid, newGid)
+		verifyOwnership(t, dir1FilePath, newUid, newGid)
+	})
+	t.Run("symlinkChown", func(t *testing.T) {
+		t.Parallel()
+		origUid, origGid, newUid, newGid, root := setupRecursiveChownUser(t)
+		if !root {
+			t.Skip("Skipping test, root is required")
+			return
+		}
+		_, dir1FilePath, dir2Path, dir2SymlinkToFile := setupRecursiveChownFiles(t)
+
+		require.NoError(t, RecursiveChown(dir2Path, newUid, newGid))
+		// Validate symlink has changed
+		verifyOwnership(t, dir2SymlinkToFile, newUid, newGid)
+		// Validate pointed file has not changed
+		verifyOwnership(t, dir1FilePath, origUid, origGid)
+	})
+}