netbird/gvisor
0
0
Fork 0
mirror of https://github.com/netbirdio/gvisor.git synced 2026-05-22 17:12:49 -07:00
Code Issues Packages Projects Releases Wiki Activity

Fix ownership change logic

Browse Source 
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
This commit is contained in:
Fabricio Voznika
2021-01-21 15:47:23 -08:00
committed by gVisor bot
parent 89df5a681c
commit 7bf656f4c6
2 changed files with 112 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files
runsc/fsgofer/fsgofer.go
+21 -31
View File
@@ -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)
}
runsc/fsgofer/fsgofer_test.go
+91 -3
View File
@@ -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++ {