Skip to content

Commit

Permalink
adding some safety around recursively chowning files owned by existin…
Browse files Browse the repository at this point in the history
…g users
  • Loading branch information
eriktate committed Oct 4, 2024
1 parent a0ffb15 commit 179162b
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 8 deletions.
21 changes: 21 additions & 0 deletions integration/hostuser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,15 +233,36 @@ func TestRootHostUsersBackend(t *testing.T) {
require.NoError(t, err)
initialOwnerUID := info.Sys().(*syscall.Stat_t).Uid

// take ownership of the home directory, but not the files inside when original owner 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

info, err = os.Stat(filepath.Join(testHome, "testfile"))
require.NoError(t, err)
finalFileOwnerUID := info.Sys().(*syscall.Stat_t).Uid

require.NotEqual(t, initialOwnerUID, finalOwnerUID)
require.Equal(t, tuser.Uid, fmt.Sprintf("%d", finalOwnerUID))
require.Equal(t, initialOwnerUID, finalFileOwnerUID) // initial owner still owns directory contents

// take ownership of the files inside 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)

info, err = os.Stat(testHome)
require.NoError(t, err)
finalOwnerUID = info.Sys().(*syscall.Stat_t).Uid
info, err = os.Stat(filepath.Join(testHome, "testfile"))
require.NoError(t, err)
finalFileOwnerUID = info.Sys().(*syscall.Stat_t).Uid

require.Equal(t, tuser.Uid, fmt.Sprintf("%d", finalOwnerUID))
require.Equal(t, finalOwnerUID, finalFileOwnerUID) // directory contents now belong to test-user
})
}

Expand Down
54 changes: 46 additions & 8 deletions lib/srv/usermgmt_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"path/filepath"
"strconv"
"strings"
"syscall"

"github.com/gravitational/trace"
log "github.com/sirupsen/logrus"
Expand Down Expand Up @@ -238,7 +239,9 @@ func readDefaultSkel() (string, error) {
return skel, trace.Wrap(err)
}

func recursiveChown(dir string, uid, gid int) error {
// 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 {
// 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 {
Expand All @@ -252,11 +255,40 @@ func recursiveChown(dir string, uid, gid int) error {
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]
uidMap := map[uint32]bool{
0: true, // root should always exist, so we can preload that UID
}

for _, path := range pathsToUpdate {
if safe {
fi, err := os.Lstat(path)
if err != nil {
return trace.Wrap(err)
}

stat := fi.Sys().(*syscall.Stat_t)
if stat == nil {
continue
}

exists, seen := uidMap[stat.Uid]
if !seen {
_, err := user.LookupId(strconv.FormatUint(uint64(stat.Uid), 10))
exists = !errors.Is(err, user.UnknownUserIdError(stat.Uid))
if err != nil && exists {
// skip files that can't have their ownership verified
continue
}

uidMap[stat.Uid] = exists
}

// don't take ownership of files owned by users that still exist
if exists {
continue
}
}

if err := os.Lchown(path, uid, gid); err != nil {
if errors.Is(err, os.ErrNotExist) {
// Unexpected condition where file was removed after discovery.
Expand All @@ -265,6 +297,12 @@ func recursiveChown(dir string, uid, gid int) error {
return trace.Wrap(err)
}
}

// ensure at least the target directory itself is owned at the end, regardless of previous owner
if err := os.Lchown(dir, uid, gid); err != nil && !errors.Is(err, os.ErrNotExist) {
return trace.Wrap(err)
}

return nil
}

Expand All @@ -282,7 +320,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)
}
}
Expand Down Expand Up @@ -315,5 +353,5 @@ func (u *HostUsersProvisioningBackend) CreateHomeDirectory(userHome, uidS, gidS
}
}

return trace.Wrap(recursiveChown(userHome, uid, gid))
return trace.Wrap(recursiveChown(userHome, uid, gid, false))
}

0 comments on commit 179162b

Please sign in to comment.