diff --git a/lib/events/auditlog.go b/lib/events/auditlog.go index ff2db8b0d27..3856fb01335 100644 --- a/lib/events/auditlog.go +++ b/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) } diff --git a/lib/service/service.go b/lib/service/service.go index a51054f1962..7c11af3b82e 100644 --- a/lib/service/service.go +++ b/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) } diff --git a/lib/srv/reexec.go b/lib/srv/reexec.go index 10aaff2682b..51042fadaa7 100644 --- a/lib/srv/reexec.go +++ b/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) } diff --git a/lib/srv/term.go b/lib/srv/term.go index 7b7b1c77357..66ab8f4d2c8 100644 --- a/lib/srv/term.go +++ b/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) } diff --git a/lib/teleagent/agent.go b/lib/teleagent/agent.go index 31cc144985a..cac5ff857c8 100644 --- a/lib/teleagent/agent.go +++ b/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) } diff --git a/lib/utils/fs.go b/lib/utils/fs.go index 5cb56a8f55c..36322877f83 100644 --- a/lib/utils/fs.go +++ b/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 { diff --git a/lib/utils/fs_unix_test.go b/lib/utils/fs_unix_test.go new file mode 100644 index 00000000000..8d430694262 --- /dev/null +++ b/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) + }) +}