From 85aec7fd7cefacf02a3ae467d416b2bc5cc60947 Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Tue, 23 Jul 2024 18:15:14 +0000 Subject: [PATCH] fix(deps): update module github.com/cyphar/filepath-securejoin to v0.3.1 Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- go.mod | 2 +- go.sum | 4 +- .../cyphar/filepath-securejoin/CHANGELOG.md | 138 +++++++++++++ .../cyphar/filepath-securejoin/VERSION | 2 +- .../filepath-securejoin/lookup_linux.go | 195 +++++++++--------- .../cyphar/filepath-securejoin/mkdir_linux.go | 7 +- .../cyphar/filepath-securejoin/open_linux.go | 50 +++-- .../filepath-securejoin/openat2_linux.go | 17 +- .../filepath-securejoin/procfs_linux.go | 85 ++++---- .../testing_mocks_linux.go | 4 +- vendor/modules.txt | 2 +- 11 files changed, 339 insertions(+), 167 deletions(-) create mode 100644 vendor/github.com/cyphar/filepath-securejoin/CHANGELOG.md diff --git a/go.mod b/go.mod index 74ff32e2de..240441f807 100644 --- a/go.mod +++ b/go.mod @@ -10,7 +10,7 @@ require ( github.com/Microsoft/go-winio v0.6.2 github.com/Microsoft/hcsshim v0.12.5 github.com/containerd/stargz-snapshotter/estargz v0.15.1 - github.com/cyphar/filepath-securejoin v0.3.0 + github.com/cyphar/filepath-securejoin v0.3.1 github.com/docker/go-units v0.5.0 github.com/google/go-intervals v0.0.2 github.com/hashicorp/go-multierror v1.1.1 diff --git a/go.sum b/go.sum index 34d56a3cf8..92bba0884a 100644 --- a/go.sum +++ b/go.sum @@ -15,8 +15,8 @@ github.com/containerd/errdefs v0.1.0 h1:m0wCRBiu1WJT/Fr+iOoQHMQS/eP5myQ8lCv4Dz5Z github.com/containerd/errdefs v0.1.0/go.mod h1:YgWiiHtLmSeBrvpw+UfPijzbLaB77mEG1WwJTDETIV0= github.com/containerd/stargz-snapshotter/estargz v0.15.1 h1:eXJjw9RbkLFgioVaTG+G/ZW/0kEe2oEKCdS/ZxIyoCU= github.com/containerd/stargz-snapshotter/estargz v0.15.1/go.mod h1:gr2RNwukQ/S9Nv33Lt6UC7xEx58C+LHRdoqbEKjz1Kk= -github.com/cyphar/filepath-securejoin v0.3.0 h1:tXpmbiaeBrS/K2US8nhgwdKYnfAOnVfkcLPKFgFHeA0= -github.com/cyphar/filepath-securejoin v0.3.0/go.mod h1:F7i41x/9cBF7lzCrVsYs9fuzwRZm4NQsGTBdpp6mETc= +github.com/cyphar/filepath-securejoin v0.3.1 h1:1V7cHiaW+C+39wEfpH6XlLBQo3j/PciWFrgfCLS8XrE= +github.com/cyphar/filepath-securejoin v0.3.1/go.mod h1:F7i41x/9cBF7lzCrVsYs9fuzwRZm4NQsGTBdpp6mETc= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= diff --git a/vendor/github.com/cyphar/filepath-securejoin/CHANGELOG.md b/vendor/github.com/cyphar/filepath-securejoin/CHANGELOG.md new file mode 100644 index 0000000000..7436896e13 --- /dev/null +++ b/vendor/github.com/cyphar/filepath-securejoin/CHANGELOG.md @@ -0,0 +1,138 @@ +# Changelog # +All notable changes to this project will be documented in this file. + +The format is based on [Keep a Changelog](http://keepachangelog.com/) +and this project adheres to [Semantic Versioning](http://semver.org/). + +## [Unreleased] ## + +## [0.3.1] - 2024-07-23 ## + +### Changed ### +- By allowing `Open(at)InRoot` to opt-out of the extra work done by `MkdirAll` + to do the necessary "partial lookups", `Open(at)InRoot` now does less work + for both implementations (resulting in a many-fold decrease in the number of + operations for `openat2`, and a modest improvement for non-`openat2`) and is + far more guaranteed to match the correct `openat2(RESOLVE_IN_ROOT)` + behaviour. +- We now use `readlinkat(fd, "")` where possible. For `Open(at)InRoot` this + effectively just means that we no longer risk getting spurious errors during + rename races. However, for our hardened procfs handler, this in theory should + prevent mount attacks from tricking us when doing magic-link readlinks (even + when using the unsafe host `/proc` handle). Unfortunately `Reopen` is still + potentially vulnerable to those kinds of somewhat-esoteric attacks. + + Technically this [will only work on post-2.6.39 kernels][linux-readlinkat-emptypath] + but it seems incredibly unlikely anyone is using `filepath-securejoin` on a + pre-2011 kernel. + +### Fixed ### +- Several improvements were made to the errors returned by `Open(at)InRoot` and + `MkdirAll` when dealing with invalid paths under the emulated (ie. + non-`openat2`) implementation. Previously, some paths would return the wrong + error (`ENOENT` when the last component was a non-directory), and other paths + would be returned as though they were acceptable (trailing-slash components + after a non-directory would be ignored by `Open(at)InRoot`). + + These changes were done to match `openat2`'s behaviour and purely is a + consistency fix (most users are going to be using `openat2` anyway). + +[linux-readlinkat-emptypath]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=65cfc6722361570bfe255698d9cd4dccaf47570d + +## [0.3.0] - 2024-07-11 ## + +### Added ### +- A new set of `*os.File`-based APIs have been added. These are adapted from + [libpathrs][] and we strongly suggest using them if possible (as they provide + far more protection against attacks than `SecureJoin`): + + - `Open(at)InRoot` resolves a path inside a rootfs and returns an `*os.File` + handle to the path. Note that the handle returned is an `O_PATH` handle, + which cannot be used for reading or writing (as well as some other + operations -- [see open(2) for more details][open.2]) + + - `Reopen` takes an `O_PATH` file handle and safely re-opens it to upgrade + it to a regular handle. This can also be used with non-`O_PATH` handles, + but `O_PATH` is the most obvious application. + + - `MkdirAll` is an implementation of `os.MkdirAll` that is safe to use to + create a directory tree within a rootfs. + + As these are new APIs, they may change in the future. However, they should be + safe to start migrating to as we have extensive tests ensuring they behave + correctly and are safe against various races and other attacks. + +[libpathrs]: https://github.com/openSUSE/libpathrs +[open.2]: https://www.man7.org/linux/man-pages/man2/open.2.html + +## [0.2.5] - 2024-05-03 ## + +### Changed ### +- Some minor changes were made to how lexical components (like `..` and `.`) + are handled during path generation in `SecureJoin`. There is no behaviour + change as a result of this fix (the resulting paths are the same). + +### Fixed ### +- The error returned when we hit a symlink loop now references the correct + path. (#10) + +## [0.2.4] - 2023-09-06 ## + +### Security ### +- This release fixes a potential security issue in filepath-securejoin when + used on Windows ([GHSA-6xv5-86q9-7xr8][], which could be used to generate + paths outside of the provided rootfs in certain cases), as well as improving + the overall behaviour of filepath-securejoin when dealing with Windows paths + that contain volume names. Thanks to Paulo Gomes for discovering and fixing + these issues. + +### Fixed ### +- Switch to GitHub Actions for CI so we can test on Windows as well as Linux + and MacOS. + +[GHSA-6xv5-86q9-7xr8]: https://github.com/advisories/GHSA-6xv5-86q9-7xr8 + +## [0.2.3] - 2021-06-04 ## + +### Changed ### +- Switch to Go 1.13-style `%w` error wrapping, letting us drop the dependency + on `github.com/pkg/errors`. + +## [0.2.2] - 2018-09-05 ## + +### Changed ### +- Use `syscall.ELOOP` as the base error for symlink loops, rather than our own + (internal) error. This allows callers to more easily use `errors.Is` to check + for this case. + +## [0.2.1] - 2018-09-05 ## + +### Fixed ### +- Use our own `IsNotExist` implementation, which lets us handle `ENOTDIR` + properly within `SecureJoin`. + +## [0.2.0] - 2017-07-19 ## + +We now have 100% test coverage! + +### Added ### +- Add a `SecureJoinVFS` API that can be used for mocking (as we do in our new + tests) or for implementing custom handling of lookup operations (such as for + rootless containers, where work is necessary to access directories with weird + modes because we don't have `CAP_DAC_READ_SEARCH` or `CAP_DAC_OVERRIDE`). + +## 0.1.0 - 2017-07-19 + +This is our first release of `github.com/cyphar/filepath-securejoin`, +containing a full implementation with a coverage of 93.5% (the only missing +cases are the error cases, which are hard to mocktest at the moment). + +[Unreleased]: https://github.com/cyphar/filepath-securejoin/compare/v0.3.1...HEAD +[0.3.1]: https://github.com/cyphar/filepath-securejoin/compare/v0.3.0...v0.3.1 +[0.3.0]: https://github.com/cyphar/filepath-securejoin/compare/v0.2.5...v0.3.0 +[0.2.5]: https://github.com/cyphar/filepath-securejoin/compare/v0.2.4...v0.2.5 +[0.2.4]: https://github.com/cyphar/filepath-securejoin/compare/v0.2.3...v0.2.4 +[0.2.3]: https://github.com/cyphar/filepath-securejoin/compare/v0.2.2...v0.2.3 +[0.2.2]: https://github.com/cyphar/filepath-securejoin/compare/v0.2.1...v0.2.2 +[0.2.1]: https://github.com/cyphar/filepath-securejoin/compare/v0.2.0...v0.2.1 +[0.2.0]: https://github.com/cyphar/filepath-securejoin/compare/v0.1.0...v0.2.0 diff --git a/vendor/github.com/cyphar/filepath-securejoin/VERSION b/vendor/github.com/cyphar/filepath-securejoin/VERSION index 0d91a54c7d..9e11b32fca 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/VERSION +++ b/vendor/github.com/cyphar/filepath-securejoin/VERSION @@ -1 +1 @@ -0.3.0 +0.3.1 diff --git a/vendor/github.com/cyphar/filepath-securejoin/lookup_linux.go b/vendor/github.com/cyphar/filepath-securejoin/lookup_linux.go index 140ac18ff5..290befa154 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/lookup_linux.go +++ b/vendor/github.com/cyphar/filepath-securejoin/lookup_linux.go @@ -40,16 +40,18 @@ func (se symlinkStackEntry) Close() { type symlinkStack []*symlinkStackEntry -func (s symlinkStack) IsEmpty() bool { - return len(s) == 0 +func (s *symlinkStack) IsEmpty() bool { + return s == nil || len(*s) == 0 } func (s *symlinkStack) Close() { - for _, link := range *s { - link.Close() + if s != nil { + for _, link := range *s { + link.Close() + } + // TODO: Switch to clear once we switch to Go 1.21. + *s = nil } - // TODO: Switch to clear once we switch to Go 1.21. - *s = nil } var ( @@ -58,11 +60,16 @@ var ( ) func (s *symlinkStack) popPart(part string) error { - if s.IsEmpty() { + if s == nil || s.IsEmpty() { // If there is nothing in the symlink stack, then the part was from the // real path provided by the user, and this is a no-op. return errEmptyStack } + if part == "." { + // "." components are no-ops -- we drop them when doing SwapLink. + return nil + } + tailEntry := (*s)[len(*s)-1] // Double-check that we are popping the component we expect. @@ -102,17 +109,13 @@ func (s *symlinkStack) PopPart(part string) error { } func (s *symlinkStack) push(dir *os.File, remainingPath, linkTarget string) error { + if s == nil { + return nil + } // Split the link target and clean up any "" parts. linkTargetParts := slices.DeleteFunc( strings.Split(linkTarget, "/"), - func(part string) bool { return part == "" }) - - // Don't add a no-op link to the stack. You can't create a no-op link - // symlink, but if the symlink is /, partialLookupInRoot has already jumped to the - // root and so there's nothing more to do. - if len(linkTargetParts) == 0 { - return nil - } + func(part string) bool { return part == "" || part == "." }) // Copy the directory so the caller doesn't close our copy. dirCopy, err := dupFile(dir) @@ -145,7 +148,7 @@ func (s *symlinkStack) SwapLink(linkPart string, dir *os.File, remainingPath, li } func (s *symlinkStack) PopTopSymlink() (*os.File, string, bool) { - if s.IsEmpty() { + if s == nil || s.IsEmpty() { return nil, "", false } tailEntry := (*s)[0] @@ -157,7 +160,22 @@ func (s *symlinkStack) PopTopSymlink() (*os.File, string, bool) { // within the provided root (a-la RESOLVE_IN_ROOT) and opens the final existing // component of the requested path, returning a file handle to the final // existing component and a string containing the remaining path components. -func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string, Err error) { +func partialLookupInRoot(root *os.File, unsafePath string) (*os.File, string, error) { + return lookupInRoot(root, unsafePath, true) +} + +func completeLookupInRoot(root *os.File, unsafePath string) (*os.File, error) { + handle, remainingPath, err := lookupInRoot(root, unsafePath, false) + if remainingPath != "" && err == nil { + // should never happen + err = fmt.Errorf("[bug] non-empty remaining path when doing a non-partial lookup: %q", remainingPath) + } + // lookupInRoot(partial=false) will always close the handle if an error is + // returned, so no need to double-check here. + return handle, err +} + +func lookupInRoot(root *os.File, unsafePath string, partial bool) (Handle *os.File, _ string, _ error) { unsafePath = filepath.ToSlash(unsafePath) // noop // This is very similar to SecureJoin, except that we operate on the @@ -166,7 +184,7 @@ func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string // Try to use openat2 if possible. if hasOpenat2() { - return partialLookupOpenat2(root, unsafePath) + return lookupOpenat2(root, unsafePath, partial) } // Get the "actual" root path from /proc/self/fd. This is necessary if the @@ -183,7 +201,8 @@ func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string return nil, "", fmt.Errorf("clone root fd: %w", err) } defer func() { - if Err != nil { + // If a handle is not returned, close the internal handle. + if Handle == nil { _ = currentDir.Close() } }() @@ -200,8 +219,11 @@ func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string // Note that the stack is ONLY used for book-keeping. All of the actual // path walking logic is still based on currentPath/remainingPath and // currentDir (as in SecureJoin). - var symlinkStack symlinkStack - defer symlinkStack.Close() + var symStack *symlinkStack + if partial { + symStack = new(symlinkStack) + defer symStack.Close() + } var ( linksWalked int @@ -220,9 +242,11 @@ func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string } else { part, remainingPath = remainingPath[:i], remainingPath[i+1:] } - // Skip any "//" components. + // If we hit an empty component, we need to treat it as though it is + // "." so that trailing "/" and "//" components on a non-directory + // correctly return the right error code. if part == "" { - continue + part = "." } // Apply the component lexically to the path we are building. @@ -233,7 +257,7 @@ func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string // If we logically hit the root, just clone the root rather than // opening the part and doing all of the other checks. if nextPath == "/" { - if err := symlinkStack.PopPart(part); err != nil { + if err := symStack.PopPart(part); err != nil { return nil, "", fmt.Errorf("walking into root with part %q failed: %w", part, err) } // Jump to root. @@ -258,63 +282,24 @@ func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string } switch st.Mode() & os.ModeType { - case os.ModeDir: - // If we are dealing with a directory, simply walk into it. - _ = currentDir.Close() - currentDir = nextDir - currentPath = nextPath - - // The part was real, so drop it from the symlink stack. - if err := symlinkStack.PopPart(part); err != nil { - return nil, "", fmt.Errorf("walking into directory %q failed: %w", part, err) - } - - // If we are operating on a .., make sure we haven't escaped. - // We only have to check for ".." here because walking down - // into a regular component component cannot cause you to - // escape. This mirrors the logic in RESOLVE_IN_ROOT, except we - // have to check every ".." rather than only checking after a - // rename or mount on the system. - if part == ".." { - // Make sure the root hasn't moved. - if err := checkProcSelfFdPath(logicalRootPath, root); err != nil { - return nil, "", fmt.Errorf("root path moved during lookup: %w", err) - } - // Make sure the path is what we expect. - fullPath := logicalRootPath + nextPath - if err := checkProcSelfFdPath(fullPath, currentDir); err != nil { - return nil, "", fmt.Errorf("walking into %q had unexpected result: %w", part, err) - } - } - case os.ModeSymlink: + // readlinkat implies AT_EMPTY_PATH since Linux 2.6.39. See + // Linux commit 65cfc6722361 ("readlinkat(), fchownat() and + // fstatat() with empty relative pathnames"). + linkDest, err := readlinkatFile(nextDir, "") // We don't need the handle anymore. _ = nextDir.Close() - - // Unfortunately, we cannot readlink through our handle and so - // we need to do a separate readlinkat (which could race to - // give us an error if the attacker swapped the symlink with a - // non-symlink). - linkDest, err := readlinkatFile(currentDir, part) if err != nil { - if errors.Is(err, unix.EINVAL) { - // The part was not a symlink, so assume that it's a - // regular file. It is possible for it to be a - // directory (if an attacker is swapping a directory - // and non-directory at this subpath) but erroring out - // here is better anyway. - err = fmt.Errorf("%w: path component %q is invalid: %w", errPossibleAttack, part, unix.ENOTDIR) - } return nil, "", err } linksWalked++ if linksWalked > maxSymlinkLimit { - return nil, "", &os.PathError{Op: "partialLookupInRoot", Path: logicalRootPath + "/" + unsafePath, Err: unix.ELOOP} + return nil, "", &os.PathError{Op: "securejoin.lookupInRoot", Path: logicalRootPath + "/" + unsafePath, Err: unix.ELOOP} } // Swap out the symlink's component for the link entry itself. - if err := symlinkStack.SwapLink(part, currentDir, oldRemainingPath, linkDest); err != nil { + if err := symStack.SwapLink(part, currentDir, oldRemainingPath, linkDest); err != nil { return nil, "", fmt.Errorf("walking into symlink %q failed: push symlink: %w", part, err) } @@ -331,50 +316,74 @@ func partialLookupInRoot(root *os.File, unsafePath string) (_ *os.File, _ string currentDir = rootClone currentPath = "/" } + default: - // For any other file type, we can't walk further and so we've - // hit the end of the lookup. The handling is very similar to - // ENOENT from openat(2), except that we return a handle to the - // component we just walked into (and we drop the component - // from the symlink stack). + // If we are dealing with a directory, simply walk into it. _ = currentDir.Close() + currentDir = nextDir + currentPath = nextPath - // The part existed, so drop it from the symlink stack. - if err := symlinkStack.PopPart(part); err != nil { - return nil, "", fmt.Errorf("walking into non-directory %q failed: %w", part, err) + // The part was real, so drop it from the symlink stack. + if err := symStack.PopPart(part); err != nil { + return nil, "", fmt.Errorf("walking into directory %q failed: %w", part, err) } - // If there are any remaining components in the symlink stack, - // we are still within a symlink resolution and thus we hit a - // dangling symlink. So pretend that the first symlink in the - // stack we hit was an ENOENT (to match openat2). - if oldDir, remainingPath, ok := symlinkStack.PopTopSymlink(); ok { - _ = nextDir.Close() - return oldDir, remainingPath, nil + // If we are operating on a .., make sure we haven't escaped. + // We only have to check for ".." here because walking down + // into a regular component component cannot cause you to + // escape. This mirrors the logic in RESOLVE_IN_ROOT, except we + // have to check every ".." rather than only checking after a + // rename or mount on the system. + if part == ".." { + // Make sure the root hasn't moved. + if err := checkProcSelfFdPath(logicalRootPath, root); err != nil { + return nil, "", fmt.Errorf("root path moved during lookup: %w", err) + } + // Make sure the path is what we expect. + fullPath := logicalRootPath + nextPath + if err := checkProcSelfFdPath(fullPath, currentDir); err != nil { + return nil, "", fmt.Errorf("walking into %q had unexpected result: %w", part, err) + } } - - // The current component exists, so return it. - return nextDir, remainingPath, nil } - case errors.Is(err, os.ErrNotExist): + default: + if !partial { + return nil, "", err + } // If there are any remaining components in the symlink stack, we // are still within a symlink resolution and thus we hit a dangling // symlink. So pretend that the first symlink in the stack we hit // was an ENOENT (to match openat2). - if oldDir, remainingPath, ok := symlinkStack.PopTopSymlink(); ok { + if oldDir, remainingPath, ok := symStack.PopTopSymlink(); ok { _ = currentDir.Close() - return oldDir, remainingPath, nil + return oldDir, remainingPath, err } // We have hit a final component that doesn't exist, so we have our // partial open result. Note that we have to use the OLD remaining // path, since the lookup failed. - return currentDir, oldRemainingPath, nil + return currentDir, oldRemainingPath, err + } + } - default: - return nil, "", err + // If the unsafePath had a trailing slash, we need to make sure we try to + // do a relative "." open so that we will correctly return an error when + // the final component is a non-directory (to match openat2). In the + // context of openat2, a trailing slash and a trailing "/." are completely + // equivalent. + if strings.HasSuffix(unsafePath, "/") { + nextDir, err := openatFile(currentDir, ".", unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if err != nil { + if !partial { + _ = currentDir.Close() + currentDir = nil + } + return currentDir, "", err } + _ = currentDir.Close() + currentDir = nextDir } + // All of the components existed! return currentDir, "", nil } diff --git a/vendor/github.com/cyphar/filepath-securejoin/mkdir_linux.go b/vendor/github.com/cyphar/filepath-securejoin/mkdir_linux.go index 05e0bde9af..ad2bd7973a 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/mkdir_linux.go +++ b/vendor/github.com/cyphar/filepath-securejoin/mkdir_linux.go @@ -49,14 +49,14 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err // Try to open as much of the path as possible. currentDir, remainingPath, err := partialLookupInRoot(root, unsafePath) - if err != nil { - return nil, fmt.Errorf("find existing subpath of %q: %w", unsafePath, err) - } defer func() { if Err != nil { _ = currentDir.Close() } }() + if err != nil && !errors.Is(err, unix.ENOENT) { + return nil, fmt.Errorf("find existing subpath of %q: %w", unsafePath, err) + } // If there is an attacker deleting directories as we walk into them, // detect this proactively. Note this is guaranteed to detect if the @@ -82,6 +82,7 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err } else if err != nil { return nil, fmt.Errorf("re-opening handle to %q: %w", currentDir.Name(), err) } else { + _ = currentDir.Close() currentDir = reopenDir } diff --git a/vendor/github.com/cyphar/filepath-securejoin/open_linux.go b/vendor/github.com/cyphar/filepath-securejoin/open_linux.go index 21700612c7..52dce76f3f 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/open_linux.go +++ b/vendor/github.com/cyphar/filepath-securejoin/open_linux.go @@ -9,6 +9,7 @@ package securejoin import ( "fmt" "os" + "strconv" "golang.org/x/sys/unix" ) @@ -16,13 +17,9 @@ import ( // OpenatInRoot is equivalent to OpenInRoot, except that the root is provided // using an *os.File handle, to ensure that the correct root directory is used. func OpenatInRoot(root *os.File, unsafePath string) (*os.File, error) { - handle, remainingPath, err := partialLookupInRoot(root, unsafePath) + handle, err := completeLookupInRoot(root, unsafePath) if err != nil { - return nil, err - } - if remainingPath != "" { - _ = handle.Close() - return nil, &os.PathError{Op: "securejoin.OpenInRoot", Path: unsafePath, Err: unix.ENOENT} + return nil, &os.PathError{Op: "securejoin.OpenInRoot", Path: unsafePath, Err: err} } return handle, nil } @@ -69,15 +66,36 @@ func Reopen(handle *os.File, flags int) (*os.File, error) { return nil, err } + // We can't operate on /proc/thread-self/fd/$n directly when doing a + // re-open, so we need to open /proc/thread-self/fd and then open a single + // final component. + procFdDir, closer, err := procThreadSelf(procRoot, "fd/") + if err != nil { + return nil, fmt.Errorf("get safe /proc/thread-self/fd handle: %w", err) + } + defer procFdDir.Close() + defer closer() + + // Try to detect if there is a mount on top of the magic-link we are about + // to open. If we are using unsafeHostProcRoot(), this could change after + // we check it (and there's nothing we can do about that) but for + // privateProcRoot() this should be guaranteed to be safe (at least since + // Linux 5.12[1], when anonymous mount namespaces were completely isolated + // from external mounts including mount propagation events). + // + // [1]: Linux commit ee2e3f50629f ("mount: fix mounting of detached mounts + // onto targets that reside on shared mounts"). + fdStr := strconv.Itoa(int(handle.Fd())) + if err := checkSymlinkOvermount(procRoot, procFdDir, fdStr); err != nil { + return nil, fmt.Errorf("check safety of /proc/thread-self/fd/%s magiclink: %w", fdStr, err) + } + flags |= unix.O_CLOEXEC - fdPath := fmt.Sprintf("fd/%d", handle.Fd()) - return doProcSelfMagiclink(procRoot, fdPath, func(procDirHandle *os.File, base string) (*os.File, error) { - // Rather than just wrapping openatFile, open-code it so we can copy - // handle.Name(). - reopenFd, err := unix.Openat(int(procDirHandle.Fd()), base, flags, 0) - if err != nil { - return nil, fmt.Errorf("reopen fd %d: %w", handle.Fd(), err) - } - return os.NewFile(uintptr(reopenFd), handle.Name()), nil - }) + // Rather than just wrapping openatFile, open-code it so we can copy + // handle.Name(). + reopenFd, err := unix.Openat(int(procFdDir.Fd()), fdStr, flags, 0) + if err != nil { + return nil, fmt.Errorf("reopen fd %d: %w", handle.Fd(), err) + } + return os.NewFile(uintptr(reopenFd), handle.Name()), nil } diff --git a/vendor/github.com/cyphar/filepath-securejoin/openat2_linux.go b/vendor/github.com/cyphar/filepath-securejoin/openat2_linux.go index fc93db8644..921b3e1d44 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/openat2_linux.go +++ b/vendor/github.com/cyphar/filepath-securejoin/openat2_linux.go @@ -87,6 +87,17 @@ func openat2File(dir *os.File, path string, how *unix.OpenHow) (*os.File, error) return nil, &os.PathError{Op: "openat2", Path: fullPath, Err: errPossibleAttack} } +func lookupOpenat2(root *os.File, unsafePath string, partial bool) (*os.File, string, error) { + if !partial { + file, err := openat2File(root, unsafePath, &unix.OpenHow{ + Flags: unix.O_PATH | unix.O_CLOEXEC, + Resolve: unix.RESOLVE_IN_ROOT | unix.RESOLVE_NO_MAGICLINKS, + }) + return file, "", err + } + return partialLookupOpenat2(root, unsafePath) +} + // partialLookupOpenat2 is an alternative implementation of // partialLookupInRoot, using openat2(RESOLVE_IN_ROOT) to more safely get a // handle to the deepest existing child of the requested path within the root. @@ -95,6 +106,7 @@ func partialLookupOpenat2(root *os.File, unsafePath string) (*os.File, string, e unsafePath = filepath.ToSlash(unsafePath) // noop endIdx := len(unsafePath) + var lastError error for endIdx > 0 { subpath := unsafePath[:endIdx] @@ -108,11 +120,12 @@ func partialLookupOpenat2(root *os.File, unsafePath string) (*os.File, string, e endIdx += 1 } // We found a subpath! - return handle, unsafePath[endIdx:], nil + return handle, unsafePath[endIdx:], lastError } if errors.Is(err, unix.ENOENT) || errors.Is(err, unix.ENOTDIR) { // That path doesn't exist, let's try the next directory up. endIdx = strings.LastIndexByte(subpath, '/') + lastError = err continue } return nil, "", fmt.Errorf("open subpath: %w", err) @@ -124,5 +137,5 @@ func partialLookupOpenat2(root *os.File, unsafePath string) (*os.File, string, e if err != nil { return nil, "", err } - return rootClone, unsafePath, nil + return rootClone, unsafePath, lastError } diff --git a/vendor/github.com/cyphar/filepath-securejoin/procfs_linux.go b/vendor/github.com/cyphar/filepath-securejoin/procfs_linux.go index daac3f0617..adf0bd08f3 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/procfs_linux.go +++ b/vendor/github.com/cyphar/filepath-securejoin/procfs_linux.go @@ -10,7 +10,6 @@ import ( "errors" "fmt" "os" - "path/filepath" "runtime" "strconv" "sync" @@ -160,7 +159,7 @@ func clonePrivateProcMount() (_ *os.File, Err error) { } func privateProcRoot() (*os.File, error) { - if !hasNewMountApi() { + if !hasNewMountApi() || testingForceGetProcRootUnsafe() { return nil, fmt.Errorf("new mount api: %w", unix.ENOTSUP) } // Try to create a new procfs mount from scratch if we can. This ensures we @@ -199,7 +198,7 @@ func unsafeHostProcRoot() (_ *os.File, Err error) { func doGetProcRoot() (*os.File, error) { procRoot, err := privateProcRoot() - if err != nil || testingForceGetProcRootUnsafe(procRoot) { + if err != nil { // Fall back to using a /proc handle if making a private mount failed. // If we have openat2, at least we can avoid some kinds of over-mount // attacks, but without openat2 there's not much we can do. @@ -286,14 +285,14 @@ func procThreadSelf(procRoot *os.File, subpath string) (_ *os.File, _ procThread // procSelfFdReadlink to clean up the returned f.Name() if we use // RESOLVE_IN_ROOT (which would lead to an infinite recursion). handle, err = openat2File(procRoot, threadSelf+subpath, &unix.OpenHow{ - Flags: unix.O_PATH | unix.O_CLOEXEC, + Flags: unix.O_PATH | unix.O_NOFOLLOW | unix.O_CLOEXEC, Resolve: unix.RESOLVE_BENEATH | unix.RESOLVE_NO_XDEV | unix.RESOLVE_NO_MAGICLINKS, }) if err != nil { return nil, nil, fmt.Errorf("%w: %w", errUnsafeProcfs, err) } } else { - handle, err = openatFile(procRoot, threadSelf+subpath, unix.O_PATH|unix.O_CLOEXEC, 0) + handle, err = openatFile(procRoot, threadSelf+subpath, unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) if err != nil { return nil, nil, fmt.Errorf("%w: %w", errUnsafeProcfs, err) } @@ -333,10 +332,10 @@ func hasStatxMountId() bool { return hasStatxMountIdBool } -func checkSymlinkOvermount(dir *os.File, path string) error { +func getMountId(dir *os.File, path string) (uint64, error) { // If we don't have statx(STATX_MNT_ID*) support, we can't do anything. if !hasStatxMountId() { - return nil + return 0, nil } var ( @@ -346,31 +345,29 @@ func checkSymlinkOvermount(dir *os.File, path string) error { wantStxMask uint32 = unix.STATX_MNT_ID_UNIQUE | unix.STATX_MNT_ID ) - // Get the mntId of our procfs handle. - err := unix.Statx(int(dir.Fd()), "", unix.AT_EMPTY_PATH, int(wantStxMask), &stx) - if err != nil { - return &os.PathError{Op: "statx", Path: dir.Name(), Err: err} - } + err := unix.Statx(int(dir.Fd()), path, unix.AT_EMPTY_PATH|unix.AT_SYMLINK_NOFOLLOW, int(wantStxMask), &stx) if stx.Mask&wantStxMask == 0 { // It's not a kernel limitation, for some reason we couldn't get a // mount ID. Assume it's some kind of attack. - return fmt.Errorf("%w: could not get mnt id of dir %s", errUnsafeProcfs, dir.Name()) + err = fmt.Errorf("%w: could not get mount id", errUnsafeProcfs) + } + if err != nil { + return 0, &os.PathError{Op: "statx(STATX_MNT_ID_...)", Path: dir.Name() + "/" + path, Err: err} } - expectedMountId := stx.Mnt_id + return stx.Mnt_id, nil +} - // Get the mntId of the target symlink. - stx = unix.Statx_t{} - err = unix.Statx(int(dir.Fd()), path, unix.AT_SYMLINK_NOFOLLOW, int(wantStxMask), &stx) +func checkSymlinkOvermount(procRoot *os.File, dir *os.File, path string) error { + // Get the mntId of our procfs handle. + expectedMountId, err := getMountId(procRoot, "") if err != nil { - return &os.PathError{Op: "statx", Path: dir.Name() + "/" + path, Err: err} + return err } - if stx.Mask&wantStxMask == 0 { - // It's not a kernel limitation, for some reason we couldn't get a - // mount ID. Assume it's some kind of attack. - return fmt.Errorf("%w: could not get mnt id of symlink %s", errUnsafeProcfs, path) + // Get the mntId of the target magic-link. + gotMountId, err := getMountId(dir, path) + if err != nil { + return err } - gotMountId := stx.Mnt_id - // As long as the directory mount is alive, even with wrapping mount IDs, // we would expect to see a different mount ID here. (Of course, if we're // using unsafeHostProcRoot() then an attaker could change this after we @@ -381,37 +378,33 @@ func checkSymlinkOvermount(dir *os.File, path string) error { return nil } -func doProcSelfMagiclink[T any](procRoot *os.File, subPath string, fn func(procDirHandle *os.File, base string) (T, error)) (T, error) { - // We cannot operate on the magic-link directly with a handle, we need to - // create a handle to the parent of the magic-link and then do - // single-component operations on it. - dir, base := filepath.Dir(subPath), filepath.Base(subPath) - - procDirHandle, closer, err := procThreadSelf(procRoot, dir) +func doRawProcSelfFdReadlink(procRoot *os.File, fd int) (string, error) { + fdPath := fmt.Sprintf("fd/%d", fd) + procFdLink, closer, err := procThreadSelf(procRoot, fdPath) if err != nil { - return *new(T), fmt.Errorf("get safe /proc/thread-self/%s handle: %w", dir, err) + return "", fmt.Errorf("get safe /proc/thread-self/%s handle: %w", fdPath, err) } - defer procDirHandle.Close() + defer procFdLink.Close() defer closer() - // Try to detect if there is a mount on top of the symlink we are about to - // read. If we are using unsafeHostProcRoot(), this could change after we - // check it (and there's nothing we can do about that) but for - // privateProcRoot() this should be guaranteed to be safe (at least since - // Linux 5.12[1], when anonymous mount namespaces were completely isolated - // from external mounts including mount propagation events). + // Try to detect if there is a mount on top of the magic-link. Since we use the handle directly + // provide to the closure. If the closure uses the handle directly, this + // should be safe in general (a mount on top of the path afterwards would + // not affect the handle itself) and will definitely be safe if we are + // using privateProcRoot() (at least since Linux 5.12[1], when anonymous + // mount namespaces were completely isolated from external mounts including + // mount propagation events). // // [1]: Linux commit ee2e3f50629f ("mount: fix mounting of detached mounts // onto targets that reside on shared mounts"). - if err := checkSymlinkOvermount(procDirHandle, base); err != nil { - return *new(T), fmt.Errorf("check safety of %s proc magiclink: %w", subPath, err) + if err := checkSymlinkOvermount(procRoot, procFdLink, ""); err != nil { + return "", fmt.Errorf("check safety of /proc/thread-self/fd/%d magiclink: %w", fd, err) } - return fn(procDirHandle, base) -} -func doRawProcSelfFdReadlink(procRoot *os.File, fd int) (string, error) { - fdPath := fmt.Sprintf("fd/%d", fd) - return doProcSelfMagiclink(procRoot, fdPath, readlinkatFile) + // readlinkat implies AT_EMPTY_PATH since Linux 2.6.39. See Linux commit + // 65cfc6722361 ("readlinkat(), fchownat() and fstatat() with empty + // relative pathnames"). + return readlinkatFile(procFdLink, "") } func rawProcSelfFdReadlink(fd int) (string, error) { diff --git a/vendor/github.com/cyphar/filepath-securejoin/testing_mocks_linux.go b/vendor/github.com/cyphar/filepath-securejoin/testing_mocks_linux.go index 2a25d08e37..a3aedf03d1 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/testing_mocks_linux.go +++ b/vendor/github.com/cyphar/filepath-securejoin/testing_mocks_linux.go @@ -42,9 +42,9 @@ func testingForcePrivateProcRootOpenTreeAtRecursive(f *os.File) bool { testingCheckClose(*testingForceGetProcRoot >= forceGetProcRootOpenTreeAtRecursive, f) } -func testingForceGetProcRootUnsafe(f *os.File) bool { +func testingForceGetProcRootUnsafe() bool { return testing.Testing() && testingForceGetProcRoot != nil && - testingCheckClose(*testingForceGetProcRoot >= forceGetProcRootUnsafe, f) + *testingForceGetProcRoot >= forceGetProcRootUnsafe } type forceProcThreadSelfLevel int diff --git a/vendor/modules.txt b/vendor/modules.txt index 9f9096629b..7379792a4f 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -48,7 +48,7 @@ github.com/containerd/errdefs ## explicit; go 1.19 github.com/containerd/stargz-snapshotter/estargz github.com/containerd/stargz-snapshotter/estargz/errorutil -# github.com/cyphar/filepath-securejoin v0.3.0 +# github.com/cyphar/filepath-securejoin v0.3.1 ## explicit; go 1.20 github.com/cyphar/filepath-securejoin # github.com/davecgh/go-spew v1.1.1