From 3d32a05a35bde4cfed861c274c32bfc55acc19c9 Mon Sep 17 00:00:00 2001 From: Kevin Krakauer Date: Fri, 2 Jul 2021 18:13:24 -0700 Subject: runsc: validate mount targets PiperOrigin-RevId: 382845950 --- runsc/cmd/chroot.go | 8 +- runsc/cmd/gofer.go | 26 +++-- runsc/specutils/safemount_test/BUILD | 23 ++++ runsc/specutils/safemount_test/safemount_runner.go | 117 +++++++++++++++++++++ runsc/specutils/safemount_test/safemount_test.go | 53 ++++++++++ runsc/specutils/specutils.go | 44 +++++++- 6 files changed, 252 insertions(+), 19 deletions(-) create mode 100644 runsc/specutils/safemount_test/BUILD create mode 100644 runsc/specutils/safemount_test/safemount_runner.go create mode 100644 runsc/specutils/safemount_test/safemount_test.go diff --git a/runsc/cmd/chroot.go b/runsc/cmd/chroot.go index e988247da..791a50135 100644 --- a/runsc/cmd/chroot.go +++ b/runsc/cmd/chroot.go @@ -30,7 +30,7 @@ func mountInChroot(chroot, src, dst, typ string, flags uint32) error { chrootDst := filepath.Join(chroot, dst) log.Infof("Mounting %q at %q", src, chrootDst) - if err := specutils.Mount(src, chrootDst, typ, flags); err != nil { + if err := specutils.Mount(src, chrootDst, typ, flags, "/proc"); err != nil { return fmt.Errorf("error mounting %q at %q: %v", src, chrootDst, err) } return nil @@ -70,11 +70,11 @@ func setUpChroot(pidns bool) error { // Convert all shared mounts into slave to be sure that nothing will be // propagated outside of our namespace. - if err := unix.Mount("", "/", "", unix.MS_SLAVE|unix.MS_REC, ""); err != nil { + if err := specutils.SafeMount("", "/", "", unix.MS_SLAVE|unix.MS_REC, "", "/proc"); err != nil { return fmt.Errorf("error converting mounts: %v", err) } - if err := unix.Mount("runsc-root", chroot, "tmpfs", unix.MS_NOSUID|unix.MS_NODEV|unix.MS_NOEXEC, ""); err != nil { + if err := specutils.SafeMount("runsc-root", chroot, "tmpfs", unix.MS_NOSUID|unix.MS_NODEV|unix.MS_NOEXEC, "", "/proc"); err != nil { return fmt.Errorf("error mounting tmpfs in choot: %v", err) } @@ -89,7 +89,7 @@ func setUpChroot(pidns bool) error { } } - if err := unix.Mount("", chroot, "", unix.MS_REMOUNT|unix.MS_RDONLY|unix.MS_BIND, ""); err != nil { + if err := specutils.SafeMount("", chroot, "", unix.MS_REMOUNT|unix.MS_RDONLY|unix.MS_BIND, "", "/proc"); err != nil { return fmt.Errorf("error remounting chroot in read-only: %v", err) } diff --git a/runsc/cmd/gofer.go b/runsc/cmd/gofer.go index 80da9c9a2..570df407c 100644 --- a/runsc/cmd/gofer.go +++ b/runsc/cmd/gofer.go @@ -265,7 +265,8 @@ func isReadonlyMount(opts []string) bool { func setupRootFS(spec *specs.Spec, conf *config.Config) error { // Convert all shared mounts into slaves to be sure that nothing will be // propagated outside of our namespace. - if err := unix.Mount("", "/", "", unix.MS_SLAVE|unix.MS_REC, ""); err != nil { + procPath := "/proc" + if err := specutils.SafeMount("", "/", "", unix.MS_SLAVE|unix.MS_REC, "", procPath); err != nil { Fatalf("error converting mounts: %v", err) } @@ -278,21 +279,24 @@ func setupRootFS(spec *specs.Spec, conf *config.Config) error { // We need a directory to construct a new root and we know that // runsc can't start without /proc, so we can use it for this. flags := uintptr(unix.MS_NOSUID | unix.MS_NODEV | unix.MS_NOEXEC) - if err := unix.Mount("runsc-root", "/proc", "tmpfs", flags, ""); err != nil { + if err := specutils.SafeMount("runsc-root", "/proc", "tmpfs", flags, "", procPath); err != nil { Fatalf("error mounting tmpfs: %v", err) } // Prepare tree structure for pivot_root(2). os.Mkdir("/proc/proc", 0755) os.Mkdir("/proc/root", 0755) + // This cannot use SafeMount because there's no available procfs. But we + // know that /proc is an empty tmpfs mount, so this is safe. if err := unix.Mount("runsc-proc", "/proc/proc", "proc", flags|unix.MS_RDONLY, ""); err != nil { Fatalf("error mounting proc: %v", err) } root = "/proc/root" + procPath = "/proc/proc" } // Mount root path followed by submounts. - if err := unix.Mount(spec.Root.Path, root, "bind", unix.MS_BIND|unix.MS_REC, ""); err != nil { + if err := specutils.SafeMount(spec.Root.Path, root, "bind", unix.MS_BIND|unix.MS_REC, "", procPath); err != nil { return fmt.Errorf("mounting root on root (%q) err: %v", root, err) } @@ -300,12 +304,12 @@ func setupRootFS(spec *specs.Spec, conf *config.Config) error { if spec.Linux != nil && spec.Linux.RootfsPropagation != "" { flags = specutils.PropOptionsToFlags([]string{spec.Linux.RootfsPropagation}) } - if err := unix.Mount("", root, "", uintptr(flags), ""); err != nil { + if err := specutils.SafeMount("", root, "", uintptr(flags), "", procPath); err != nil { return fmt.Errorf("mounting root (%q) with flags: %#x, err: %v", root, flags, err) } // Replace the current spec, with the clean spec with symlinks resolved. - if err := setupMounts(conf, spec.Mounts, root); err != nil { + if err := setupMounts(conf, spec.Mounts, root, procPath); err != nil { Fatalf("error setting up FS: %v", err) } @@ -327,7 +331,7 @@ func setupRootFS(spec *specs.Spec, conf *config.Config) error { // to make it read-only for extra safety. log.Infof("Remounting root as readonly: %q", root) flags := uintptr(unix.MS_BIND | unix.MS_REMOUNT | unix.MS_RDONLY | unix.MS_REC) - if err := unix.Mount(root, root, "bind", flags, ""); err != nil { + if err := specutils.SafeMount(root, root, "bind", flags, "", procPath); err != nil { return fmt.Errorf("remounting root as read-only with source: %q, target: %q, flags: %#x, err: %v", root, root, flags, err) } } @@ -343,10 +347,10 @@ func setupRootFS(spec *specs.Spec, conf *config.Config) error { return nil } -// setupMounts binds mount all mounts specified in the spec in their correct +// setupMounts bind mounts all mounts specified in the spec in their correct // location inside root. It will resolve relative paths and symlinks. It also // creates directories as needed. -func setupMounts(conf *config.Config, mounts []specs.Mount, root string) error { +func setupMounts(conf *config.Config, mounts []specs.Mount, root, procPath string) error { for _, m := range mounts { if !specutils.Is9PMount(m, conf.VFS2) { continue @@ -364,14 +368,14 @@ func setupMounts(conf *config.Config, mounts []specs.Mount, root string) error { } log.Infof("Mounting src: %q, dst: %q, flags: %#x", m.Source, dst, flags) - if err := specutils.Mount(m.Source, dst, m.Type, flags); err != nil { - return fmt.Errorf("mounting %v: %v", m, err) + if err := specutils.Mount(m.Source, dst, m.Type, flags, procPath); err != nil { + return fmt.Errorf("mounting %+v: %v", m, err) } // Set propagation options that cannot be set together with other options. flags = specutils.PropOptionsToFlags(m.Options) if flags != 0 { - if err := unix.Mount("", dst, "", uintptr(flags), ""); err != nil { + if err := specutils.SafeMount("", dst, "", uintptr(flags), "", procPath); err != nil { return fmt.Errorf("mount dst: %q, flags: %#x, err: %v", dst, flags, err) } } diff --git a/runsc/specutils/safemount_test/BUILD b/runsc/specutils/safemount_test/BUILD new file mode 100644 index 000000000..c39c40492 --- /dev/null +++ b/runsc/specutils/safemount_test/BUILD @@ -0,0 +1,23 @@ +load("//tools:defs.bzl", "go_binary", "go_test") + +package(licenses = ["notice"]) + +go_test( + name = "safemount_test", + size = "small", + srcs = ["safemount_test.go"], + data = [":safemount_runner"], + deps = [ + "//pkg/test/testutil", + "@org_golang_x_sys//unix:go_default_library", + ], +) + +go_binary( + name = "safemount_runner", + srcs = ["safemount_runner.go"], + deps = [ + "//runsc/specutils", + "@org_golang_x_sys//unix:go_default_library", + ], +) diff --git a/runsc/specutils/safemount_test/safemount_runner.go b/runsc/specutils/safemount_test/safemount_runner.go new file mode 100644 index 000000000..b23193033 --- /dev/null +++ b/runsc/specutils/safemount_test/safemount_runner.go @@ -0,0 +1,117 @@ +// Copyright 2021 The gVisor Authors. +// +// 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. + +// safemount_runner is used to test the SafeMount function. Because use of +// unix.Mount requires privilege, tests must launch this process with +// CLONE_NEWNS and CLONE_NEWUSER. +package main + +import ( + "errors" + "fmt" + "log" + "os" + "path/filepath" + + "golang.org/x/sys/unix" + "gvisor.dev/gvisor/runsc/specutils" +) + +func main() { + // The test temporary directory is the first argument. + tempdir := os.Args[1] + + tcs := []struct { + name string + testfunc func() error + }{{ + name: "unix.Mount to folder succeeds", + testfunc: func() error { + dir2Path := filepath.Join(tempdir, "subdir2") + if err := unix.Mount(filepath.Join(tempdir, "subdir"), dir2Path, "bind", unix.MS_BIND, ""); err != nil { + return fmt.Errorf("mount: %v", err) + } + return unix.Unmount(dir2Path, unix.MNT_DETACH) + }, + }, { + // unix.Mount doesn't care whether the target is a symlink. + name: "unix.Mount to symlink succeeds", + testfunc: func() error { + symlinkPath := filepath.Join(tempdir, "symlink") + if err := unix.Mount(filepath.Join(tempdir, "subdir"), symlinkPath, "bind", unix.MS_BIND, ""); err != nil { + return fmt.Errorf("mount: %v", err) + } + return unix.Unmount(symlinkPath, unix.MNT_DETACH) + }, + }, { + name: "SafeMount to folder succeeds", + testfunc: func() error { + dir2Path := filepath.Join(tempdir, "subdir2") + if err := specutils.SafeMount(filepath.Join(tempdir, "subdir"), dir2Path, "bind", unix.MS_BIND, "", "/proc"); err != nil { + return fmt.Errorf("SafeMount: %v", err) + } + return unix.Unmount(dir2Path, unix.MNT_DETACH) + }, + }, { + name: "SafeMount to symlink fails", + testfunc: func() error { + err := specutils.SafeMount(filepath.Join(tempdir, "subdir"), filepath.Join(tempdir, "symlink"), "bind", unix.MS_BIND, "", "/proc") + if err == nil { + return fmt.Errorf("SafeMount didn't fail, but should have") + } + var symErr *specutils.ErrSymlinkMount + if !errors.As(err, &symErr) { + return fmt.Errorf("expected SafeMount to fail with ErrSymlinkMount, but got: %v", err) + } + return nil + }, + }} + + for _, tc := range tcs { + if err := runTest(tempdir, tc.testfunc); err != nil { + log.Fatalf("failed test %q: %v", tc.name, err) + } + } +} + +// runTest runs testfunc with the following directory structure: +// tempdir/ +// subdir/ +// subdir2/ +// symlink --> ./subdir2 +func runTest(tempdir string, testfunc func() error) error { + // Create tempdir/subdir/. + dirPath := filepath.Join(tempdir, "subdir") + if err := os.Mkdir(dirPath, 0777); err != nil { + return fmt.Errorf("os.Mkdir(%s, 0777)", dirPath) + } + defer os.Remove(dirPath) + + // Create tempdir/subdir2/. + dir2Path := filepath.Join(tempdir, "subdir2") + if err := os.Mkdir(dir2Path, 0777); err != nil { + return fmt.Errorf("os.Mkdir(%s, 0777)", dir2Path) + } + defer os.Remove(dir2Path) + + // Create tempdir/symlink, which points to ./subdir2. + symlinkPath := filepath.Join(tempdir, "symlink") + if err := os.Symlink("./subdir2", symlinkPath); err != nil { + return fmt.Errorf("failed to create symlink %s: %v", symlinkPath, err) + } + defer os.Remove(symlinkPath) + + // Run the actual test. + return testfunc() +} diff --git a/runsc/specutils/safemount_test/safemount_test.go b/runsc/specutils/safemount_test/safemount_test.go new file mode 100644 index 000000000..8820978c4 --- /dev/null +++ b/runsc/specutils/safemount_test/safemount_test.go @@ -0,0 +1,53 @@ +// Copyright 2021 The gVisor Authors. +// +// 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 safemount_test + +import ( + "os" + "os/exec" + "syscall" + "testing" + + "golang.org/x/sys/unix" + "gvisor.dev/gvisor/pkg/test/testutil" +) + +func TestSafeMount(t *testing.T) { + // We run the actual tests in another process, as we need CAP_SYS_ADMIN to + // call mount(2). The new process runs in its own user and mount namespaces. + runner, err := testutil.FindFile("runsc/specutils/safemount_test/safemount_runner") + if err != nil { + t.Fatalf("failed to find test runner binary: %v", err) + } + cmd := exec.Command(runner, t.TempDir()) + cmd.SysProcAttr = &unix.SysProcAttr{ + Cloneflags: unix.CLONE_NEWNS | unix.CLONE_NEWUSER, + UidMappings: []syscall.SysProcIDMap{ + {ContainerID: 0, HostID: os.Getuid(), Size: 1}, + }, + GidMappings: []syscall.SysProcIDMap{ + {ContainerID: 0, HostID: os.Getgid(), Size: 1}, + }, + GidMappingsEnableSetgroups: false, + Credential: &syscall.Credential{ + Uid: 0, + Gid: 0, + }, + } + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("failed running %s with error: %v\ntest output:\n%s", cmd, err, output) + } +} diff --git a/runsc/specutils/specutils.go b/runsc/specutils/specutils.go index c228d6299..48a574373 100644 --- a/runsc/specutils/specutils.go +++ b/runsc/specutils/specutils.go @@ -217,7 +217,7 @@ func ReadMounts(f *os.File) ([]specs.Mount, error) { } var mounts []specs.Mount if err := json.Unmarshal(bytes, &mounts); err != nil { - return nil, fmt.Errorf("error unmarshaling mounts: %v\n %s", err, string(bytes)) + return nil, fmt.Errorf("error unmarshaling mounts: %v\nJSON bytes:\n%s", err, string(bytes)) } return mounts, nil } @@ -434,8 +434,10 @@ func DebugLogFile(logPattern, command, test string) (*os.File, error) { return os.OpenFile(logPattern, os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0664) } -// Mount creates the mount point and calls Mount with the given flags. -func Mount(src, dst, typ string, flags uint32) error { +// Mount creates the mount point and calls Mount with the given flags. procPath +// is the path to procfs. If it is "", procfs is assumed to be mounted at +// /proc. +func Mount(src, dst, typ string, flags uint32, procPath string) error { // Create the mount point inside. The type must be the same as the // source (file or directory). var isDir bool @@ -468,12 +470,46 @@ func Mount(src, dst, typ string, flags uint32) error { } // Do the mount. - if err := unix.Mount(src, dst, typ, uintptr(flags), ""); err != nil { + if err := SafeMount(src, dst, typ, uintptr(flags), "", procPath); err != nil { return fmt.Errorf("mount(%q, %q, %d) failed: %v", src, dst, flags, err) } return nil } +// ErrSymlinkMount is returned by SafeMount when the mount destination is found +// to be a symlink. +type ErrSymlinkMount struct { + error +} + +// SafeMount is like unix.Mount, but will fail if dst is a symlink. procPath is +// the path to procfs. If it is "", procfs is assumed to be mounted at /proc. +func SafeMount(src, dst, fstype string, flags uintptr, data, procPath string) error { + // Open the destination. + fd, err := unix.Open(dst, unix.O_PATH|unix.O_CLOEXEC, 0) + if err != nil { + return fmt.Errorf("failed to safely mount: Open(%s, _, _): %w", dst, err) + } + defer unix.Close(fd) + + // Use /proc/self/fd/ to verify that we opened the intended destination. This + // guards against dst being a symlink, in which case we could accidentally + // mount over the symlink's target. + if procPath == "" { + procPath = "/proc" + } + safePath := filepath.Join(procPath, "self/fd", strconv.Itoa(fd)) + target, err := os.Readlink(safePath) + if err != nil { + return fmt.Errorf("failed to safely mount: Readlink(%s): %w", safePath, err) + } + if dst != target { + return &ErrSymlinkMount{fmt.Errorf("failed to safely mount: expected to open %s, but found %s", dst, target)} + } + + return unix.Mount(src, safePath, fstype, flags, data) +} + // ContainsStr returns true if 'str' is inside 'strs'. func ContainsStr(strs []string, str string) bool { for _, s := range strs { -- cgit v1.2.3