From ff2f72766abb4932ea50fab6e6e57652f9ab2c8a Mon Sep 17 00:00:00 2001 From: Mike Jensen Date: Tue, 10 Oct 2023 15:32:19 -0600 Subject: [PATCH] `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 | 6 +- lib/service/service.go | 2 +- lib/srv/reexec.go | 2 +- lib/srv/term.go | 2 +- lib/teleagent/agent.go | 4 +- lib/utils/fs.go | 31 +++++++--- lib/utils/fs_unix_test.go | 121 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 151 insertions(+), 17 deletions(-) create mode 100644 lib/utils/fs_unix_test.go 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) + }) +}