From c0633e38da4134135d35d32b56194790ca8acecd Mon Sep 17 00:00:00 2001 From: TomSweeneyRedHat Date: Fri, 3 May 2019 18:06:21 -0400 Subject: [PATCH] Handle WORKDIR with dangling target When the WORKDIR is a symlink and the target directory does not exist, RUN commands would fail. If the target doesn't exist, create the directory in the container. This also converts the code to use SecureJoin so that if a symlink is in the middle of a filepath, it will be converted correctly. Fixes: #1569 Tip of the hat to @karelyatin for the initial fix Signed-off-by: TomSweeneyRedHat Closes: #1576 Approved by: rhatdan --- imagebuildah/build.go | 31 ++-- tests/bud.bats | 43 +++++- tests/bud/workdir-symlink/Dockerfile-2 | 7 + tests/bud/workdir-symlink/Dockerfile-3 | 10 ++ vendor.conf | 1 + .../cyphar/filepath-securejoin/LICENSE | 28 ++++ .../cyphar/filepath-securejoin/README.md | 65 +++++++++ .../cyphar/filepath-securejoin/join.go | 135 ++++++++++++++++++ .../cyphar/filepath-securejoin/vendor.conf | 1 + .../cyphar/filepath-securejoin/vfs.go | 41 ++++++ 10 files changed, 346 insertions(+), 16 deletions(-) create mode 100644 tests/bud/workdir-symlink/Dockerfile-2 create mode 100644 tests/bud/workdir-symlink/Dockerfile-3 create mode 100644 vendor/github.com/cyphar/filepath-securejoin/LICENSE create mode 100644 vendor/github.com/cyphar/filepath-securejoin/README.md create mode 100644 vendor/github.com/cyphar/filepath-securejoin/join.go create mode 100644 vendor/github.com/cyphar/filepath-securejoin/vendor.conf create mode 100644 vendor/github.com/cyphar/filepath-securejoin/vfs.go diff --git a/imagebuildah/build.go b/imagebuildah/build.go index 70e2fbeaaa8..ac85084132a 100644 --- a/imagebuildah/build.go +++ b/imagebuildah/build.go @@ -27,6 +27,7 @@ import ( "github.com/containers/image/types" "github.com/containers/storage" "github.com/containers/storage/pkg/archive" + "github.com/cyphar/filepath-securejoin" docker "github.com/fsouza/go-dockerclient" v1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/opencontainers/runtime-spec/specs-go" @@ -480,22 +481,22 @@ func (s *StageExecutor) volumeCacheRestore() error { } // Copy copies data into the working tree. The "Download" field is how -// imagebuilder tells us the instruction was "ADD" and not "COPY". +// imagebuilder tells us the instruction was "ADD" and not "COPY" func (s *StageExecutor) Copy(excludes []string, copies ...imagebuilder.Copy) error { for _, copy := range copies { - // If the file exists, check to see if it's a symlink. - // If it is a symlink, convert to it's target otherwise - // the symlink will be overwritten. - fileDest, _ := os.Lstat(filepath.Join(s.mountPoint, copy.Dest)) - if fileDest != nil { - if fileDest.Mode()&os.ModeSymlink != 0 { - if symLink, err := resolveSymlink(s.mountPoint, copy.Dest); err == nil { - copy.Dest = symLink - } else { - return errors.Wrapf(err, "error reading symbolic link to %q", copy.Dest) - } - } + // Check the file and see if part of it is a symlink. + // Convert it to the target if so. To be ultrasafe + // do the same for the mountpoint. + secureMountPoint, err := securejoin.SecureJoin("", s.mountPoint) + finalPath, err := securejoin.SecureJoin(secureMountPoint, copy.Dest) + if err != nil { + return errors.Wrapf(err, "error resolving symlinks for copy destination %s", copy.Dest) + } + if !strings.HasPrefix(finalPath, secureMountPoint) { + return errors.Wrapf(err, "error resolving copy destination %s", copy.Dest) } + copy.Dest = strings.TrimPrefix(finalPath, secureMountPoint) + if copy.Download { logrus.Debugf("ADD %#v, %#v", excludes, copy) } else { @@ -1819,8 +1820,8 @@ func (b *Executor) deleteSuccessfulIntermediateCtrs() error { } func (s *StageExecutor) EnsureContainerPath(path string) error { - targetPath := filepath.Join(s.mountPoint, path) - _, err := os.Lstat(targetPath) + targetPath, err := securejoin.SecureJoin(s.mountPoint, path) + _, err = os.Lstat(targetPath) if err != nil && os.IsNotExist(err) { err = os.MkdirAll(targetPath, 0755) } diff --git a/tests/bud.bats b/tests/bud.bats index ff07deefde5..5bea0d040cf 100644 --- a/tests/bud.bats +++ b/tests/bud.bats @@ -1198,7 +1198,48 @@ load helpers run_buildah --debug=false run ${ctr} ls -alF /tempest expect_output --substring "/tempest -> /var/lib/tempest/" - run_buildah --debug=false run ${ctr} ls -alF /etc/notareal.conf + run_buildah --debug=false run ${ctr} ls -alF /etc/notareal.conf + expect_output --substring "\-rw\-rw\-r\-\-" +} + +@test "bud WORKDIR isa symlink no target dir" { + target=alpine-image + ctr=alpine-ctr + run_buildah --debug=false bud --signature-policy ${TESTSDIR}/policy.json -t ${target} -f Dockerfile-2 ${TESTSDIR}/bud/workdir-symlink + expect_output --substring "STEP 2: RUN ln -sf " + + run_buildah --debug=false from --signature-policy ${TESTSDIR}/policy.json --name=${ctr} ${target} + expect_output --substring ${ctr} + + run_buildah --debug=false run ${ctr} ls -alF /tempest + expect_output --substring "/tempest -> /var/lib/tempest/" + + run_buildah --debug=false run ${ctr} ls /tempest + expect_output --substring "Dockerfile-2" + + run_buildah --debug=false run ${ctr} ls -alF /etc/notareal.conf + expect_output --substring "\-rw\-rw\-r\-\-" +} + +@test "bud WORKDIR isa symlink no target dir and follow on dir" { + target=alpine-image + ctr=alpine-ctr + run_buildah --debug=false bud --signature-policy ${TESTSDIR}/policy.json -t ${target} -f Dockerfile-3 ${TESTSDIR}/bud/workdir-symlink + expect_output --substring "STEP 2: RUN ln -sf " + + run_buildah --debug=false from --signature-policy ${TESTSDIR}/policy.json --name=${ctr} ${target} + expect_output --substring ${ctr} + + run_buildah --debug=false run ${ctr} ls -alF /tempest + expect_output --substring "/tempest -> /var/lib/tempest/" + + run_buildah --debug=false run ${ctr} ls /tempest + expect_output --substring "Dockerfile-3" + + run_buildah --debug=false run ${ctr} ls /tempest/lowerdir + expect_output --substring "Dockerfile-3" + + run_buildah --debug=false run ${ctr} ls -alF /etc/notareal.conf expect_output --substring "\-rw\-rw\-r\-\-" } diff --git a/tests/bud/workdir-symlink/Dockerfile-2 b/tests/bud/workdir-symlink/Dockerfile-2 new file mode 100644 index 00000000000..f6fc1c358bd --- /dev/null +++ b/tests/bud/workdir-symlink/Dockerfile-2 @@ -0,0 +1,7 @@ +# No directory created for the target of the symlink +FROM alpine +RUN ln -sf /var/lib/tempest /tempest +WORKDIR /tempest +RUN touch /etc/notareal.conf +RUN chmod 664 /etc/notareal.conf +COPY Dockerfile-2 ./Dockerfile-2 diff --git a/tests/bud/workdir-symlink/Dockerfile-3 b/tests/bud/workdir-symlink/Dockerfile-3 new file mode 100644 index 00000000000..2f21e3893c6 --- /dev/null +++ b/tests/bud/workdir-symlink/Dockerfile-3 @@ -0,0 +1,10 @@ +# No directory created for the target of the symlink +FROM alpine +RUN ln -sf /var/lib/tempest /tempest +WORKDIR /tempest/lowerdir +RUN touch /etc/notareal.conf +RUN chmod 664 /etc/notareal.conf +RUN mkdir -p /tempest/lowerdir +COPY Dockerfile-3 ./Dockerfile-3 +COPY Dockerfile-3 /tempest/Dockerfile-3 +COPY Dockerfile-3 /tempest/lowerdir/Dockerfile-3 diff --git a/vendor.conf b/vendor.conf index 5dfafd924e9..0c982626a4f 100644 --- a/vendor.conf +++ b/vendor.conf @@ -4,6 +4,7 @@ github.com/BurntSushi/toml v0.2.0 github.com/containerd/continuity 004b46473808b3e7a4a3049c20e4376c91eb966d github.com/containernetworking/cni v0.7.0-rc2 github.com/containers/image 9467ac9cfd92c545aa389f22f27e552de053c0f2 +github.com/cyphar/filepath-securejoin v0.2.1 github.com/vbauerster/mpb v3.3.4 github.com/mattn/go-isatty v0.0.4 github.com/VividCortex/ewma v1.1.1 diff --git a/vendor/github.com/cyphar/filepath-securejoin/LICENSE b/vendor/github.com/cyphar/filepath-securejoin/LICENSE new file mode 100644 index 00000000000..bec842f294f --- /dev/null +++ b/vendor/github.com/cyphar/filepath-securejoin/LICENSE @@ -0,0 +1,28 @@ +Copyright (C) 2014-2015 Docker Inc & Go Authors. All rights reserved. +Copyright (C) 2017 SUSE LLC. All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are +met: + + * Redistributions of source code must retain the above copyright +notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above +copyright notice, this list of conditions and the following disclaimer +in the documentation and/or other materials provided with the +distribution. + * Neither the name of Google Inc. nor the names of its +contributors may be used to endorse or promote products derived from +this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. diff --git a/vendor/github.com/cyphar/filepath-securejoin/README.md b/vendor/github.com/cyphar/filepath-securejoin/README.md new file mode 100644 index 00000000000..49b2baa9f35 --- /dev/null +++ b/vendor/github.com/cyphar/filepath-securejoin/README.md @@ -0,0 +1,65 @@ +## `filepath-securejoin` ## + +[![Build Status](https://travis-ci.org/cyphar/filepath-securejoin.svg?branch=master)](https://travis-ci.org/cyphar/filepath-securejoin) + +An implementation of `SecureJoin`, a [candidate for inclusion in the Go +standard library][go#20126]. The purpose of this function is to be a "secure" +alternative to `filepath.Join`, and in particular it provides certain +guarantees that are not provided by `filepath.Join`. + +This is the function prototype: + +```go +func SecureJoin(root, unsafePath string) (string, error) +``` + +This library **guarantees** the following: + +* If no error is set, the resulting string **must** be a child path of + `SecureJoin` and will not contain any symlink path components (they will all + be expanded). + +* When expanding symlinks, all symlink path components **must** be resolved + relative to the provided root. In particular, this can be considered a + userspace implementation of how `chroot(2)` operates on file paths. Note that + these symlinks will **not** be expanded lexically (`filepath.Clean` is not + called on the input before processing). + +* Non-existant path components are unaffected by `SecureJoin` (similar to + `filepath.EvalSymlinks`'s semantics). + +* The returned path will always be `filepath.Clean`ed and thus not contain any + `..` components. + +A (trivial) implementation of this function on GNU/Linux systems could be done +with the following (note that this requires root privileges and is far more +opaque than the implementation in this library, and also requires that +`readlink` is inside the `root` path): + +```go +package securejoin + +import ( + "os/exec" + "path/filepath" +) + +func SecureJoin(root, unsafePath string) (string, error) { + unsafePath = string(filepath.Separator) + unsafePath + cmd := exec.Command("chroot", root, + "readlink", "--canonicalize-missing", "--no-newline", unsafePath) + output, err := cmd.CombinedOutput() + if err != nil { + return "", err + } + expanded := string(output) + return filepath.Join(root, expanded), nil +} +``` + +[go#20126]: https://github.com/golang/go/issues/20126 + +### License ### + +The license of this project is the same as Go, which is a BSD 3-clause license +available in the `LICENSE` file. diff --git a/vendor/github.com/cyphar/filepath-securejoin/join.go b/vendor/github.com/cyphar/filepath-securejoin/join.go new file mode 100644 index 00000000000..f20985479d4 --- /dev/null +++ b/vendor/github.com/cyphar/filepath-securejoin/join.go @@ -0,0 +1,135 @@ +// Copyright (C) 2014-2015 Docker Inc & Go Authors. All rights reserved. +// Copyright (C) 2017 SUSE LLC. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package securejoin is an implementation of the hopefully-soon-to-be-included +// SecureJoin helper that is meant to be part of the "path/filepath" package. +// The purpose of this project is to provide a PoC implementation to make the +// SecureJoin proposal (https://github.com/golang/go/issues/20126) more +// tangible. +package securejoin + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "strings" + "syscall" + + "github.com/pkg/errors" +) + +// ErrSymlinkLoop is returned by SecureJoinVFS when too many symlinks have been +// evaluated in attempting to securely join the two given paths. +var ErrSymlinkLoop = fmt.Errorf("SecureJoin: too many links") + +// IsNotExist tells you if err is an error that implies that either the path +// accessed does not exist (or path components don't exist). This is +// effectively a more broad version of os.IsNotExist. +func IsNotExist(err error) bool { + // If it's a bone-fide ENOENT just bail. + if os.IsNotExist(errors.Cause(err)) { + return true + } + + // Check that it's not actually an ENOTDIR, which in some cases is a more + // convoluted case of ENOENT (usually involving weird paths). + var errno error + switch err := errors.Cause(err).(type) { + case *os.PathError: + errno = err.Err + case *os.LinkError: + errno = err.Err + case *os.SyscallError: + errno = err.Err + } + return errno == syscall.ENOTDIR || errno == syscall.ENOENT +} + +// SecureJoinVFS joins the two given path components (similar to Join) except +// that the returned path is guaranteed to be scoped inside the provided root +// path (when evaluated). Any symbolic links in the path are evaluated with the +// given root treated as the root of the filesystem, similar to a chroot. The +// filesystem state is evaluated through the given VFS interface (if nil, the +// standard os.* family of functions are used). +// +// Note that the guarantees provided by this function only apply if the path +// components in the returned string are not modified (in other words are not +// replaced with symlinks on the filesystem) after this function has returned. +// Such a symlink race is necessarily out-of-scope of SecureJoin. +func SecureJoinVFS(root, unsafePath string, vfs VFS) (string, error) { + // Use the os.* VFS implementation if none was specified. + if vfs == nil { + vfs = osVFS{} + } + + var path bytes.Buffer + n := 0 + for unsafePath != "" { + if n > 255 { + return "", ErrSymlinkLoop + } + + // Next path component, p. + i := strings.IndexRune(unsafePath, filepath.Separator) + var p string + if i == -1 { + p, unsafePath = unsafePath, "" + } else { + p, unsafePath = unsafePath[:i], unsafePath[i+1:] + } + + // Create a cleaned path, using the lexical semantics of /../a, to + // create a "scoped" path component which can safely be joined to fullP + // for evaluation. At this point, path.String() doesn't contain any + // symlink components. + cleanP := filepath.Clean(string(filepath.Separator) + path.String() + p) + if cleanP == string(filepath.Separator) { + path.Reset() + continue + } + fullP := filepath.Clean(root + cleanP) + + // Figure out whether the path is a symlink. + fi, err := vfs.Lstat(fullP) + if err != nil && !IsNotExist(err) { + return "", err + } + // Treat non-existent path components the same as non-symlinks (we + // can't do any better here). + if IsNotExist(err) || fi.Mode()&os.ModeSymlink == 0 { + path.WriteString(p) + path.WriteRune(filepath.Separator) + continue + } + + // Only increment when we actually dereference a link. + n++ + + // It's a symlink, expand it by prepending it to the yet-unparsed path. + dest, err := vfs.Readlink(fullP) + if err != nil { + return "", err + } + // Absolute symlinks reset any work we've already done. + if filepath.IsAbs(dest) { + path.Reset() + } + unsafePath = dest + string(filepath.Separator) + unsafePath + } + + // We have to clean path.String() here because it may contain '..' + // components that are entirely lexical, but would be misleading otherwise. + // And finally do a final clean to ensure that root is also lexically + // clean. + fullP := filepath.Clean(string(filepath.Separator) + path.String()) + return filepath.Clean(root + fullP), nil +} + +// SecureJoin is a wrapper around SecureJoinVFS that just uses the os.* library +// of functions as the VFS. If in doubt, use this function over SecureJoinVFS. +func SecureJoin(root, unsafePath string) (string, error) { + return SecureJoinVFS(root, unsafePath, nil) +} diff --git a/vendor/github.com/cyphar/filepath-securejoin/vendor.conf b/vendor/github.com/cyphar/filepath-securejoin/vendor.conf new file mode 100644 index 00000000000..66bb574b955 --- /dev/null +++ b/vendor/github.com/cyphar/filepath-securejoin/vendor.conf @@ -0,0 +1 @@ +github.com/pkg/errors v0.8.0 diff --git a/vendor/github.com/cyphar/filepath-securejoin/vfs.go b/vendor/github.com/cyphar/filepath-securejoin/vfs.go new file mode 100644 index 00000000000..a82a5eae11e --- /dev/null +++ b/vendor/github.com/cyphar/filepath-securejoin/vfs.go @@ -0,0 +1,41 @@ +// Copyright (C) 2017 SUSE LLC. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package securejoin + +import "os" + +// In future this should be moved into a separate package, because now there +// are several projects (umoci and go-mtree) that are using this sort of +// interface. + +// VFS is the minimal interface necessary to use SecureJoinVFS. A nil VFS is +// equivalent to using the standard os.* family of functions. This is mainly +// used for the purposes of mock testing, but also can be used to otherwise use +// SecureJoin with VFS-like system. +type VFS interface { + // Lstat returns a FileInfo describing the named file. If the file is a + // symbolic link, the returned FileInfo describes the symbolic link. Lstat + // makes no attempt to follow the link. These semantics are identical to + // os.Lstat. + Lstat(name string) (os.FileInfo, error) + + // Readlink returns the destination of the named symbolic link. These + // semantics are identical to os.Readlink. + Readlink(name string) (string, error) +} + +// osVFS is the "nil" VFS, in that it just passes everything through to the os +// module. +type osVFS struct{} + +// Lstat returns a FileInfo describing the named file. If the file is a +// symbolic link, the returned FileInfo describes the symbolic link. Lstat +// makes no attempt to follow the link. These semantics are identical to +// os.Lstat. +func (o osVFS) Lstat(name string) (os.FileInfo, error) { return os.Lstat(name) } + +// Readlink returns the destination of the named symbolic link. These +// semantics are identical to os.Readlink. +func (o osVFS) Readlink(name string) (string, error) { return os.Readlink(name) }