From 7bf656f4c635e4acdc0a61523220dfc9a1f3d1ba Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Thu, 21 Jan 2021 15:38:22 -0800 Subject: Fix ownership change logic Previously fsgofer was skipping chown call if the uid and gid were the same as the current user/group. However, when setgid is set, the group may not be the same as the caller. Instead, compare the actual uid/gid of the file after it has been created and change ownership only if needed. Updates #180 PiperOrigin-RevId: 353118733 --- runsc/fsgofer/fsgofer.go | 52 ++++++++++-------------- runsc/fsgofer/fsgofer_test.go | 94 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 112 insertions(+), 34 deletions(-) (limited to 'runsc/fsgofer') diff --git a/runsc/fsgofer/fsgofer.go b/runsc/fsgofer/fsgofer.go index c3bba0973..f25681233 100644 --- a/runsc/fsgofer/fsgofer.go +++ b/runsc/fsgofer/fsgofer.go @@ -49,13 +49,6 @@ const ( allowedOpenFlags = unix.O_TRUNC ) -var ( - // Remember the process uid/gid to skip chown calls when file owner/group - // doesn't need to be changed. - processUID = p9.UID(os.Getuid()) - processGID = p9.GID(os.Getgid()) -) - // join is equivalent to path.Join() but skips path.Clean() which is expensive. func join(parent, child string) string { if child == "." || child == ".." { @@ -377,6 +370,23 @@ func fchown(fd int, uid p9.UID, gid p9.GID) error { return unix.Fchownat(fd, "", int(uid), int(gid), linux.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW) } +func setOwnerIfNeeded(fd int, uid p9.UID, gid p9.GID) (unix.Stat_t, error) { + stat, err := fstat(fd) + if err != nil { + return unix.Stat_t{}, err + } + + // Change ownership if not set accordinly. + if uint32(uid) != stat.Uid || uint32(gid) != stat.Gid { + if err := fchown(fd, uid, gid); err != nil { + return unix.Stat_t{}, err + } + stat.Uid = uint32(uid) + stat.Gid = uint32(gid) + } + return stat, nil +} + // Open implements p9.File. func (l *localFile) Open(flags p9.OpenFlags) (*fd.FD, p9.QID, uint32, error) { if l.isOpen() { @@ -457,12 +467,7 @@ func (l *localFile) Create(name string, p9Flags p9.OpenFlags, perm p9.FileMode, }) defer cu.Clean() - if uid != processUID || gid != processGID { - if err := fchown(child.FD(), uid, gid); err != nil { - return nil, nil, p9.QID{}, 0, extractErrno(err) - } - } - stat, err := fstat(child.FD()) + stat, err := setOwnerIfNeeded(child.FD(), uid, gid) if err != nil { return nil, nil, p9.QID{}, 0, extractErrno(err) } @@ -505,12 +510,7 @@ func (l *localFile) Mkdir(name string, perm p9.FileMode, uid p9.UID, gid p9.GID) } defer f.Close() - if uid != processUID || gid != processGID { - if err := fchown(f.FD(), uid, gid); err != nil { - return p9.QID{}, extractErrno(err) - } - } - stat, err := fstat(f.FD()) + stat, err := setOwnerIfNeeded(f.FD(), uid, gid) if err != nil { return p9.QID{}, extractErrno(err) } @@ -900,12 +900,7 @@ func (l *localFile) Symlink(target, newName string, uid p9.UID, gid p9.GID) (p9. } defer f.Close() - if uid != processUID || gid != processGID { - if err := fchown(f.FD(), uid, gid); err != nil { - return p9.QID{}, extractErrno(err) - } - } - stat, err := fstat(f.FD()) + stat, err := setOwnerIfNeeded(f.FD(), uid, gid) if err != nil { return p9.QID{}, extractErrno(err) } @@ -959,12 +954,7 @@ func (l *localFile) Mknod(name string, mode p9.FileMode, _ uint32, _ uint32, uid } defer child.Close() - if uid != processUID || gid != processGID { - if err := fchown(child.FD(), uid, gid); err != nil { - return p9.QID{}, extractErrno(err) - } - } - stat, err := fstat(child.FD()) + stat, err := setOwnerIfNeeded(child.FD(), uid, gid) if err != nil { return p9.QID{}, extractErrno(err) } diff --git a/runsc/fsgofer/fsgofer_test.go b/runsc/fsgofer/fsgofer_test.go index c5daebe5e..99ea9bd32 100644 --- a/runsc/fsgofer/fsgofer_test.go +++ b/runsc/fsgofer/fsgofer_test.go @@ -32,6 +32,9 @@ import ( "gvisor.dev/gvisor/runsc/specutils" ) +// Nodoby is the standard UID/GID for the nobody user/group. +const nobody = 65534 + var allOpenFlags = []p9.OpenFlags{p9.ReadOnly, p9.WriteOnly, p9.ReadWrite} var ( @@ -281,6 +284,92 @@ func TestCreate(t *testing.T) { }) } +func checkIDs(f p9.File, uid, gid int) error { + _, _, stat, err := f.GetAttr(p9.AttrMask{UID: true, GID: true}) + if err != nil { + return fmt.Errorf("GetAttr() failed, err: %v", err) + } + if want := p9.UID(uid); stat.UID != want { + return fmt.Errorf("Wrong UID, want: %v, got: %v", want, stat.UID) + } + if want := p9.GID(gid); stat.GID != want { + return fmt.Errorf("Wrong GID, want: %v, got: %v", want, stat.GID) + } + return nil +} + +// TestCreateSetGID checks files/dirs/symlinks are created with the proper +// owner when the parent directory has setgid set, +func TestCreateSetGID(t *testing.T) { + if !specutils.HasCapabilities(capability.CAP_CHOWN) { + t.Skipf("Test requires CAP_CHOWN") + } + + runCustom(t, []uint32{unix.S_IFDIR}, rwConfs, func(t *testing.T, s state) { + // Change group and set setgid to the parent dir. + if err := unix.Chown(s.file.hostPath, os.Getuid(), nobody); err != nil { + t.Fatalf("Chown() failed: %v", err) + } + if err := unix.Chmod(s.file.hostPath, 02777); err != nil { + t.Fatalf("Chmod() failed: %v", err) + } + + t.Run("create", func(t *testing.T) { + _, l, _, _, err := s.file.Create("test", p9.ReadOnly, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())) + if err != nil { + t.Fatalf("WriteAt() failed: %v", err) + } + defer l.Close() + if err := checkIDs(l, os.Getuid(), os.Getgid()); err != nil { + t.Error(err) + } + }) + + t.Run("mkdir", func(t *testing.T) { + _, err := s.file.Mkdir("test-dir", 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())) + if err != nil { + t.Fatalf("WriteAt() failed: %v", err) + } + _, l, err := s.file.Walk([]string{"test-dir"}) + if err != nil { + t.Fatalf("Walk() failed: %v", err) + } + defer l.Close() + if err := checkIDs(l, os.Getuid(), os.Getgid()); err != nil { + t.Error(err) + } + }) + + t.Run("symlink", func(t *testing.T) { + if _, err := s.file.Symlink("/some/target", "symlink", p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != nil { + t.Fatalf("Symlink() failed: %v", err) + } + _, l, err := s.file.Walk([]string{"symlink"}) + if err != nil { + t.Fatalf("Walk() failed, err: %v", err) + } + defer l.Close() + if err := checkIDs(l, os.Getuid(), os.Getgid()); err != nil { + t.Error(err) + } + }) + + t.Run("mknod", func(t *testing.T) { + if _, err := s.file.Mknod("nod", p9.ModeRegular|0777, 0, 0, p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != nil { + t.Fatalf("Mknod() failed: %v", err) + } + _, l, err := s.file.Walk([]string{"nod"}) + if err != nil { + t.Fatalf("Walk() failed, err: %v", err) + } + defer l.Close() + if err := checkIDs(l, os.Getuid(), os.Getgid()); err != nil { + t.Error(err) + } + }) + }) +} + // TestReadWriteDup tests that a file opened in any mode can be dup'ed and // reopened in any other mode. func TestReadWriteDup(t *testing.T) { @@ -458,7 +547,7 @@ func TestSetAttrTime(t *testing.T) { } func TestSetAttrOwner(t *testing.T) { - if os.Getuid() != 0 { + if !specutils.HasCapabilities(capability.CAP_CHOWN) { t.Skipf("SetAttr(owner) test requires CAP_CHOWN, running as %d", os.Getuid()) } @@ -477,7 +566,7 @@ func TestSetAttrOwner(t *testing.T) { } func TestLink(t *testing.T) { - if os.Getuid() != 0 { + if !specutils.HasCapabilities(capability.CAP_DAC_READ_SEARCH) { t.Skipf("Link test requires CAP_DAC_READ_SEARCH, running as %d", os.Getuid()) } runCustom(t, allTypes, rwConfs, func(t *testing.T, s state) { @@ -995,7 +1084,6 @@ func BenchmarkCreateDiffOwner(b *testing.B) { files := make([]p9.File, 0, 500) fds := make([]*fd.FD, 0, 500) gid := p9.GID(os.Getgid()) - const nobody = 65534 b.ResetTimer() for i := 0; i < b.N; i++ { -- cgit v1.2.3