Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
openSUSE:Leap:15.5:Update
kubevirt.25865
0007-improve-non-root-path-handling.patch
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File 0007-improve-non-root-path-handling.patch of Package kubevirt.25865
From e4851f8251fe73542231a41a1f75606fb1443e30 Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Tue, 19 Jul 2022 11:00:03 +0200 Subject: [PATCH 01/16] Add a safepath package to encapsule path operations Privileged virt-handler operations inside non-root virt-launcher pods need extra precautions to ensure that symbolic links do not lead to pointing to unintended locations. The safepath package introduces path operations based on `*At` unit file syscall (OpenAt, MkDirAt, ...) which have built-in support for not following symlink and therefore allow atomic check- and modify operations. Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit ba30ed6a883d6801c3c79544d6282e5fd3ab0434) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit ea57f1928539b5db4968d5ebf86b634c14f1ab34) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit de0ef67b6363caf8a0ec40d754745f4a2742784b) Signed-off-by: Jed Lejosne <jed@redhat.com> --- pkg/safepath/BUILD.bazel | 31 ++ pkg/safepath/safepath.go | 433 ++++++++++++++++++++++++++++ pkg/safepath/safepath_linux.go | 103 +++++++ pkg/safepath/safepath_suite_test.go | 30 ++ pkg/safepath/safepath_test.go | 325 +++++++++++++++++++++ 5 files changed, 922 insertions(+) create mode 100644 pkg/safepath/BUILD.bazel create mode 100644 pkg/safepath/safepath.go create mode 100644 pkg/safepath/safepath_linux.go create mode 100644 pkg/safepath/safepath_suite_test.go create mode 100644 pkg/safepath/safepath_test.go diff --git a/pkg/safepath/BUILD.bazel b/pkg/safepath/BUILD.bazel new file mode 100644 index 000000000..9095d1b98 --- /dev/null +++ b/pkg/safepath/BUILD.bazel @@ -0,0 +1,31 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = [ + "safepath.go", + "safepath_linux.go", + ], + importpath = "kubevirt.io/kubevirt/pkg/safepath", + visibility = ["//visibility:public"], + deps = [ + "//pkg/unsafepath:go_default_library", + "//vendor/golang.org/x/sys/unix:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = [ + "safepath_suite_test.go", + "safepath_test.go", + ], + embed = [":go_default_library"], + deps = [ + "//pkg/unsafepath:go_default_library", + "//staging/src/kubevirt.io/client-go/testutils:go_default_library", + "//vendor/github.com/onsi/ginkgo:go_default_library", + "//vendor/github.com/onsi/ginkgo/extensions/table:go_default_library", + "//vendor/github.com/onsi/gomega:go_default_library", + ], +) diff --git a/pkg/safepath/safepath.go b/pkg/safepath/safepath.go new file mode 100644 index 000000000..99822067b --- /dev/null +++ b/pkg/safepath/safepath.go @@ -0,0 +1,433 @@ +package safepath + +import ( + "container/list" + "fmt" + "net" + "os" + "path/filepath" + "strings" + "syscall" + + "kubevirt.io/kubevirt/pkg/unsafepath" + + "golang.org/x/sys/unix" +) + +// JoinAndResolveWithRelativeRoot joins an absolute relativeRoot base path with +// additional elements which have to be kept below the relativeRoot base. +// Relative and absolute links will be resolved relative to the provided rootBase +// and can not escape it. +func JoinAndResolveWithRelativeRoot(rootBase string, elems ...string) (*Path, error) { + // ensure that rootBase is absolute + if !filepath.IsAbs(rootBase) { + return nil, fmt.Errorf("basepath is not absolute: %q", rootBase) + } + + path := pathRoot + fifo := newLimitedFifo(256) + for i := len(elems) - 1; i >= 0; i-- { + if err := fifo.push(strings.Split(filepath.Clean(elems[i]), pathSeparator)); err != nil { + return nil, err + } + } + + for !fifo.empty() { + child := fifo.pop() + var link string + var err error + + path, link, err = advance(rootBase, path, child) + if err != nil { + return nil, err + } + if link != "" { + if err := fifo.push(strings.Split(link, pathSeparator)); err != nil { + return nil, err + } + } + } + + // Assert that the result is indeed a clean path in the expected format + // at this point in time. + finalPath := newPath(rootBase, path) + fd, err := OpenAtNoFollow(finalPath) + if err != nil { + return nil, err + } + _ = fd.Close() + + return finalPath, nil +} + +type fifo struct { + ops uint + store *list.List + maxOps uint +} + +func (f *fifo) push(pathElements []string) error { + for i := len(pathElements) - 1; i >= 0; i-- { + if f.ops > f.maxOps { + return fmt.Errorf("more than %v path elements evaluated", f.maxOps) + } + if pathElements[i] == "" { + continue + } + f.ops++ + f.store.PushFront(pathElements[i]) + } + return nil +} + +func (f *fifo) pop() string { + if val := f.store.Front(); val != nil { + f.store.Remove(val) + return val.Value.(string) + } + return "" +} + +func (f *fifo) empty() bool { + return f.store.Len() == 0 +} + +// newLimitedFifo creates a fifo with a maximum enqueue limit to +// avoid abuse on filepath operations. +func newLimitedFifo(maxOps uint) *fifo { + return &fifo{ + store: list.New(), + maxOps: maxOps, + } +} + +// OpenAtNoFollow safely opens a filedescriptor to a path relative to +// rootBase. Any symlink encountered will be treated as invalid and the operation will be aborted. +// This works best together with a path first resolved with JoinAndResolveWithRelativeRoot +// which can resolve relative paths and symlinks. +func OpenAtNoFollow(path *Path) (file *File, err error) { + fd, err := open(path.rootBase) + if err != nil { + return nil, err + } + for _, child := range strings.Split(filepath.Clean(path.relativePath), pathSeparator) { + if child == "" { + continue + } + newfd, err := openat(fd, child) + _ = syscall.Close(fd) // always close the parent after the lookup + if err != nil { + return nil, err + } + fd = newfd + } + return &File{fd: fd, path: path}, nil +} + +func ChmodAtNoFollow(path *Path, mode os.FileMode) error { + f, err := OpenAtNoFollow(path) + if err != nil { + return err + } + defer f.Close() + return os.Chmod(f.SafePath(), mode) +} + +func ChownAtNoFollow(path *Path, uid, gid int) error { + f, err := OpenAtNoFollow(path) + if err != nil { + return err + } + defer f.Close() + return os.Chown(f.SafePath(), uid, gid) +} + +func ChpermAtNoFollow(path *Path, uid, gid int, mode os.FileMode) error { + // first set the ownership, to avoid that someone may change back the file mode + // after we set it. This is necessary if the file got somehow created without + // the right owners, maybe with malicious intent. + if err := ChownAtNoFollow(path, uid, gid); err != nil { + return err + } + if err := ChmodAtNoFollow(path, mode); err != nil { + return err + } + return nil +} + +func MkdirAtNoFollow(path *Path, dirName string, mode os.FileMode) error { + if err := isSingleElement(dirName); err != nil { + return err + } + f, err := OpenAtNoFollow(path) + if err != nil { + return err + } + defer f.Close() + if err := unix.Mkdirat(f.fd, dirName, uint32(mode)); err != nil { + return err + } + return nil +} + +// TouchAtNoFollow safely touches a file relative to +// rootBase. The additional elements form the relative path. Any symlink +// encountered will be treated as invalid and the operation will be aborted. +// This works best together with a path first resolved with JoinAndResolveWithRelativeRoot +// which can resolve relative paths to their real path without symlinks. +// If the target file exists already, the function will fail. +func TouchAtNoFollow(path *Path, fileName string, mode os.FileMode) (err error) { + if err := isSingleElement(fileName); err != nil { + return err + } + parent, err := OpenAtNoFollow(path) + if err != nil { + return err + } + defer parent.Close() + fd, err := touchat(parent.fd, fileName, uint32(mode)) + if err != nil { + return err + } + _ = syscall.Close(fd) + return nil +} + +func MknodAtNoFollow(path *Path, fileName string, mode os.FileMode, dev uint64) (err error) { + if err := isSingleElement(fileName); err != nil { + return err + } + parent, err := OpenAtNoFollow(path) + if err != nil { + return err + } + defer parent.Close() + return mknodat(parent.fd, fileName, uint32(mode), dev) +} + +func StatAtNoFollow(path *Path) (os.FileInfo, error) { + pathFd, err := OpenAtNoFollow(path) + if err != nil { + return nil, err + } + defer pathFd.Close() + return os.Stat(pathFd.SafePath()) +} + +type File struct { + fd int + path *Path +} + +func (f *File) Close() error { + return syscall.Close(f.fd) +} + +func (f *File) String() string { + return f.Path().String() +} + +// SafePath returns a path pointing to the associated file descriptor. +// It is safe to reuse this path without additional checks. The kernel +// will ensure that this path always points to the resolved file. +// To operate on the file just use os.Open and related calls. +func (f *File) SafePath() string { + return path(f.fd) +} + +func (f *File) Path() *Path { + return f.path +} + +// Path is a path which was at the time of creation a real path +// re +type Path struct { + rootBase string + relativePath string +} + +// Raw returns an "unsafe" path. It's properties are not safe to use without certain precautions. +// It exposes no access functions. All access happens via functions in the "unsafepath" package. +func (p *Path) Raw() *unsafepath.Path { + return unsafepath.New(p.rootBase, p.relativePath) +} + +func (p *Path) IsRoot() bool { + return unsafepath.UnsafeAbsolute(p.Raw()) == pathRoot +} + +// AppendAndResolveWithRelativeRoot returns a new path with the passed elements resolve relative +// to the current absolute path. +func (p *Path) AppendAndResolveWithRelativeRoot(relativeRootElems ...string) (*Path, error) { + tmpPath, err := JoinAndResolveWithRelativeRoot(unsafepath.UnsafeAbsolute(p.Raw()), relativeRootElems...) + if err != nil { + return nil, err + } + + newPath := newPath(p.rootBase, filepath.Join(p.relativePath, tmpPath.relativePath)) + fd, err := OpenAtNoFollow(newPath) + if err != nil { + return nil, err + } + _ = fd.Close() + + return newPath, err +} + +func (p *Path) String() string { + return fmt.Sprintf("root: %v, relative: %v", p.rootBase, p.relativePath) +} + +// ExecuteNoFollow opens the file in question and provides the file descriptor path as safePath string. +// This safePath string can be (re)opened with normal os.* operations. The file descriptor path is +// managed by the kernel and there is no way to inject malicious symlinks. +func (p *Path) ExecuteNoFollow(callback func(safePath string) error) error { + f, err := OpenAtNoFollow(p) + if err != nil { + return err + } + defer f.Close() + return callback(f.SafePath()) +} + +// DirNoFollow returns the parent directory of the safepath.Path as safepath.Path. +func (p *Path) DirNoFollow() (*Path, error) { + if len(p.relativePath) == 0 { + return nil, fmt.Errorf("already at relative root, can't get parent") + } + newPath := newPath(p.rootBase, filepath.Dir(p.relativePath)) + return newPath, nil +} + +// Base returns the basename of the relative untrusted part of the safepath. +func (p *Path) Base() (string, error) { + if len(p.relativePath) == 0 { + return "", fmt.Errorf("already at relative root, can't get parent") + } + return filepath.Base(p.relativePath), nil +} + +func newPath(rootBase, relativePath string) *Path { + return &Path{ + rootBase: rootBase, + relativePath: filepath.Join("/", relativePath), + } +} + +// NewFileNoFollow assumes that a real path to a file is given. It will validate that +// the file is indeed absolute by doing the following checks: +// * ensure that the path is absolute +// * ensure that the path does not container relative path elements +// * ensure that no symlinks are provided +// It will return the opened file which contains a link to a safe-to-use path +// to the file, which can't be tampered with. To operate on the file just use os.Open and related calls. +func NewFileNoFollow(path string) (*File, error) { + if filepath.Clean(path) != path || !filepath.IsAbs(path) { + return nil, fmt.Errorf("path %q must be absolute and must not contain relative elements", path) + } + p := newPath("/", path) + return OpenAtNoFollow(p) +} + +// NewPathNoFollow is a convenience method to get out of a supposedly link-free path a safepath.Path. +// If there is a symlink included the command will fail. +func NewPathNoFollow(path string) (*Path, error) { + fd, err := NewFileNoFollow(path) + if err != nil { + return nil, err + } + defer fd.Close() + return fd.Path(), nil +} + +// JoinNoFollow joins the root path with the given additional path. +// If the additional path element is not a real path (like containing symlinks), it fails. +func JoinNoFollow(rootPath *Path, path string) (*Path, error) { + if filepath.Clean(path) != path || path == "" { + return nil, fmt.Errorf("path %q must not contain relative elements and must not be empty", path) + } + p := newPath(unsafepath.UnsafeAbsolute(rootPath.Raw()), path) + f, err := OpenAtNoFollow(p) + if err != nil { + return nil, err + } + return f.Path(), f.Close() +} + +func isSingleElement(path string) error { + cleanedPath := filepath.Clean(path) + if cleanedPath != path || strings.ContainsAny(path, pathSeparator) { + return fmt.Errorf("path %q must be a single non-relative path segment", path) + } + switch path { + case "", "..", ".": + return fmt.Errorf("path %q must be a single non-relative path segment", path) + default: + return nil + } +} + +// UnlinkAtNoFollow allows deleting the specified file or directory (directory must be empty to succeed). +func UnlinkAtNoFollow(path *Path) error { + parent, err := path.DirNoFollow() + if err != nil { + return err + } + basename, err := path.Base() + if err != nil { + return nil + } + info, err := StatAtNoFollow(path) + if err != nil { + return err + } + fd, err := OpenAtNoFollow(parent) + if err != nil { + return err + } + defer fd.Close() + if info.IsDir() { + // if dir is empty we can delete it with AT_REMOVEDIR + return unlinkat(fd.fd, basename, unix.AT_REMOVEDIR) + } else { + return unlinkat(fd.fd, basename, 0) + } +} + +// ListenUnixNoFollow safely creates a socket in user-owned path +// Since there exists no socketat on unix, first a safe delete is performed, +// then the socket is created. +func ListenUnixNoFollow(socketDir *Path, socketName string) (net.Listener, error) { + if err := isSingleElement(socketName); err != nil { + return nil, err + } + + addr, err := net.ResolveUnixAddr("unix", filepath.Join(unsafepath.UnsafeAbsolute(socketDir.Raw()), socketName)) + if err != nil { + return nil, err + } + + socketPath, err := JoinNoFollow(socketDir, socketName) + if err == nil { + // This ensures that we don't allow unlinking arbitrary files + if err := UnlinkAtNoFollow(socketPath); err != nil { + return nil, err + } + } else if !os.IsNotExist(err) { + return nil, err + } + + listener, err := net.ListenUnix("unix", addr) + if err != nil { + return nil, err + } + + // Ensure that the socket path is a real path + // this does not 100% remove the chance of + // having a socket created at the wrong place, but it makes it unlikely + _, err = JoinNoFollow(socketDir, socketName) + if err != nil { + return nil, err + } + return listener, nil +} diff --git a/pkg/safepath/safepath_linux.go b/pkg/safepath/safepath_linux.go new file mode 100644 index 000000000..126fcbc5c --- /dev/null +++ b/pkg/safepath/safepath_linux.go @@ -0,0 +1,103 @@ +// +build linux + +package safepath + +import ( + "fmt" + "io/fs" + "os" + "path/filepath" + "strings" + "syscall" + + "golang.org/x/sys/unix" +) + +const pathSeparator = string(os.PathSeparator) +const pathRoot = string(os.PathSeparator) + +// advance will try to add the child to the parent. If it is a relative symlink it will resolve it +// and return the parent with the new symlink. If it is an absolute symlink, parent will be reset to '/' +// and returned together with the absolute symlink. If the joined result is no symlink, the joined result will +// be returned as the new parent. +func advance(rootBase string, parent string, child string) (string, string, error) { + // Ensure parent is absolute and never empty + parent = filepath.Clean(parent) + if !filepath.IsAbs(parent) { + return "", "", fmt.Errorf("parent path %v must be absolute", parent) + } + + if strings.Contains(child, pathSeparator) { + return "", "", fmt.Errorf("child %q must not contain a path separator", child) + } + + // Deal with relative path elements like '.', '//' and '..' + // Since parent is absolute, worst case we get '/' as result + path := filepath.Join(parent, child) + + if path == rootBase { + // don't evaluate the root itself, since rootBase is allowed to be a symlink + return path, "", nil + } + + fi, err := os.Lstat(filepath.Join(rootBase, path)) + if err != nil { + return "", "", err + } + + if fi.Mode()&fs.ModeSymlink == 0 { + // no symlink, we are done, return the joined result of parent and child + return filepath.Clean(path), "", nil + } + + link, err := os.Readlink(filepath.Join(rootBase, path)) + if err != nil { + return "", "", err + } + + if filepath.IsAbs(link) { + // the link is absolute, let's reset the parent and the discovered link path + return pathRoot, filepath.Clean(link), nil + } else { + // on relative links, don't advance parent and return the link + return parent, filepath.Clean(link), nil + } +} + +// openat helps traversing a path without following symlinks +// to ensure safe path references on user-owned paths by privileged processes +func openat(dirfd int, path string) (fd int, err error) { + if err := isSingleElement(path); err != nil { + return -1, err + } + return unix.Openat(dirfd, path, unix.O_NOFOLLOW|unix.O_PATH, 0) +} + +func unlinkat(dirfd int, path string, flags int) error { + if err := isSingleElement(path); err != nil { + return err + } + return unix.Unlinkat(dirfd, path, flags) +} + +func touchat(dirfd int, path string, mode uint32) (fd int, err error) { + if err := isSingleElement(path); err != nil { + return -1, err + } + return unix.Openat(dirfd, path, unix.O_NOFOLLOW|syscall.O_CREAT|syscall.O_EXCL, mode) +} + +func mknodat(dirfd int, path string, mode uint32, dev uint64) (err error) { + if err := isSingleElement(path); err != nil { + return err + } + return unix.Mknodat(dirfd, path, mode, int(dev)) +} + +func open(path string) (fd int, err error) { + return syscall.Open(path, unix.O_PATH, 0) +} + +func path(fd int) string { + return fmt.Sprintf("/proc/self/fd/%d", fd) +} diff --git a/pkg/safepath/safepath_suite_test.go b/pkg/safepath/safepath_suite_test.go new file mode 100644 index 000000000..92f1da11f --- /dev/null +++ b/pkg/safepath/safepath_suite_test.go @@ -0,0 +1,30 @@ +/* + * This file is part of the KubeVirt project + * + * 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. + * + * Copyright 2018 Red Hat, Inc. + * + */ + +package safepath + +import ( + "testing" + + "kubevirt.io/client-go/testutils" +) + +func TestSafePath(t *testing.T) { + testutils.KubeVirtTestSuiteSetup(t) +} diff --git a/pkg/safepath/safepath_test.go b/pkg/safepath/safepath_test.go new file mode 100644 index 000000000..2353d44fb --- /dev/null +++ b/pkg/safepath/safepath_test.go @@ -0,0 +1,325 @@ +package safepath + +import ( + "fmt" + "io/fs" + "io/ioutil" + "os" + "os/user" + "path/filepath" + "strconv" + "syscall" + + "github.com/onsi/ginkgo/extensions/table" + + "kubevirt.io/kubevirt/pkg/unsafepath" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +type pathBuilder struct { + segments [][]string + relativeRoot string + systemRoot string +} + +const tempDirPrefix = "safepath-" + +// Path adds a new path segment to the final path to construct +func (p *pathBuilder) Path(path string) *pathBuilder { + p.segments = append(p.segments, []string{path}) + return p +} + +// Link adds a path segemtn and a link location to the final path to construct. +// The link target can be an absolute or a relative path. +func (p *pathBuilder) Link(path string, target string) *pathBuilder { + p.segments = append(p.segments, []string{path, target}) + return p +} + +// new returns a new path builder with the given relative root prefix. +func new(root string) *pathBuilder { + return &pathBuilder{segments: [][]string{}, relativeRoot: root} +} + +// RelativeRoot returns the final full relative root path. +// Must be called after Builder() to be valid. +func (p *pathBuilder) RelativeRoot() string { + return filepath.Join(p.systemRoot, p.relativeRoot) +} + +// SystemRoot returns the emulated system root path, where the +// RelativeRoot path is a child of. +// Must be called after Builder() to be valid. +func (p *pathBuilder) SystemRoot() string { + return p.systemRoot +} + +// Build the defined path. Absolute links are prefixed wit the SystemRoot which +// will be the base of a ginkgo managed tmp directory. +func (p *pathBuilder) Build() (string, error) { + var err error + p.systemRoot, err = ioutil.TempDir("", tempDirPrefix) + Expect(err).ToNot(HaveOccurred()) + relativeRoot := filepath.Join(p.systemRoot, p.relativeRoot) + parent := relativeRoot + if err := os.MkdirAll(parent, os.ModePerm); err != nil { + return "", err + } + for _, elem := range p.segments { + parent = filepath.Join(parent, elem[0]) + if len(elem) == 2 { + link := elem[1] + if err := os.Symlink(link, parent); err != nil { + return "", err + } + } else { + if err := os.MkdirAll(parent, os.ModePerm); err != nil { + return "", err + } + } + } + + relativePath := "" + for _, elem := range p.segments { + relativePath = filepath.Join(relativePath, elem[0]) + } + + return relativePath, nil +} + +var _ = Describe("safepath", func() { + + table.DescribeTable("should prevent an escape via", func(builder *pathBuilder, expectedPath string) { + path, err := builder.Build() + Expect(err).ToNot(HaveOccurred()) + constructedPath, err := JoinAndResolveWithRelativeRoot(builder.RelativeRoot(), path) + Expect(err).ToNot(HaveOccurred()) + Expect(unsafepath.UnsafeAbsolute(constructedPath.Raw())).To(Equal(filepath.Join(builder.RelativeRoot(), expectedPath))) + }, + table.Entry("an absolute link to root subdirectory", new("/var/lib/rel/root").Path("link/back/to").Link("link", "/link"), + "/link", + ), + table.Entry("an absolute link to root", new("/var/lib/rel/root").Path("link/back/to").Link("link", "/"), + "/", + ), + table.Entry("a relative link", new("/var/lib/rel/root").Path("link/back/to").Link("var", "../../../../../"), + "/", + ), + ) + + table.DescribeTable("should be able to", func(builder *pathBuilder, expectedPath string) { + path, err := builder.Build() + Expect(err).ToNot(HaveOccurred()) + constructedPath, err := JoinAndResolveWithRelativeRoot(builder.RelativeRoot(), path) + Expect(err).ToNot(HaveOccurred()) + Expect(unsafepath.UnsafeAbsolute(constructedPath.Raw())).To(Equal(filepath.Join(builder.RelativeRoot(), expectedPath))) + }, + table.Entry("handle relative paths by cutting off at the relative root", new("/var/lib/rel/root").Path("link/back/to").Path("../../../../../"), + `/`, + ), + table.Entry("handle relative legitimate paths", new("/var/lib/rel/root").Path("link/back/to").Path("../../"), + `/link`, + ), + table.Entry("handle legitimate paths with relative symlinks", new("/var/lib/rel/root").Path("link/back/to").Link("test", "../../"), + `/link`, + ), + table.Entry("handle multiple legitimate symlink redirects", new("/var/lib/rel/root").Path("link/back/to").Link("test", "../../").Path("b/c").Link("yeah", "../"), + `/link/b`, + ), + ) + + It("should detect self-referencing links", func() { + builder := new("/var/lib/rel/root").Path("link/back/to").Link("test", "../test") + path, err := builder.Build() + Expect(err).ToNot(HaveOccurred()) + _, err = JoinAndResolveWithRelativeRoot(builder.RelativeRoot(), path) + Expect(err).To(HaveOccurred()) + }) + + It("should follow a sequence of linked links", func() { + root, err := ioutil.TempDir("", tempDirPrefix) + Expect(err).ToNot(HaveOccurred()) + relativeRoot := filepath.Join(root, "testroot") + path := "some/path/to/follow" + Expect(os.MkdirAll(filepath.Join(relativeRoot, path, "test3", "test4"), os.ModePerm)) + Expect(os.Symlink("test3", filepath.Join(relativeRoot, path, "test2"))).To(Succeed()) + Expect(os.Symlink("test2", filepath.Join(relativeRoot, path, "test1"))).To(Succeed()) + // try to reach the test4 directory over the test1 link + pp, err := JoinAndResolveWithRelativeRoot(relativeRoot, path, "/test1/test4") + Expect(err).ToNot(HaveOccurred()) + // don't use join to avoid any clean operations + Expect(unsafepath.UnsafeAbsolute(pp.Raw())).To(Equal(relativeRoot + "/some/path/to/follow/test3/test4")) + }) + + It("should detect too many redirects", func() { + root, err := ioutil.TempDir("", tempDirPrefix) + Expect(err).ToNot(HaveOccurred()) + relativeRoot := filepath.Join(root, "testroot") + path := "some/path/to/follow" + Expect(os.MkdirAll(filepath.Join(relativeRoot, path, "test3", "test4"), os.ModePerm)) + Expect(os.Symlink("test3", filepath.Join(relativeRoot, path, "test100"))).To(Succeed()) + for i := 101; i < 401+50; i++ { + Expect(os.Symlink(fmt.Sprintf("test%d", i-1), filepath.Join(relativeRoot, path, fmt.Sprintf("test%d", i)))).To(Succeed()) + } + + // try to reach the test4 directory over the test1 link + _, err = JoinAndResolveWithRelativeRoot(relativeRoot, path, "/test435/test4") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(Equal("more than 256 path elements evaluated")) + }) + + It("should not resolve symlinks in the root path", func() { + root, err := ioutil.TempDir("", tempDirPrefix) + Expect(err).ToNot(HaveOccurred()) + relativeRoot := filepath.Join(root, "testroot") + path := "some/path/to/follow" + Expect(os.MkdirAll(filepath.Join(relativeRoot, path, "test3", "test4"), os.ModePerm)) + Expect(os.Symlink("test3", filepath.Join(relativeRoot, path, "test2"))).To(Succeed()) + Expect(os.Symlink("test2", filepath.Join(relativeRoot, path, "test1"))).To(Succeed()) + // include the symlink in the root path + pp, err := JoinAndResolveWithRelativeRoot(filepath.Join(relativeRoot, path, "test1"), "test4") + Expect(err).ToNot(HaveOccurred()) + // don't use join to avoid any clean operations + Expect(unsafepath.UnsafeAbsolute(pp.Raw())).To(Equal(relativeRoot + "/some/path/to/follow/test1/test4")) + }) + + It("should create a socket repeatedly the safe way", func() { + tempDir, err := ioutil.TempDir("", tempDirPrefix) + Expect(err).ToNot(HaveOccurred()) + root, err := JoinAndResolveWithRelativeRoot("/", tempDir) + Expect(err).ToNot(HaveOccurred()) + l, err := ListenUnixNoFollow(root, "my.sock") + Expect(err).ToNot(HaveOccurred()) + l.Close() + l, err = ListenUnixNoFollow(root, "my.sock") + Expect(err).ToNot(HaveOccurred()) + l.Close() + }) + + It("should open a safepath and provide its filedescriptor path with execute", func() { + tempDir, err := ioutil.TempDir("", tempDirPrefix) + Expect(err).ToNot(HaveOccurred()) + root, err := JoinAndResolveWithRelativeRoot("/", tempDir) + Expect(err).ToNot(HaveOccurred()) + Expect(os.MkdirAll(filepath.Join(unsafepath.UnsafeAbsolute(root.Raw()), "test"), os.ModePerm)).To(Succeed()) + + Expect(root.ExecuteNoFollow(func(safePath string) error { + Expect(safePath).To(ContainSubstring("/proc/self/fd/")) + _, err := os.Stat(filepath.Join(safePath, "test")) + Expect(err).ToNot(HaveOccurred()) + return nil + })).To(Succeed()) + }) + + It("should create a child directory", func() { + tempDir, err := ioutil.TempDir("", tempDirPrefix) + Expect(err).ToNot(HaveOccurred()) + root, err := JoinAndResolveWithRelativeRoot("/", tempDir) + Expect(err).ToNot(HaveOccurred()) + Expect(MkdirAtNoFollow(root, "test", os.ModePerm)).To(Succeed()) + _, err = os.Stat(filepath.Join(unsafepath.UnsafeAbsolute(root.Raw()), "test")) + Expect(err).ToNot(HaveOccurred()) + }) + + It("should set owner and file permissions", func() { + tempDir, err := ioutil.TempDir("", tempDirPrefix) + Expect(err).ToNot(HaveOccurred()) + root, err := JoinAndResolveWithRelativeRoot("/", tempDir) + Expect(err).ToNot(HaveOccurred()) + u, err := user.Current() + Expect(err).ToNot(HaveOccurred()) + uid, err := strconv.Atoi(u.Uid) + Expect(err).ToNot(HaveOccurred()) + gid, err := strconv.Atoi(u.Gid) + Expect(err).ToNot(HaveOccurred()) + Expect(ChpermAtNoFollow(root, uid, gid, 0777)).To(Succeed()) + stat, err := StatAtNoFollow(root) + Expect(err).ToNot(HaveOccurred()) + Expect(stat.Sys().(*syscall.Stat_t).Gid).To(Equal(uint32(gid))) + Expect(stat.Sys().(*syscall.Stat_t).Uid).To(Equal(uint32(uid))) + Expect(stat.Mode() & 0777).To(Equal(fs.FileMode(0777))) + Expect(ChpermAtNoFollow(root, uid, gid, 0770)).To(Succeed()) + stat, err = StatAtNoFollow(root) + Expect(err).ToNot(HaveOccurred()) + Expect(stat.Mode() & 0777).To(Equal(fs.FileMode(0770))) + Expect(stat.Sys().(*syscall.Stat_t).Gid).To(Equal(uint32(gid))) + Expect(stat.Sys().(*syscall.Stat_t).Uid).To(Equal(uint32(uid))) + }) + + It("should unlink files and directories", func() { + tempDir, err := ioutil.TempDir("", tempDirPrefix) + Expect(err).ToNot(HaveOccurred()) + root, err := JoinAndResolveWithRelativeRoot("/", tempDir) + Expect(err).ToNot(HaveOccurred()) + Expect(TouchAtNoFollow(root, "test", os.ModePerm)).To(Succeed()) + Expect(MkdirAtNoFollow(root, "testdir", os.ModePerm)).To(Succeed()) + _, err = os.Stat(filepath.Join(unsafepath.UnsafeAbsolute(root.Raw()), "test")) + Expect(err).ToNot(HaveOccurred()) + _, err = os.Stat(filepath.Join(unsafepath.UnsafeAbsolute(root.Raw()), "testdir")) + Expect(err).ToNot(HaveOccurred()) + p, err := JoinNoFollow(root, "test") + Expect(err).ToNot(HaveOccurred()) + dir, err := JoinNoFollow(root, "testdir") + Expect(err).ToNot(HaveOccurred()) + Expect(UnlinkAtNoFollow(p)).To(Succeed()) + Expect(UnlinkAtNoFollow(dir)).To(Succeed()) + _, err = os.Stat(filepath.Join(unsafepath.UnsafeAbsolute(root.Raw()), "test")) + Expect(err).To(HaveOccurred()) + _, err = os.Stat(filepath.Join(unsafepath.UnsafeAbsolute(root.Raw()), "testdir")) + Expect(err).To(HaveOccurred()) + }) + + It("should return base and relative paths correctly", func() { + baseDir, err := ioutil.TempDir("", tempDirPrefix) + Expect(err).ToNot(HaveOccurred()) + root, err := JoinAndResolveWithRelativeRoot(baseDir) + Expect(err).ToNot(HaveOccurred()) + Expect(MkdirAtNoFollow(root, "test", os.ModePerm)).To(Succeed()) + child, err := JoinNoFollow(root, "test") + Expect(err).ToNot(HaveOccurred()) + Expect(unsafepath.UnsafeRoot(child.Raw())).To(Equal(baseDir)) + Expect(unsafepath.UnsafeRelative(child.Raw())).To(Equal("/test")) + }) + + It("should append new relative root components to the relative path", func() { + baseDir, err := ioutil.TempDir("", tempDirPrefix) + Expect(err).ToNot(HaveOccurred()) + root, err := JoinAndResolveWithRelativeRoot(baseDir) + Expect(err).ToNot(HaveOccurred()) + Expect(MkdirAtNoFollow(root, "test", os.ModePerm)).To(Succeed()) + child, err := root.AppendAndResolveWithRelativeRoot("test") + Expect(err).ToNot(HaveOccurred()) + Expect(unsafepath.UnsafeRoot(child.Raw())).To(Equal(baseDir)) + Expect(unsafepath.UnsafeRelative(child.Raw())).To(Equal("/test")) + }) + + It("should detect absolute root", func() { + p, err := NewPathNoFollow("/") + Expect(err).ToNot(HaveOccurred()) + Expect(p.IsRoot()).To(BeTrue()) + tempDir, err := ioutil.TempDir("", tempDirPrefix) + Expect(err).ToNot(HaveOccurred()) + tmpDir, err := JoinAndResolveWithRelativeRoot("/", tempDir) + Expect(err).ToNot(HaveOccurred()) + Expect(tmpDir.IsRoot()).To(BeFalse()) + }) + + It("should be possible to use os.ReadDir on a safepath", func() { + baseDir, err := ioutil.TempDir("", tempDirPrefix) + Expect(err).ToNot(HaveOccurred()) + root, err := JoinAndResolveWithRelativeRoot(baseDir) + Expect(err).ToNot(HaveOccurred()) + + Expect(os.MkdirAll(filepath.Join(baseDir, "test"), os.ModePerm)).To(Succeed()) + + var files []os.DirEntry + Expect(root.ExecuteNoFollow(func(safePath string) (err error) { + files, err = os.ReadDir(safePath) + return err + })).To(Succeed()) + Expect(files).To(HaveLen(1)) + }) +}) -- 2.37.1 From 0d867eeb80f9c8d54431dec5459da4db0b11d422 Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Tue, 19 Jul 2022 11:02:29 +0200 Subject: [PATCH 02/16] Add unsafepath package Under some circumstances it is necessary for virt-handler to get "unsafe" path segments which potentially cross mount-namespaces or cross directories where at some point in the path the path suffixes are owned by users and not by privileged entities anymore. To make such unsafe access visible, encapsulate them in an unsafe package. In the future imports can be allow-listed to avoid unintentional wrong use. Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 1a8c0367230a907f368f3b7d4d5b1034dcd2f764) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 835022da490e33666be028e59b1dcabfba1da59b) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit c7c85811e262e9eb6e93b32642352389165d3172) Signed-off-by: Jed Lejosne <jed@redhat.com> --- pkg/unsafepath/BUILD.bazel | 8 ++++++++ pkg/unsafepath/unsafepath.go | 27 +++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 pkg/unsafepath/BUILD.bazel create mode 100644 pkg/unsafepath/unsafepath.go diff --git a/pkg/unsafepath/BUILD.bazel b/pkg/unsafepath/BUILD.bazel new file mode 100644 index 000000000..1f3ec55b6 --- /dev/null +++ b/pkg/unsafepath/BUILD.bazel @@ -0,0 +1,8 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["unsafepath.go"], + importpath = "kubevirt.io/kubevirt/pkg/unsafepath", + visibility = ["//visibility:public"], +) diff --git a/pkg/unsafepath/unsafepath.go b/pkg/unsafepath/unsafepath.go new file mode 100644 index 000000000..e1ca82fdd --- /dev/null +++ b/pkg/unsafepath/unsafepath.go @@ -0,0 +1,27 @@ +package unsafepath + +import "path/filepath" + +type Path struct { + rootBase string + relativePath string +} + +func New(rootBase string, relativePath string) *Path { + return &Path{ + rootBase: rootBase, + relativePath: relativePath, + } +} + +func UnsafeAbsolute(path *Path) string { + return filepath.Join(path.rootBase, path.relativePath) +} + +func UnsafeRelative(path *Path) string { + return path.relativePath +} + +func UnsafeRoot(path *Path) string { + return path.rootBase +} -- 2.37.1 From efe547197104d5dfaf8af2adc1a5462f0b6bc55e Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Tue, 19 Jul 2022 11:04:44 +0200 Subject: [PATCH 03/16] Move isolation detection code over to safepath (cherry picked from commit 7de70e9a17eb0ce951a9ef05d526752aa2aeb88c) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 44a01e79d8524296e933afbed04d9b685c5b9962) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit 9f04b0039545af7661d532c0935b2b4a4e0c7501) Signed-off-by: Jed Lejosne <jed@redhat.com> --- pkg/virt-handler/isolation/BUILD.bazel | 4 + pkg/virt-handler/isolation/detector_test.go | 6 +- .../isolation/generated_mock_isolation.go | 9 +- pkg/virt-handler/isolation/isolation.go | 82 +++++++++++++------ pkg/virt-handler/isolation/isolation_test.go | 73 ++++++++--------- pkg/virt-handler/isolation/validation.go | 11 --- 6 files changed, 105 insertions(+), 80 deletions(-) diff --git a/pkg/virt-handler/isolation/BUILD.bazel b/pkg/virt-handler/isolation/BUILD.bazel index dc8fec60b..8215136ad 100644 --- a/pkg/virt-handler/isolation/BUILD.bazel +++ b/pkg/virt-handler/isolation/BUILD.bazel @@ -14,6 +14,8 @@ go_library( visibility = ["//visibility:public"], deps = [ "//pkg/container-disk:go_default_library", + "//pkg/safepath:go_default_library", + "//pkg/unsafepath:go_default_library", "//pkg/util:go_default_library", "//pkg/virt-controller/services:go_default_library", "//pkg/virt-handler/cgroup:go_default_library", @@ -40,6 +42,8 @@ go_test( data = glob(["testdata/**"]), embed = [":go_default_library"], deps = [ + "//pkg/safepath:go_default_library", + "//pkg/unsafepath:go_default_library", "//pkg/util:go_default_library", "//pkg/virt-handler/cgroup:go_default_library", "//pkg/virt-handler/cmd-client:go_default_library", diff --git a/pkg/virt-handler/isolation/detector_test.go b/pkg/virt-handler/isolation/detector_test.go index 1bd80fd4d..e6a2d2feb 100644 --- a/pkg/virt-handler/isolation/detector_test.go +++ b/pkg/virt-handler/isolation/detector_test.go @@ -38,6 +38,8 @@ import ( v1 "kubevirt.io/api/core/v1" "kubevirt.io/kubevirt/pkg/virt-handler/cgroup" + + "kubevirt.io/kubevirt/pkg/unsafepath" cmdclient "kubevirt.io/kubevirt/pkg/virt-handler/cmd-client" ) @@ -139,7 +141,9 @@ var _ = Describe("Isolation Detector", func() { It("Should detect the Mount root of the test suite", func() { result, err := NewSocketBasedIsolationDetector(tmpDir, cgroupParser).Allowlist([]string{"devices"}).Detect(vm) Expect(err).ToNot(HaveOccurred()) - Expect(result.MountRoot()).To(Equal(fmt.Sprintf("/proc/%d/root", os.Getpid()))) + root, err := result.MountRoot() + Expect(err).ToNot(HaveOccurred()) + Expect(unsafepath.UnsafeAbsolute(root.Raw())).To(Equal(fmt.Sprintf("/proc/%d/root", os.Getpid()))) }) }) }) diff --git a/pkg/virt-handler/isolation/generated_mock_isolation.go b/pkg/virt-handler/isolation/generated_mock_isolation.go index 4c08ec44a..4d8f817b5 100644 --- a/pkg/virt-handler/isolation/generated_mock_isolation.go +++ b/pkg/virt-handler/isolation/generated_mock_isolation.go @@ -6,6 +6,8 @@ package isolation import ( gomock "github.com/golang/mock/gomock" mountinfo "github.com/moby/sys/mountinfo" + + safepath "kubevirt.io/kubevirt/pkg/safepath" ) // Mock of IsolationResult interface @@ -69,10 +71,11 @@ func (_mr *_MockIsolationResultRecorder) PIDNamespace() *gomock.Call { return _mr.mock.ctrl.RecordCall(_mr.mock, "PIDNamespace") } -func (_m *MockIsolationResult) MountRoot() string { +func (_m *MockIsolationResult) MountRoot() (*safepath.Path, error) { ret := _m.ctrl.Call(_m, "MountRoot") - ret0, _ := ret[0].(string) - return ret0 + ret0, _ := ret[0].(*safepath.Path) + ret1, _ := ret[1].(error) + return ret0, ret1 } func (_mr *_MockIsolationResultRecorder) MountRoot() *gomock.Call { diff --git a/pkg/virt-handler/isolation/isolation.go b/pkg/virt-handler/isolation/isolation.go index 25618031d..685e397f8 100644 --- a/pkg/virt-handler/isolation/isolation.go +++ b/pkg/virt-handler/isolation/isolation.go @@ -28,12 +28,15 @@ package isolation import ( "fmt" "os" - "path/filepath" "sort" "strings" + "kubevirt.io/kubevirt/pkg/unsafepath" + mount "github.com/moby/sys/mountinfo" + "kubevirt.io/kubevirt/pkg/safepath" + "kubevirt.io/client-go/log" "kubevirt.io/kubevirt/pkg/util" ) @@ -49,7 +52,7 @@ type IsolationResult interface { // full path to the process namespace PIDNamespace() string // full path to the process root mount - MountRoot() string + MountRoot() (*safepath.Path, error) // full path to the mount namespace MountNamespace() string // mounts for the process @@ -79,27 +82,33 @@ func (r *RealIsolationResult) MountNamespace() string { return fmt.Sprintf("/proc/%d/ns/mnt", r.pid) } -// IsMounted checks if the given path is a mount point or not. Works with symlinks. -func (r *RealIsolationResult) IsMounted(mountPoint string) (isMounted bool, err error) { - mountPoint, err = filepath.Abs(mountPoint) +// IsMounted checks if the given path is a mount point or not. +func IsMounted(mountPoint *safepath.Path) (isMounted bool, err error) { + // Ensure that the path is still a valid absolute path without symlinks + f, err := safepath.OpenAtNoFollow(mountPoint) if err != nil { - return false, fmt.Errorf("failed to resolve %v to an absolute path: %v", mountPoint, err) + // treat os.IsNotExist() as error too + // since the inherent property of a safepath.Path is that the path must + // have existed at the point of object creation + return false, err } - mountPoint, err = filepath.EvalSymlinks(mountPoint) - if err != nil { - if os.IsNotExist(err) { - return false, nil - } - return false, fmt.Errorf("could not resolve symlinks in path %v: %v", mountPoint, err) + defer f.Close() + if mountPoint.IsRoot() { + // mount.Mounted has purely string matching based special logic on how to treat "/". + // Emulating this for safepath here without ever having to call an unsafe method on our + // safepath. + return true, nil + } else { + // TODO: Unsafe full path is required, and not a fd, since otherwise mount table lookups and such would not work. + return mount.Mounted(unsafepath.UnsafeAbsolute(mountPoint.Raw())) } - return mount.Mounted(mountPoint) } // AreMounted checks if given paths are mounted by calling IsMounted. // If error occurs, the first error is returned. -func (r *RealIsolationResult) AreMounted(mountPoints ...string) (isMounted bool, err error) { +func (r *RealIsolationResult) AreMounted(mountPoints ...*safepath.Path) (isMounted bool, err error) { for _, mountPoint := range mountPoints { - isMounted, err = r.IsMounted(mountPoint) + isMounted, err = IsMounted(mountPoint) if !isMounted || err != nil { return } @@ -109,19 +118,27 @@ func (r *RealIsolationResult) AreMounted(mountPoints ...string) (isMounted bool, } // IsBlockDevice checks if the given path is a block device or not. -func (r *RealIsolationResult) IsBlockDevice(path string) (bool, error) { - fileInfo, err := os.Stat(path) +func IsBlockDevice(path *safepath.Path) (bool, error) { + fileInfo, err := safepath.StatAtNoFollow(path) if err != nil { return false, fmt.Errorf("error checking for block device: %v", err) } if fileInfo.IsDir() || (fileInfo.Mode()&os.ModeDevice) == 0 { - return false, fmt.Errorf("found %v, but it's not a block device", path) + return false, nil } return true, nil } -func (r *RealIsolationResult) MountRoot() string { - return fmt.Sprintf("/proc/%d/root", r.pid) +func (r *RealIsolationResult) MountRoot() (*safepath.Path, error) { + return safepath.JoinAndResolveWithRelativeRoot(fmt.Sprintf("/proc/%d/root", r.pid)) +} + +func (r *RealIsolationResult) MountRootRelative(relativePath string) (*safepath.Path, error) { + mountRoot, err := r.MountRoot() + if err != nil { + return nil, err + } + return mountRoot.AppendAndResolveWithRelativeRoot(relativePath) } func (r *RealIsolationResult) Pid() int { @@ -189,14 +206,31 @@ func parentMountInfoFor(parent IsolationResult, mountInfo *mount.Info) (*mount.I // ParentPathForRootMount takes a container (child) and composes a path to // the root mount point in the context of the parent. -func ParentPathForRootMount(parent IsolationResult, child IsolationResult) (string, error) { +func ParentPathForRootMount(parent IsolationResult, child IsolationResult) (*safepath.Path, error) { childRootMountInfo, err := MountInfoRoot(child) if err != nil { - return "", err + return nil, err } parentMountInfo, err := parentMountInfoFor(parent, childRootMountInfo) if err != nil { - return "", err + return nil, err + } + parentMountRoot, err := parent.MountRoot() + if err != nil { + return nil, err + } + path := parentMountRoot + path, err = path.AppendAndResolveWithRelativeRoot(parentMountInfo.Mountpoint) + if err != nil { + return nil, err + } + return path.AppendAndResolveWithRelativeRoot(strings.TrimPrefix(childRootMountInfo.Root, parentMountInfo.Root)) +} + +func SafeJoin(res IsolationResult, elems ...string) (*safepath.Path, error) { + mountRoot, err := res.MountRoot() + if err != nil { + return nil, err } - return filepath.Join(parent.MountRoot(), parentMountInfo.Mountpoint, strings.TrimPrefix(childRootMountInfo.Root, parentMountInfo.Root)), nil + return mountRoot.AppendAndResolveWithRelativeRoot(elems...) } diff --git a/pkg/virt-handler/isolation/isolation_test.go b/pkg/virt-handler/isolation/isolation_test.go index 0e7017037..07049a65f 100644 --- a/pkg/virt-handler/isolation/isolation_test.go +++ b/pkg/virt-handler/isolation/isolation_test.go @@ -2,7 +2,6 @@ package isolation import ( "fmt" - "io/ioutil" "os" "path/filepath" @@ -11,6 +10,9 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "kubevirt.io/kubevirt/pkg/safepath" + "kubevirt.io/kubevirt/pkg/unsafepath" + "kubevirt.io/kubevirt/pkg/util" ) @@ -28,41 +30,12 @@ var _ = Describe("IsolationResult", func() { }) It("Should have root mounted", func() { - mounted, err := isolationResult.IsMounted("/") - Expect(err).ToNot(HaveOccurred()) - Expect(mounted).To(BeTrue()) - }) - - It("Should resolve absolute paths with relative navigation", func() { - mounted, err := isolationResult.IsMounted("/var/..") - Expect(err).ToNot(HaveOccurred()) - Expect(mounted).To(BeTrue()) - }) - - It("Should resolve relative paths", func() { - _, err := isolationResult.IsMounted(".") - Expect(err).ToNot(HaveOccurred()) - }) - - It("Should resolve symlinks", func() { - tmpDir, err := ioutil.TempDir("", "kubevirt") + root, err := safepath.NewPathNoFollow("/") Expect(err).ToNot(HaveOccurred()) - defer os.RemoveAll(tmpDir) - - symlinkPath := filepath.Join(tmpDir, "mysymlink") - err = os.Symlink("/", symlinkPath) - Expect(err).ToNot(HaveOccurred()) - - mounted, err := isolationResult.IsMounted(symlinkPath) + mounted, err := IsMounted(root) Expect(err).ToNot(HaveOccurred()) Expect(mounted).To(BeTrue()) }) - - It("Should regard a non-existent path as not mounted, not as an error", func() { - mounted, err := isolationResult.IsMounted("/aasdfjhk") - Expect(err).ToNot(HaveOccurred()) - Expect(mounted).To(BeFalse()) - }) }) Context("Container IsolationResult", func() { @@ -82,9 +55,15 @@ var _ = Describe("IsolationResult", func() { var ctrl *gomock.Controller var mockIsolationResultNode *MockIsolationResult var mockIsolationResultContainer *MockIsolationResult + var tmpDir string BeforeEach(func() { + var err error ctrl = gomock.NewController(GinkgoT()) + tmpDir, err = os.MkdirTemp("", "ginkgo") + Expect(err).ToNot(HaveOccurred()) + root, err := safepath.JoinAndResolveWithRelativeRoot(filepath.Join("/proc/self/root", tmpDir)) + Expect(err).ToNot(HaveOccurred()) mockIsolationResultNode = NewMockIsolationResult(ctrl) mockIsolationResultNode.EXPECT(). @@ -93,7 +72,7 @@ var _ = Describe("IsolationResult", func() { AnyTimes() mockIsolationResultNode.EXPECT(). MountRoot(). - Return("/proc/1/root"). + Return(root, nil). AnyTimes() mockIsolationResultContainer = NewMockIsolationResult(ctrl) @@ -104,6 +83,7 @@ var _ = Describe("IsolationResult", func() { }) AfterEach(func() { + _ = os.RemoveAll(tmpDir) ctrl.Finish() }) @@ -121,17 +101,17 @@ var _ = Describe("IsolationResult", func() { mountDriver: "overlay", hostMountInfoFile: "overlay_host", launcherMountInfoFile: "overlay_launcher", - expectedPathToRootOnNode: "/proc/1/root/var/lib/docker/overlay2/f15d9ce07df72e80d809aa99ab4a171f2f3636f65f0653e75db8ca0befd8ae02/merged", + expectedPathToRootOnNode: "/var/lib/docker/overlay2/f15d9ce07df72e80d809aa99ab4a171f2f3636f65f0653e75db8ca0befd8ae02/merged", }, { mountDriver: "devicemapper", hostMountInfoFile: "devicemapper_host", launcherMountInfoFile: "devicemapper_launcher", - expectedPathToRootOnNode: "/proc/1/root/var/lib/docker/devicemapper/mnt/d0990551ba8254871a449b2ff0d9063061ae96a2c195d7a850b62f030eae1710/rootfs", + expectedPathToRootOnNode: "/var/lib/docker/devicemapper/mnt/d0990551ba8254871a449b2ff0d9063061ae96a2c195d7a850b62f030eae1710/rootfs", }, { mountDriver: "btrfs", hostMountInfoFile: "btrfs_host", launcherMountInfoFile: "btrfs_launcher", - expectedPathToRootOnNode: "/proc/1/root/var/lib/containers/storage/btrfs/subvolumes/e9a94e2cde75c54834378d4835d4eda6bebb56b02068b9254780de6f9344ad0e", + expectedPathToRootOnNode: "/var/lib/containers/storage/btrfs/subvolumes/e9a94e2cde75c54834378d4835d4eda6bebb56b02068b9254780de6f9344ad0e", }, } @@ -150,6 +130,7 @@ var _ = Describe("IsolationResult", func() { Context(fmt.Sprintf("Using storage driver %v", dataset.mountDriver), func() { BeforeEach(func() { + Expect(os.MkdirAll(filepath.Join(tmpDir, dataset.expectedPathToRootOnNode), os.ModePerm)).To(Succeed()) mockIsolationResultNode.EXPECT(). Mounts(gomock.Any()). DoAndReturn(func(f mount.FilterFunc) ([]*mount.Info, error) { @@ -174,7 +155,7 @@ var _ = Describe("IsolationResult", func() { It("Should detect the full path to the root mount point of a container on the node", func() { path, err := ParentPathForRootMount(mockIsolationResultNode, mockIsolationResultContainer) Expect(err).ToNot(HaveOccurred()) - Expect(path).To(Equal(dataset.expectedPathToRootOnNode)) + Expect(unsafepath.UnsafeAbsolute(path.Raw())).To(Equal(filepath.Join("/proc/self/root", tmpDir, dataset.expectedPathToRootOnNode))) }) }) } @@ -225,6 +206,9 @@ var _ = Describe("IsolationResult", func() { }) It("Should match the correct device", func() { + Expect(os.MkdirAll(filepath.Join(tmpDir, "/12"), os.ModePerm)).To(Succeed()) + Expect(os.MkdirAll(filepath.Join(tmpDir, "/21"), os.ModePerm)).To(Succeed()) + Expect(os.MkdirAll(filepath.Join(tmpDir, "/11"), os.ModePerm)).To(Succeed()) initMountsMock(mockIsolationResultContainer, []*mount.Info{rootMountPoint}) initMountsMock(mockIsolationResultNode, []*mount.Info{ { @@ -247,10 +231,12 @@ var _ = Describe("IsolationResult", func() { path, err := ParentPathForRootMount(mockIsolationResultNode, mockIsolationResultContainer) Expect(err).ToNot(HaveOccurred()) - Expect(path).To(Equal("/proc/1/root/11")) + Expect(unsafepath.UnsafeAbsolute(path.Raw())).To(Equal(filepath.Join("/proc/self/root", tmpDir, "/11"))) }) It("Should construct a valid path when the node mountpoint does not match the filesystem path", func() { + Expect(os.MkdirAll(filepath.Join(tmpDir, "/some/path"), os.ModePerm)).To(Succeed()) + Expect(os.MkdirAll(filepath.Join(tmpDir, "/other/location"), os.ModePerm)).To(Succeed()) initMountsMock(mockIsolationResultContainer, []*mount.Info{ { Major: 1, @@ -270,10 +256,12 @@ var _ = Describe("IsolationResult", func() { path, err := ParentPathForRootMount(mockIsolationResultNode, mockIsolationResultContainer) Expect(err).ToNot(HaveOccurred()) - Expect(path).To(Equal("/proc/1/root/other/location")) + Expect(unsafepath.UnsafeAbsolute(path.Raw())).To(Equal(filepath.Join("/proc/self/root", tmpDir, "/other/location"))) }) It("Should find the longest match for a filesystem path", func() { + Expect(os.MkdirAll(filepath.Join(tmpDir, "/some/path/quite/deeply/located/on/the/filesystem"), os.ModePerm)).To(Succeed()) + Expect(os.MkdirAll(filepath.Join(tmpDir, "/long/filesystem"), os.ModePerm)).To(Succeed()) initMountsMock(mockIsolationResultContainer, []*mount.Info{ { Major: 1, @@ -303,7 +291,7 @@ var _ = Describe("IsolationResult", func() { path, err := ParentPathForRootMount(mockIsolationResultNode, mockIsolationResultContainer) Expect(err).ToNot(HaveOccurred()) - Expect(path).To(Equal("/proc/1/root/long/filesystem")) + Expect(unsafepath.UnsafeAbsolute(path.Raw())).To(Equal(filepath.Join("/proc/self/root/", tmpDir, "long/filesystem"))) }) It("Should fail when the device does not exist", func() { @@ -322,6 +310,7 @@ var _ = Describe("IsolationResult", func() { }) It("Should fail if the target filesystem path is not mounted", func() { + Expect(os.MkdirAll(filepath.Join(tmpDir, "/other/path"), os.ModePerm)).To(Succeed()) initMountsMock(mockIsolationResultContainer, []*mount.Info{rootMountPoint}) initMountsMock(mockIsolationResultNode, []*mount.Info{ { @@ -337,6 +326,8 @@ var _ = Describe("IsolationResult", func() { }) It("Should not fail for duplicate mountpoints", func() { + Expect(os.MkdirAll(filepath.Join(tmpDir, "/mymounts/first"), os.ModePerm)).To(Succeed()) + Expect(os.MkdirAll(filepath.Join(tmpDir, "/mymounts/second"), os.ModePerm)).To(Succeed()) initMountsMock(mockIsolationResultContainer, []*mount.Info{rootMountPoint}) initMountsMock(mockIsolationResultNode, []*mount.Info{ @@ -355,7 +346,7 @@ var _ = Describe("IsolationResult", func() { path, err := ParentPathForRootMount(mockIsolationResultNode, mockIsolationResultContainer) Expect(err).ToNot(HaveOccurred()) - Expect(path).To(HavePrefix("/proc/1/root/mymounts/")) + Expect(unsafepath.UnsafeAbsolute(path.Raw())).To(HavePrefix(filepath.Join("/proc/self/root", tmpDir, "/mymounts/"))) }) }) }) diff --git a/pkg/virt-handler/isolation/validation.go b/pkg/virt-handler/isolation/validation.go index d68118852..766f9264b 100644 --- a/pkg/virt-handler/isolation/validation.go +++ b/pkg/virt-handler/isolation/validation.go @@ -3,9 +3,7 @@ package isolation import ( "encoding/json" "fmt" - "os" "os/exec" - "path/filepath" v1 "kubevirt.io/api/core/v1" virt_chroot "kubevirt.io/kubevirt/pkg/virt-handler/virt-chroot" @@ -41,12 +39,3 @@ func GetImageInfo(imagePath string, context IsolationResult, config *v1.DiskVeri } return info, err } - -func GetFileSize(imagePath string, context IsolationResult) (int64, error) { - path := filepath.Join(context.MountRoot(), imagePath) - info, err := os.Stat(path) - if err != nil { - return -1, err - } - return info.Size(), nil -} -- 2.37.1 From 7baf0ac7e351528f0707a93b2569854d5805984f Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Tue, 19 Jul 2022 11:07:09 +0200 Subject: [PATCH 04/16] Move ephemeral-disk-utils over to safepath Change `SetFileOwnership` to use a safepath.Path and introduce `UnsafeSetFileOwnership` as temporary solution for all places where a safepath.Path does not have to be used at the moment to limit the refactor scope. Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 76d446f7968eae8b2a4b6d153bdab61f4292a033) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 27aa04b0b5bbef6290999550c017d11174435380) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit 3d46fd7186933b45dac79fa9696fcf2544568c59) Signed-off-by: Jed Lejosne <jed@redhat.com> --- pkg/ephemeral-disk-utils/BUILD.bazel | 5 ++++- pkg/ephemeral-disk-utils/utils.go | 22 +++++++++++++++++++--- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/pkg/ephemeral-disk-utils/BUILD.bazel b/pkg/ephemeral-disk-utils/BUILD.bazel index 70d197c3f..d685cb234 100644 --- a/pkg/ephemeral-disk-utils/BUILD.bazel +++ b/pkg/ephemeral-disk-utils/BUILD.bazel @@ -5,7 +5,10 @@ go_library( srcs = ["utils.go"], importpath = "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils", visibility = ["//visibility:public"], - deps = ["//pkg/virt-launcher/virtwrap/api:go_default_library"], + deps = [ + "//pkg/safepath:go_default_library", + "//pkg/virt-launcher/virtwrap/api:go_default_library", + ], ) go_test( diff --git a/pkg/ephemeral-disk-utils/utils.go b/pkg/ephemeral-disk-utils/utils.go index 726e7c0c6..77016d1d5 100644 --- a/pkg/ephemeral-disk-utils/utils.go +++ b/pkg/ephemeral-disk-utils/utils.go @@ -26,6 +26,7 @@ import ( "strconv" "syscall" + "kubevirt.io/kubevirt/pkg/safepath" "kubevirt.io/kubevirt/pkg/virt-launcher/virtwrap/api" ) @@ -40,7 +41,11 @@ func MockDefaultOwnershipManager() { type nonOpManager struct { } -func (no *nonOpManager) SetFileOwnership(file string) error { +func (no *nonOpManager) UnsafeSetFileOwnership(file string) error { + return nil +} + +func (no *nonOpManager) SetFileOwnership(file *safepath.Path) error { return nil } @@ -48,7 +53,16 @@ type OwnershipManager struct { user string } -func (om *OwnershipManager) SetFileOwnership(file string) error { +func (om *OwnershipManager) SetFileOwnership(file *safepath.Path) error { + fd, err := safepath.OpenAtNoFollow(file) + if err != nil { + return err + } + defer fd.Close() + return om.UnsafeSetFileOwnership(fd.SafePath()) +} + +func (om *OwnershipManager) UnsafeSetFileOwnership(file string) error { owner, err := user.Lookup(om.user) if err != nil { return fmt.Errorf("failed to look up user %s: %v", om.user, err) @@ -102,7 +116,9 @@ func FileExists(path string) (bool, error) { } type OwnershipManagerInterface interface { - SetFileOwnership(file string) error + // Deprecated: UnsafeSetFileOwnership should not be used. Use SetFileOwnership instead. + UnsafeSetFileOwnership(file string) error + SetFileOwnership(file *safepath.Path) error } func GetEphemeralBackingSourceBlockDevices(domain *api.Domain) map[string]bool { -- 2.37.1 From 5e57a91c521761a0db39d8faea4ec3a1e327aa6e Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Tue, 19 Jul 2022 11:19:45 +0200 Subject: [PATCH 05/16] Move virt-chroot over to safepath usage Pass fully resolved paths to virt-chroot and ensure that virt-chroot will re-create safepath.Paths out of the input before trying to use it, to ensure no path escapes. Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 21ee498079cfee9cacc646ac91340a3384e60ce2) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit c2a50a6e87370716b2058bd970bbb13af3682d43) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit b03896ccf02835bcb0f6de14451ecedfe102673f) Signed-off-by: Jed Lejosne <jed@redhat.com> --- cmd/virt-chroot/BUILD.bazel | 1 + cmd/virt-chroot/main.go | 40 +++++++++++++++++++-- cmd/virt-chroot/selinux.go | 40 +++++++++++++++++++-- pkg/virt-handler/selinux/BUILD.bazel | 2 ++ pkg/virt-handler/selinux/labels.go | 6 ++-- pkg/virt-handler/virt-chroot/BUILD.bazel | 4 +++ pkg/virt-handler/virt-chroot/virt-chroot.go | 28 +++++++++++++-- 7 files changed, 111 insertions(+), 10 deletions(-) diff --git a/cmd/virt-chroot/BUILD.bazel b/cmd/virt-chroot/BUILD.bazel index 0d2d0d5b1..d7691f9d6 100644 --- a/cmd/virt-chroot/BUILD.bazel +++ b/cmd/virt-chroot/BUILD.bazel @@ -11,6 +11,7 @@ go_library( importpath = "kubevirt.io/kubevirt/cmd/virt-chroot", visibility = ["//visibility:private"], deps = [ + "//pkg/safepath:go_default_library", "//vendor/github.com/opencontainers/selinux/go-selinux:go_default_library", "//vendor/github.com/spf13/cobra:go_default_library", "//vendor/github.com/vishvananda/netlink:go_default_library", diff --git a/cmd/virt-chroot/main.go b/cmd/virt-chroot/main.go index 05f7648d2..e862757aa 100644 --- a/cmd/virt-chroot/main.go +++ b/cmd/virt-chroot/main.go @@ -11,6 +11,8 @@ import ( "github.com/spf13/cobra" "golang.org/x/sys/unix" + + "kubevirt.io/kubevirt/pkg/safepath" ) var ( @@ -149,7 +151,25 @@ func main() { } } - return syscall.Mount(args[0], args[1], fsType, uintptr(mntOpts), "") + // Ensure that sourceFile is a real path. It will be kept open until used + // by the syscall via the file descriptor path in proc (SafePath) to ensure + // that no symlink injection can happen after the check. + sourceFile, err := safepath.NewFileNoFollow(args[0]) + if err != nil { + return fmt.Errorf("mount source invalid: %v", err) + } + defer sourceFile.Close() + + // Ensure that targetFile is a real path. It will be kept open until used + // by the syscall via the file descriptor path in proc (SafePath) to ensure + // that no symlink injection can happen after the check. + targetFile, err := safepath.NewFileNoFollow(args[1]) + if err != nil { + return fmt.Errorf("mount target invalid: %v", err) + } + defer targetFile.Close() + + return syscall.Mount(sourceFile.SafePath(), targetFile.SafePath(), fsType, uintptr(mntOpts), "") }, } mntCmd.Flags().StringP("options", "o", "", "comma separated list of mount options") @@ -160,7 +180,23 @@ func main() { Short: "unmount in a specific mount namespace", Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - return syscall.Unmount(args[0], 0) + // Ensure that targetFile is a real path. It will be kept open until used + // by the syscall via the file descriptor path in proc (SafePath) to ensure + // that no symlink injection can happen after the check. + targetFile, err := safepath.NewPathNoFollow(args[0]) + if err != nil { + return fmt.Errorf("mount target invalid: %v", err) + } + err = targetFile.ExecuteNoFollow(func(safePath string) error { + // we actively hold an open reference to the mount point, + // we have to lazy unmount, to not block ourselves + // with the active file-descriptor. + return syscall.Unmount(safePath, unix.MNT_DETACH) + }) + if err != nil { + return fmt.Errorf("umount failed: %v", err) + } + return nil }, } diff --git a/cmd/virt-chroot/selinux.go b/cmd/virt-chroot/selinux.go index af9174c1a..29cc03712 100644 --- a/cmd/virt-chroot/selinux.go +++ b/cmd/virt-chroot/selinux.go @@ -4,11 +4,20 @@ import ( "bytes" "fmt" "io/ioutil" + "os" + "path/filepath" "github.com/opencontainers/selinux/go-selinux" "github.com/spf13/cobra" + "golang.org/x/sys/unix" + + "kubevirt.io/kubevirt/pkg/safepath" ) +const xattrNameSelinux = "security.selinux" + +var root string + // NewGetEnforceCommand determines if selinux is enabled in the kernel (enforced or permissive) func NewGetEnforceCommand() *cobra.Command { cmd := &cobra.Command{ @@ -31,7 +40,7 @@ func NewGetEnforceCommand() *cobra.Command { } func RelabelCommand() *cobra.Command { - return &cobra.Command{ + relabelCommad := &cobra.Command{ Use: "relabel", Short: "relabel a file with the given selinux label, if the path is not labeled like this already", Example: "virt-chroot selinux relabel <new-label> <file-path>", @@ -39,15 +48,38 @@ func RelabelCommand() *cobra.Command { Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { label := args[0] - filePath := args[1] + if root == "" { + root = "/" + } + + rootPath, err := safepath.JoinAndResolveWithRelativeRoot(root) + if err != nil { + return fmt.Errorf("failed to open root path %v: %v", rootPath, err) + } + safePath, err := safepath.JoinNoFollow(rootPath, args[1]) + if err != nil { + return fmt.Errorf("failed to open final path %v: %v", filepath.Join(root, args[1]), err) + } + fd, err := safepath.OpenAtNoFollow(safePath) + if err != nil { + return fmt.Errorf("could not open file %v. Reason: %v", safePath, err) + } + defer fd.Close() + filePath := fd.SafePath() currentFileLabel, err := selinux.FileLabel(filePath) if err != nil { return fmt.Errorf("could not retrieve label of file %s. Reason: %v", filePath, err) } + writeableFD, err := os.OpenFile(filePath, os.O_APPEND|unix.S_IWRITE, os.ModePerm) + if err != nil { + return fmt.Errorf("error reopening file %s to write label %s. Reason: %v", filePath, label, err) + } + defer writeableFD.Close() + if currentFileLabel != label { - if err := selinux.Chcon(filePath, label, false); err != nil { + if err := unix.Fsetxattr(int(writeableFD.Fd()), xattrNameSelinux, []byte(label), 0); err != nil { return fmt.Errorf("error relabeling file %s with label %s. Reason: %v", filePath, label, err) } } @@ -55,4 +87,6 @@ func RelabelCommand() *cobra.Command { return nil }, } + relabelCommad.Flags().StringVar(&root, "root", "/", "safe root path which will be prepended to passed in files") + return relabelCommad } diff --git a/pkg/virt-handler/selinux/BUILD.bazel b/pkg/virt-handler/selinux/BUILD.bazel index 5facc095f..0523bf851 100644 --- a/pkg/virt-handler/selinux/BUILD.bazel +++ b/pkg/virt-handler/selinux/BUILD.bazel @@ -11,6 +11,8 @@ go_library( importpath = "kubevirt.io/kubevirt/pkg/virt-handler/selinux", visibility = ["//visibility:public"], deps = [ + "//pkg/safepath:go_default_library", + "//pkg/unsafepath:go_default_library", "//pkg/virt-handler/virt-chroot:go_default_library", "//staging/src/kubevirt.io/client-go/log:go_default_library", "//vendor/github.com/golang/mock/gomock:go_default_library", diff --git a/pkg/virt-handler/selinux/labels.go b/pkg/virt-handler/selinux/labels.go index 76f37d891..0aa8e1ec9 100644 --- a/pkg/virt-handler/selinux/labels.go +++ b/pkg/virt-handler/selinux/labels.go @@ -8,6 +8,8 @@ import ( "path/filepath" "strings" + "kubevirt.io/kubevirt/pkg/safepath" + "kubevirt.io/kubevirt/pkg/unsafepath" virt_chroot "kubevirt.io/kubevirt/pkg/virt-handler/virt-chroot" "kubevirt.io/client-go/log" @@ -164,10 +166,10 @@ type SELinux interface { IsPermissive() bool } -func RelabelFiles(newLabel string, continueOnError bool, files ...string) error { +func RelabelFiles(newLabel string, continueOnError bool, files ...*safepath.Path) error { relabelArgs := []string{"selinux", "relabel", newLabel} for _, file := range files { - cmd := exec.Command("virt-chroot", append(relabelArgs, file)...) + cmd := exec.Command("virt-chroot", append(relabelArgs, "--root", unsafepath.UnsafeRoot(file.Raw()), unsafepath.UnsafeRelative(file.Raw()))...) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr err := cmd.Run() diff --git a/pkg/virt-handler/virt-chroot/BUILD.bazel b/pkg/virt-handler/virt-chroot/BUILD.bazel index 776719e08..1c59d345c 100644 --- a/pkg/virt-handler/virt-chroot/BUILD.bazel +++ b/pkg/virt-handler/virt-chroot/BUILD.bazel @@ -5,4 +5,8 @@ go_library( srcs = ["virt-chroot.go"], importpath = "kubevirt.io/kubevirt/pkg/virt-handler/virt-chroot", visibility = ["//visibility:public"], + deps = [ + "//pkg/safepath:go_default_library", + "//pkg/unsafepath:go_default_library", + ], ) diff --git a/pkg/virt-handler/virt-chroot/virt-chroot.go b/pkg/virt-handler/virt-chroot/virt-chroot.go index e00dbc54d..4160212b7 100644 --- a/pkg/virt-handler/virt-chroot/virt-chroot.go +++ b/pkg/virt-handler/virt-chroot/virt-chroot.go @@ -19,7 +19,13 @@ package virt_chroot -import "os/exec" +import ( + "os/exec" + "strings" + + "kubevirt.io/kubevirt/pkg/safepath" + "kubevirt.io/kubevirt/pkg/unsafepath" +) const ( binaryPath = "/usr/bin/virt-chroot" @@ -38,7 +44,13 @@ func GetChrootMountNamespace() string { return mountNamespace } -func MountChroot(sourcePath, targetPath string, ro bool) *exec.Cmd { +func MountChroot(sourcePath, targetPath *safepath.Path, ro bool) *exec.Cmd { + return UnsafeMountChroot(trimProcPrefix(sourcePath), trimProcPrefix(targetPath), ro) +} + +// Deprecated: UnsafeMountChroot is used to connect to code which needs to be refactored +// to handle mounts securely. +func UnsafeMountChroot(sourcePath, targetPath string, ro bool) *exec.Cmd { args := append(getBaseArgs(), "mount", "-o") optionArgs := "bind" @@ -50,7 +62,13 @@ func MountChroot(sourcePath, targetPath string, ro bool) *exec.Cmd { return exec.Command(binaryPath, args...) } -func UmountChroot(path string) *exec.Cmd { +func UmountChroot(path *safepath.Path) *exec.Cmd { + return UnsafeUmountChroot(trimProcPrefix(path)) +} + +// Deprecated: UnsafeUmountChroot is used to connect to code which needs to be refactored +// to handle mounts securely. +func UnsafeUmountChroot(path string) *exec.Cmd { args := append(getBaseArgs(), "umount", path) return exec.Command(binaryPath, args...) } @@ -71,3 +89,7 @@ func RemoveMDEVType(mdevUUID string) *exec.Cmd { func ExecChroot(args ...string) *exec.Cmd { return exec.Command(binaryPath, args...) } + +func trimProcPrefix(path *safepath.Path) string { + return strings.TrimPrefix(unsafepath.UnsafeAbsolute(path.Raw()), "/proc/1/root") +} -- 2.37.1 From 0e22650d770fe0ccbb460e50f015a91e2638abe5 Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Tue, 19 Jul 2022 11:09:15 +0200 Subject: [PATCH 06/16] Move containerdisk path resolution over to use the safepath package Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit f0da2cc5778178ec3de85e66239579fc373ad8d9) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 422a7baf1b04595627ccc8138f37ebd1eb8649f5) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit 5b47828290fe19b0d6adec63ac0bdf742e34b095) Signed-off-by: Jed Lejosne <jed@redhat.com> --- pkg/container-disk/BUILD.bazel | 3 + pkg/container-disk/container-disk.go | 70 ++++++++++++----------- pkg/container-disk/container-disk_test.go | 31 +++++----- pkg/ephemeral-disk/ephemeral-disk.go | 2 +- 4 files changed, 54 insertions(+), 52 deletions(-) diff --git a/pkg/container-disk/BUILD.bazel b/pkg/container-disk/BUILD.bazel index d4052dbcf..6c84f15d6 100644 --- a/pkg/container-disk/BUILD.bazel +++ b/pkg/container-disk/BUILD.bazel @@ -11,6 +11,8 @@ go_library( deps = [ "//pkg/ephemeral-disk:go_default_library", "//pkg/ephemeral-disk-utils:go_default_library", + "//pkg/safepath:go_default_library", + "//pkg/unsafepath:go_default_library", "//pkg/util:go_default_library", "//staging/src/kubevirt.io/api/core/v1:go_default_library", "//staging/src/kubevirt.io/client-go/log:go_default_library", @@ -28,6 +30,7 @@ go_test( ], embed = [":go_default_library"], deps = [ + "//pkg/unsafepath:go_default_library", "//staging/src/kubevirt.io/api/core/v1:go_default_library", "//staging/src/kubevirt.io/client-go/api:go_default_library", "//staging/src/kubevirt.io/client-go/testutils:go_default_library", diff --git a/pkg/container-disk/container-disk.go b/pkg/container-disk/container-disk.go index e0ff3038b..2240877e3 100644 --- a/pkg/container-disk/container-disk.go +++ b/pkg/container-disk/container-disk.go @@ -30,6 +30,9 @@ import ( "kubevirt.io/client-go/log" + "kubevirt.io/kubevirt/pkg/safepath" + "kubevirt.io/kubevirt/pkg/unsafepath" + kubev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -64,7 +67,7 @@ func GetVolumeMountDirOnGuest(vmi *v1.VirtualMachineInstance) string { return filepath.Join(mountBaseDir, string(vmi.UID)) } -func GetVolumeMountDirOnHost(vmi *v1.VirtualMachineInstance) (string, bool, error) { +func GetVolumeMountDirOnHost(vmi *v1.VirtualMachineInstance) (*safepath.Path, error) { basepath := "" foundEntries := 0 foundBasepath := "" @@ -72,7 +75,7 @@ func GetVolumeMountDirOnHost(vmi *v1.VirtualMachineInstance) (string, bool, erro basepath = fmt.Sprintf("%s/%s/volumes/kubernetes.io~empty-dir/container-disks", podsBaseDir, string(podUID)) exists, err := diskutils.FileExists(basepath) if err != nil { - return "", false, err + return nil, err } else if exists { foundEntries++ foundBasepath = basepath @@ -80,37 +83,29 @@ func GetVolumeMountDirOnHost(vmi *v1.VirtualMachineInstance) (string, bool, erro } if foundEntries == 1 { - return foundBasepath, true, nil + return safepath.JoinAndResolveWithRelativeRoot("/", foundBasepath) } else if foundEntries > 1 { // Don't mount until outdated pod environments are removed // otherwise we might stomp on a previous cleanup - return "", false, fmt.Errorf("Found multiple pods active for vmi %s/%s. Waiting on outdated pod directories to be removed", vmi.Namespace, vmi.Name) + return nil, fmt.Errorf("Found multiple pods active for vmi %s/%s. Waiting on outdated pod directories to be removed", vmi.Namespace, vmi.Name) } - return "", false, nil + return nil, os.ErrNotExist } -func GetDiskTargetDirFromHostView(vmi *v1.VirtualMachineInstance) (string, error) { - basepath, found, err := GetVolumeMountDirOnHost(vmi) +func GetDiskTargetDirFromHostView(vmi *v1.VirtualMachineInstance) (*safepath.Path, error) { + basepath, err := GetVolumeMountDirOnHost(vmi) if err != nil { - return "", err - } else if !found { - return "", fmt.Errorf("container disk volume for vmi not found") + return nil, err } - return basepath, nil } -func GetDiskTargetPathFromHostView(vmi *v1.VirtualMachineInstance, volumeIndex int) (string, error) { - basepath, err := GetDiskTargetDirFromHostView(vmi) - if err != nil { - return "", err - } - - return fmt.Sprintf("%s/disk_%d.img", basepath, volumeIndex), nil +func GetDiskTargetName(volumeIndex int) string { + return fmt.Sprintf("disk_%d.img", volumeIndex) } func GetDiskTargetPathFromLauncherView(volumeIndex int) string { - return filepath.Join(mountBaseDir, fmt.Sprintf("disk_%d.img", volumeIndex)) + return filepath.Join(mountBaseDir, GetDiskTargetName(volumeIndex)) } func GetKernelBootArtifactPathFromLauncherView(artifact string) string { @@ -184,28 +179,35 @@ func NewKernelBootSocketPathGetter(baseDir string) KernelBootSocketPathGetter { } } -func GetImage(root string, imagePath string) (string, error) { - fallbackPath := filepath.Join(root, DiskSourceFallbackPath) +func GetImage(root *safepath.Path, imagePath string) (*safepath.Path, error) { if imagePath != "" { - imagePath = filepath.Join(root, imagePath) - if _, err := os.Stat(imagePath); err != nil { - if os.IsNotExist(err) { - return "", fmt.Errorf("No image on path %s", imagePath) - } else { - return "", fmt.Errorf("Failed to check if an image exists at %s", imagePath) - } + var err error + resolvedPath, err := root.AppendAndResolveWithRelativeRoot(imagePath) + if err != nil { + return nil, fmt.Errorf("failed to determine custom image path %s: %v", imagePath, err) } + return resolvedPath, nil } else { - files, err := os.ReadDir(fallbackPath) + fallbackPath, err := root.AppendAndResolveWithRelativeRoot(DiskSourceFallbackPath) if err != nil { - return "", fmt.Errorf("Failed to check %s for disks: %v", fallbackPath, err) + return nil, fmt.Errorf("failed to determine default image path %v: %v", fallbackPath, err) } - if len(files) > 1 { - return "", fmt.Errorf("More than one file found in folder %s, only one disk is allowed", DiskSourceFallbackPath) + files, err := os.ReadDir(unsafepath.UnsafeAbsolute(fallbackPath.Raw())) + if err != nil { + return nil, fmt.Errorf("failed to check default image path %s: %v", fallbackPath, err) + } + if len(files) == 0 { + return nil, fmt.Errorf("no file found in folder %s, no disk present", DiskSourceFallbackPath) + } else if len(files) > 1 { + return nil, fmt.Errorf("more than one file found in folder %s, only one disk is allowed", DiskSourceFallbackPath) + } + fileName := files[0].Name() + resolvedPath, err := root.AppendAndResolveWithRelativeRoot(DiskSourceFallbackPath, fileName) + if err != nil { + return nil, fmt.Errorf("failed to check default image path %s: %v", imagePath, err) } - imagePath = filepath.Join(fallbackPath, files[0].Name()) + return resolvedPath, nil } - return imagePath, nil } func GenerateInitContainers(vmi *v1.VirtualMachineInstance, imageIDs map[string]string, podVolumeName string, binVolumeName string) []kubev1.Container { diff --git a/pkg/container-disk/container-disk_test.go b/pkg/container-disk/container-disk_test.go index ef447a628..df9d63aa7 100644 --- a/pkg/container-disk/container-disk_test.go +++ b/pkg/container-disk/container-disk_test.go @@ -34,6 +34,8 @@ import ( "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/types" + "kubevirt.io/kubevirt/pkg/unsafepath" + "kubevirt.io/client-go/api" v1 "kubevirt.io/api/core/v1" @@ -103,28 +105,26 @@ var _ = Describe("ContainerDisk", func() { } // should not be found if dir doesn't exist - path, found, err := GetVolumeMountDirOnHost(vmi) - Expect(err).ToNot(HaveOccurred()) - Expect(found).To(BeFalse()) - Expect(path).To(Equal("")) + path, err := GetVolumeMountDirOnHost(vmi) + Expect(err).To(HaveOccurred()) + Expect(os.IsNotExist(err)).To(BeTrue()) // should be found if dir does exist expectedPath := fmt.Sprintf("%s/1234/volumes/kubernetes.io~empty-dir/container-disks", tmpDir) os.MkdirAll(expectedPath, 0755) - path, found, err = GetVolumeMountDirOnHost(vmi) + path, err = GetVolumeMountDirOnHost(vmi) Expect(err).ToNot(HaveOccurred()) - Expect(found).To(BeTrue()) - Expect(path).To(Equal(expectedPath)) + Expect(unsafepath.UnsafeAbsolute(path.Raw())).To(Equal(expectedPath)) // should be able to generate legacy socket path dir legacySocket := GetLegacyVolumeMountDirOnHost(vmi) Expect(legacySocket).To(Equal(filepath.Join(tmpDir, "6789"))) - // should return error if disk target doesn't exist - targetPath, err := GetDiskTargetPathFromHostView(vmi, 1) - expectedPath = fmt.Sprintf("%s/1234/volumes/kubernetes.io~empty-dir/container-disks/disk_1.img", tmpDir) + // should return error if disk target dir doesn't exist + targetPath, err := GetDiskTargetDirFromHostView(vmi) + expectedPath = fmt.Sprintf("%s/1234/volumes/kubernetes.io~empty-dir/container-disks", tmpDir) Expect(err).ToNot(HaveOccurred()) - Expect(targetPath).To(Equal(expectedPath)) + Expect(unsafepath.UnsafeAbsolute(targetPath.Raw())).To(Equal(expectedPath)) }) @@ -139,18 +139,15 @@ var _ = Describe("ContainerDisk", func() { // should not return error if only one dir exists expectedPath := fmt.Sprintf("%s/1234/volumes/kubernetes.io~empty-dir/container-disks", tmpDir) os.MkdirAll(expectedPath, 0755) - path, found, err := GetVolumeMountDirOnHost(vmi) + path, err := GetVolumeMountDirOnHost(vmi) Expect(err).ToNot(HaveOccurred()) - Expect(found).To(BeTrue()) - Expect(path).To(Equal(expectedPath)) + Expect(unsafepath.UnsafeAbsolute(path.Raw())).To(Equal(expectedPath)) // return error if two dirs exist secondPath := fmt.Sprintf("%s/5678/volumes/kubernetes.io~empty-dir/container-disks", tmpDir) os.MkdirAll(secondPath, 0755) - path, found, err = GetVolumeMountDirOnHost(vmi) + path, err = GetVolumeMountDirOnHost(vmi) Expect(err).To(HaveOccurred()) - Expect(found).To(BeFalse()) - Expect(path).To(Equal("")) }) It("by verifying launcher directory locations", func() { diff --git a/pkg/ephemeral-disk/ephemeral-disk.go b/pkg/ephemeral-disk/ephemeral-disk.go index 03dafed17..526049d5d 100644 --- a/pkg/ephemeral-disk/ephemeral-disk.go +++ b/pkg/ephemeral-disk/ephemeral-disk.go @@ -118,7 +118,7 @@ func (c *ephemeralDiskCreator) CreateBackedImageForVolume(volume v1.Volume, back } // We need to ensure that the permissions are setup correctly. - err = diskutils.DefaultOwnershipManager.SetFileOwnership(imagePath) + err = diskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(imagePath) return err } -- 2.37.1 From 0421571afc6cdd10f22e29f907ac0f645fd16ae1 Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Tue, 19 Jul 2022 11:26:47 +0200 Subject: [PATCH 07/16] Move containerdisk mount operations to safepath operations Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 0f817a789cdbcb770a401a32b11af5653987d585) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit d72c2953a281927c7c9cc8b2f4ee63a0f8abf2d8) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit c90c98df44fe4a0693b52add7149c59025a48b00) Signed-off-by: Jed Lejosne <jed@redhat.com> --- pkg/container-disk/BUILD.bazel | 1 - pkg/container-disk/container-disk.go | 11 +- pkg/virt-handler/container-disk/BUILD.bazel | 2 + pkg/virt-handler/container-disk/mount.go | 171 +++++++++----------- 4 files changed, 88 insertions(+), 97 deletions(-) diff --git a/pkg/container-disk/BUILD.bazel b/pkg/container-disk/BUILD.bazel index 6c84f15d6..8b48d8ccd 100644 --- a/pkg/container-disk/BUILD.bazel +++ b/pkg/container-disk/BUILD.bazel @@ -12,7 +12,6 @@ go_library( "//pkg/ephemeral-disk:go_default_library", "//pkg/ephemeral-disk-utils:go_default_library", "//pkg/safepath:go_default_library", - "//pkg/unsafepath:go_default_library", "//pkg/util:go_default_library", "//staging/src/kubevirt.io/api/core/v1:go_default_library", "//staging/src/kubevirt.io/client-go/log:go_default_library", diff --git a/pkg/container-disk/container-disk.go b/pkg/container-disk/container-disk.go index 2240877e3..d0cdce20f 100644 --- a/pkg/container-disk/container-disk.go +++ b/pkg/container-disk/container-disk.go @@ -30,12 +30,11 @@ import ( "kubevirt.io/client-go/log" - "kubevirt.io/kubevirt/pkg/safepath" - "kubevirt.io/kubevirt/pkg/unsafepath" - kubev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + "kubevirt.io/kubevirt/pkg/safepath" + ephemeraldisk "kubevirt.io/kubevirt/pkg/ephemeral-disk" v1 "kubevirt.io/api/core/v1" @@ -192,7 +191,11 @@ func GetImage(root *safepath.Path, imagePath string) (*safepath.Path, error) { if err != nil { return nil, fmt.Errorf("failed to determine default image path %v: %v", fallbackPath, err) } - files, err := os.ReadDir(unsafepath.UnsafeAbsolute(fallbackPath.Raw())) + var files []os.DirEntry + err = fallbackPath.ExecuteNoFollow(func(safePath string) (err error) { + files, err = os.ReadDir(safePath) + return err + }) if err != nil { return nil, fmt.Errorf("failed to check default image path %s: %v", fallbackPath, err) } diff --git a/pkg/virt-handler/container-disk/BUILD.bazel b/pkg/virt-handler/container-disk/BUILD.bazel index 62a5f07bf..58b0d2100 100644 --- a/pkg/virt-handler/container-disk/BUILD.bazel +++ b/pkg/virt-handler/container-disk/BUILD.bazel @@ -11,6 +11,8 @@ go_library( deps = [ "//pkg/container-disk:go_default_library", "//pkg/ephemeral-disk-utils:go_default_library", + "//pkg/safepath:go_default_library", + "//pkg/unsafepath:go_default_library", "//pkg/util:go_default_library", "//pkg/virt-config:go_default_library", "//pkg/virt-handler/isolation:go_default_library", diff --git a/pkg/virt-handler/container-disk/mount.go b/pkg/virt-handler/container-disk/mount.go index df018f687..41237d14a 100644 --- a/pkg/virt-handler/container-disk/mount.go +++ b/pkg/virt-handler/container-disk/mount.go @@ -9,6 +9,9 @@ import ( "sync" "time" + "kubevirt.io/kubevirt/pkg/unsafepath" + + "kubevirt.io/kubevirt/pkg/safepath" "kubevirt.io/kubevirt/pkg/util" virtconfig "kubevirt.io/kubevirt/pkg/virt-config" virt_chroot "kubevirt.io/kubevirt/pkg/virt-handler/virt-chroot" @@ -197,7 +200,18 @@ func (m *mounter) MountAndVerify(vmi *v1.VirtualMachineInstance) (map[string]*co for i, volume := range vmi.Spec.Volumes { if volume.ContainerDisk != nil { - targetFile, err := containerdisk.GetDiskTargetPathFromHostView(vmi, i) + diskTargetDir, err := containerdisk.GetDiskTargetDirFromHostView(vmi) + if err != nil { + return nil, err + } + diskName := containerdisk.GetDiskTargetName(i) + // If diskName is a symlink it will fail if the target exists. + if err := safepath.TouchAtNoFollow(diskTargetDir, diskName, os.ModePerm); err != nil { + if err != nil && !os.IsExist(err) { + return nil, fmt.Errorf("failed to create mount point target: %v", err) + } + } + targetFile, err := safepath.JoinNoFollow(diskTargetDir, diskName) if err != nil { return nil, err } @@ -208,7 +222,7 @@ func (m *mounter) MountAndVerify(vmi *v1.VirtualMachineInstance) (map[string]*co } record.MountTargetEntries = append(record.MountTargetEntries, vmiMountTargetEntry{ - TargetFile: targetFile, + TargetFile: unsafepath.UnsafeAbsolute(targetFile.Raw()), SocketFile: sock, }) } @@ -228,14 +242,19 @@ func (m *mounter) MountAndVerify(vmi *v1.VirtualMachineInstance) (map[string]*co for i, volume := range vmi.Spec.Volumes { if volume.ContainerDisk != nil { - targetFile, err := containerdisk.GetDiskTargetPathFromHostView(vmi, i) + diskTargetDir, err := containerdisk.GetDiskTargetDirFromHostView(vmi) + if err != nil { + return nil, err + } + diskName := containerdisk.GetDiskTargetName(i) + targetFile, err := safepath.JoinNoFollow(diskTargetDir, diskName) if err != nil { return nil, err } nodeRes := isolation.NodeIsolationResult() - if isMounted, err := nodeRes.IsMounted(targetFile); err != nil { + if isMounted, err := isolation.IsMounted(targetFile); err != nil { return nil, fmt.Errorf("failed to determine if %s is already mounted: %v", targetFile, err) } else if !isMounted { sock, err := m.socketPathGetter(vmi, i) @@ -255,14 +274,9 @@ func (m *mounter) MountAndVerify(vmi *v1.VirtualMachineInstance) (map[string]*co if err != nil { return nil, fmt.Errorf("failed to find a sourceFile in containerDisk %v: %v", volume.Name, err) } - f, err := os.Create(targetFile) - if err != nil { - return nil, fmt.Errorf("failed to create mount point target %v: %v", targetFile, err) - } - f.Close() - log.DefaultLogger().Object(vmi).Infof("Bind mounting container disk at %s to %s", strings.TrimPrefix(sourceFile, nodeRes.MountRoot()), targetFile) - out, err := virt_chroot.MountChroot(strings.TrimPrefix(sourceFile, nodeRes.MountRoot()), targetFile, true).CombinedOutput() + log.DefaultLogger().Object(vmi).Infof("Bind mounting container disk at %s to %s", sourceFile, targetFile) + out, err := virt_chroot.MountChroot(sourceFile, targetFile, true).CombinedOutput() if err != nil { return nil, fmt.Errorf("failed to bindmount containerDisk %v: %v : %v", volume.Name, string(out), err) } @@ -281,51 +295,9 @@ func (m *mounter) MountAndVerify(vmi *v1.VirtualMachineInstance) (map[string]*co return disksInfo, nil } -// Legacy Unmount unmounts all container disks of a given VMI when the hold HostPath method was in use. -// This exists for backwards compatibility for VMIs running before a KubeVirt update occurs. -func (m *mounter) legacyUnmount(vmi *v1.VirtualMachineInstance) error { - mountDir := containerdisk.GetLegacyVolumeMountDirOnHost(vmi) - - files, err := os.ReadDir(mountDir) - if err != nil && !os.IsNotExist(err) { - return fmt.Errorf("failed to list container disk mounts: %v", err) - } - - if vmi.UID != "" { - for _, file := range files { - path := filepath.Join(mountDir, file.Name()) - if strings.HasSuffix(path, ".sock") { - continue - } - if mounted, err := isolation.NodeIsolationResult().IsMounted(path); err != nil { - return fmt.Errorf(failedCheckMountPointFmt, path, err) - } else if mounted { - // #nosec No risk for attacket injection. Parameters are predefined strings - out, err := virt_chroot.UmountChroot(path).CombinedOutput() - if err != nil { - return fmt.Errorf(failedUnmountFmt, path, string(out), err) - } - } - } - - if err := os.RemoveAll(mountDir); err != nil { - return fmt.Errorf("failed to remove containerDisk files: %v", err) - } - } - return nil -} - // Unmount unmounts all container disks of a given VMI. func (m *mounter) Unmount(vmi *v1.VirtualMachineInstance) error { if vmi.UID != "" { - - // this will catch unmounting a vmi's container disk when - // an old VMI is left over after a KubeVirt update - err := m.legacyUnmount(vmi) - if err != nil { - return err - } - record, err := m.getMountTargetRecord(vmi) if err != nil { return err @@ -338,19 +310,22 @@ func (m *mounter) Unmount(vmi *v1.VirtualMachineInstance) error { log.DefaultLogger().Object(vmi).Infof("Found container disk mount entries") for _, entry := range record.MountTargetEntries { - path := entry.TargetFile - log.DefaultLogger().Object(vmi).Infof("Looking to see if containerdisk is mounted at path %s", path) - if mounted, err := isolation.NodeIsolationResult().IsMounted(path); err != nil { + log.DefaultLogger().Object(vmi).Infof("Looking to see if containerdisk is mounted at path %s", entry.TargetFile) + path, err := safepath.NewFileNoFollow(entry.TargetFile) + if err != nil { + return fmt.Errorf(failedCheckMountPointFmt, path, err) + } + _ = path.Close() + if mounted, err := isolation.IsMounted(path.Path()); err != nil { return fmt.Errorf(failedCheckMountPointFmt, path, err) } else if mounted { log.DefaultLogger().Object(vmi).Infof("unmounting container disk at path %s", path) // #nosec No risk for attacket injection. Parameters are predefined strings - out, err := virt_chroot.UmountChroot(path).CombinedOutput() + out, err := virt_chroot.UmountChroot(path.Path()).CombinedOutput() if err != nil { return fmt.Errorf(failedUnmountFmt, path, string(out), err) } } - } err = m.deleteMountTargetRecord(vmi) if err != nil { @@ -394,7 +369,19 @@ func (m *mounter) MountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo if err != nil { return fmt.Errorf("failed to get disk target dir: %v", err) } - targetDir = filepath.Join(targetDir, containerdisk.KernelBootName) + if err := safepath.MkdirAtNoFollow(targetDir, containerdisk.KernelBootName, 0755); err != nil { + if !os.IsExist(err) { + return err + } + } + + targetDir, err = safepath.JoinNoFollow(targetDir, containerdisk.KernelBootName) + if err != nil { + return err + } + if err := safepath.ChpermAtNoFollow(targetDir, 0, 0, 0755); err != nil { + return err + } socketFilePath, err := m.kernelBootSocketPathGetter(vmi) if err != nil { @@ -403,7 +390,7 @@ func (m *mounter) MountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo record := vmiMountTargetRecord{ MountTargetEntries: []vmiMountTargetEntry{{ - TargetFile: targetDir, + TargetFile: unsafepath.UnsafeAbsolute(targetDir.Raw()), SocketFile: socketFilePath, }}, } @@ -415,11 +402,23 @@ func (m *mounter) MountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo nodeRes := isolation.NodeIsolationResult() - targetInitrdPath := filepath.Join(targetDir, filepath.Base(kb.InitrdPath)) - targetKernelPath := filepath.Join(targetDir, filepath.Base(kb.KernelPath)) + if err := safepath.TouchAtNoFollow(targetDir, filepath.Base(kb.InitrdPath), 0655); err != nil && !os.IsExist(err) { + return err + } + if err := safepath.TouchAtNoFollow(targetDir, filepath.Base(kb.KernelPath), 0655); err != nil && !os.IsExist(err) { + return err + } + targetInitrdPath, err := safepath.JoinNoFollow(targetDir, filepath.Base(kb.InitrdPath)) + if err != nil { + return err + } + targetKernelPath, err := safepath.JoinNoFollow(targetDir, filepath.Base(kb.KernelPath)) + if err != nil { + return err + } - areKernelArtifactsMounted := func(artifactsDir string, artifactFiles ...string) (bool, error) { - if _, err = os.Stat(artifactsDir); os.IsNotExist(err) { + areKernelArtifactsMounted := func(artifactsDir *safepath.Path, artifactFiles ...*safepath.Path) (bool, error) { + if _, err = safepath.StatAtNoFollow(artifactsDir); os.IsNotExist(err) { return false, nil } else if err != nil { return false, err @@ -432,7 +431,7 @@ func (m *mounter) MountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo if isMounted, err := areKernelArtifactsMounted(targetDir, targetInitrdPath, targetKernelPath); err != nil { return fmt.Errorf("failed to determine if %s is already mounted: %v", targetDir, err) } else if !isMounted { - log.Log.Object(vmi).Infof("mounting kernel artifacts are not mounted - mounting...") + log.Log.Object(vmi).Infof("kernel artifacts are not mounted - mounting...") res, err := m.podIsolationDetector.DetectForSocket(vmi, socketFilePath) if err != nil { @@ -443,27 +442,13 @@ func (m *mounter) MountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo return fmt.Errorf("failed to detect root mount point of %v on the node: %v", kernelBootName, err) } - err = os.Mkdir(targetDir, 0755) - if err != nil { - return fmt.Errorf("failed to create mount point target %v: %v", targetDir, err) - } - - mount := func(artifactPath, targetPath string) error { - if artifactPath == "" { - return nil - } + mount := func(artifactPath string, targetPath *safepath.Path) error { sourcePath, err := containerdisk.GetImage(mountRootPath, artifactPath) if err != nil { return err } - file, err := os.Create(targetPath) - if err != nil { - return err - } - file.Close() - out, err := virt_chroot.MountChroot(sourcePath, targetPath, true).CombinedOutput() if err != nil { return fmt.Errorf("failed to bindmount %v: %v : %v", kernelBootName, string(out), err) @@ -509,14 +494,17 @@ func (m *mounter) UnmountKernelArtifacts(vmi *v1.VirtualMachineInstance) error { return nil } - unmount := func(targetDir string, artifactPaths ...string) error { + unmount := func(targetDir *safepath.Path, artifactPaths ...string) error { for _, artifactPath := range artifactPaths { if artifactPath == "" { continue } - targetPath := filepath.Join(targetDir, filepath.Base(artifactPath)) - if mounted, err := isolation.NodeIsolationResult().IsMounted(targetPath); err != nil { + targetPath, err := safepath.JoinNoFollow(targetDir, filepath.Base(artifactPath)) + if err != nil { + return fmt.Errorf(failedCheckMountPointFmt, targetPath, err) + } + if mounted, err := isolation.IsMounted(targetPath); err != nil { return fmt.Errorf(failedCheckMountPointFmt, targetPath, err) } else if mounted { log.DefaultLogger().Object(vmi).Infof("unmounting container disk at targetDir %s", targetPath) @@ -531,23 +519,22 @@ func (m *mounter) UnmountKernelArtifacts(vmi *v1.VirtualMachineInstance) error { } for idx, entry := range record.MountTargetEntries { - targetDir := entry.TargetFile - if !strings.Contains(targetDir, containerdisk.KernelBootName) { + if !strings.Contains(entry.TargetFile, containerdisk.KernelBootName) { continue } - log.DefaultLogger().Object(vmi).Infof("unmounting kernel artifacts in path: %s", targetDir) + targetDir, err := safepath.NewFileNoFollow(entry.TargetFile) + if err != nil { + return fmt.Errorf("failed to obtaining a reference to the target directory %q: %v", targetDir, err) + } + _ = targetDir.Close() + log.DefaultLogger().Object(vmi).Infof("unmounting kernel artifacts in path: %v", targetDir) - if err = unmount(targetDir, kb.InitrdPath, kb.KernelPath); err != nil { + if err = unmount(targetDir.Path(), kb.InitrdPath, kb.KernelPath); err != nil { // Not returning here since even if unmount wasn't successful it's better to keep // cleaning the mounted files. log.Log.Object(vmi).Reason(err).Error("unable to unmount kernel artifacts") } - err = os.Remove(targetDir) - if err != nil { - log.DefaultLogger().Object(vmi).Infof("cannot delete dir %s. err: %v", targetDir, err) - } - removeSliceElement := func(s []vmiMountTargetEntry, idxToRemove int) []vmiMountTargetEntry { // removes slice element efficiently s[idxToRemove] = s[len(s)-1] -- 2.37.1 From 871b0b77e3755803eb2deee1094bd903d4b0333b Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Tue, 19 Jul 2022 11:11:45 +0200 Subject: [PATCH 08/16] Only accept absolute paths for containerdisks Let virt-api reject relative paths on containerDisk, kernel or initrd path fields. A relative path would not cause any harm due to the safepath usage, but it is not clear what it means either. Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit b465bffbf7ea86cbc15dad68118eea0721c6af87) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 6b125ab8b045f3a5d0ee1a976b3cc178c344351c) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit 16f99262598a477a326868fb4cc30ddeb6b36b6b) Signed-off-by: Jed Lejosne <jed@redhat.com> --- pkg/testutils/mock_config.go | 2 +- .../admitters/vmi-create-admitter.go | 52 +++++++++++++++++-- .../admitters/vmi-create-admitter_test.go | 46 +++++++++++++++- 3 files changed, 94 insertions(+), 6 deletions(-) diff --git a/pkg/testutils/mock_config.go b/pkg/testutils/mock_config.go index f04874cc4..c47baf324 100644 --- a/pkg/testutils/mock_config.go +++ b/pkg/testutils/mock_config.go @@ -53,7 +53,7 @@ func NewFakeContainerDiskSource() *KVv1.ContainerDiskSource { return &KVv1.ContainerDiskSource{ Image: "fake-image", ImagePullSecret: "fake-pull-secret", - Path: "fake-path", + Path: "/fake-path", } } diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go b/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go index 2b3c2d517..ec6ecdbc1 100644 --- a/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go +++ b/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go @@ -23,6 +23,7 @@ import ( "encoding/base64" "fmt" "net" + "path/filepath" "regexp" "strings" @@ -192,6 +193,7 @@ func ValidateVirtualMachineInstanceSpec(field *k8sfield.Path, spec *v1.VirtualMa causes = append(causes, validateDomainSpec(field.Child("domain"), &spec.Domain)...) causes = append(causes, validateVolumes(field.Child("volumes"), spec.Volumes, config)...) + causes = append(causes, validateContainerDisks(field, spec)...) causes = append(causes, validateAccessCredentials(field.Child("accessCredentials"), spec.AccessCredentials, spec.Volumes)...) @@ -1443,6 +1445,43 @@ func validateRealtime(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec) return causes } +func validateContainerDisks(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec) (causes []metav1.StatusCause) { + for idx, volume := range spec.Volumes { + if volume.ContainerDisk == nil || volume.ContainerDisk.Path == "" { + continue + } + causes = append(causes, validatePath(field.Child("volumes").Index(idx).Child("conatinerDisk"), volume.ContainerDisk.Path)...) + } + return causes +} + +func validatePath(field *k8sfield.Path, path string) (causes []metav1.StatusCause) { + if path == "/" { + causes = append(causes, metav1.StatusCause{ + Type: metav1.CauseTypeFieldValueInvalid, + Message: fmt.Sprintf("%s must not point to root", + field.String(), + ), + Field: field.String(), + }) + } else { + cleanedPath := filepath.Join("/", path) + providedPath := strings.TrimSuffix(path, "/") // Join trims suffix slashes + + if cleanedPath != providedPath { + causes = append(causes, metav1.StatusCause{ + Type: metav1.CauseTypeFieldValueInvalid, + Message: fmt.Sprintf("%s must be an absolute path to a file without relative components", + field.String(), + ), + Field: field.String(), + }) + } + } + return causes + +} + func validateCPURealtime(field *k8sfield.Path, spec *v1.VirtualMachineInstanceSpec) (causes []metav1.StatusCause) { if !spec.Domain.CPU.DedicatedCPUPlacement { causes = append(causes, metav1.StatusCause{ @@ -2399,13 +2438,13 @@ func validateKernelBoot(field *k8sfield.Path, kernelBoot *v1.KernelBoot) (causes } container := kernelBoot.Container - containerField := field.Child("container").String() + containerField := field.Child("container") if container.Image == "" { causes = append(causes, metav1.StatusCause{ Type: metav1.CauseTypeFieldValueRequired, Message: fmt.Sprintf("%s must be defined with an image", containerField), - Field: containerField, + Field: containerField.Child("image").String(), }) } @@ -2413,9 +2452,16 @@ func validateKernelBoot(field *k8sfield.Path, kernelBoot *v1.KernelBoot) (causes causes = append(causes, metav1.StatusCause{ Type: metav1.CauseTypeFieldValueRequired, Message: fmt.Sprintf("%s must be defined with at least one of the following: kernelPath, initrdPath", containerField), - Field: containerField, + Field: containerField.String(), }) } + if container.KernelPath != "" { + causes = append(causes, validatePath(containerField.Child("kernelPath"), container.KernelPath)...) + } + if container.InitrdPath != "" { + causes = append(causes, validatePath(containerField.Child("initrdPath"), container.InitrdPath)...) + } + return } diff --git a/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go b/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go index 139b46d56..264a98469 100644 --- a/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go +++ b/pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter_test.go @@ -139,6 +139,21 @@ var _ = Describe("Validating VMICreate Admitter", func() { Expect(resp.Result.Message).To(ContainSubstring("no memory requested")) }) + table.DescribeTable("path validation should fail", func(path string) { + Expect(validatePath(k8sfield.NewPath("fake"), path)).To(HaveLen(1)) + }, + table.Entry("if path is not absolute", "a/b/c"), + table.Entry("if path contains relative elements", "/a/b/c/../d"), + table.Entry("if path is root", "/"), + ) + + table.DescribeTable("path validation should succeed", func(path string) { + Expect(validatePath(k8sfield.NewPath("fake"), path)).To(BeEmpty()) + }, + table.Entry("if path is absolute", "/a/b/c"), + table.Entry("if path is absolute and has trailing slash", "/a/b/c/"), + ) + Context("tolerations with eviction policies given", func() { var vmi *v1.VirtualMachineInstance var policy = v1.EvictionStrategyLiveMigrate @@ -2215,8 +2230,10 @@ var _ = Describe("Validating VMICreate Admitter", func() { const ( fakeKernelArgs = "args" fakeImage = "image" - fakeInitrd = "initrd" - fakeKernel = "kernel" + fakeInitrd = "/initrd" + fakeKernel = "/kernel" + invalidInitrd = "initrd" + invalidKernel = "kernel" ) table.DescribeTable("", func(kernelArgs, initrdPath, kernelPath, image string, defineContainerNil bool, shouldBeValid bool) { @@ -2256,11 +2273,36 @@ var _ = Describe("Validating VMICreate Admitter", func() { fakeKernelArgs, fakeInitrd, "", fakeImage, false, true), table.Entry("with kernel args, with container that has only image defined - should reject", fakeKernelArgs, "", "", fakeImage, false, false), + table.Entry("with invalid kernel path - should reject", + fakeKernelArgs, fakeInitrd, invalidKernel, fakeImage, false, false), + table.Entry("with invalid initrd path - should reject", + fakeKernelArgs, invalidInitrd, fakeKernel, fakeImage, false, false), table.Entry("with kernel args, with container that has initrd and kernel defined but without image - should reject", fakeKernelArgs, fakeInitrd, fakeKernel, "", false, false), table.Entry("with kernel args, with container that has nothing defined", "", "", "", "", false, false), ) }) + + It("should detect invalid containerDisk paths", func() { + spec := &v1.VirtualMachineInstanceSpec{} + disk := v1.Disk{ + Name: "testdisk", + Serial: "SN-1_a", + } + spec.Domain.Devices.Disks = []v1.Disk{disk} + volume := v1.Volume{ + Name: "testdisk", + VolumeSource: v1.VolumeSource{ + ContainerDisk: testutils.NewFakeContainerDiskSource(), + }, + } + volume.ContainerDisk.Path = "invalid" + + spec.Volumes = []v1.Volume{volume} + spec.Domain.Devices.Disks = []v1.Disk{disk} + causes := ValidateVirtualMachineInstanceSpec(k8sfield.NewPath("fake"), spec, config) + Expect(causes).To(HaveLen(1)) + }) }) Context("with cpu pinning", func() { -- 2.37.1 From 0e4ef6ce1a1ba5e241736183ee4fde0ec60a54a6 Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Tue, 19 Jul 2022 11:15:15 +0200 Subject: [PATCH 09/16] Use UnsafeSetFileOwnership on virt-launcher code When code is executed inside virt-launcher, no special path considerations are necessary. This can be refactored later to remove the `Unsafe` trigger from these code paths. Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit d7e01de4a3e3abc11a0e525f0d1b2eecdeb7138c) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit c81df341bfa54aefe073894528a182b3a8c4b06c) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit 85acf6c7bd771b16b03061267631563b9dccffee) Signed-off-by: Jed Lejosne <jed@redhat.com> --- pkg/cloud-init/cloud-init.go | 4 ++-- pkg/config/config-map.go | 2 +- pkg/config/downwardapi.go | 2 +- pkg/config/secret.go | 2 +- pkg/config/service-account.go | 2 +- pkg/config/sysprep.go | 2 +- pkg/emptydisk/emptydisk.go | 2 +- pkg/network/cache/infocache.go | 2 +- pkg/virt-handler/migration-proxy/migration-proxy.go | 2 +- pkg/virt-launcher/monitor.go | 4 ++-- pkg/virt-launcher/virtwrap/live-migration-target.go | 2 +- 11 files changed, 13 insertions(+), 13 deletions(-) diff --git a/pkg/cloud-init/cloud-init.go b/pkg/cloud-init/cloud-init.go index 1e2c615d7..32507cc95 100644 --- a/pkg/cloud-init/cloud-init.go +++ b/pkg/cloud-init/cloud-init.go @@ -511,7 +511,7 @@ func GenerateEmptyIso(vmiName string, namespace string, data *CloudInitData, siz return err } - if err := diskutils.DefaultOwnershipManager.SetFileOwnership(isoStaging); err != nil { + if err := diskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(isoStaging); err != nil { return err } err = os.Rename(isoStaging, iso) @@ -628,7 +628,7 @@ func GenerateLocalData(vmiName string, namespace string, data *CloudInitData) er return err } - if err := diskutils.DefaultOwnershipManager.SetFileOwnership(isoStaging); err != nil { + if err := diskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(isoStaging); err != nil { return err } diff --git a/pkg/config/config-map.go b/pkg/config/config-map.go index cd5f7a181..9ed328a71 100644 --- a/pkg/config/config-map.go +++ b/pkg/config/config-map.go @@ -55,7 +55,7 @@ func CreateConfigMapDisks(vmi *v1.VirtualMachineInstance, emptyIso bool) error { return err } - if err := ephemeraldiskutils.DefaultOwnershipManager.SetFileOwnership(disk); err != nil { + if err := ephemeraldiskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(disk); err != nil { return err } } diff --git a/pkg/config/downwardapi.go b/pkg/config/downwardapi.go index 8b921cf8d..00e9d60d9 100644 --- a/pkg/config/downwardapi.go +++ b/pkg/config/downwardapi.go @@ -56,7 +56,7 @@ func CreateDownwardAPIDisks(vmi *v1.VirtualMachineInstance, emptyIso bool) error return err } - if err := ephemeraldiskutils.DefaultOwnershipManager.SetFileOwnership(disk); err != nil { + if err := ephemeraldiskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(disk); err != nil { return err } } diff --git a/pkg/config/secret.go b/pkg/config/secret.go index 5061c6942..f3f238110 100644 --- a/pkg/config/secret.go +++ b/pkg/config/secret.go @@ -56,7 +56,7 @@ func CreateSecretDisks(vmi *v1.VirtualMachineInstance, emptyIso bool) error { return err } - if err := ephemeraldiskutils.DefaultOwnershipManager.SetFileOwnership(disk); err != nil { + if err := ephemeraldiskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(disk); err != nil { return err } } diff --git a/pkg/config/service-account.go b/pkg/config/service-account.go index 46506abeb..26e2dbb3a 100644 --- a/pkg/config/service-account.go +++ b/pkg/config/service-account.go @@ -50,7 +50,7 @@ func CreateServiceAccountDisk(vmi *v1.VirtualMachineInstance, emptyIso bool) err return err } - if err := ephemeraldiskutils.DefaultOwnershipManager.SetFileOwnership(disk); err != nil { + if err := ephemeraldiskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(disk); err != nil { return err } } diff --git a/pkg/config/sysprep.go b/pkg/config/sysprep.go index b1b492e47..874d0069a 100644 --- a/pkg/config/sysprep.go +++ b/pkg/config/sysprep.go @@ -102,7 +102,7 @@ func createIsoImageAndSetFileOwnership(volumeName string, filesPath []string, si if err := createIsoConfigImage(disk, sysprepVolumeLabel, filesPath, size); err != nil { return err } - if err := ephemeraldiskutils.DefaultOwnershipManager.SetFileOwnership(disk); err != nil { + if err := ephemeraldiskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(disk); err != nil { return err } diff --git a/pkg/emptydisk/emptydisk.go b/pkg/emptydisk/emptydisk.go index ad72a7004..4389a43d5 100644 --- a/pkg/emptydisk/emptydisk.go +++ b/pkg/emptydisk/emptydisk.go @@ -45,7 +45,7 @@ func (c *emptyDiskCreator) CreateTemporaryDisks(vmi *v1.VirtualMachineInstance) } else if err != nil { return err } - if err := ephemeraldiskutils.DefaultOwnershipManager.SetFileOwnership(file); err != nil { + if err := ephemeraldiskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(file); err != nil { return err } } diff --git a/pkg/network/cache/infocache.go b/pkg/network/cache/infocache.go index 3bfbddd5d..e92b7ec50 100644 --- a/pkg/network/cache/infocache.go +++ b/pkg/network/cache/infocache.go @@ -171,7 +171,7 @@ func writeToCachedFile(fs fs.Fs, obj interface{}, fileName string) error { if err != nil { return fmt.Errorf("error writing cached object: %v", err) } - return dutils.DefaultOwnershipManager.SetFileOwnership(fileName) + return dutils.DefaultOwnershipManager.UnsafeSetFileOwnership(fileName) } func readFromCachedFile(fs fs.Fs, obj interface{}, fileName string) error { diff --git a/pkg/virt-handler/migration-proxy/migration-proxy.go b/pkg/virt-handler/migration-proxy/migration-proxy.go index 557c65f14..0a0112a7c 100644 --- a/pkg/virt-handler/migration-proxy/migration-proxy.go +++ b/pkg/virt-handler/migration-proxy/migration-proxy.go @@ -416,7 +416,7 @@ func (m *migrationProxy) createUnixListener() error { m.logger.Reason(err).Error("failed to create unix socket for proxy service") return err } - if err := diskutils.DefaultOwnershipManager.SetFileOwnership(m.unixSocketPath); err != nil { + if err := diskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(m.unixSocketPath); err != nil { log.Log.Reason(err).Error("failed to change ownership on migration unix socket") return err } diff --git a/pkg/virt-launcher/monitor.go b/pkg/virt-launcher/monitor.go index 179a3383b..24c940deb 100644 --- a/pkg/virt-launcher/monitor.go +++ b/pkg/virt-launcher/monitor.go @@ -57,7 +57,7 @@ func InitializePrivateDirectories(baseDir string) error { if err := util.MkdirAllWithNosec(baseDir); err != nil { return err } - if err := diskutils.DefaultOwnershipManager.SetFileOwnership(baseDir); err != nil { + if err := diskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(baseDir); err != nil { return err } return nil @@ -74,7 +74,7 @@ func InitializeDisksDirectories(baseDir string) error { if err != nil { return err } - err = diskutils.DefaultOwnershipManager.SetFileOwnership(baseDir) + err = diskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(baseDir) if err != nil { return err } diff --git a/pkg/virt-launcher/virtwrap/live-migration-target.go b/pkg/virt-launcher/virtwrap/live-migration-target.go index c0f79cc14..d59c066b5 100644 --- a/pkg/virt-launcher/virtwrap/live-migration-target.go +++ b/pkg/virt-launcher/virtwrap/live-migration-target.go @@ -109,7 +109,7 @@ func (l *LibvirtDomainManager) prepareMigrationTarget( logger.Reason(err).Error("failed to create the migration sockets directory") return err } - if err := diskutils.DefaultOwnershipManager.SetFileOwnership(migrationSocketsPath); err != nil { + if err := diskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(migrationSocketsPath); err != nil { logger.Reason(err).Error("failed to change ownership on migration sockets directory") return err } -- 2.37.1 From c3017255c804a967913d70df39a7613825bb81eb Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Tue, 19 Jul 2022 11:22:33 +0200 Subject: [PATCH 10/16] Don't change selinux labels on hostDisk mounts When virt-launcher runs in non-root mode, don't change selinux labels on the target disk to be accessible for the user. This task should be performed as a preparation step by admins on selinux enabled nodes which "allow" this path to be accessed by VMIs. This way we avoid working against the selinux security intent. Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 61d6dbe24d2808fa5c55ea0d83096c36729d91f9) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 824168b723998dd89c00a157ea90e729f5ecc17d) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit 77214ceac6f057aed7b56bec4d8ea66aa3e5cb9f) Signed-off-by: Jed Lejosne <jed@redhat.com> --- pkg/virt-handler/BUILD.bazel | 1 + pkg/virt-handler/non-root.go | 58 ++++++++++++++++++++++-------------- 2 files changed, 37 insertions(+), 22 deletions(-) diff --git a/pkg/virt-handler/BUILD.bazel b/pkg/virt-handler/BUILD.bazel index 279203643..a1ee72b89 100644 --- a/pkg/virt-handler/BUILD.bazel +++ b/pkg/virt-handler/BUILD.bazel @@ -22,6 +22,7 @@ go_library( "//pkg/network/errors:go_default_library", "//pkg/network/setup:go_default_library", "//pkg/network/vmispec:go_default_library", + "//pkg/safepath:go_default_library", "//pkg/util:go_default_library", "//pkg/util/migrations:go_default_library", "//pkg/util/types:go_default_library", diff --git a/pkg/virt-handler/non-root.go b/pkg/virt-handler/non-root.go index 773dca543..d19df711a 100644 --- a/pkg/virt-handler/non-root.go +++ b/pkg/virt-handler/non-root.go @@ -11,15 +11,18 @@ import ( v1 "kubevirt.io/api/core/v1" diskutils "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils" hostdisk "kubevirt.io/kubevirt/pkg/host-disk" + "kubevirt.io/kubevirt/pkg/safepath" "kubevirt.io/kubevirt/pkg/virt-handler/isolation" - "kubevirt.io/kubevirt/pkg/virt-handler/selinux" ) func changeOwnershipOfBlockDevices(vmiWithOnlyBlockPVCs *v1.VirtualMachineInstance, res isolation.IsolationResult) error { for i := range vmiWithOnlyBlockPVCs.Spec.Volumes { if volumeSource := &vmiWithOnlyBlockPVCs.Spec.Volumes[i].VolumeSource; volumeSource.PersistentVolumeClaim != nil { - devPath := filepath.Join(string(filepath.Separator), "dev", vmiWithOnlyBlockPVCs.Spec.Volumes[i].Name) - if err := diskutils.DefaultOwnershipManager.SetFileOwnership(filepath.Join(res.MountRoot(), devPath)); err != nil { + devPath, err := isolation.SafeJoin(res, string(filepath.Separator), "dev", vmiWithOnlyBlockPVCs.Spec.Volumes[i].Name) + if err != nil { + return nil + } + if err := diskutils.DefaultOwnershipManager.SetFileOwnership(devPath); err != nil { return err } } @@ -27,22 +30,12 @@ func changeOwnershipOfBlockDevices(vmiWithOnlyBlockPVCs *v1.VirtualMachineInstan return nil } -func changeOwnershipAndRelabel(path string) error { +func changeOwnership(path *safepath.Path) error { err := diskutils.DefaultOwnershipManager.SetFileOwnership(path) if err != nil { return err } - - seLinux, selinuxEnabled, err := selinux.NewSELinux() - if err == nil && selinuxEnabled { - unprivilegedContainerSELinuxLabel := "system_u:object_r:container_file_t:s0" - err = selinux.RelabelFiles(unprivilegedContainerSELinuxLabel, seLinux.IsPermissive(), filepath.Join(path)) - if err != nil { - return (fmt.Errorf("error relabeling %s: %v", path, err)) - } - - } - return err + return nil } func changeOwnershipOfHostDisks(vmiWithAllPVCs *v1.VirtualMachineInstance, res isolation.IsolationResult) error { @@ -55,7 +48,11 @@ func changeOwnershipOfHostDisks(vmiWithAllPVCs *v1.VirtualMachineInstance, res i if err != nil { if os.IsNotExist(err) { diskDir := hostdisk.GetMountedHostDiskDir(volumeName) - if err := changeOwnershipAndRelabel(filepath.Join(res.MountRoot(), diskDir)); err != nil { + path, err := isolation.SafeJoin(res, diskDir) + if err != nil { + return fmt.Errorf("Failed to change ownership of HostDisk dir %s, %s", volumeName, err) + } + if err := changeOwnership(path); err != nil { return fmt.Errorf("Failed to change ownership of HostDisk dir %s, %s", volumeName, err) } continue @@ -63,7 +60,11 @@ func changeOwnershipOfHostDisks(vmiWithAllPVCs *v1.VirtualMachineInstance, res i return fmt.Errorf("Failed to recognize if hostdisk contains image, %s", err) } - err = changeOwnershipAndRelabel(filepath.Join(res.MountRoot(), diskPath)) + path, err := isolation.SafeJoin(res, diskPath) + if err != nil { + return fmt.Errorf("Failed to change ownership of HostDisk image: %s", err) + } + err = changeOwnership(path) if err != nil { return fmt.Errorf("Failed to change ownership of HostDisk image: %s", err) } @@ -101,18 +102,31 @@ func getTapDevices(vmi *v1.VirtualMachineInstance) []string { func (d *VirtualMachineController) prepareTap(vmi *v1.VirtualMachineInstance, res isolation.IsolationResult) error { tapDevices := getTapDevices(vmi) for _, tap := range tapDevices { - path := filepath.Join(res.MountRoot(), "sys", "class", "net", tap, "ifindex") - b, err := ioutil.ReadFile(path) + path, err := isolation.SafeJoin(res, "sys", "class", "net", tap, "ifindex") if err != nil { - return fmt.Errorf("Failed to read if index, %v", err) + return err } + index, err := func(path *safepath.Path) (int, error) { + df, err := safepath.OpenAtNoFollow(path) + if err != nil { + return 0, err + } + defer df.Close() + b, err := ioutil.ReadFile(df.SafePath()) + if err != nil { + return 0, fmt.Errorf("Failed to read if index, %v", err) + } - index, err := strconv.Atoi(strings.TrimSpace(string(b))) + return strconv.Atoi(strings.TrimSpace(string(b))) + }(path) if err != nil { return err } - pathToTap := filepath.Join(res.MountRoot(), "dev", fmt.Sprintf("tap%d", index)) + pathToTap, err := isolation.SafeJoin(res, "dev", fmt.Sprintf("tap%d", index)) + if err != nil { + return err + } if err := diskutils.DefaultOwnershipManager.SetFileOwnership(pathToTap); err != nil { return err -- 2.37.1 From 2b9ac8ae7088ed97c3a91f2b79a8029d941c9e65 Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Tue, 19 Jul 2022 11:25:11 +0200 Subject: [PATCH 11/16] Let host-disk continue using unsafe path manipulations host-disk has not further restrictions on what to access. Users can already freely decide what path they want to access. Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 0e28639da59d97c9cfcaf28843fe48738f959362) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 46b4f6c5114d3f9f8f920f9b464f34fcfcfffbdc) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit 014d08b1b5433eb7b4fde08199fae3a29bed8d86) Signed-off-by: Jed Lejosne <jed@redhat.com> --- pkg/host-disk/BUILD.bazel | 3 +++ pkg/host-disk/host-disk.go | 12 +++++++----- pkg/host-disk/host-disk_test.go | 9 +++++++-- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/pkg/host-disk/BUILD.bazel b/pkg/host-disk/BUILD.bazel index 88e4b6eea..544f53396 100644 --- a/pkg/host-disk/BUILD.bazel +++ b/pkg/host-disk/BUILD.bazel @@ -7,6 +7,8 @@ go_library( visibility = ["//visibility:public"], deps = [ "//pkg/ephemeral-disk-utils:go_default_library", + "//pkg/safepath:go_default_library", + "//pkg/unsafepath:go_default_library", "//pkg/util:go_default_library", "//pkg/util/types:go_default_library", "//staging/src/kubevirt.io/api/core/v1:go_default_library", @@ -25,6 +27,7 @@ go_test( embed = [":go_default_library"], deps = [ "//pkg/ephemeral-disk-utils:go_default_library", + "//pkg/safepath:go_default_library", "//pkg/testutils:go_default_library", "//staging/src/kubevirt.io/api/core/v1:go_default_library", "//staging/src/kubevirt.io/client-go/api:go_default_library", diff --git a/pkg/host-disk/host-disk.go b/pkg/host-disk/host-disk.go index a44231794..887989630 100644 --- a/pkg/host-disk/host-disk.go +++ b/pkg/host-disk/host-disk.go @@ -28,6 +28,8 @@ import ( "kubevirt.io/client-go/log" ephemeraldiskutils "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils" + "kubevirt.io/kubevirt/pkg/safepath" + "kubevirt.io/kubevirt/pkg/unsafepath" k8sv1 "k8s.io/api/core/v1" "k8s.io/client-go/tools/record" @@ -194,10 +196,10 @@ type DiskImgCreator struct { recorder record.EventRecorder lessPVCSpaceToleration int minimumPVCReserveBytes uint64 - mountRoot string + mountRoot *safepath.Path } -func NewHostDiskCreator(recorder record.EventRecorder, lessPVCSpaceToleration int, minimumPVCReserveBytes uint64, mountRoot string) DiskImgCreator { +func NewHostDiskCreator(recorder record.EventRecorder, lessPVCSpaceToleration int, minimumPVCReserveBytes uint64, mountRoot *safepath.Path) DiskImgCreator { return DiskImgCreator{ dirBytesAvailableFunc: dirBytesAvailable, recorder: recorder, @@ -227,8 +229,8 @@ func shouldMountHostDisk(hostDisk *v1.HostDisk) bool { } func (hdc *DiskImgCreator) mountHostDiskAndSetOwnership(vmi *v1.VirtualMachineInstance, volumeName string, hostDisk *v1.HostDisk) error { - diskPath := GetMountedHostDiskPathFromHandler(hdc.mountRoot, volumeName, hostDisk.Path) - diskDir := GetMountedHostDiskDirFromHandler(hdc.mountRoot, volumeName) + diskPath := GetMountedHostDiskPathFromHandler(unsafepath.UnsafeAbsolute(hdc.mountRoot.Raw()), volumeName, hostDisk.Path) + diskDir := GetMountedHostDiskDirFromHandler(unsafepath.UnsafeAbsolute(hdc.mountRoot.Raw()), volumeName) fileExists, err := ephemeraldiskutils.FileExists(diskPath) if err != nil { return err @@ -239,7 +241,7 @@ func (hdc *DiskImgCreator) mountHostDiskAndSetOwnership(vmi *v1.VirtualMachineIn } } // Change file ownership to the qemu user. - if err := ephemeraldiskutils.DefaultOwnershipManager.SetFileOwnership(diskPath); err != nil { + if err := ephemeraldiskutils.DefaultOwnershipManager.UnsafeSetFileOwnership(diskPath); err != nil { log.Log.Reason(err).Errorf("Couldn't set Ownership on %s: %v", diskPath, err) return err } diff --git a/pkg/host-disk/host-disk_test.go b/pkg/host-disk/host-disk_test.go index 68f3fe8aa..a9497957c 100644 --- a/pkg/host-disk/host-disk_test.go +++ b/pkg/host-disk/host-disk_test.go @@ -36,6 +36,8 @@ import ( "k8s.io/client-go/kubernetes/fake" "k8s.io/client-go/tools/record" + "kubevirt.io/kubevirt/pkg/safepath" + "kubevirt.io/client-go/api" v1 "kubevirt.io/api/core/v1" @@ -99,8 +101,11 @@ var _ = Describe("HostDisk", func() { recorder = record.NewFakeRecorder(100) recorder.IncludeObject = true - hostDiskCreator = NewHostDiskCreator(recorder, 0, 0, "") - hostDiskCreatorWithReserve = NewHostDiskCreator(recorder, 10, 1048576, "") + root, err := safepath.JoinAndResolveWithRelativeRoot("/") + Expect(err).NotTo(HaveOccurred()) + + hostDiskCreator = NewHostDiskCreator(recorder, 0, 0, root) + hostDiskCreatorWithReserve = NewHostDiskCreator(recorder, 10, 1048576, root) }) -- 2.37.1 From bcddf8399569f3f5e7ecca0aded9cea17107f973 Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Tue, 19 Jul 2022 11:28:46 +0200 Subject: [PATCH 12/16] Move hotplug code over to safepath usage Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 59d54ee7f23d20ce17e6ec08b47458e78715811b) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit ea03a834dd47f8c4ada25e597c6e247a840c2fc6) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit e71c9bec7484bbd3cdbe3e82929e02168be8787d) Signed-off-by: Jed Lejosne <jed@redhat.com> --- pkg/hotplug-disk/BUILD.bazel | 3 +- pkg/hotplug-disk/hotplug-disk.go | 31 +- pkg/hotplug-disk/hotplug-disk_test.go | 5 +- pkg/virt-handler/hotplug-disk/BUILD.bazel | 8 +- .../hotplug-disk/hotplug-disk_suite_test.go | 2 +- pkg/virt-handler/hotplug-disk/mount.go | 439 ++++++++------ pkg/virt-handler/hotplug-disk/mount_test.go | 572 ++++++++++-------- 7 files changed, 586 insertions(+), 474 deletions(-) diff --git a/pkg/hotplug-disk/BUILD.bazel b/pkg/hotplug-disk/BUILD.bazel index 7e893718c..760a0fa82 100644 --- a/pkg/hotplug-disk/BUILD.bazel +++ b/pkg/hotplug-disk/BUILD.bazel @@ -6,7 +6,7 @@ go_library( importpath = "kubevirt.io/kubevirt/pkg/hotplug-disk", visibility = ["//visibility:public"], deps = [ - "//pkg/ephemeral-disk-utils:go_default_library", + "//pkg/safepath:go_default_library", "//pkg/util:go_default_library", "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", ], @@ -21,6 +21,7 @@ go_test( embed = [":go_default_library"], deps = [ "//pkg/ephemeral-disk-utils:go_default_library", + "//pkg/unsafepath:go_default_library", "//staging/src/kubevirt.io/client-go/testutils:go_default_library", "//vendor/github.com/onsi/ginkgo:go_default_library", "//vendor/github.com/onsi/gomega:go_default_library", diff --git a/pkg/hotplug-disk/hotplug-disk.go b/pkg/hotplug-disk/hotplug-disk.go index 17761d3fb..3cf3b2212 100644 --- a/pkg/hotplug-disk/hotplug-disk.go +++ b/pkg/hotplug-disk/hotplug-disk.go @@ -26,7 +26,8 @@ import ( "k8s.io/apimachinery/pkg/types" - diskutils "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils" + "kubevirt.io/kubevirt/pkg/safepath" + "kubevirt.io/kubevirt/pkg/util" ) @@ -42,8 +43,8 @@ var ( ) type HotplugDiskManagerInterface interface { - GetHotplugTargetPodPathOnHost(virtlauncherPodUID types.UID) (string, error) - GetFileSystemDiskTargetPathFromHostView(virtlauncherPodUID types.UID, volumeName string, create bool) (string, error) + GetHotplugTargetPodPathOnHost(virtlauncherPodUID types.UID) (*safepath.Path, error) + GetFileSystemDiskTargetPathFromHostView(virtlauncherPodUID types.UID, volumeName string, create bool) (*safepath.Path, error) } func NewHotplugDiskManager() *hotplugDiskManager { @@ -66,32 +67,22 @@ type hotplugDiskManager struct { } // GetHotplugTargetPodPathOnHost retrieves the target pod (virt-launcher) path on the host. -func (h *hotplugDiskManager) GetHotplugTargetPodPathOnHost(virtlauncherPodUID types.UID) (string, error) { +func (h *hotplugDiskManager) GetHotplugTargetPodPathOnHost(virtlauncherPodUID types.UID) (*safepath.Path, error) { podpath := TargetPodBasePath(h.podsBaseDir, virtlauncherPodUID) - exists, _ := diskutils.FileExists(podpath) - if exists { - return podpath, nil - } - - return "", fmt.Errorf("Unable to locate target path: %s", podpath) + return safepath.JoinAndResolveWithRelativeRoot("/", podpath) } // GetFileSystemDiskTargetPathFromHostView gets the disk image file in the target pod (virt-launcher) on the host. -func (h *hotplugDiskManager) GetFileSystemDiskTargetPathFromHostView(virtlauncherPodUID types.UID, volumeName string, create bool) (string, error) { +func (h *hotplugDiskManager) GetFileSystemDiskTargetPathFromHostView(virtlauncherPodUID types.UID, volumeName string, create bool) (*safepath.Path, error) { targetPath, err := h.GetHotplugTargetPodPathOnHost(virtlauncherPodUID) if err != nil { return targetPath, err } - diskFile := filepath.Join(targetPath, fmt.Sprintf("%s.img", volumeName)) - exists, _ := diskutils.FileExists(diskFile) - if !exists && create { - file, err := os.Create(diskFile) - if err != nil { - return diskFile, err - } - defer file.Close() + diskName := fmt.Sprintf("%s.img", volumeName) + if err := safepath.TouchAtNoFollow(targetPath, diskName, 0666); err != nil && !os.IsExist(err) { + return nil, err } - return diskFile, err + return safepath.JoinNoFollow(targetPath, diskName) } // CreateLocalDirectory creates the base directory where disk images will be mounted when hotplugged. File system volumes will be in diff --git a/pkg/hotplug-disk/hotplug-disk_test.go b/pkg/hotplug-disk/hotplug-disk_test.go index e65656539..47d69f0ba 100644 --- a/pkg/hotplug-disk/hotplug-disk_test.go +++ b/pkg/hotplug-disk/hotplug-disk_test.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/types" diskutils "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils" + "kubevirt.io/kubevirt/pkg/unsafepath" ) var _ = Describe("HotplugDisk", func() { @@ -80,7 +81,7 @@ var _ = Describe("HotplugDisk", func() { testPath := filepath.Join(TargetPodBasePath(podsBaseDir, testUID), "testvolume.img") exists, _ := diskutils.FileExists(testPath) Expect(exists).To(BeTrue()) - Expect(res).To(Equal(testPath)) + Expect(unsafepath.UnsafeAbsolute(res.Raw())).To(Equal(testPath)) }) It("GetFileSystemDiskTargetPathFromHostView should return the volume directory", func() { @@ -90,7 +91,7 @@ var _ = Describe("HotplugDisk", func() { err = os.MkdirAll(testPath, os.FileMode(0755)) res, err := hotplug.GetFileSystemDiskTargetPathFromHostView(testUID, "testvolume", false) Expect(err).ToNot(HaveOccurred()) - Expect(res).To(Equal(testPath)) + Expect(unsafepath.UnsafeAbsolute(res.Raw())).To(Equal(testPath)) }) It("GetFileSystemDiskTargetPathFromHostView should fail on invalid UID", func() { diff --git a/pkg/virt-handler/hotplug-disk/BUILD.bazel b/pkg/virt-handler/hotplug-disk/BUILD.bazel index d6b54e517..56d3084f4 100644 --- a/pkg/virt-handler/hotplug-disk/BUILD.bazel +++ b/pkg/virt-handler/hotplug-disk/BUILD.bazel @@ -12,7 +12,8 @@ go_library( deps = [ "//pkg/ephemeral-disk-utils:go_default_library", "//pkg/hotplug-disk:go_default_library", - "//pkg/util:go_default_library", + "//pkg/safepath:go_default_library", + "//pkg/unsafepath:go_default_library", "//pkg/virt-handler/cgroup:go_default_library", "//pkg/virt-handler/isolation:go_default_library", "//pkg/virt-handler/virt-chroot:go_default_library", @@ -23,6 +24,7 @@ go_library( "//vendor/github.com/opencontainers/runc/libcontainer/cgroups/fscommon:go_default_library", "//vendor/github.com/opencontainers/runc/libcontainer/devices:go_default_library", "//vendor/github.com/pkg/errors:go_default_library", + "//vendor/golang.org/x/sys/unix:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", ], @@ -38,14 +40,16 @@ go_test( embed = [":go_default_library"], deps = [ "//pkg/hotplug-disk:go_default_library", + "//pkg/safepath:go_default_library", + "//pkg/unsafepath:go_default_library", "//pkg/virt-handler/isolation:go_default_library", "//staging/src/kubevirt.io/api/core/v1:go_default_library", "//staging/src/kubevirt.io/client-go/api:go_default_library", - "//staging/src/kubevirt.io/client-go/log:go_default_library", "//staging/src/kubevirt.io/client-go/testutils:go_default_library", "//vendor/github.com/onsi/ginkgo:go_default_library", "//vendor/github.com/onsi/ginkgo/extensions/table:go_default_library", "//vendor/github.com/onsi/gomega:go_default_library", + "//vendor/golang.org/x/sys/unix:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", ], diff --git a/pkg/virt-handler/hotplug-disk/hotplug-disk_suite_test.go b/pkg/virt-handler/hotplug-disk/hotplug-disk_suite_test.go index 9c4b60760..321195557 100644 --- a/pkg/virt-handler/hotplug-disk/hotplug-disk_suite_test.go +++ b/pkg/virt-handler/hotplug-disk/hotplug-disk_suite_test.go @@ -25,6 +25,6 @@ import ( "kubevirt.io/client-go/testutils" ) -func TestHostDisk(t *testing.T) { +func TestHotplugDisk(t *testing.T) { testutils.KubeVirtTestSuiteSetup(t) } diff --git a/pkg/virt-handler/hotplug-disk/mount.go b/pkg/virt-handler/hotplug-disk/mount.go index 920e55a3f..baecdec28 100644 --- a/pkg/virt-handler/hotplug-disk/mount.go +++ b/pkg/virt-handler/hotplug-disk/mount.go @@ -5,21 +5,22 @@ import ( "encoding/json" "errors" "fmt" - "io" "io/ioutil" "os" - "os/exec" "path/filepath" "reflect" - "strconv" - "strings" "sync" + "syscall" + "kubevirt.io/kubevirt/pkg/unsafepath" + + "golang.org/x/sys/unix" + + "kubevirt.io/kubevirt/pkg/safepath" virt_chroot "kubevirt.io/kubevirt/pkg/virt-handler/virt-chroot" diskutils "kubevirt.io/kubevirt/pkg/ephemeral-disk-utils" hotplugdisk "kubevirt.io/kubevirt/pkg/hotplug-disk" - "kubevirt.io/kubevirt/pkg/util" "kubevirt.io/kubevirt/pkg/virt-handler/cgroup" "kubevirt.io/kubevirt/pkg/virt-handler/isolation" @@ -37,44 +38,87 @@ import ( const unableFindHotplugMountedDir = "unable to find hotplug mounted directories for vmi without uid" var ( - deviceBasePath = func(podUID types.UID) string { - return fmt.Sprintf("/proc/1/root/var/lib/kubelet/pods/%s/volumes/kubernetes.io~empty-dir/hotplug-disks", string(podUID)) + nodeIsolationResult = func() isolation.IsolationResult { + return isolation.NodeIsolationResult() + } + deviceBasePath = func(podUID types.UID) (*safepath.Path, error) { + return safepath.JoinAndResolveWithRelativeRoot("/proc/1/root", fmt.Sprintf("/var/lib/kubelet/pods/%s/volumes/kubernetes.io~empty-dir/hotplug-disks", string(podUID))) } - sourcePodBasePath = func(podUID types.UID) string { - return fmt.Sprintf("/proc/1/root/var/lib/kubelet/pods/%s/volumes", string(podUID)) + sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) { + return safepath.JoinAndResolveWithRelativeRoot("/proc/1/root", fmt.Sprintf("root/var/lib/kubelet/pods/%s/volumes", string(podUID))) } socketPath = func(podUID types.UID) string { return fmt.Sprintf("pods/%s/volumes/kubernetes.io~empty-dir/hotplug-disks/hp.sock", string(podUID)) } - cgroupsBasePath = func() string { - return filepath.Join("/proc/1/root", cgroup.ControllerPath("devices")) + cgroupsBasePath = func() (*safepath.Path, error) { + return safepath.JoinAndResolveWithRelativeRoot("/proc/1/root", cgroup.ControllerPath("devices")) + } + + statDevice = func(fileName *safepath.Path) (os.FileInfo, error) { + info, err := safepath.StatAtNoFollow(fileName) + if err != nil { + return nil, err + } + if info.Mode()&os.ModeDevice == 0 { + return info, fmt.Errorf("%v is not a block device", fileName) + } + return info, nil } - statCommand = func(fileName string) ([]byte, error) { - return exec.Command("/usr/bin/stat", fileName, "-L", "-c%t,%T,%a,%F").CombinedOutput() + statSourceDevice = func(fileName *safepath.Path) (os.FileInfo, error) { + // we don't know the device name, we only know that it is the only + // device in a specific directory, let's look it up + var devName string + err := fileName.ExecuteNoFollow(func(safePath string) error { + entries, err := os.ReadDir(safePath) + if err != nil { + return err + } + for _, entry := range entries { + info, err := entry.Info() + if err != nil { + return err + } + if info.Mode()&os.ModeDevice == 0 { + // not a device + continue + } + devName = entry.Name() + return nil + } + return fmt.Errorf("no device in %v", fileName) + }) + if err != nil { + return nil, err + } + devPath, err := safepath.JoinNoFollow(fileName, devName) + if err != nil { + return nil, err + } + return statDevice(devPath) } - mknodCommand = func(deviceName string, major, minor int64, blockDevicePermissions string) ([]byte, error) { - return exec.Command("/usr/bin/mknod", "--mode", fmt.Sprintf("0%s", blockDevicePermissions), deviceName, "b", strconv.FormatInt(major, 10), strconv.FormatInt(minor, 10)).CombinedOutput() + mknodCommand = func(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error { + return safepath.MknodAtNoFollow(basePath, deviceName, blockDevicePermissions|syscall.S_IFBLK, dev) } - mountCommand = func(sourcePath, targetPath string) ([]byte, error) { - return virt_chroot.MountChroot(strings.TrimPrefix(sourcePath, isolation.NodeIsolationResult().MountRoot()), targetPath, false).CombinedOutput() + mountCommand = func(sourcePath, targetPath *safepath.Path) ([]byte, error) { + return virt_chroot.MountChroot(sourcePath, targetPath, false).CombinedOutput() } - unmountCommand = func(diskPath string) ([]byte, error) { + unmountCommand = func(diskPath *safepath.Path) ([]byte, error) { return virt_chroot.UmountChroot(diskPath).CombinedOutput() } - isMounted = func(path string) (bool, error) { - return isolation.NodeIsolationResult().IsMounted(path) + isMounted = func(path *safepath.Path) (bool, error) { + return isolation.IsMounted(path) } - isBlockDevice = func(path string) (bool, error) { - return isolation.NodeIsolationResult().IsBlockDevice(path) + isBlockDevice = func(path *safepath.Path) (bool, error) { + return isolation.IsBlockDevice(path) } isolationDetector = func(path string) isolation.PodIsolationDetector { @@ -228,11 +272,9 @@ func (m *volumeMounter) setMountTargetRecord(vmi *v1.VirtualMachineInstance, rec } func (m *volumeMounter) writePathToMountRecord(path string, vmi *v1.VirtualMachineInstance, record *vmiMountTargetRecord) error { - if path != "" { - record.MountTargetEntries = append(record.MountTargetEntries, vmiMountTargetEntry{ - TargetFile: path, - }) - } + record.MountTargetEntries = append(record.MountTargetEntries, vmiMountTargetEntry{ + TargetFile: path, + }) if err := m.setMountTargetRecord(vmi, record); err != nil { return err } @@ -246,12 +288,12 @@ func (m *volumeMounter) mountHotplugVolume(vmi *v1.VirtualMachineInstance, volum if m.isBlockVolume(&vmi.Status, volumeName) { logger.V(4).Infof("Mounting block volume: %s", volumeName) if err := m.mountBlockHotplugVolume(vmi, volumeName, sourceUID, record); err != nil { - return err + return fmt.Errorf("failed to mount block hotplug volume %s: %v", volumeName, err) } } else { logger.V(4).Infof("Mounting file system volume: %s", volumeName) if err := m.mountFileSystemHotplugVolume(vmi, volumeName, sourceUID, record); err != nil { - return err + return fmt.Errorf("failed to mount filesystem hotplug volume %s: %v", volumeName, err) } } } @@ -316,43 +358,60 @@ func (m *volumeMounter) mountBlockHotplugVolume(vmi *v1.VirtualMachineInstance, return err } - deviceName := filepath.Join(targetPath, volume) - - isMigrationInProgress := vmi.Status.MigrationState != nil && !vmi.Status.MigrationState.Completed - - if isBlockExists, _ := isBlockDevice(deviceName); !isBlockExists { + if _, err := safepath.JoinNoFollow(targetPath, volume); os.IsNotExist(err) { computeCGroupPath, err := m.getTargetCgroupPath(vmi) if err != nil { return err } - sourceMajor, sourceMinor, permissions, err := m.getSourceMajorMinor(sourceUID, volume) + dev, permissions, err := m.getSourceMajorMinor(sourceUID, volume) if err != nil { return err } - if err := m.writePathToMountRecord(deviceName, vmi, record); err != nil { + + if err := m.writePathToMountRecord(filepath.Join(unsafepath.UnsafeAbsolute(targetPath.Raw()), volume), vmi, record); err != nil { return err } - // allow block devices - if err := m.allowBlockMajorMinor(sourceMajor, sourceMinor, computeCGroupPath); err != nil { + + if err := m.allowBlockMajorMinor(dev, computeCGroupPath); err != nil { return err } - if _, err = m.createBlockDeviceFile(deviceName, sourceMajor, sourceMinor, permissions); err != nil { + + if err := m.createBlockDeviceFile(targetPath, volume, dev, permissions); err != nil && !os.IsExist(err) { return err } - } else if isBlockExists && (!m.volumeStatusReady(volume, vmi) || isMigrationInProgress) { - // Block device exists already, but the volume is not ready yet, ensure that the device is allowed. + log.DefaultLogger().V(1).Infof("successfully created block device %v", volume) + } else if err != nil { + return err + } + + devicePath, err := safepath.JoinNoFollow(targetPath, volume) + if err != nil { + return err + } + if isBlockExists, err := isBlockDevice(devicePath); err != nil { + return err + } else if !isBlockExists { + return fmt.Errorf("target device %v exists but it is not a block device", devicePath) + } + + isMigrationInProgress := vmi.Status.MigrationState != nil && !vmi.Status.MigrationState.Completed + volumeNotReady := !m.volumeStatusReady(volume, vmi) + + if isMigrationInProgress || volumeNotReady { computeCGroupPath, err := m.getTargetCgroupPath(vmi) if err != nil { return err } - sourceMajor, sourceMinor, _, err := m.getSourceMajorMinor(sourceUID, volume) + dev, _, err := m.getSourceMajorMinor(sourceUID, volume) if err != nil { return err } - if err := m.allowBlockMajorMinor(sourceMajor, sourceMinor, computeCGroupPath); err != nil { + // allow block devices + if err := m.allowBlockMajorMinor(dev, computeCGroupPath); err != nil { return err } } + return nil } @@ -369,127 +428,65 @@ func (m *volumeMounter) volumeStatusReady(volumeName string, vmi *v1.VirtualMach return true } -func (m *volumeMounter) getSourceMajorMinor(sourceUID types.UID, volumeName string) (int64, int64, string, error) { - result := make([]int64, 2) - perms := "" - if sourceUID != types.UID("") { - basepath := filepath.Join(deviceBasePath(sourceUID), volumeName) - err := filepath.Walk(basepath, func(filePath string, info os.FileInfo, err error) error { - if info != nil && !info.IsDir() { - // Walk doesn't follow symlinks which is good because I need to massage symlinks - linkInfo, err := os.Lstat(filePath) - if err != nil { - return err - } - path := filePath - if linkInfo.Mode()&os.ModeSymlink != 0 { - // Its a symlink, follow it - link, err := os.Readlink(filePath) - if err != nil { - return err - } - if !strings.HasPrefix(link, util.HostRootMount) { - path = filepath.Join(util.HostRootMount, link) - } else { - path = link - } - } - if m.isBlockFile(path) { - result[0], result[1], perms, err = m.getBlockFileMajorMinor(path) - // Err != nil means not a block device or unable to determine major/minor, try next file - if err == nil { - // Successfully located - return io.EOF - } - } - return nil - } - return nil - }) - if err != nil && err != io.EOF { - return -1, -1, "", err - } - } - if perms == "" { - return -1, -1, "", fmt.Errorf("Unable to find block device") - } - return result[0], result[1], perms, nil -} - -func (m *volumeMounter) isBlockFile(fileName string) bool { - // Stat the file and see if there is no error - out, err := statCommand(fileName) +func (m *volumeMounter) getSourceMajorMinor(sourceUID types.UID, volumeName string) (uint64, os.FileMode, error) { + basePath, err := deviceBasePath(sourceUID) if err != nil { - // Not a block device skip to next file - return false + return 0, 0, err } - split := strings.Split(string(out), ",") - // Verify I got 4 strings - if len(split) != 4 { - return false + devicePath, err := basePath.AppendAndResolveWithRelativeRoot(volumeName) + if err != nil { + return 0, 0, err } - return strings.TrimSpace(split[3]) == "block special file" + return m.getBlockFileMajorMinor(devicePath, statSourceDevice) } -func (m *volumeMounter) getBlockFileMajorMinor(fileName string) (int64, int64, string, error) { - result := make([]int, 2) - // Stat the file and see if there is no error - out, err := statCommand(fileName) +func (m *volumeMounter) getBlockFileMajorMinor(devicePath *safepath.Path, getter func(fileName *safepath.Path) (os.FileInfo, error)) (uint64, os.FileMode, error) { + fileInfo, err := getter(devicePath) if err != nil { - // Not a block device skip to next file - return -1, -1, "", err - } - split := strings.Split(string(out), ",") - // Verify I got 4 strings - if len(split) != 4 { - return -1, -1, "", fmt.Errorf("Output invalid") - } - if strings.TrimSpace(split[3]) != "block special file" { - return -1, -1, "", fmt.Errorf("Not a block device") + return 0, 0, err } - // Verify that both values are ints. - for i := 0; i < 2; i++ { - val, err := strconv.ParseInt(split[i], 16, 32) - if err != nil { - return -1, -1, "", err - } - result[i] = int(val) - } - return int64(result[0]), int64(result[1]), split[2], nil + info := fileInfo.Sys().(*syscall.Stat_t) + return info.Rdev, fileInfo.Mode(), nil } // getTargetCgroupPath returns the container cgroup path of the compute container in the pod. -func (m *volumeMounter) getTargetCgroupPath(vmi *v1.VirtualMachineInstance) (string, error) { - basePath := cgroupsBasePath() +func (m *volumeMounter) getTargetCgroupPath(vmi *v1.VirtualMachineInstance) (*safepath.Path, error) { + basePath, err := cgroupsBasePath() + if err != nil { + return nil, err + } isoRes, err := m.podIsolationDetector.Detect(vmi) if err != nil { - return "", err + return nil, err } - virtlauncherCgroupPath := filepath.Join(basePath, isoRes.Slice()) - fileInfo, err := os.Stat(virtlauncherCgroupPath) + virtlauncherCgroupPath, err := safepath.JoinNoFollow(basePath, isoRes.Slice()) + if err != nil { + return nil, fmt.Errorf("failed to determine custom image path %s: %v", virtlauncherCgroupPath, err) + } + fileInfo, err := safepath.StatAtNoFollow(virtlauncherCgroupPath) if err != nil { - return "", err + return nil, err } if !fileInfo.IsDir() { - return "", fmt.Errorf("detected path %s, but it is not a directory", virtlauncherCgroupPath) + return nil, fmt.Errorf("detected path %s, but it is not a directory", virtlauncherCgroupPath) } return virtlauncherCgroupPath, nil } -func (m *volumeMounter) removeBlockMajorMinor(major, minor int64, path string) error { - return m.updateBlockMajorMinor(major, minor, path, false) +func (m *volumeMounter) removeBlockMajorMinor(dev uint64, path *safepath.Path) error { + return m.updateBlockMajorMinor(dev, path, false) } -func (m *volumeMounter) allowBlockMajorMinor(major, minor int64, path string) error { - return m.updateBlockMajorMinor(major, minor, path, true) +func (m *volumeMounter) allowBlockMajorMinor(dev uint64, path *safepath.Path) error { + return m.updateBlockMajorMinor(dev, path, true) } -func (m *volumeMounter) updateBlockMajorMinor(major, minor int64, path string, allow bool) error { +func (m *volumeMounter) updateBlockMajorMinor(dev uint64, path *safepath.Path, allow bool) error { deviceRule := &devices.Rule{ Type: devices.BlockDevice, - Major: major, - Minor: minor, + Major: int64(unix.Major(dev)), + Minor: int64(unix.Minor(dev)), Permissions: "rwm", Allow: allow, } @@ -499,15 +496,15 @@ func (m *volumeMounter) updateBlockMajorMinor(major, minor int64, path string, a return nil } -func (m *volumeMounter) loadEmulator(path string) (*emulator.Emulator, error) { - list, err := fscommon.ReadFile(path, "devices.list") +func (m *volumeMounter) loadEmulator(path *safepath.Path) (*emulator.Emulator, error) { + list, err := fscommon.ReadFile(unsafepath.UnsafeAbsolute(path.Raw()), "devices.list") if err != nil { return nil, err } return emulator.EmulatorFromList(bytes.NewBufferString(list)) } -func (m *volumeMounter) updateDevicesList(path string, rule *devices.Rule) error { +func (m *volumeMounter) updateDevicesList(path *safepath.Path, rule *devices.Rule) error { // Create the target emulator for comparison later. target, err := m.loadEmulator(path) if err != nil { @@ -519,7 +516,7 @@ func (m *volumeMounter) updateDevicesList(path string, rule *devices.Rule) error if rule.Allow { file = "devices.allow" } - if err := fscommon.WriteFile(path, file, rule.CgroupString()); err != nil { + if err := fscommon.WriteFile(unsafepath.UnsafeAbsolute(path.Raw()), file, rule.CgroupString()); err != nil { return err } @@ -541,19 +538,12 @@ func (m *volumeMounter) updateDevicesList(path string, rule *devices.Rule) error return nil } -func (m *volumeMounter) createBlockDeviceFile(deviceName string, major, minor int64, blockDevicePermissions string) (string, error) { - exists, err := diskutils.FileExists(deviceName) - if err != nil { - return "", err - } - if !exists { - out, err := mknodCommand(deviceName, major, minor, blockDevicePermissions) - if err != nil { - log.DefaultLogger().Errorf("Error creating block device file: %s, %v", out, err) - return "", err - } +func (m *volumeMounter) createBlockDeviceFile(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error { + if _, err := safepath.JoinNoFollow(basePath, deviceName); os.IsNotExist(err) { + return mknodCommand(basePath, deviceName, dev, blockDevicePermissions) + } else { + return err } - return deviceName, nil } func (m *volumeMounter) mountFileSystemHotplugVolume(vmi *v1.VirtualMachineInstance, volume string, sourceUID types.UID, record *vmiMountTargetRecord) error { @@ -562,7 +552,7 @@ func (m *volumeMounter) mountFileSystemHotplugVolume(vmi *v1.VirtualMachineInsta // This is not the node the pod is running on. return nil } - targetDisk, err := m.hotplugDiskManager.GetFileSystemDiskTargetPathFromHostView(virtlauncherUID, volume, false) + targetDisk, err := m.hotplugDiskManager.GetFileSystemDiskTargetPathFromHostView(virtlauncherUID, volume, true) if err != nil { return err } @@ -577,14 +567,18 @@ func (m *volumeMounter) mountFileSystemHotplugVolume(vmi *v1.VirtualMachineInsta // to get mounted on the node, and this will error until the volume is mounted. return nil } - if err := m.writePathToMountRecord(targetDisk, vmi, record); err != nil { + if err := m.writePathToMountRecord(unsafepath.UnsafeAbsolute(targetDisk.Raw()), vmi, record); err != nil { return err } targetDisk, err := m.hotplugDiskManager.GetFileSystemDiskTargetPathFromHostView(virtlauncherUID, volume, true) if err != nil { return err } - if out, err := mountCommand(filepath.Join(sourcePath, "disk.img"), targetDisk); err != nil { + fullPath, err := sourcePath.AppendAndResolveWithRelativeRoot("disk.img") + if err != nil { + return err + } + if out, err := mountCommand(fullPath, targetDisk); err != nil { return fmt.Errorf("failed to bindmount hotplug-disk %v: %v : %v", volume, string(out), err) } } else { @@ -609,43 +603,58 @@ func (m *volumeMounter) findVirtlauncherUID(vmi *v1.VirtualMachineInstance) (uid return types.UID("") } -func (m *volumeMounter) getSourcePodFilePath(sourceUID types.UID, vmi *v1.VirtualMachineInstance, volume string) (string, error) { +func (m *volumeMounter) getSourcePodFilePath(sourceUID types.UID, vmi *v1.VirtualMachineInstance, volume string) (*safepath.Path, error) { iso := isolationDetector("/path") isoRes, err := iso.DetectForSocket(vmi, socketPath(sourceUID)) if err != nil { - return "", err + return nil, err } findmounts, err := LookupFindmntInfoByVolume(volume, isoRes.Pid()) if err != nil { - return "", err + return nil, err + } + mountRoot, err := nodeIsolationResult().MountRoot() + if err != nil { + return nil, err } + for _, findmnt := range findmounts { if filepath.Base(findmnt.Target) == volume { source := findmnt.GetSourcePath() - isBlock, _ := isBlockDevice(filepath.Join(util.HostRootMount, source)) - if _, err := os.Stat(filepath.Join(util.HostRootMount, source)); os.IsNotExist(err) || isBlock { + path, err := mountRoot.AppendAndResolveWithRelativeRoot(source) + exists := !os.IsNotExist(err) + if err != nil && !os.IsNotExist(err) { + return nil, err + } + + isBlock := false + if exists { + isBlock, _ = isBlockDevice(path) + } + + if !exists || isBlock { // file not found, or block device, or directory check if we can find the mount. deviceFindMnt, err := LookupFindmntInfoByDevice(source) if err != nil { // Try the device found from the source deviceFindMnt, err = LookupFindmntInfoByDevice(findmnt.GetSourceDevice()) if err != nil { - return "", err + return nil, err } // Check if the path was relative to the device. - if _, err := os.Stat(filepath.Join(util.HostRootMount, source)); err != nil { - return filepath.Join(deviceFindMnt[0].Target, source), nil + if !exists { + return mountRoot.AppendAndResolveWithRelativeRoot(deviceFindMnt[0].Target, source) } - return "", err + return nil, err } - return deviceFindMnt[0].Target, nil + return mountRoot.AppendAndResolveWithRelativeRoot(deviceFindMnt[0].Target) } else { - return source, nil + return path, nil } } } // Did not find the disk image file, return error - return "", fmt.Errorf("unable to find source disk image path for pod %s", sourceUID) + return nil, fmt.Errorf("unable to find source disk image path for pod %s", sourceUID) } // Unmount unmounts all hotplug disk that are no longer part of the VMI @@ -667,6 +676,13 @@ func (m *volumeMounter) Unmount(vmi *v1.VirtualMachineInstance) error { basePath, err := m.hotplugDiskManager.GetHotplugTargetPodPathOnHost(virtlauncherUID) if err != nil { + if os.IsNotExist(err) { + // no mounts left, the base path does not even exist anymore + if err := m.deleteMountTargetRecord(vmi); err != nil { + return fmt.Errorf("failed to delete mount target records: %v", err) + } + return nil + } return err } for _, volumeStatus := range vmi.Status.VolumeStatus { @@ -674,34 +690,47 @@ func (m *volumeMounter) Unmount(vmi *v1.VirtualMachineInstance) error { continue } if m.isBlockVolume(&vmi.Status, volumeStatus.Name) { - path := filepath.Join(basePath, volumeStatus.Name) - currentHotplugPaths[path] = virtlauncherUID + path, err := safepath.JoinNoFollow(basePath, volumeStatus.Name) + if err != nil { + return err + } + currentHotplugPaths[unsafepath.UnsafeAbsolute(path.Raw())] = virtlauncherUID } else { path, err := m.hotplugDiskManager.GetFileSystemDiskTargetPathFromHostView(virtlauncherUID, volumeStatus.Name, false) + if os.IsNotExist(err) { + // already unmounted or never mounted + continue + } if err != nil { return err } - currentHotplugPaths[path] = virtlauncherUID + currentHotplugPaths[unsafepath.UnsafeAbsolute(path.Raw())] = virtlauncherUID } } newRecord := vmiMountTargetRecord{ MountTargetEntries: make([]vmiMountTargetEntry, 0), } for _, entry := range record.MountTargetEntries { - diskPath := entry.TargetFile - if _, ok := currentHotplugPaths[diskPath]; !ok { - if m.isBlockFile(diskPath) { + fd, err := safepath.NewFileNoFollow(entry.TargetFile) + if err != nil { + return err + } + fd.Close() + diskPath := fd.Path() + + if _, ok := currentHotplugPaths[unsafepath.UnsafeAbsolute(diskPath.Raw())]; !ok { + if blockDevice, err := isBlockDevice(diskPath); err != nil { + return err + } else if blockDevice { if err := m.unmountBlockHotplugVolumes(diskPath, vmi); err != nil { return err } - } else { - if err := m.unmountFileSystemHotplugVolumes(diskPath); err != nil { - return err - } + } else if err := m.unmountFileSystemHotplugVolumes(diskPath); err != nil { + return err } } else { newRecord.MountTargetEntries = append(newRecord.MountTargetEntries, vmiMountTargetEntry{ - TargetFile: diskPath, + TargetFile: unsafepath.UnsafeAbsolute(diskPath.Raw()), }) } } @@ -717,7 +746,7 @@ func (m *volumeMounter) Unmount(vmi *v1.VirtualMachineInstance) error { return nil } -func (m *volumeMounter) unmountFileSystemHotplugVolumes(diskPath string) error { +func (m *volumeMounter) unmountFileSystemHotplugVolumes(diskPath *safepath.Path) error { if mounted, err := isMounted(diskPath); err != nil { return fmt.Errorf("failed to check mount point for hotplug disk %v: %v", diskPath, err) } else if mounted { @@ -725,7 +754,7 @@ func (m *volumeMounter) unmountFileSystemHotplugVolumes(diskPath string) error { if err != nil { return fmt.Errorf("failed to unmount hotplug disk %v: %v : %v", diskPath, string(out), err) } - err = os.Remove(diskPath) + err = safepath.UnlinkAtNoFollow(diskPath) if err != nil { return fmt.Errorf("failed to remove hotplug disk directory %v: %v : %v", diskPath, string(out), err) } @@ -734,22 +763,21 @@ func (m *volumeMounter) unmountFileSystemHotplugVolumes(diskPath string) error { return nil } -func (m *volumeMounter) unmountBlockHotplugVolumes(diskPath string, vmi *v1.VirtualMachineInstance) error { +func (m *volumeMounter) unmountBlockHotplugVolumes(diskPath *safepath.Path, vmi *v1.VirtualMachineInstance) error { // Get major and minor so we can deny the container. - major, minor, _, err := m.getBlockFileMajorMinor(diskPath) + dev, _, err := m.getBlockFileMajorMinor(diskPath, statDevice) if err != nil { return err } // Delete block device file - err = os.Remove(diskPath) - if err != nil { + if err := safepath.UnlinkAtNoFollow(diskPath); err != nil { return err } path, err := m.getTargetCgroupPath(vmi) if err != nil { return err } - if err := m.removeBlockMajorMinor(major, minor, path); err != nil { + if err := m.removeBlockMajorMinor(dev, path); err != nil { return err } return nil @@ -770,14 +798,25 @@ func (m *volumeMounter) UnmountAll(vmi *v1.VirtualMachineInstance) error { } for _, entry := range record.MountTargetEntries { - diskPath := entry.TargetFile - if m.isBlockFile(diskPath) { - if err := m.unmountBlockHotplugVolumes(diskPath, vmi); err != nil { + diskPath, err := safepath.NewFileNoFollow(entry.TargetFile) + if err != nil { + if os.IsNotExist(err) { + logger.Infof("Device %v is not mounted anymore, continuing.", entry.TargetFile) + continue + } + logger.Infof("Unable to unmount volume at path %s: %v", entry.TargetFile, err) + continue + } + diskPath.Close() + if isBlock, err := isBlockDevice(diskPath.Path()); err != nil { + logger.Infof("Unable to remove block device at path %s: %v", diskPath, err) + } else if isBlock { + if err := m.unmountBlockHotplugVolumes(diskPath.Path(), vmi); err != nil { logger.Infof("Unable to remove block device at path %s: %v", diskPath, err) // Don't return error, try next. } } else { - if err := m.unmountFileSystemHotplugVolumes(diskPath); err != nil { + if err := m.unmountFileSystemHotplugVolumes(diskPath.Path()); err != nil { logger.Infof("Unable to unmount volume at path %s: %v", diskPath, err) // Don't return error, try next. } @@ -799,12 +838,28 @@ func (m *volumeMounter) IsMounted(vmi *v1.VirtualMachineInstance, volume string, } targetPath, err := m.hotplugDiskManager.GetHotplugTargetPodPathOnHost(virtlauncherUID) if err != nil { + if os.IsNotExist(err) { + return false, nil + } return false, err } if m.isBlockVolume(&vmi.Status, volume) { - deviceName := filepath.Join(targetPath, volume) + deviceName, err := safepath.JoinNoFollow(targetPath, volume) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, err + } isBlockExists, _ := isBlockDevice(deviceName) return isBlockExists, nil } - return isMounted(filepath.Join(targetPath, fmt.Sprintf("%s.img", volume))) + path, err := safepath.JoinNoFollow(targetPath, fmt.Sprintf("%s.img", volume)) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, err + } + return isMounted(path) } diff --git a/pkg/virt-handler/hotplug-disk/mount_test.go b/pkg/virt-handler/hotplug-disk/mount_test.go index 66bae2924..5bcc3995f 100644 --- a/pkg/virt-handler/hotplug-disk/mount_test.go +++ b/pkg/virt-handler/hotplug-disk/mount_test.go @@ -22,11 +22,19 @@ package hotplug_volume import ( "encoding/json" "fmt" + "io/fs" "io/ioutil" "os" "path/filepath" "reflect" "strings" + "syscall" + "time" + + "golang.org/x/sys/unix" + + "kubevirt.io/kubevirt/pkg/safepath" + "kubevirt.io/kubevirt/pkg/unsafepath" . "github.com/onsi/ginkgo" "github.com/onsi/ginkgo/extensions/table" @@ -38,7 +46,6 @@ import ( "k8s.io/apimachinery/pkg/types" v1 "kubevirt.io/api/core/v1" - "kubevirt.io/client-go/log" hotplugdisk "kubevirt.io/kubevirt/pkg/hotplug-disk" "kubevirt.io/kubevirt/pkg/virt-handler/isolation" ) @@ -49,9 +56,11 @@ const ( var ( tempDir string + tmpDirSafe *safepath.Path orgIsoDetector = isolationDetector orgDeviceBasePath = deviceBasePath - orgStatCommand = statCommand + orgStatSourceCommand = statSourceDevice + orgStatCommand = statDevice orgCgroupsBasePath = cgroupsBasePath orgMknodCommand = mknodCommand orgSourcePodBasePath = sourcePodBasePath @@ -102,7 +111,7 @@ var _ = Describe("HotplugVolume mount target records", func() { AfterEach(func() { os.RemoveAll(tempDir) deviceBasePath = orgDeviceBasePath - statCommand = orgStatCommand + statSourceDevice = orgStatSourceCommand cgroupsBasePath = orgCgroupsBasePath mknodCommand = orgMknodCommand }) @@ -151,7 +160,7 @@ var _ = Describe("HotplugVolume mount target records", func() { Expect(err).ToNot(HaveOccurred()) res, err := m.getMountTargetRecord(vmi) Expect(err).ToNot(HaveOccurred()) - Expect(res).To(Equal(&vmiMountTargetRecord{})) + Expect(res).To(Equal(&vmiMountTargetRecord{UsesSafePaths: true})) }) It("deleteMountTargetRecord should remove both record file and entry file", func() { @@ -179,13 +188,15 @@ var _ = Describe("HotplugVolume block devices", func() { BeforeEach(func() { tempDir, err = ioutil.TempDir("", "hotplug-volume-test") Expect(err).ToNot(HaveOccurred()) + tmpDirSafe, err = safepath.JoinAndResolveWithRelativeRoot(tempDir) + Expect(err).ToNot(HaveOccurred()) vmi = api.NewMinimalVMI("fake-vmi") vmi.UID = "1234" activePods := make(map[types.UID]string, 0) activePods["abcd"] = "host" vmi.Status.ActivePods = activePods - targetPodPath = filepath.Join(tempDir, "abcd/volumes/kubernetes.io~empty-dir/hotplug-disks/testvolume") + targetPodPath = filepath.Join(tempDir, "abcd/volumes/kubernetes.io~empty-dir/hotplug-disks/") err = os.MkdirAll(targetPodPath, 0755) Expect(err).ToNot(HaveOccurred()) @@ -198,14 +209,14 @@ var _ = Describe("HotplugVolume block devices", func() { } record = &vmiMountTargetRecord{} - deviceBasePath = func(sourceUID types.UID) string { - return filepath.Join(tempDir, string(sourceUID), "volumes") + deviceBasePath = func(sourceUID types.UID) (*safepath.Path, error) { + return newDir(tempDir, string(sourceUID), "volumes") } - statCommand = func(fileName string) ([]byte, error) { - return []byte("6,6,0777,block special file"), nil + statSourceDevice = func(*safepath.Path) (os.FileInfo, error) { + return fakeStat(true, 0777, 123456), nil } - cgroupsBasePath = func() string { - return tempDir + cgroupsBasePath = func() (*safepath.Path, error) { + return tmpDirSafe, nil } }) @@ -213,7 +224,8 @@ var _ = Describe("HotplugVolume block devices", func() { AfterEach(func() { os.RemoveAll(tempDir) deviceBasePath = orgDeviceBasePath - statCommand = orgStatCommand + statSourceDevice = orgStatSourceCommand + statDevice = orgStatCommand cgroupsBasePath = orgCgroupsBasePath mknodCommand = orgMknodCommand isBlockDevice = orgIsBlockDevice @@ -266,20 +278,19 @@ var _ = Describe("HotplugVolume block devices", func() { It("mountBlockHotplugVolume and unmountBlockHotplugVolumes should make appropriate calls", func() { blockSourcePodUID := types.UID("fghij") - mknodCommand = func(deviceName string, major, minor int64, blockDevicePermissions string) ([]byte, error) { - Expect(os.MkdirAll(deviceName, 0755)).To(Succeed()) - return []byte("Yay"), nil + mknodCommand = func(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error { + _, err := newFile(unsafepath.UnsafeAbsolute(basePath.Raw()), deviceName) + Expect(err).ToNot(HaveOccurred()) + return nil } - isBlockDevice = func(path string) (bool, error) { - if strings.Contains(path, string(vmi.UID)) { - return true, nil - } - return false, fmt.Errorf("Not a block device") + isBlockDevice = func(path *safepath.Path) (bool, error) { + return strings.Contains(unsafepath.UnsafeAbsolute(path.Raw()), "blockvolume"), nil } targetPodPath := hotplugdisk.TargetPodBasePath(tempDir, m.findVirtlauncherUID(vmi)) err = os.MkdirAll(targetPodPath, 0755) Expect(err).ToNot(HaveOccurred()) - deviceFile := filepath.Join(tempDir, string(blockSourcePodUID), "volumes", "testvolume", "file") + deviceFile, err := newFile(tempDir, string(blockSourcePodUID), "volumes", "testvolume", "file") + Expect(err).ToNot(HaveOccurred()) slicePath := "slice" m.podIsolationDetector = &mockIsolationDetector{ slice: slicePath, @@ -289,16 +300,23 @@ var _ = Describe("HotplugVolume block devices", func() { devicesFile := filepath.Join(tempDir, slicePath, "devices.list") allowFile := filepath.Join(tempDir, slicePath, "devices.allow") denyFile := filepath.Join(tempDir, slicePath, "devices.deny") - _, err := os.Create(allowFile) + _, err = os.Create(allowFile) Expect(err).ToNot(HaveOccurred()) _, err = os.Create(denyFile) Expect(err).ToNot(HaveOccurred()) _, err = os.Create(devicesFile) Expect(err).ToNot(HaveOccurred()) - err = os.MkdirAll(filepath.Dir(deviceFile), 0755) - Expect(err).ToNot(HaveOccurred()) - err = ioutil.WriteFile(deviceFile, []byte("test"), 0644) + err = ioutil.WriteFile(unsafepath.UnsafeAbsolute(deviceFile.Raw()), []byte("test"), 0644) Expect(err).ToNot(HaveOccurred()) + statSourceDevice = func(fileName *safepath.Path) (os.FileInfo, error) { + return fakeStat(true, 0666, 123456), nil + } + statDevice = func(fileName *safepath.Path) (os.FileInfo, error) { + return fakeStat(true, 0666, 123456), nil + } + isBlockDevice = func(fileName *safepath.Path) (bool, error) { + return true, nil + } err = m.mountBlockHotplugVolume(vmi, "testvolume", blockSourcePodUID, record) Expect(err).ToNot(HaveOccurred()) By("Verifying the block file exists") @@ -307,21 +325,23 @@ var _ = Describe("HotplugVolume block devices", func() { By("Verifying the correct values are written to the allow file") content, err := ioutil.ReadFile(allowFile) Expect(err).ToNot(HaveOccurred()) - Expect("b 6:6 rwm").To(Equal(string(content))) + Expect("b 482:64 rwm").To(Equal(string(content))) By("Unmounting, we verify the reverse process happens") - err = m.unmountBlockHotplugVolumes(filepath.Join(targetPodPath, "testvolume"), vmi) + path, err := safepath.JoinAndResolveWithRelativeRoot(targetPodPath, "testvolume") + Expect(err).ToNot(HaveOccurred()) + err = m.unmountBlockHotplugVolumes(path, vmi) Expect(err).ToNot(HaveOccurred()) content, err = ioutil.ReadFile(denyFile) Expect(err).ToNot(HaveOccurred()) - Expect("b 6:6 rwm").To(Equal(string(content))) + Expect("b 482:64 rwm").To(Equal(string(content))) _, err = os.Stat(filepath.Join(targetPodPath, "testvolume")) Expect(err).To(HaveOccurred()) }) It("getSourceMajorMinor should return an error if no uid", func() { vmi.UID = "" - _, _, _, err := m.getSourceMajorMinor("fghij", "invalid") + _, _, err := m.getSourceMajorMinor("fghij", "invalid") Expect(err).To(HaveOccurred()) }) @@ -331,80 +351,42 @@ var _ = Describe("HotplugVolume block devices", func() { Expect(err).ToNot(HaveOccurred()) err = ioutil.WriteFile(deviceFile, []byte("test"), 0644) Expect(err).ToNot(HaveOccurred()) - major, minor, perm, err := m.getSourceMajorMinor("fghij", "test-volume") + numbers, perm, err := m.getSourceMajorMinor("fghij", "test-volume") Expect(err).ToNot(HaveOccurred()) - Expect(major).To(Equal(int64(6))) - Expect(minor).To(Equal(int64(6))) - Expect(perm).To(Equal("0777")) + Expect(unix.Major(numbers)).To(Equal(uint32(482))) + Expect(unix.Minor(numbers)).To(Equal(uint32(64))) + Expect(perm & 0777).To(Equal(os.FileMode(0777))) }) It("getSourceMajorMinor should return error if file doesn't exists", func() { deviceFile := filepath.Join(tempDir, "fghij", "volumes", "file") err = os.MkdirAll(filepath.Dir(deviceFile), 0755) Expect(err).ToNot(HaveOccurred()) - major, minor, perm, err := m.getSourceMajorMinor("fghij", "test-volume") + _, _, err := m.getSourceMajorMinor("fghij", "test-volume") Expect(err).To(HaveOccurred()) - Expect(major).To(Equal(int64(-1))) - Expect(minor).To(Equal(int64(-1))) - Expect(perm).To(Equal("")) + Expect(os.IsNotExist(err)).To(BeTrue()) }) - It("isBlockFile should return proper value based on stat command", func() { - testFileName := "test-file" - statCommand = func(fileName string) ([]byte, error) { - Expect(testFileName).To(Equal(fileName)) - return []byte("6,6,0777,block special file"), nil - } - Expect(m.isBlockFile(testFileName)).To(BeTrue()) - statCommand = func(fileName string) ([]byte, error) { - Expect(testFileName).To(Equal(fileName)) - return []byte("6,6,0777,block special file"), fmt.Errorf("Error") - } - Expect(m.isBlockFile(testFileName)).To(BeFalse()) - statCommand = func(fileName string) ([]byte, error) { - Expect(testFileName).To(Equal(fileName)) - return []byte("6,6,0777"), nil - } - Expect(m.isBlockFile(testFileName)).To(BeFalse()) - statCommand = func(fileName string) ([]byte, error) { - Expect(testFileName).To(Equal(fileName)) - return []byte("6,6,0777,block special"), nil - } - Expect(m.isBlockFile(testFileName)).To(BeFalse()) - }) - - table.DescribeTable("Should return proper values", func(stat func(fileName string) ([]byte, error), major, minor int, perm string, expectErr bool) { - testFileName := "test-file" - statCommand = stat - majorRes, minorRes, permRes, err := m.getBlockFileMajorMinor(testFileName) + table.DescribeTable("Should return proper values", func(stat func(safePath *safepath.Path) (os.FileInfo, error), major, minor uint32, perm os.FileMode, expectErr bool) { + statSourceDevice = stat + testFileName, err := newFile(tempDir, "test-file") + Expect(err).ToNot(HaveOccurred()) + numbers, permRes, err := m.getBlockFileMajorMinor(testFileName, statSourceDevice) if expectErr { Expect(err).To(HaveOccurred()) } else { Expect(err).ToNot(HaveOccurred()) } - // Values are translated to hex (245->580, 32->50) - Expect(int64(major)).To(Equal(majorRes)) - Expect(int64(minor)).To(Equal(minorRes)) - Expect(perm).To(Equal(permRes)) + Expect(unix.Major(numbers)).To(Equal(major)) + Expect(unix.Minor(numbers)).To(Equal(minor)) + Expect(perm).To(Equal(permRes & 0777)) }, - table.Entry("Should return values if stat command successful", func(fileName string) ([]byte, error) { - return []byte("245,32,0664,block special file"), nil - }, 581, 50, "0664", false), - table.Entry("Should not return values if stat command errors", func(fileName string) ([]byte, error) { - return []byte("245,32,0664,block special file"), fmt.Errorf("Error") - }, -1, -1, "", true), - table.Entry("Should not return values if stat command doesn't return 4 fields", func(fileName string) ([]byte, error) { - return []byte("245,32,0664"), nil - }, -1, -1, "", true), - table.Entry("Should not return values if stat command doesn't return block special file", func(fileName string) ([]byte, error) { - return []byte("245,32,0664, block file"), nil - }, -1, -1, "", true), - table.Entry("Should not return values if stat command doesn't int major", func(fileName string) ([]byte, error) { - return []byte("kk,32,0664,block special file"), nil - }, -1, -1, "", true), - table.Entry("Should not return values if stat command doesn't int minor", func(fileName string) ([]byte, error) { - return []byte("254,gg,0664,block special file"), nil - }, -1, -1, "", true), + table.Entry("Should return values if stat command successful", func(safePath *safepath.Path) (os.FileInfo, error) { + return fakeStat(true, 0664, 123456), nil + }, uint32(482), uint32(64), os.FileMode(0664), false), + table.Entry("Should not return error if stat command errors", func(safePath *safepath.Path) (os.FileInfo, error) { + return fakeStat(true, 0664, 123456), fmt.Errorf("Error") + }, uint32(0), uint32(0), os.FileMode(0), true), ) It("getTargetCgroupPath should return cgroup path", func() { @@ -417,7 +399,9 @@ var _ = Describe("HotplugVolume block devices", func() { Expect(err).ToNot(HaveOccurred()) path, err := m.getTargetCgroupPath(vmi) Expect(err).ToNot(HaveOccurred()) - Expect(path).To(Equal(filepath.Join(tempDir, slicePath))) + expectedPath, err := safepath.JoinNoFollow(tmpDirSafe, slicePath) + Expect(err).ToNot(HaveOccurred()) + Expect(path).To(Equal(expectedPath)) }) It("getTargetCgroupPath should return error if detect returns error", func() { @@ -464,71 +448,66 @@ var _ = Describe("HotplugVolume block devices", func() { Expect(err).ToNot(HaveOccurred()) _, err = os.Create(denyFile) Expect(err).ToNot(HaveOccurred()) - err = m.allowBlockMajorMinor(34, 53, filepath.Dir(allowFile)) + err = m.allowBlockMajorMinor(unix.Mkdev(482, 64), tmpDirSafe) Expect(err).ToNot(HaveOccurred()) content, err := ioutil.ReadFile(allowFile) Expect(err).ToNot(HaveOccurred()) - Expect("b 34:53 rwm").To(Equal(string(content))) + Expect("b 482:64 rwm").To(Equal(string(content))) - err = m.removeBlockMajorMinor(34, 53, filepath.Dir(denyFile)) + err = m.removeBlockMajorMinor(unix.Mkdev(482, 64), tmpDirSafe) Expect(err).ToNot(HaveOccurred()) content, err = ioutil.ReadFile(denyFile) Expect(err).ToNot(HaveOccurred()) - Expect("b 34:53 rwm").To(Equal(string(content))) + Expect("b 482:64 rwm").To(Equal(string(content))) }) It("Should error if allow/deny cannot be found", func() { - allowFile := filepath.Join(tempDir, "devices.allow") - denyFile := filepath.Join(tempDir, "devices.deny") - err = m.allowBlockMajorMinor(34, 53, filepath.Dir(allowFile)) + err = m.allowBlockMajorMinor(unix.Mkdev(482, 64), tmpDirSafe) Expect(err).To(HaveOccurred()) - err = m.removeBlockMajorMinor(34, 53, filepath.Dir(denyFile)) + err = m.removeBlockMajorMinor(unix.Mkdev(482, 64), tmpDirSafe) Expect(err).To(HaveOccurred()) }) It("Should attempt to create a block device file if it doesn't exist", func() { - testFile := filepath.Join(tempDir, "testfile") - testMajor := int64(100) - testMinor := int64(53) - testPerm := "0664" - mknodCommand = func(deviceName string, major, minor int64, blockDevicePermissions string) ([]byte, error) { - Expect(deviceName).To(Equal(testFile)) - Expect(major).To(Equal(testMajor)) - Expect(minor).To(Equal(testMinor)) + testMajor := uint32(482) + testMinor := uint32(64) + testPerm := os.FileMode(0664) + mknodCommand = func(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error { + Expect(basePath).To(Equal(tmpDirSafe)) + Expect(deviceName).To(Equal("testfile")) + Expect(unix.Major(dev)).To(Equal(testMajor)) + Expect(unix.Minor(dev)).To(Equal(testMinor)) Expect(blockDevicePermissions).To(Equal(testPerm)) - return []byte("Yay"), nil + return nil } - res, err := m.createBlockDeviceFile(testFile, testMajor, testMinor, testPerm) + err := m.createBlockDeviceFile(tmpDirSafe, "testfile", 123456, testPerm) Expect(err).ToNot(HaveOccurred()) - Expect(res).To(Equal(testFile)) - mknodCommand = func(deviceName string, major, minor int64, blockDevicePermissions string) ([]byte, error) { - Expect(deviceName).To(Equal(testFile)) - Expect(major).To(Equal(testMajor)) - Expect(minor).To(Equal(testMinor)) + mknodCommand = func(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error { + Expect(basePath).To(Equal(tmpDirSafe)) + Expect(deviceName).To(Equal("testfile")) + Expect(unix.Major(dev)).To(Equal(testMajor)) + Expect(unix.Minor(dev)).To(Equal(testMinor)) Expect(blockDevicePermissions).To(Equal(testPerm)) - return []byte("Yay"), fmt.Errorf("Error creating block file") + return fmt.Errorf("Error creating block file") } - _, err = m.createBlockDeviceFile(testFile, testMajor, testMinor, testPerm) + err = m.createBlockDeviceFile(tmpDirSafe, "testfile", 123456, testPerm) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("Error creating block file")) }) It("Should not attempt to create a block device file if it exists", func() { testFile := filepath.Join(tempDir, "testfile") - testMajor := int64(100) - testMinor := int64(53) - testPerm := "0664" + testPerm := os.FileMode(0664) _, err = os.Create(testFile) Expect(err).ToNot(HaveOccurred()) - mknodCommand = func(deviceName string, major, minor int64, blockDevicePermissions string) ([]byte, error) { + mknodCommand = func(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error { Fail("Should not get called") - return nil, nil + return nil } - res, err := m.createBlockDeviceFile(testFile, testMajor, testMinor, testPerm) + err := m.createBlockDeviceFile(tmpDirSafe, "testfile", 123456, testPerm) Expect(err).ToNot(HaveOccurred()) - Expect(res).To(Equal(testFile)) }) It("Should remove the block device and permissions on unmount", func() { @@ -539,26 +518,29 @@ var _ = Describe("HotplugVolume block devices", func() { } err = os.MkdirAll(expectedCgroupPath, 0755) Expect(err).ToNot(HaveOccurred()) - statCommand = func(fileName string) ([]byte, error) { - return []byte("245,32,0664,block special file"), nil + statSourceDevice = func(fileName *safepath.Path) (os.FileInfo, error) { + return fakeStat(true, 0664, 123456), nil } - deviceFileName := filepath.Join(tempDir, "devicefile") + statDevice = func(fileName *safepath.Path) (os.FileInfo, error) { + return fakeStat(true, 0664, 123456), nil + } + isBlockDevice = func(fileName *safepath.Path) (bool, error) { + return true, nil + } + deviceFile, err := newFile(tempDir, "devicefile") + Expect(err).ToNot(HaveOccurred()) denyFile := filepath.Join(expectedCgroupPath, "devices.deny") listFile := filepath.Join(expectedCgroupPath, "devices.list") - _, err := os.Create(deviceFileName) - Expect(err).ToNot(HaveOccurred()) _, err = os.Create(denyFile) Expect(err).ToNot(HaveOccurred()) _, err = os.Create(listFile) Expect(err).ToNot(HaveOccurred()) - err = m.unmountBlockHotplugVolumes(deviceFileName, vmi) + err = m.unmountBlockHotplugVolumes(deviceFile, vmi) Expect(err).ToNot(HaveOccurred()) content, err := ioutil.ReadFile(denyFile) Expect(err).ToNot(HaveOccurred()) // Since stat returns values in hex, we need to get hex value as int. - Expect("b 581:50 rwm").To(Equal(string(content))) - _, err = os.Stat(deviceFileName) - Expect(err).To(HaveOccurred()) + Expect("b 482:64 rwm").To(Equal(string(content))) }) It("Should return error if deviceFile doesn' exist", func() { @@ -569,11 +551,15 @@ var _ = Describe("HotplugVolume block devices", func() { } err = os.MkdirAll(expectedCgroupPath, 0755) Expect(err).ToNot(HaveOccurred()) - statCommand = func(fileName string) ([]byte, error) { - return []byte("245,32,0664,block special file"), nil + statSourceDevice = func(fileName *safepath.Path) (os.FileInfo, error) { + return fakeStat(true, 0664, 123456), nil + } + statDevice = func(fileName *safepath.Path) (os.FileInfo, error) { + return fakeStat(true, 0664, 123456), nil } - deviceFileName := filepath.Join(tempDir, "devicefile") - err = m.unmountBlockHotplugVolumes(deviceFileName, vmi) + deviceFile, err := newFile(tempDir, "devicefile") + Expect(err).ToNot(HaveOccurred()) + err = m.unmountBlockHotplugVolumes(deviceFile, vmi) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("no such file or directory")) }) @@ -587,13 +573,15 @@ var _ = Describe("HotplugVolume block devices", func() { } err = os.MkdirAll(expectedCgroupPath, 0755) Expect(err).ToNot(HaveOccurred()) - statCommand = func(fileName string) ([]byte, error) { - return []byte("245,32,0664,block special file"), nil + statSourceDevice = func(fileName *safepath.Path) (os.FileInfo, error) { + return fakeStat(true, 0644, 123456), nil + } + statDevice = func(fileName *safepath.Path) (os.FileInfo, error) { + return fakeStat(true, 0664, 123456), nil } - deviceFileName := filepath.Join(tempDir, "devicefile") - _, err := os.Create(deviceFileName) + deviceFile, err := newFile(tempDir, "devicefile") Expect(err).ToNot(HaveOccurred()) - err = m.unmountBlockHotplugVolumes(deviceFileName, vmi) + err = m.unmountBlockHotplugVolumes(deviceFile, vmi) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("Error detecting")) }) @@ -605,7 +593,7 @@ var _ = Describe("HotplugVolume filesystem volumes", func() { err error vmi *v1.VirtualMachineInstance record *vmiMountTargetRecord - targetPodPath string + targetPodPath *safepath.Path ) BeforeEach(func() { @@ -617,8 +605,7 @@ var _ = Describe("HotplugVolume filesystem volumes", func() { activePods["abcd"] = "host" vmi.Status.ActivePods = activePods - targetPodPath = filepath.Join(tempDir, "abcd/volumes/kubernetes.io~empty-dir/hotplug-disks") - err = os.MkdirAll(targetPodPath, 0755) + targetPodPath, err = newDir(tempDir, "abcd/volumes/kubernetes.io~empty-dir/hotplug-disks") Expect(err).ToNot(HaveOccurred()) record = &vmiMountTargetRecord{} @@ -630,8 +617,8 @@ var _ = Describe("HotplugVolume filesystem volumes", func() { hotplugDiskManager: hotplugdisk.NewHotplugDiskWithOptions(tempDir), } - deviceBasePath = func(sourceUID types.UID) string { - return filepath.Join(tempDir, string(sourceUID), "volumes") + deviceBasePath = func(sourceUID types.UID) (*safepath.Path, error) { + return newDir(tempDir, string(sourceUID), "volumes") } isolationDetector = func(path string) isolation.PodIsolationDetector { return &mockIsolationDetector{ @@ -652,20 +639,19 @@ var _ = Describe("HotplugVolume filesystem volumes", func() { }) It("getSourcePodFile should find the disk.img file, if it exists", func() { - path := filepath.Join(tempDir, "ghfjk", "volumes") - err = os.MkdirAll(path, 0755) - sourcePodBasePath = func(podUID types.UID) string { - return path + path, err := newDir(tempDir, "ghfjk", "volumes") + Expect(err).ToNot(HaveOccurred()) + sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) { + return path, nil } findMntByVolume = func(volumeName string, pid int) ([]byte, error) { - return []byte(fmt.Sprintf(findmntByVolumeRes, "pvc", path)), nil + return []byte(fmt.Sprintf(findmntByVolumeRes, "pvc", unsafepath.UnsafeAbsolute(path.Raw()))), nil } - diskFile := filepath.Join(path, "disk.img") - _, err := os.Create(diskFile) + _, err = newFile(unsafepath.UnsafeAbsolute(path.Raw()), "disk.img") Expect(err).ToNot(HaveOccurred()) file, err := m.getSourcePodFilePath("ghfjk", vmi, "pvc") Expect(err).ToNot(HaveOccurred()) - Expect(file).To(Equal(path)) + Expect(unsafepath.UnsafeRelative(file.Raw())).To(Equal(unsafepath.UnsafeAbsolute(path.Raw()))) }) It("getSourcePodFile should return error if no UID", func() { @@ -674,82 +660,78 @@ var _ = Describe("HotplugVolume filesystem volumes", func() { }) It("getSourcePodFile should return error if disk.img doesn't exist", func() { - path := filepath.Join(tempDir, "ghfjk", "volumes") - err = os.MkdirAll(path, 0755) - sourcePodBasePath = func(podUID types.UID) string { - return path - } + path, err := newDir(tempDir, "ghfjk", "volumes") Expect(err).ToNot(HaveOccurred()) - _, err := m.getSourcePodFilePath("ghfjk", vmi, "") + sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) { + return path, nil + } + _, err = m.getSourcePodFilePath("ghfjk", vmi, "") Expect(err).To(HaveOccurred()) }) It("getSourcePodFile should return error if iso detection returns error", func() { - expectedPath := filepath.Join(tempDir, "ghfjk", "volumes") - err = os.MkdirAll(expectedPath, 0755) - sourcePodBasePath = func(podUID types.UID) string { - return expectedPath + expectedPath, err := newDir(tempDir, "ghfjk", "volumes") + Expect(err).ToNot(HaveOccurred()) + sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) { + return expectedPath, nil } isolationDetector = func(path string) isolation.PodIsolationDetector { return &mockIsolationDetector{ - pid: 40, + pid: 9999, } } - Expect(err).ToNot(HaveOccurred()) - _, err := m.getSourcePodFilePath("ghfjk", vmi, "") + _, err = m.getSourcePodFilePath("ghfjk", vmi, "") Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("isolation error")) }) It("getSourcePodFile should return error if find mounts returns error", func() { - expectedPath := filepath.Join(tempDir, "ghfjk", "volumes") - err = os.MkdirAll(expectedPath, 0755) - sourcePodBasePath = func(podUID types.UID) string { - return expectedPath + expectedPath, err := newDir(tempDir, "ghfjk", "volumes") + Expect(err).ToNot(HaveOccurred()) + sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) { + return expectedPath, nil } findMntByVolume = func(volumeName string, pid int) ([]byte, error) { return []byte(""), fmt.Errorf("findmnt error") } - Expect(err).ToNot(HaveOccurred()) - _, err := m.getSourcePodFilePath("ghfjk", vmi, "") + _, err = m.getSourcePodFilePath("ghfjk", vmi, "") Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("findmnt error")) }) It("getSourcePodFile should return the findmnt value", func() { - expectedPath := filepath.Join(tempDir, "ghfjk", "volumes") - err = os.MkdirAll(expectedPath, 0755) - sourcePodBasePath = func(podUID types.UID) string { - return expectedPath + expectedPath, err := newDir(tempDir, "ghfjk", "volumes") + Expect(err).ToNot(HaveOccurred()) + sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) { + return expectedPath, nil } findMntByVolume = func(volumeName string, pid int) ([]byte, error) { - return []byte(fmt.Sprintf(findmntByVolumeRes, "pvc", expectedPath)), nil + return []byte(fmt.Sprintf(findmntByVolumeRes, "pvc", unsafepath.UnsafeAbsolute(expectedPath.Raw()))), nil } - Expect(err).ToNot(HaveOccurred()) res, err := m.getSourcePodFilePath("ghfjk", vmi, "pvc") Expect(err).ToNot(HaveOccurred()) - Expect(res).To(Equal(expectedPath)) + Expect(unsafepath.UnsafeRelative(res.Raw())).To(Equal(unsafepath.UnsafeAbsolute(expectedPath.Raw()))) }) It("should properly mount and unmount filesystem", func() { sourcePodUID := "ghfjk" - path := filepath.Join(tempDir, sourcePodUID, "volumes", "disk.img") - err = os.MkdirAll(path, 0755) - sourcePodBasePath = func(podUID types.UID) string { - return path + path, err := newDir(tempDir, sourcePodUID, "volumes") + Expect(err).ToNot(HaveOccurred()) + sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) { + return path, nil } - diskFile := filepath.Join(path, "disk.img") - _, err := os.Create(diskFile) + diskFile, err := newFile(unsafepath.UnsafeAbsolute(path.Raw()), "disk.img") Expect(err).ToNot(HaveOccurred()) findMntByVolume = func(volumeName string, pid int) ([]byte, error) { - return []byte(fmt.Sprintf(findmntByVolumeRes, "testvolume", path)), nil + return []byte(fmt.Sprintf(findmntByVolumeRes, "testvolume", unsafepath.UnsafeAbsolute(path.Raw()))), nil } - targetFilePath := filepath.Join(targetPodPath, "testvolume.img") - mountCommand = func(sourcePath, targetPath string) ([]byte, error) { - Expect(sourcePath).To(Equal(diskFile)) + targetFilePath, err := newFile(unsafepath.UnsafeAbsolute(targetPodPath.Raw()), "testvolume.img") + Expect(err).ToNot(HaveOccurred()) + mountCommand = func(sourcePath, targetPath *safepath.Path) ([]byte, error) { + Expect(unsafepath.UnsafeRelative(sourcePath.Raw())).To(Equal(unsafepath.UnsafeAbsolute(diskFile.Raw()))) Expect(targetPath).To(Equal(targetFilePath)) return []byte("Success"), nil } @@ -757,28 +739,27 @@ var _ = Describe("HotplugVolume filesystem volumes", func() { err = m.mountFileSystemHotplugVolume(vmi, "testvolume", types.UID(sourcePodUID), record) Expect(err).ToNot(HaveOccurred()) Expect(len(record.MountTargetEntries)).To(Equal(1)) - Expect(record.MountTargetEntries[0].TargetFile).To(Equal(targetFilePath)) + Expect(record.MountTargetEntries[0].TargetFile).To(Equal(unsafepath.UnsafeAbsolute(targetFilePath.Raw()))) - unmountCommand = func(diskPath string) ([]byte, error) { + unmountCommand = func(diskPath *safepath.Path) ([]byte, error) { Expect(targetFilePath).To(Equal(diskPath)) return []byte("Success"), nil } - isMounted = func(diskPath string) (bool, error) { + isMounted = func(diskPath *safepath.Path) (bool, error) { Expect(targetFilePath).To(Equal(diskPath)) return true, nil } - err = m.unmountFileSystemHotplugVolumes(record.MountTargetEntries[0].TargetFile) + err = m.unmountFileSystemHotplugVolumes(targetFilePath) Expect(err).ToNot(HaveOccurred()) - _, err = os.Stat(targetFilePath) - Expect(err).To(HaveOccurred()) }) It("unmountFileSystemHotplugVolumes should return error if isMounted returns error", func() { - testPath := "test" - isMounted = func(diskPath string) (bool, error) { - Expect(testPath).To(Equal(diskPath)) + testPath, err := newFile(tempDir, "test") + Expect(err).ToNot(HaveOccurred()) + isMounted = func(path *safepath.Path) (bool, error) { + Expect(testPath).To(Equal(path)) return false, fmt.Errorf("isMounted error") } @@ -788,8 +769,9 @@ var _ = Describe("HotplugVolume filesystem volumes", func() { }) It("unmountFileSystemHotplugVolumes should return nil if isMounted returns false", func() { - testPath := "test" - isMounted = func(diskPath string) (bool, error) { + testPath, err := newFile(tempDir, "test") + Expect(err).ToNot(HaveOccurred()) + isMounted = func(diskPath *safepath.Path) (bool, error) { Expect(testPath).To(Equal(diskPath)) return false, nil } @@ -799,12 +781,13 @@ var _ = Describe("HotplugVolume filesystem volumes", func() { }) It("unmountFileSystemHotplugVolumes should return error if unmountCommand returns error", func() { - testPath := "test" - isMounted = func(diskPath string) (bool, error) { + testPath, err := newFile(tempDir, "test") + Expect(err).ToNot(HaveOccurred()) + isMounted = func(diskPath *safepath.Path) (bool, error) { Expect(testPath).To(Equal(diskPath)) return true, nil } - unmountCommand = func(diskPath string) ([]byte, error) { + unmountCommand = func(diskPath *safepath.Path) ([]byte, error) { return []byte("Failure"), fmt.Errorf("unmount error") } @@ -825,6 +808,8 @@ var _ = Describe("HotplugVolume volumes", func() { BeforeEach(func() { tempDir, err = ioutil.TempDir("", "hotplug-volume-test") Expect(err).ToNot(HaveOccurred()) + tmpDirSafe, err = safepath.JoinAndResolveWithRelativeRoot(tempDir) + Expect(err).ToNot(HaveOccurred()) vmi = api.NewMinimalVMI("fake-vmi") vmi.UID = "1234" activePods := make(map[types.UID]string, 0) @@ -843,14 +828,17 @@ var _ = Describe("HotplugVolume volumes", func() { hotplugDiskManager: hotplugdisk.NewHotplugDiskWithOptions(tempDir), } - deviceBasePath = func(sourceUID types.UID) string { - return filepath.Join(tempDir, string(sourceUID), "volumes") + deviceBasePath = func(podUID types.UID) (*safepath.Path, error) { + return newDir(tempDir, string(podUID), "volumes") } - statCommand = func(fileName string) ([]byte, error) { - return []byte("6,6,0777,block special file"), nil + statSourceDevice = func(fileName *safepath.Path) (os.FileInfo, error) { + return fakeStat(true, 0777, 123456), nil } - cgroupsBasePath = func() string { - return tempDir + statDevice = func(fileName *safepath.Path) (os.FileInfo, error) { + return fakeStat(true, 0777, 123456), nil + } + cgroupsBasePath = func() (*safepath.Path, error) { + return tmpDirSafe, nil } isolationDetector = func(path string) isolation.PodIsolationDetector { return &mockIsolationDetector{ @@ -866,7 +854,8 @@ var _ = Describe("HotplugVolume volumes", func() { mountCommand = orgMountCommand unmountCommand = orgUnMountCommand isMounted = orgIsMounted - statCommand = orgStatCommand + statSourceDevice = orgStatSourceCommand + statDevice = orgStatCommand cgroupsBasePath = orgCgroupsBasePath mknodCommand = orgMknodCommand isBlockDevice = orgIsBlockDevice @@ -902,19 +891,16 @@ var _ = Describe("HotplugVolume volumes", func() { }, }) vmi.Status.VolumeStatus = volumeStatuses - deviceBasePath = func(sourceUID types.UID) string { - return filepath.Join(tempDir, string(sourceUID), "volumeDevices") + deviceBasePath = func(podUID types.UID) (*safepath.Path, error) { + return newDir(tempDir, string(podUID), "volumeDevices") } - blockDevicePath := filepath.Join(tempDir, string(sourcePodUID), "volumeDevices", "blockvolume") - fileSystemPath := filepath.Join(tempDir, string(sourcePodUID), "volumes", "disk.img") - By(fmt.Sprintf("Creating block path: %s", blockDevicePath)) - err = os.MkdirAll(blockDevicePath, 0755) + blockDevicePath, err := newDir(tempDir, string(sourcePodUID), "volumeDevices", "blockvolume") Expect(err).ToNot(HaveOccurred()) - By(fmt.Sprintf("Creating filesystem path: %s", fileSystemPath)) - err = os.MkdirAll(fileSystemPath, 0755) + fileSystemPath, err := newDir(tempDir, string(sourcePodUID), "volumes") Expect(err).ToNot(HaveOccurred()) - deviceFile := filepath.Join(blockDevicePath, "blockvolumefile") + deviceFile, err := newFile(unsafepath.UnsafeAbsolute(blockDevicePath.Raw()), "blockvolumefile") + Expect(err).ToNot(HaveOccurred()) slicePath := "slice" m.podIsolationDetector = &mockIsolationDetector{ slice: slicePath, @@ -924,37 +910,41 @@ var _ = Describe("HotplugVolume volumes", func() { allowFile := filepath.Join(tempDir, slicePath, "devices.allow") listFile := filepath.Join(tempDir, slicePath, "devices.list") denyFile := filepath.Join(tempDir, slicePath, "devices.deny") - _, err := os.Create(allowFile) + _, err = os.Create(allowFile) Expect(err).ToNot(HaveOccurred()) _, err = os.Create(denyFile) Expect(err).ToNot(HaveOccurred()) _, err = os.Create(listFile) Expect(err).ToNot(HaveOccurred()) - err = ioutil.WriteFile(deviceFile, []byte("test"), 0644) + err = ioutil.WriteFile(unsafepath.UnsafeAbsolute(deviceFile.Raw()), []byte("test"), 0644) Expect(err).ToNot(HaveOccurred()) - sourcePodBasePath = func(podUID types.UID) string { + sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) { if podUID == sourcePodUID { - return blockDevicePath + return blockDevicePath, nil } - return fileSystemPath + return fileSystemPath, nil } findMntByVolume = func(volumeName string, pid int) ([]byte, error) { - return []byte(fmt.Sprintf(findmntByVolumeRes, "filesystemvolume", fileSystemPath)), nil + return []byte(fmt.Sprintf(findmntByVolumeRes, "filesystemvolume", unsafepath.UnsafeAbsolute(fileSystemPath.Raw()))), nil } - diskFile := filepath.Join(fileSystemPath, "disk.img") - _, err = os.Create(diskFile) + diskFile, err := newFile(unsafepath.UnsafeAbsolute(fileSystemPath.Raw()), "disk.img") Expect(err).ToNot(HaveOccurred()) - mknodCommand = func(deviceName string, major, minor int64, blockDevicePermissions string) ([]byte, error) { - Expect(os.MkdirAll(deviceName, 0755)).To(Succeed()) - return []byte("Yay"), nil + mknodCommand = func(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error { + _, err := newFile(unsafepath.UnsafeAbsolute(basePath.Raw()), deviceName) + Expect(err).ToNot(HaveOccurred()) + return nil } + blockVolume := filepath.Join(targetPodPath, "blockvolume") targetFilePath := filepath.Join(targetPodPath, "filesystemvolume.img") - mountCommand = func(sourcePath, targetPath string) ([]byte, error) { - Expect(sourcePath).To(Equal(filepath.Join(fileSystemPath, "disk.img"))) - Expect(targetPath).To(Equal(targetFilePath)) + isBlockDevice = func(path *safepath.Path) (bool, error) { + return strings.Contains(unsafepath.UnsafeAbsolute(path.Raw()), "blockvolume"), nil + } + mountCommand = func(sourcePath, targetPath *safepath.Path) ([]byte, error) { + Expect(unsafepath.UnsafeRelative(sourcePath.Raw())).To(Equal(unsafepath.UnsafeAbsolute(diskFile.Raw()))) + Expect(unsafepath.UnsafeAbsolute(targetPath.Raw())).To(Equal(targetFilePath)) return []byte("Success"), nil } err = m.Mount(vmi) @@ -969,6 +959,7 @@ var _ = Describe("HotplugVolume volumes", func() { TargetFile: blockVolume, }, }, + UsesSafePaths: true, } expectedBytes, err := json.Marshal(record) Expect(err).ToNot(HaveOccurred()) @@ -1005,12 +996,8 @@ var _ = Describe("HotplugVolume volumes", func() { }) It("unmountAll should cleanup regardless of vmi volumestatuses", func() { - log.DefaultLogger().Infof("tempdir: %s", tempDir) sourcePodUID := types.UID("klmno") volumeStatuses := make([]v1.VolumeStatus, 0) - mknodCommand = func(deviceName string, major, minor int64, blockDevicePermissions string) ([]byte, error) { - return []byte("Yay"), nil - } volumeStatuses = append(volumeStatuses, v1.VolumeStatus{ Name: "permanent", }) @@ -1037,20 +1024,17 @@ var _ = Describe("HotplugVolume volumes", func() { }, }) vmi.Status.VolumeStatus = volumeStatuses - deviceBasePath = func(sourceUID types.UID) string { - return filepath.Join(tempDir, string(sourceUID), "volumeDevices") + deviceBasePath = func(podUID types.UID) (*safepath.Path, error) { + return newDir(tempDir, string(podUID), "volumeDevices") } - blockDevicePath := filepath.Join(tempDir, string(sourcePodUID), "volumeDevices", "blockvolume") - fileSystemPath := filepath.Join(tempDir, string(sourcePodUID), "volumes") - err = os.MkdirAll(blockDevicePath, 0755) + blockDevicePath, err := newDir(tempDir, string(sourcePodUID), "volumeDevices", "blockvolume") Expect(err).ToNot(HaveOccurred()) - err = os.MkdirAll(fileSystemPath, 0755) + fileSystemPath, err := newDir(tempDir, string(sourcePodUID), "volumes") + Expect(err).ToNot(HaveOccurred()) + deviceFile, err := newFile(unsafepath.UnsafeAbsolute(blockDevicePath.Raw()), "blockvolumefile") + Expect(err).ToNot(HaveOccurred()) + err = ioutil.WriteFile(unsafepath.UnsafeAbsolute(deviceFile.Raw()), []byte("test"), 0644) Expect(err).ToNot(HaveOccurred()) - findMntByVolume = func(volumeName string, pid int) ([]byte, error) { - return []byte(fmt.Sprintf(findmntByVolumeRes, "filesystemvolume", fileSystemPath)), nil - } - - deviceFile := filepath.Join(blockDevicePath, "file") slicePath := "slice" m.podIsolationDetector = &mockIsolationDetector{ slice: slicePath, @@ -1060,28 +1044,39 @@ var _ = Describe("HotplugVolume volumes", func() { allowFile := filepath.Join(tempDir, slicePath, "devices.allow") listFile := filepath.Join(tempDir, slicePath, "devices.list") denyFile := filepath.Join(tempDir, slicePath, "devices.deny") - _, err := os.Create(allowFile) + _, err = os.Create(allowFile) Expect(err).ToNot(HaveOccurred()) _, err = os.Create(denyFile) Expect(err).ToNot(HaveOccurred()) _, err = os.Create(listFile) Expect(err).ToNot(HaveOccurred()) - err = ioutil.WriteFile(deviceFile, []byte("test"), 0644) + err = ioutil.WriteFile(unsafepath.UnsafeAbsolute(deviceFile.Raw()), []byte("test"), 0644) Expect(err).ToNot(HaveOccurred()) - sourcePodBasePath = func(podUID types.UID) string { + sourcePodBasePath = func(podUID types.UID) (*safepath.Path, error) { if podUID == sourcePodUID { - return blockDevicePath + return blockDevicePath, nil } - return fileSystemPath + return fileSystemPath, nil } - diskFile := filepath.Join(fileSystemPath, "disk.img") - _, err = os.Create(diskFile) + findMntByVolume = func(volumeName string, pid int) ([]byte, error) { + return []byte(fmt.Sprintf(findmntByVolumeRes, "filesystemvolume", unsafepath.UnsafeAbsolute(fileSystemPath.Raw()))), nil + } + + diskFile, err := newFile(unsafepath.UnsafeAbsolute(fileSystemPath.Raw()), "disk.img") Expect(err).ToNot(HaveOccurred()) + mknodCommand = func(basePath *safepath.Path, deviceName string, dev uint64, blockDevicePermissions os.FileMode) error { + _, err := newFile(unsafepath.UnsafeAbsolute(basePath.Raw()), deviceName) + Expect(err).ToNot(HaveOccurred()) + return nil + } blockVolume := filepath.Join(targetPodPath, "blockvolume") targetFilePath := filepath.Join(targetPodPath, "filesystemvolume.img") - mountCommand = func(sourcePath, targetPath string) ([]byte, error) { - Expect(sourcePath).To(Equal(filepath.Join(fileSystemPath, "disk.img"))) - Expect(targetPath).To(Equal(targetFilePath)) + isBlockDevice = func(path *safepath.Path) (bool, error) { + return strings.Contains(unsafepath.UnsafeAbsolute(path.Raw()), "blockvolume"), nil + } + mountCommand = func(sourcePath, targetPath *safepath.Path) ([]byte, error) { + Expect(unsafepath.UnsafeRelative(sourcePath.Raw())).To(Equal(unsafepath.UnsafeAbsolute(diskFile.Raw()))) + Expect(unsafepath.UnsafeAbsolute(targetPath.Raw())).To(Equal(targetFilePath)) return []byte("Success"), nil } err = m.Mount(vmi) @@ -1096,6 +1091,7 @@ var _ = Describe("HotplugVolume volumes", func() { TargetFile: blockVolume, }, }, + UsesSafePaths: true, } expectedBytes, err := json.Marshal(record) Expect(err).ToNot(HaveOccurred()) @@ -1142,3 +1138,67 @@ func (i *mockIsolationDetector) Allowlist(_ []string) isolation.PodIsolationDete func (i *mockIsolationDetector) AdjustResources(_ *v1.VirtualMachineInstance) error { return nil } + +func newFile(baseDir string, elems ...string) (*safepath.Path, error) { + targetPath := filepath.Join(append([]string{baseDir}, elems...)...) + err := os.MkdirAll(filepath.Dir(targetPath), os.ModePerm) + if err != nil { + return nil, err + } + f, err := os.Create(targetPath) + if err != nil { + return nil, err + } + f.Close() + return safepath.JoinAndResolveWithRelativeRoot(baseDir, elems...) +} + +func newDir(baseDir string, elems ...string) (*safepath.Path, error) { + targetPath := filepath.Join(append([]string{baseDir}, elems...)...) + err := os.MkdirAll(targetPath, os.ModePerm) + if err != nil { + return nil, err + } + return safepath.JoinAndResolveWithRelativeRoot(baseDir, elems...) +} + +func fakeStat(isDevice bool, mode os.FileMode, dev uint64) os.FileInfo { + return fakeFileInfo{isDevice: isDevice, mode: mode, dev: dev} +} + +type fakeFileInfo struct { + isDevice bool + mode os.FileMode + dev uint64 +} + +func (f fakeFileInfo) Name() string { + //TODO implement me + panic("implement me") +} + +func (f fakeFileInfo) Size() int64 { + //TODO implement me + panic("implement me") +} + +func (f fakeFileInfo) Mode() fs.FileMode { + if f.isDevice { + return f.mode | fs.ModeDevice + } + return f.mode +} + +func (f fakeFileInfo) ModTime() time.Time { + //TODO implement me + panic("implement me") +} + +func (f fakeFileInfo) IsDir() bool { + //TODO implement me + panic("implement me") +} + +func (f fakeFileInfo) Sys() interface{} { + return &syscall.Stat_t{Rdev: f.dev} +} -- 2.37.1 From ae6a45dd26135bd870b19e389974ec9e22550632 Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Tue, 19 Jul 2022 11:29:46 +0200 Subject: [PATCH 13/16] Let virt-handler set up required devices with safepath operations Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit ddb060e2ed8d582191eb634b56655b10d109e045) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 829302a9d740bc40a78dfdbb02343407744d7361) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit ac875568d202c01cb017ad900c0fb5f679e643ed) Signed-off-by: Jed Lejosne <jed@redhat.com> --- cmd/virt-handler/BUILD.bazel | 1 + cmd/virt-handler/virt-handler.go | 14 +++++++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/cmd/virt-handler/BUILD.bazel b/cmd/virt-handler/BUILD.bazel index bbfb35b84..5c93988a0 100644 --- a/cmd/virt-handler/BUILD.bazel +++ b/cmd/virt-handler/BUILD.bazel @@ -17,6 +17,7 @@ go_library( "//pkg/monitoring/profiler:go_default_library", "//pkg/monitoring/reflector/prometheus:go_default_library", "//pkg/monitoring/workqueue/prometheus:go_default_library", + "//pkg/safepath:go_default_library", "//pkg/service:go_default_library", "//pkg/util:go_default_library", "//pkg/util/ratelimiter:go_default_library", diff --git a/cmd/virt-handler/virt-handler.go b/cmd/virt-handler/virt-handler.go index c61231c8c..d093d966c 100644 --- a/cmd/virt-handler/virt-handler.go +++ b/cmd/virt-handler/virt-handler.go @@ -45,6 +45,8 @@ import ( "k8s.io/client-go/util/certificate" "k8s.io/client-go/util/flowcontrol" + "kubevirt.io/kubevirt/pkg/safepath" + "kubevirt.io/kubevirt/pkg/util/ratelimiter" "kubevirt.io/kubevirt/pkg/virt-handler/node-labeller/api" @@ -384,7 +386,17 @@ func (app *virtHandlerApp) Run() { // relabel tun device unprivilegedContainerSELinuxLabel := "system_u:object_r:container_file_t:s0" - err = selinux.RelabelFiles(unprivilegedContainerSELinuxLabel, se.IsPermissive(), "/dev/net/tun", "/dev/null") + + devTun, err := safepath.JoinAndResolveWithRelativeRoot("/", "/dev/net/tun") + if err != nil { + panic(err) + } + devNull, err := safepath.JoinAndResolveWithRelativeRoot("/", "/dev/null") + if err != nil { + panic(err) + } + + err = selinux.RelabelFiles(unprivilegedContainerSELinuxLabel, se.IsPermissive(), devTun, devNull) if err != nil { panic(fmt.Errorf("error relabeling required files: %v", err)) } -- 2.37.1 From 67e7ddf0540a20c587a048d68e240841cae7137c Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Tue, 19 Jul 2022 11:30:25 +0200 Subject: [PATCH 14/16] Move virt-handler controller over to safepath usage Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 11120f94acbdd0110529b9e046d9881cde66bca2) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 1a7d75929a3fadc34e94752dbba8af1da17d2ada) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit ee1a75f8dcc1b056aabee518f19313c9ae515073) Signed-off-by: Jed Lejosne <jed@redhat.com> --- pkg/virt-handler/BUILD.bazel | 2 +- pkg/virt-handler/hotplug-disk/mount.go | 9 ++-- pkg/virt-handler/vm.go | 72 +++++++++++++++++++------- pkg/virt-handler/vm_test.go | 13 ++++- 4 files changed, 69 insertions(+), 27 deletions(-) diff --git a/pkg/virt-handler/BUILD.bazel b/pkg/virt-handler/BUILD.bazel index a1ee72b89..07d83165d 100644 --- a/pkg/virt-handler/BUILD.bazel +++ b/pkg/virt-handler/BUILD.bazel @@ -36,7 +36,6 @@ go_library( "//pkg/virt-handler/isolation:go_default_library", "//pkg/virt-handler/migration-proxy:go_default_library", "//pkg/virt-handler/node-labeller/api:go_default_library", - "//pkg/virt-handler/selinux:go_default_library", "//pkg/virt-launcher/virtwrap/api:go_default_library", "//pkg/watchdog:go_default_library", "//staging/src/kubevirt.io/api/core/v1:go_default_library", @@ -71,6 +70,7 @@ go_test( "//pkg/handler-launcher-com/cmd/v1:go_default_library", "//pkg/network/cache:go_default_library", "//pkg/network/errors:go_default_library", + "//pkg/safepath:go_default_library", "//pkg/testutils:go_default_library", "//pkg/util:go_default_library", "//pkg/virt-config:go_default_library", diff --git a/pkg/virt-handler/hotplug-disk/mount.go b/pkg/virt-handler/hotplug-disk/mount.go index baecdec28..b2fa125f4 100644 --- a/pkg/virt-handler/hotplug-disk/mount.go +++ b/pkg/virt-handler/hotplug-disk/mount.go @@ -574,15 +574,14 @@ func (m *volumeMounter) mountFileSystemHotplugVolume(vmi *v1.VirtualMachineInsta if err != nil { return err } - fullPath, err := sourcePath.AppendAndResolveWithRelativeRoot("disk.img") + sourcePath, err = sourcePath.AppendAndResolveWithRelativeRoot("disk.img") if err != nil { return err } - if out, err := mountCommand(fullPath, targetDisk); err != nil { - return fmt.Errorf("failed to bindmount hotplug-disk %v: %v : %v", volume, string(out), err) + if out, err := mountCommand(sourcePath, targetDisk); err != nil { + return fmt.Errorf("failed to bindmount hotplug-disk %v to %v: %v : %v", sourcePath, targetDisk, string(out), err) } - } else { - return nil + log.DefaultLogger().V(1).Infof("successfully mounted %v", volume) } return nil } diff --git a/pkg/virt-handler/vm.go b/pkg/virt-handler/vm.go index 9ef4810ed..ade252d2c 100644 --- a/pkg/virt-handler/vm.go +++ b/pkg/virt-handler/vm.go @@ -33,6 +33,8 @@ import ( "strings" "time" + "kubevirt.io/kubevirt/pkg/safepath" + "kubevirt.io/kubevirt/pkg/config" "github.com/opencontainers/runc/libcontainer/cgroups" @@ -373,20 +375,24 @@ func (d *VirtualMachineController) startDomainNotifyPipe(domainPipeStopChan chan } // inject the domain-notify.sock into the VMI pod. - socketPath := filepath.Join(res.MountRoot(), d.virtShareDir, "domain-notify-pipe.sock") - - os.RemoveAll(socketPath) - err = util.MkdirAllWithNosec(filepath.Dir(socketPath)) + root, err := res.MountRoot() + if err != nil { + return err + } + socketDir, err := root.AppendAndResolveWithRelativeRoot(d.virtShareDir) if err != nil { - log.Log.Reason(err).Error("unable to create directory for unix socket") return err } - listener, err := net.Listen("unix", socketPath) + listener, err := safepath.ListenUnixNoFollow(socketDir, "domain-notify-pipe.sock") if err != nil { log.Log.Reason(err).Error("failed to create unix socket for proxy service") return err } + socketPath, err := safepath.JoinNoFollow(socketDir, "domain-notify-pipe.sock") + if err != nil { + return err + } if util.IsNonRootVMI(vmi) { err := diskutils.DefaultOwnershipManager.SetFileOwnership(socketPath) @@ -498,7 +504,10 @@ func (d *VirtualMachineController) setupNetwork(vmi *v1.VirtualMachineInstance) if err != nil { return fmt.Errorf(failedDetectIsolationFmt, err) } - rootMount := isolationRes.MountRoot() + rootMount, err := isolationRes.MountRoot() + if err != nil { + return err + } requiresDeviceClaim := virtutil.IsNonRootVMI(vmi) && virtutil.WantVirtioNetDevice(vmi) return d.netConf.Setup(vmi, isolationRes.Pid(), func() error { @@ -1024,16 +1033,30 @@ func (d *VirtualMachineController) updateIsoSizeStatus(vmi *v1.VirtualMachineIns } for _, volume := range vmi.Spec.Volumes { + volPath, found := IsoGuestVolumePath(vmi, &volume) if !found { continue } + res, err := d.podIsolationDetector.Detect(vmi) if err != nil { - log.DefaultLogger().V(2).Warningf("failed to detect VMI %s", vmi.Name) + log.DefaultLogger().V(2).Reason(err).Warningf("failed to detect VMI %s", vmi.Name) continue } - size, err := isolation.GetFileSize(volPath, res) + + rootPath, err := res.MountRoot() + if err != nil { + log.DefaultLogger().V(2).Reason(err).Warningf("failed to detect VMI %s", vmi.Name) + continue + } + + safeVolPath, err := rootPath.AppendAndResolveWithRelativeRoot(volPath) + if err != nil { + log.DefaultLogger().V(2).Warningf("failed to determine file size for volume %s", volPath) + continue + } + fileInfo, err := safepath.StatAtNoFollow(safeVolPath) if err != nil { log.DefaultLogger().V(2).Warningf("failed to determine file size for volume %s", volPath) continue @@ -1041,7 +1064,7 @@ func (d *VirtualMachineController) updateIsoSizeStatus(vmi *v1.VirtualMachineIns for i, _ := range vmi.Status.VolumeStatus { if vmi.Status.VolumeStatus[i].Name == volume.Name { - vmi.Status.VolumeStatus[i].Size = size + vmi.Status.VolumeStatus[i].Size = fileInfo.Size() continue } } @@ -2392,7 +2415,10 @@ func (d *VirtualMachineController) vmUpdateHelperMigrationTarget(origVMI *v1.Vir if err != nil { return fmt.Errorf(failedDetectIsolationFmt, err) } - virtLauncherRootMount := isolationRes.MountRoot() + virtLauncherRootMount, err := isolationRes.MountRoot() + if err != nil { + return err + } err = d.claimDeviceOwnership(virtLauncherRootMount, "kvm") if err != nil { @@ -2481,7 +2507,10 @@ func (d *VirtualMachineController) vmUpdateHelperDefault(origVMI *v1.VirtualMach if err != nil { return fmt.Errorf(failedDetectIsolationFmt, err) } - virtLauncherRootMount := isolationRes.MountRoot() + virtLauncherRootMount, err := isolationRes.MountRoot() + if err != nil { + return err + } err = d.claimDeviceOwnership(virtLauncherRootMount, "kvm") if err != nil { @@ -2499,7 +2528,10 @@ func (d *VirtualMachineController) vmUpdateHelperDefault(origVMI *v1.VirtualMach } if virtutil.IsSEVVMI(vmi) { - sevDevice := path.Join(virtLauncherRootMount, "dev", "sev") + sevDevice, err := safepath.JoinNoFollow(virtLauncherRootMount, filepath.Join("dev", "sev")) + if err != nil { + return err + } if err := diskutils.DefaultOwnershipManager.SetFileOwnership(sevDevice); err != nil { return fmt.Errorf("failed to set SEV device owner: %v", err) } @@ -2757,15 +2789,17 @@ func (d *VirtualMachineController) isHostModelMigratable(vmi *v1.VirtualMachineI return nil } -func (d *VirtualMachineController) claimDeviceOwnership(virtLauncherRootMount, deviceName string) error { - kvmPath := filepath.Join(virtLauncherRootMount, "dev", deviceName) - - softwareEmulation, err := util.UseSoftwareEmulationForDevice(kvmPath, d.clusterConfig.AllowEmulation()) - if err != nil || softwareEmulation { +func (d *VirtualMachineController) claimDeviceOwnership(virtLauncherRootMount *safepath.Path, deviceName string) error { + softwareEmulation := d.clusterConfig.AllowEmulation() + devicePath, err := safepath.JoinNoFollow(virtLauncherRootMount, filepath.Join("dev", deviceName)) + if err != nil { + if softwareEmulation { + return nil + } return err } - return diskutils.DefaultOwnershipManager.SetFileOwnership(kvmPath) + return diskutils.DefaultOwnershipManager.SetFileOwnership(devicePath) } func nodeHasHostModelLabel(node *k8sv1.Node) bool { diff --git a/pkg/virt-handler/vm_test.go b/pkg/virt-handler/vm_test.go index c8e86b41f..d84d3ce71 100644 --- a/pkg/virt-handler/vm_test.go +++ b/pkg/virt-handler/vm_test.go @@ -32,6 +32,8 @@ import ( "k8s.io/utils/pointer" + "kubevirt.io/kubevirt/pkg/safepath" + k8sruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/testing" @@ -181,9 +183,16 @@ var _ = Describe("VirtualMachineInstance", func() { mockGracefulShutdown = &MockGracefulShutdown{shareDir} config, _, _ := testutils.NewFakeClusterConfigUsingKVConfig(&v1.KubeVirtConfiguration{}) + Expect(os.MkdirAll(filepath.Join(vmiShareDir, "dev"), 0755)).To(Succeed()) + f, err := os.OpenFile(filepath.Join(vmiShareDir, "dev", "kvm"), os.O_CREATE, 0755) + Expect(err).ToNot(HaveOccurred()) + f.Close() + mockIsolationResult = isolation.NewMockIsolationResult(ctrl) mockIsolationResult.EXPECT().Pid().Return(1).AnyTimes() - mockIsolationResult.EXPECT().MountRoot().Return(vmiShareDir).AnyTimes() + rootDir, err := safepath.JoinAndResolveWithRelativeRoot(vmiShareDir) + Expect(err).ToNot(HaveOccurred()) + mockIsolationResult.EXPECT().MountRoot().Return(rootDir, nil).AnyTimes() mockIsolationDetector = isolation.NewMockPodIsolationDetector(ctrl) mockIsolationDetector.EXPECT().Detect(gomock.Any()).Return(mockIsolationResult, nil).AnyTimes() @@ -220,7 +229,7 @@ var _ = Describe("VirtualMachineInstance", func() { podTestUUID = uuid.NewUUID() sockFile = cmdclient.SocketFilePathOnHost(string(podTestUUID)) Expect(os.MkdirAll(filepath.Dir(sockFile), 0755)).To(Succeed()) - f, err := os.Create(sockFile) + f, err = os.Create(sockFile) Expect(err).ToNot(HaveOccurred()) f.Close() -- 2.37.1 From 05589f2eaf78f9502da648e30899dd0db7900655 Mon Sep 17 00:00:00 2001 From: Roman Mohr <rmohr@google.com> Date: Mon, 25 Jul 2022 12:54:15 +0200 Subject: [PATCH 15/16] Ensure compatibility with old mount paths of old KubeVirt installs virt-handler needs to detect if mount record files contain old records which may not have been resolved yet. In that case it has to resolve them once to ensure that that symlinks, which were valid until now, would not break unmount operations on already running VMIs. After an initial load and write, the paths are clean and safe to use. Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit 5cbc2736c8655c8f500b035fb37b08f54cec9aee) Signed-off-by: Roman Mohr <rmohr@google.com> (cherry picked from commit fe14247d038f65f548ffbe698573fee9c8c7772c) Signed-off-by: Jed Lejosne <jed@redhat.com> (cherry picked from commit ecebd33b1ca06ccd05337aaf0758683fcdee1b8b) Signed-off-by: Jed Lejosne <jed@redhat.com> --- pkg/virt-handler/container-disk/mount.go | 17 +++++++++++++++++ pkg/virt-handler/hotplug-disk/mount.go | 20 +++++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/pkg/virt-handler/container-disk/mount.go b/pkg/virt-handler/container-disk/mount.go index 41237d14a..3e3fdb829 100644 --- a/pkg/virt-handler/container-disk/mount.go +++ b/pkg/virt-handler/container-disk/mount.go @@ -60,6 +60,7 @@ type vmiMountTargetEntry struct { type vmiMountTargetRecord struct { MountTargetEntries []vmiMountTargetEntry `json:"mountTargetEntries"` + UsesSafePaths bool `json:"usesSafePaths"` } func NewMounter(isoDetector isolation.PodIsolationDetector, mountStateDir string, clusterConfig *virtconfig.ClusterConfig) Mounter { @@ -144,6 +145,19 @@ func (m *mounter) getMountTargetRecord(vmi *v1.VirtualMachineInstance) (*vmiMoun return nil, err } + // XXX: backward compatibility for old unresolved paths, can be removed in July 2023 + // After a one-time convert and persist, old records are safe too. + if !record.UsesSafePaths { + record.UsesSafePaths = true + for i, entry := range record.MountTargetEntries { + safePath, err := safepath.JoinAndResolveWithRelativeRoot("/", entry.TargetFile) + if err != nil { + return nil, fmt.Errorf("failed converting legacy path to safepath: %v", err) + } + record.MountTargetEntries[i].TargetFile = unsafepath.UnsafeAbsolute(safePath.Raw()) + } + } + m.mountRecords[vmi.UID] = &record return &record, nil } @@ -156,6 +170,9 @@ func (m *mounter) setMountTargetRecord(vmi *v1.VirtualMachineInstance, record *v if string(vmi.UID) == "" { return fmt.Errorf("unable to set container disk mounted directories for vmi without uid") } + // XXX: backward compatibility for old unresolved paths, can be removed in July 2023 + // After a one-time convert and persist, old records are safe too. + record.UsesSafePaths = true recordFile := filepath.Join(m.mountStateDir, string(vmi.UID)) fileExists, err := diskutils.FileExists(recordFile) diff --git a/pkg/virt-handler/hotplug-disk/mount.go b/pkg/virt-handler/hotplug-disk/mount.go index b2fa125f4..f78c812ed 100644 --- a/pkg/virt-handler/hotplug-disk/mount.go +++ b/pkg/virt-handler/hotplug-disk/mount.go @@ -156,6 +156,7 @@ type vmiMountTargetEntry struct { type vmiMountTargetRecord struct { MountTargetEntries []vmiMountTargetEntry `json:"mountTargetEntries"` + UsesSafePaths bool `json:"usesSafePaths"` } // NewVolumeMounter creates a new VolumeMounter @@ -235,12 +236,25 @@ func (m *volumeMounter) getMountTargetRecord(vmi *v1.VirtualMachineInstance) (*v return nil, err } + // XXX: backward compatibility for old unresolved paths, can be removed in July 2023 + // After a one-time convert and persist, old records are safe too. + if !record.UsesSafePaths { + for i, path := range record.MountTargetEntries { + record.UsesSafePaths = true + safePath, err := safepath.JoinAndResolveWithRelativeRoot("/", path.TargetFile) + if err != nil { + return nil, fmt.Errorf("failed converting legacy path to safepath: %v", err) + } + record.MountTargetEntries[i].TargetFile = unsafepath.UnsafeAbsolute(safePath.Raw()) + } + } + m.mountRecords[vmi.UID] = &record return &record, nil } // not found - return &vmiMountTargetRecord{}, nil + return &vmiMountTargetRecord{UsesSafePaths: true}, nil } func (m *volumeMounter) setMountTargetRecord(vmi *v1.VirtualMachineInstance, record *vmiMountTargetRecord) error { @@ -248,6 +262,10 @@ func (m *volumeMounter) setMountTargetRecord(vmi *v1.VirtualMachineInstance, rec return fmt.Errorf(unableFindHotplugMountedDir) } + // XXX: backward compatibility for old unresolved paths, can be removed in July 2023 + // After a one-time convert and persist, old records are safe too. + record.UsesSafePaths = true + recordFile := filepath.Join(m.mountStateDir, string(vmi.UID)) m.mountRecordsLock.Lock() -- 2.37.1 From 082d4ccc1f1be9a016de92bc9ab7115e4fca7ac3 Mon Sep 17 00:00:00 2001 From: "L. Pivarc" <lpivarc@redhat.com> Date: Tue, 9 Aug 2022 21:41:29 +0200 Subject: [PATCH 16/16] Fix containerdisk unmount logic It is possible that record of mount is already unmouted and therefore the target path does not exists. Signed-off-by: L. Pivarc <lpivarc@redhat.com> (cherry picked from commit 5ce26afd3c6ff739d1b50e01bba20786805071ec) Signed-off-by: Jed Lejosne <jed@redhat.com> --- pkg/virt-handler/container-disk/mount.go | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/pkg/virt-handler/container-disk/mount.go b/pkg/virt-handler/container-disk/mount.go index 3e3fdb829..8b076ccba 100644 --- a/pkg/virt-handler/container-disk/mount.go +++ b/pkg/virt-handler/container-disk/mount.go @@ -328,19 +328,22 @@ func (m *mounter) Unmount(vmi *v1.VirtualMachineInstance) error { log.DefaultLogger().Object(vmi).Infof("Found container disk mount entries") for _, entry := range record.MountTargetEntries { log.DefaultLogger().Object(vmi).Infof("Looking to see if containerdisk is mounted at path %s", entry.TargetFile) - path, err := safepath.NewFileNoFollow(entry.TargetFile) + file, err := safepath.NewFileNoFollow(entry.TargetFile) if err != nil { - return fmt.Errorf(failedCheckMountPointFmt, path, err) + if os.IsNotExist(err) { + continue + } + return fmt.Errorf(failedCheckMountPointFmt, entry.TargetFile, err) } - _ = path.Close() - if mounted, err := isolation.IsMounted(path.Path()); err != nil { - return fmt.Errorf(failedCheckMountPointFmt, path, err) + _ = file.Close() + if mounted, err := isolation.IsMounted(file.Path()); err != nil { + return fmt.Errorf(failedCheckMountPointFmt, file, err) } else if mounted { - log.DefaultLogger().Object(vmi).Infof("unmounting container disk at path %s", path) + log.DefaultLogger().Object(vmi).Infof("unmounting container disk at file %s", file) // #nosec No risk for attacket injection. Parameters are predefined strings - out, err := virt_chroot.UmountChroot(path.Path()).CombinedOutput() + out, err := virt_chroot.UmountChroot(file.Path()).CombinedOutput() if err != nil { - return fmt.Errorf(failedUnmountFmt, path, string(out), err) + return fmt.Errorf(failedUnmountFmt, file, string(out), err) } } } -- 2.37.1
Locations
Projects
Search
Status Monitor
Help
OpenBuildService.org
Documentation
API Documentation
Code of Conduct
Contact
Support
@OBShq
Terms
openSUSE Build Service is sponsored by
The Open Build Service is an
openSUSE project
.
Sign Up
Log In
Places
Places
All Projects
Status Monitor