adding some safety around recursively chowning files owned by existin…

…g users

integration/hostuser_test.go

@@ -206,7 +206,14 @@ func TestRootHostUsersBackend(t *testing.T) {
 	})
 
 	t.Run("Test CreateHomeDirectory recursively takes ownership of existing directory", func(t *testing.T) {
+		getOwnerUID := func(path string) uint32 {
+			info, err := os.Stat(path)
+			require.NoError(t, err)
+			return info.Sys().(*syscall.Stat_t).Uid
+		}
+
 		otheruser := "other-user"
+
 		t.Cleanup(func() { cleanupUsersAndGroups([]string{testuser, otheruser}, nil) })
 		err := usersbk.CreateUser(testuser, nil, host.UserOpts{})
 		require.NoError(t, err)
@@ -229,19 +236,42 @@ func TestRootHostUsersBackend(t *testing.T) {
 		})
 		require.NoError(t, err)
 
-		info, err := os.Stat(testHome)
-		require.NoError(t, err)
-		initialOwnerUID := info.Sys().(*syscall.Stat_t).Uid
+		// add a file owned by root that _shouldn't_ be owned at the end
+		require.NoError(t, os.WriteFile(filepath.Join(testHome, "rootfile"), []byte("test\n"), 0o700))
+
+		// grab initial owners
+		initialRootFileOwner := getOwnerUID(filepath.Join(testHome, "rootfile"))
+		initialHomeOwner := getOwnerUID(testHome)
+		initialTestFileOwner := getOwnerUID(filepath.Join(testHome, "testfile"))
 
+		// don't take ownership when the user still exists
 		err = usersbk.CreateHomeDirectory(testHome, tuser.Uid, tuser.Gid)
 		require.ErrorIs(t, err, os.ErrExist)
 
-		info, err = os.Stat(testHome)
-		require.NoError(t, err)
-		finalOwnerUID := info.Sys().(*syscall.Stat_t).Uid
+		finalHomeOwner := getOwnerUID(testHome)
+		finalTestFileOwner := getOwnerUID(filepath.Join(testHome, "testfile"))
+		finalRootFileOwner := getOwnerUID(filepath.Join(testHome, "rootfile"))
+
+		require.Equal(t, initialRootFileOwner, finalRootFileOwner) // "rootfile" ownership unchanged
+		require.Equal(t, initialHomeOwner, finalHomeOwner)         // initial owner still owns directory
+		require.Equal(t, initialTestFileOwner, finalTestFileOwner) // initial owner still owns directory contents
+		require.NotEqual(t, tuser.Uid, fmt.Sprintf("%d", finalHomeOwner))
+		require.NotEqual(t, tuser.Uid, fmt.Sprintf("%d", finalTestFileOwner))
+
+		// take ownership of directory and contained files if the original user no longer exists
+		cleanupUsersAndGroups([]string{otheruser}, nil)
+		err = usersbk.CreateHomeDirectory(testHome, tuser.Uid, tuser.Gid)
+		require.ErrorIs(t, err, os.ErrExist)
+
+		finalHomeOwner = getOwnerUID(testHome)
+		finalTestFileOwner = getOwnerUID(filepath.Join(testHome, "testfile"))
+		finalRootFileOwner = getOwnerUID(filepath.Join(testHome, "rootfile"))
 
-		require.NotEqual(t, initialOwnerUID, finalOwnerUID)
-		require.Equal(t, tuser.Uid, fmt.Sprintf("%d", finalOwnerUID))
+		require.Equal(t, initialRootFileOwner, finalRootFileOwner)         // root is still the owner
+		require.NotEqual(t, initialHomeOwner, finalHomeOwner)              // owner is no longer initial user
+		require.NotEqual(t, initialTestFileOwner, finalTestFileOwner)      // owner is no longer initial user
+		require.Equal(t, tuser.Uid, fmt.Sprintf("%d", finalHomeOwner))     // ensure new owner is test-user
+		require.Equal(t, tuser.Uid, fmt.Sprintf("%d", finalTestFileOwner)) // ensure new owner is test-user
 	})
 }
 

lib/srv/usermgmt_linux.go

@@ -29,6 +29,7 @@ import (
 	"path/filepath"
 	"strconv"
 	"strings"
+	"syscall"
 
 	"github.com/gravitational/trace"
 	log "github.com/sirupsen/logrus"
@@ -238,33 +239,85 @@ func readDefaultSkel() (string, error) {
 	return skel, trace.Wrap(err)
 }
 
-func recursiveChown(dir string, uid, gid int) error {
-	// First, walk the directory to gather a list of files and directories to update before we open up to modifications
-	var pathsToUpdate []string
+func canTakeOwnership(observedUIDs map[uint32]bool, uid int, path string) (bool, error) {
+	fi, err := os.Lstat(path)
+	if err != nil {
+		return false, trace.Wrap(err)
+	}
+
+	stat := fi.Sys().(*syscall.Stat_t)
+	if stat == nil {
+		return false, nil
+	}
+
+	exists, seen := observedUIDs[stat.Uid]
+	if seen {
+		return !exists, nil
+	}
+
+	_, err = user.LookupId(strconv.FormatUint(uint64(stat.Uid), 10))
+	exists = true
+	if err != nil {
+		if !errors.Is(err, user.UnknownUserIdError(stat.Uid)) {
+			return false, trace.Wrap(err)
+		}
+
+		exists = false
+	}
+	observedUIDs[stat.Uid] = exists
+
+	return !exists, nil
+}
+
+// recursiveChown changes ownership of a directory and its contents. If called in safe mode, the contained files' ownership will
+// be left unchanged if the original owner still exists.
+func recursiveChown(dir string, uid, gid int, safe bool) error {
+	observedUIDs := map[uint32]bool{
+		0: true, // root should always exist, so we can preload that UID
+	}
+
+	if safe {
+		canTakeDir, err := canTakeOwnership(observedUIDs, uid, dir)
+		if err != nil {
+			return trace.WrapWithMessage(err, "taking ownership of %q", dir)
+		}
+
+		if !canTakeDir {
+			return trace.WrapWithMessage(os.ErrExist, "can not safely take ownership of %q when owning user still exists", dir)
+		}
+	}
+
 	err := filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error {
 		if err != nil {
 			return trace.Wrap(err)
 		}
-		pathsToUpdate = append(pathsToUpdate, path)
-		return nil
-	})
-	if err != nil {
-		return trace.Wrap(err)
-	}
 
-	pathsToUpdate = append(pathsToUpdate, dir)
-	// 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 safe {
+			takeOwnership, err := canTakeOwnership(observedUIDs, uid, path)
+			if err != nil {
+				return nil
+			}
+
+			if !takeOwnership {
+				return nil
+			}
+		}
+
 		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 nil
 			}
 			return trace.Wrap(err)
 		}
+
+		return nil
+	})
+
+	if err != nil {
+		return trace.Wrap(err)
 	}
+
 	return nil
 }
 
@@ -282,7 +335,7 @@ func (u *HostUsersProvisioningBackend) CreateHomeDirectory(userHome, uidS, gidS
 	err = os.Mkdir(userHome, 0o700)
 	if err != nil {
 		if os.IsExist(err) {
-			if chownErr := recursiveChown(userHome, uid, gid); chownErr != nil {
+			if chownErr := recursiveChown(userHome, uid, gid, true); chownErr != nil {
 				return trace.Wrap(chownErr)
 			}
 		}
@@ -315,5 +368,5 @@ func (u *HostUsersProvisioningBackend) CreateHomeDirectory(userHome, uidS, gidS
 		}
 	}
 
-	return trace.Wrap(recursiveChown(userHome, uid, gid))
+	return trace.Wrap(recursiveChown(userHome, uid, gid, false))
 }