From 00ee4cb1a26d8f3cabbbb7fc05d719d8aabbee60 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Wed, 19 Aug 2020 18:03:15 -0700 Subject: Remove path walk from localFile.Mknod Replace mknod call with mknodat equivalent to protect against symlink attacks. Also added Mknod tests. Remove goferfs reliance on gofer to check for file existence before creating a synthetic entry. Updates #2923 PiperOrigin-RevId: 327544516 --- runsc/fsgofer/fsgofer_test.go | 147 +++++++++++++++++++++++++++--------------- 1 file changed, 96 insertions(+), 51 deletions(-) (limited to 'runsc/fsgofer/fsgofer_test.go') diff --git a/runsc/fsgofer/fsgofer_test.go b/runsc/fsgofer/fsgofer_test.go index 8ed703584..c91cfd094 100644 --- a/runsc/fsgofer/fsgofer_test.go +++ b/runsc/fsgofer/fsgofer_test.go @@ -21,9 +21,9 @@ import ( "os" "path" "path/filepath" - "syscall" "testing" + "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/p9" "gvisor.dev/gvisor/pkg/test/testutil" @@ -32,7 +32,7 @@ import ( var allOpenFlags = []p9.OpenFlags{p9.ReadOnly, p9.WriteOnly, p9.ReadWrite} var ( - allTypes = []uint32{syscall.S_IFREG, syscall.S_IFDIR, syscall.S_IFLNK} + allTypes = []uint32{unix.S_IFREG, unix.S_IFDIR, unix.S_IFLNK} // allConfs is set in init(). allConfs []Config @@ -83,7 +83,7 @@ func testReadWrite(f p9.File, flags p9.OpenFlags, content []byte) error { } want = append(want, b...) } else { - if e, ok := err.(syscall.Errno); !ok || e != syscall.EBADF { + if e, ok := err.(unix.Errno); !ok || e != unix.EBADF { return fmt.Errorf("WriteAt() should have failed, got: %d, want: EBADFD", err) } } @@ -101,7 +101,7 @@ func testReadWrite(f p9.File, flags p9.OpenFlags, content []byte) error { return fmt.Errorf("ReadAt() wrong data, got: %s, want: %s", string(rBuf), want) } } else { - if e, ok := err.(syscall.Errno); !ok || e != syscall.EBADF { + if e, ok := err.(unix.Errno); !ok || e != unix.EBADF { return fmt.Errorf("ReadAt() should have failed, got: %d, want: EBADFD", err) } } @@ -121,11 +121,11 @@ func (s state) String() string { func typeName(fileType uint32) string { switch fileType { - case syscall.S_IFREG: + case unix.S_IFREG: return "file" - case syscall.S_IFDIR: + case unix.S_IFDIR: return "directory" - case syscall.S_IFLNK: + case unix.S_IFLNK: return "symlink" default: panic(fmt.Sprintf("invalid file type for test: %d", fileType)) @@ -195,19 +195,19 @@ func setup(fileType uint32) (string, string, error) { var name string switch fileType { - case syscall.S_IFREG: + case unix.S_IFREG: name = "file" _, f, _, _, err := root.Create(name, p9.ReadWrite, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())) if err != nil { return "", "", fmt.Errorf("createFile(root, %q) failed, err: %v", "test", err) } defer f.Close() - case syscall.S_IFDIR: + case unix.S_IFDIR: name = "dir" if _, err := root.Mkdir(name, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != nil { return "", "", fmt.Errorf("root.MkDir(%q) failed, err: %v", name, err) } - case syscall.S_IFLNK: + case unix.S_IFLNK: name = "symlink" if _, err := root.Symlink("/some/target", name, p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != nil { return "", "", fmt.Errorf("root.Symlink(%q) failed, err: %v", name, err) @@ -227,7 +227,7 @@ func createFile(dir *localFile, name string) (*localFile, error) { } func TestReadWrite(t *testing.T) { - runCustom(t, []uint32{syscall.S_IFDIR}, rwConfs, func(t *testing.T, s state) { + runCustom(t, []uint32{unix.S_IFDIR}, rwConfs, func(t *testing.T, s state) { child, err := createFile(s.file, "test") if err != nil { t.Fatalf("%v: createFile() failed, err: %v", s, err) @@ -246,9 +246,13 @@ func TestReadWrite(t *testing.T) { if err != nil { t.Fatalf("%v: Walk(%s) failed, err: %v", s, "test", err) } - if _, _, _, err := l.Open(flags); err != nil { + fd, _, _, err := l.Open(flags) + if err != nil { t.Fatalf("%v: Open(%v) failed, err: %v", s, flags, err) } + if fd != nil { + defer fd.Close() + } if err := testReadWrite(l, flags, want); err != nil { t.Fatalf("%v: testReadWrite(%v) failed: %v", s, flags, err) } @@ -257,14 +261,14 @@ func TestReadWrite(t *testing.T) { } func TestCreate(t *testing.T) { - runCustom(t, []uint32{syscall.S_IFDIR}, rwConfs, func(t *testing.T, s state) { + runCustom(t, []uint32{unix.S_IFDIR}, rwConfs, func(t *testing.T, s state) { for i, flags := range allOpenFlags { _, l, _, _, err := s.file.Create(fmt.Sprintf("test-%d", i), flags, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())) if err != nil { t.Fatalf("%v, %v: WriteAt() failed, err: %v", s, flags, err) } - if err := testReadWrite(l, flags, []byte{}); err != nil { + if err := testReadWrite(l, flags, nil); err != nil { t.Fatalf("%v: testReadWrite(%v) failed: %v", s, flags, err) } } @@ -274,7 +278,7 @@ func TestCreate(t *testing.T) { // TestReadWriteDup tests that a file opened in any mode can be dup'ed and // reopened in any other mode. func TestReadWriteDup(t *testing.T) { - runCustom(t, []uint32{syscall.S_IFDIR}, rwConfs, func(t *testing.T, s state) { + runCustom(t, []uint32{unix.S_IFDIR}, rwConfs, func(t *testing.T, s state) { child, err := createFile(s.file, "test") if err != nil { t.Fatalf("%v: createFile() failed, err: %v", s, err) @@ -304,9 +308,13 @@ func TestReadWriteDup(t *testing.T) { t.Fatalf("%v: Walk() failed: %v", s, err) } defer dup.Close() - if _, _, _, err := dup.Open(dupFlags); err != nil { + fd, _, _, err := dup.Open(dupFlags) + if err != nil { t.Fatalf("%v: Open(%v) failed: %v", s, flags, err) } + if fd != nil { + defer fd.Close() + } if err := testReadWrite(dup, dupFlags, want); err != nil { t.Fatalf("%v: testReadWrite(%v) failed: %v", s, dupFlags, err) } @@ -316,19 +324,19 @@ func TestReadWriteDup(t *testing.T) { } func TestUnopened(t *testing.T) { - runCustom(t, []uint32{syscall.S_IFREG}, allConfs, func(t *testing.T, s state) { + runCustom(t, []uint32{unix.S_IFREG}, allConfs, func(t *testing.T, s state) { b := []byte("foobar") - if _, err := s.file.WriteAt(b, 0); err != syscall.EBADF { - t.Errorf("%v: WriteAt() should have failed, got: %v, expected: syscall.EBADF", s, err) + if _, err := s.file.WriteAt(b, 0); err != unix.EBADF { + t.Errorf("%v: WriteAt() should have failed, got: %v, expected: unix.EBADF", s, err) } - if _, err := s.file.ReadAt(b, 0); err != syscall.EBADF { - t.Errorf("%v: ReadAt() should have failed, got: %v, expected: syscall.EBADF", s, err) + if _, err := s.file.ReadAt(b, 0); err != unix.EBADF { + t.Errorf("%v: ReadAt() should have failed, got: %v, expected: unix.EBADF", s, err) } - if _, err := s.file.Readdir(0, 100); err != syscall.EBADF { - t.Errorf("%v: Readdir() should have failed, got: %v, expected: syscall.EBADF", s, err) + if _, err := s.file.Readdir(0, 100); err != unix.EBADF { + t.Errorf("%v: Readdir() should have failed, got: %v, expected: unix.EBADF", s, err) } - if err := s.file.FSync(); err != syscall.EBADF { - t.Errorf("%v: FSync() should have failed, got: %v, expected: syscall.EBADF", s, err) + if err := s.file.FSync(); err != unix.EBADF { + t.Errorf("%v: FSync() should have failed, got: %v, expected: unix.EBADF", s, err) } }) } @@ -338,7 +346,7 @@ func TestUnopened(t *testing.T) { // was open with O_PATH, but Open() was not checking for it and allowing the // control file to be reused. func TestOpenOPath(t *testing.T) { - runCustom(t, []uint32{syscall.S_IFREG}, rwConfs, func(t *testing.T, s state) { + runCustom(t, []uint32{unix.S_IFREG}, rwConfs, func(t *testing.T, s state) { // Fist remove all permissions on the file. if err := s.file.SetAttr(p9.SetAttrMask{Permissions: true}, p9.SetAttr{Permissions: p9.FileMode(0)}); err != nil { t.Fatalf("SetAttr(): %v", err) @@ -353,7 +361,7 @@ func TestOpenOPath(t *testing.T) { if newFile.(*localFile).controlReadable { t.Fatalf("control file didn't open with O_PATH: %+v", newFile) } - if _, _, _, err := newFile.Open(p9.ReadOnly); err != syscall.EACCES { + if _, _, _, err := newFile.Open(p9.ReadOnly); err != unix.EACCES { t.Fatalf("Open() should have failed, got: %v, wanted: EACCES", err) } }) @@ -375,7 +383,7 @@ func TestSetAttrPerm(t *testing.T) { valid := p9.SetAttrMask{Permissions: true} attr := p9.SetAttr{Permissions: 0777} got, err := SetGetAttr(s.file, valid, attr) - if s.fileType == syscall.S_IFLNK { + if s.fileType == unix.S_IFLNK { if err == nil { t.Fatalf("%v: SetGetAttr(valid, %v) should have failed", s, attr.Permissions) } @@ -396,7 +404,7 @@ func TestSetAttrSize(t *testing.T) { valid := p9.SetAttrMask{Size: true} attr := p9.SetAttr{Size: size} got, err := SetGetAttr(s.file, valid, attr) - if s.fileType == syscall.S_IFLNK || s.fileType == syscall.S_IFDIR { + if s.fileType == unix.S_IFLNK || s.fileType == unix.S_IFDIR { if err == nil { t.Fatalf("%v: SetGetAttr(valid, %v) should have failed", s, attr.Permissions) } @@ -478,9 +486,9 @@ func TestLink(t *testing.T) { } err = dir.Link(s.file, linkFile) - if s.fileType == syscall.S_IFDIR { - if err != syscall.EPERM { - t.Errorf("%v: Link(target, %s) should have failed, got: %v, expected: syscall.EPERM", s, linkFile, err) + if s.fileType == unix.S_IFDIR { + if err != unix.EPERM { + t.Errorf("%v: Link(target, %s) should have failed, got: %v, expected: unix.EPERM", s, linkFile, err) } return } @@ -491,9 +499,12 @@ func TestLink(t *testing.T) { } func TestROMountChecks(t *testing.T) { - const want = syscall.EROFS + const want = unix.EROFS + uid := p9.UID(os.Getuid()) + gid := p9.GID(os.Getgid()) + runCustom(t, allTypes, roConfs, func(t *testing.T, s state) { - if s.fileType != syscall.S_IFLNK { + if s.fileType != unix.S_IFLNK { if _, _, _, err := s.file.Open(p9.WriteOnly); err != want { t.Errorf("Open() should have failed, got: %v, expected: %v", err, want) } @@ -512,16 +523,16 @@ func TestROMountChecks(t *testing.T) { } } - if _, _, _, _, err := s.file.Create("some_file", p9.ReadWrite, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != want { + if _, _, _, _, err := s.file.Create("some_file", p9.ReadWrite, 0777, uid, gid); err != want { t.Errorf("Create() should have failed, got: %v, expected: %v", err, want) } - if _, err := s.file.Mkdir("some_dir", 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != want { + if _, err := s.file.Mkdir("some_dir", 0777, uid, gid); err != want { t.Errorf("MkDir() should have failed, got: %v, expected: %v", err, want) } if err := s.file.RenameAt("some_file", s.file, "other_file"); err != want { t.Errorf("Rename() should have failed, got: %v, expected: %v", err, want) } - if _, err := s.file.Symlink("some_place", "some_symlink", p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != want { + if _, err := s.file.Symlink("some_place", "some_symlink", uid, gid); err != want { t.Errorf("Symlink() should have failed, got: %v, expected: %v", err, want) } if err := s.file.UnlinkAt("some_file", 0); err != want { @@ -530,6 +541,9 @@ func TestROMountChecks(t *testing.T) { if err := s.file.Link(s.file, "some_link"); err != want { t.Errorf("Link() should have failed, got: %v, expected: %v", err, want) } + if _, err := s.file.Mknod("some-nod", 0777, 1, 2, uid, gid); err != want { + t.Errorf("Mknod() should have failed, got: %v, expected: %v", err, want) + } valid := p9.SetAttrMask{Size: true} attr := p9.SetAttr{Size: 0} @@ -541,16 +555,20 @@ func TestROMountChecks(t *testing.T) { func TestROMountPanics(t *testing.T) { conf := Config{ROMount: true, PanicOnWrite: true} + uid := p9.UID(os.Getuid()) + gid := p9.GID(os.Getgid()) + runCustom(t, allTypes, []Config{conf}, func(t *testing.T, s state) { - if s.fileType != syscall.S_IFLNK { + if s.fileType != unix.S_IFLNK { assertPanic(t, func() { s.file.Open(p9.WriteOnly) }) } - assertPanic(t, func() { s.file.Create("some_file", p9.ReadWrite, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())) }) - assertPanic(t, func() { s.file.Mkdir("some_dir", 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())) }) + assertPanic(t, func() { s.file.Create("some_file", p9.ReadWrite, 0777, uid, gid) }) + assertPanic(t, func() { s.file.Mkdir("some_dir", 0777, uid, gid) }) assertPanic(t, func() { s.file.RenameAt("some_file", s.file, "other_file") }) - assertPanic(t, func() { s.file.Symlink("some_place", "some_symlink", p9.UID(os.Getuid()), p9.GID(os.Getgid())) }) + assertPanic(t, func() { s.file.Symlink("some_place", "some_symlink", uid, gid) }) assertPanic(t, func() { s.file.UnlinkAt("some_file", 0) }) assertPanic(t, func() { s.file.Link(s.file, "some_link") }) + assertPanic(t, func() { s.file.Mknod("some-nod", 0777, 1, 2, uid, gid) }) valid := p9.SetAttrMask{Size: true} attr := p9.SetAttr{Size: 0} @@ -559,9 +577,9 @@ func TestROMountPanics(t *testing.T) { } func TestWalkNotFound(t *testing.T) { - runCustom(t, []uint32{syscall.S_IFDIR}, allConfs, func(t *testing.T, s state) { - if _, _, err := s.file.Walk([]string{"nobody-here"}); err != syscall.ENOENT { - t.Errorf("%v: Walk(%q) should have failed, got: %v, expected: syscall.ENOENT", s, "nobody-here", err) + runCustom(t, []uint32{unix.S_IFDIR}, allConfs, func(t *testing.T, s state) { + if _, _, err := s.file.Walk([]string{"nobody-here"}); err != unix.ENOENT { + t.Errorf("%v: Walk(%q) should have failed, got: %v, expected: unix.ENOENT", s, "nobody-here", err) } }) } @@ -580,7 +598,7 @@ func TestWalkDup(t *testing.T) { } func TestReaddir(t *testing.T) { - runCustom(t, []uint32{syscall.S_IFDIR}, rwConfs, func(t *testing.T, s state) { + runCustom(t, []uint32{unix.S_IFDIR}, rwConfs, func(t *testing.T, s state) { name := "dir" if _, err := s.file.Mkdir(name, 0777, p9.UID(os.Getuid()), p9.GID(os.Getgid())); err != nil { t.Fatalf("%v: MkDir(%s) failed, err: %v", s, name, err) @@ -705,7 +723,7 @@ func TestAttachInvalidType(t *testing.T) { defer os.RemoveAll(dir) fifo := filepath.Join(dir, "fifo") - if err := syscall.Mkfifo(fifo, 0755); err != nil { + if err := unix.Mkfifo(fifo, 0755); err != nil { t.Fatalf("Mkfifo(%q): %v", fifo, err) } @@ -766,16 +784,16 @@ func TestDoubleAttachError(t *testing.T) { } func TestTruncate(t *testing.T) { - runCustom(t, []uint32{syscall.S_IFDIR}, rwConfs, func(t *testing.T, s state) { + runCustom(t, []uint32{unix.S_IFDIR}, rwConfs, func(t *testing.T, s state) { child, err := createFile(s.file, "test") if err != nil { - t.Fatalf("createFile() failed, err: %v", err) + t.Fatalf("createFile() failed: %v", err) } defer child.Close() want := []byte("foobar") w, err := child.WriteAt(want, 0) if err != nil { - t.Fatalf("Write() failed, err: %v", err) + t.Fatalf("Write() failed: %v", err) } if w != len(want) { t.Fatalf("Write() was partial, got: %d, expected: %d", w, len(want)) @@ -783,12 +801,15 @@ func TestTruncate(t *testing.T) { _, l, err := s.file.Walk([]string{"test"}) if err != nil { - t.Fatalf("Walk(%s) failed, err: %v", "test", err) + t.Fatalf("Walk(%s) failed: %v", "test", err) } if _, _, _, err := l.Open(p9.ReadOnly | p9.OpenTruncate); err != nil { - t.Fatalf("Open() failed, err: %v", err) + t.Fatalf("Open() failed: %v", err) } _, mask, attr, err := l.GetAttr(p9.AttrMask{Size: true}) + if err != nil { + t.Fatalf("GetAttr() failed: %v", err) + } if !mask.Size { t.Fatalf("GetAttr() didn't return size: %+v", mask) } @@ -797,3 +818,27 @@ func TestTruncate(t *testing.T) { } }) } + +func TestMknod(t *testing.T) { + runCustom(t, []uint32{unix.S_IFDIR}, rwConfs, func(t *testing.T, s state) { + _, err := s.file.Mknod("test", p9.ModeRegular|0777, 1, 2, p9.UID(os.Getuid()), p9.GID(os.Getgid())) + if err != nil { + t.Fatalf("Mknod() failed: %v", err) + } + + _, f, err := s.file.Walk([]string{"test"}) + if err != nil { + t.Fatalf("Walk() failed: %v", err) + } + fd, _, _, err := f.Open(p9.ReadWrite) + if err != nil { + t.Fatalf("Open() failed: %v", err) + } + if fd != nil { + defer fd.Close() + } + if err := testReadWrite(f, p9.ReadWrite, nil); err != nil { + t.Fatalf("testReadWrite() failed: %v", err) + } + }) +} -- cgit v1.2.3