[v14] utils.RecursiveChown: Fix for Privilege Escalation due to following symlinks

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

@@ -2874,7 +2874,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,15 +131,15 @@ 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)
 	}
 
 	testPermissions()
 
 	// 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

@@ -369,19 +369,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)
+	})
+}