From dd827f7b715ac1d48a336d101fce00dca6e20f9b Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Sat, 3 Aug 2024 16:27:42 +1000 Subject: [PATCH] utils: switch to securejoin.MkdirAllHandle filepath-securejoin has a bunch of extra hardening features and is very well-tested, so we should use it instead of our own homebrew solution. A lot of rootfs_linux.go callers pass a SecureJoin'd path, which means we need to keep the wrapper helpers in utils, but at least the core logic is no longer in runc. In future we will want to remove this dodgy logic and just use file handles for everything (using libpathrs, ideally). Signed-off-by: Aleksa Sarai --- libcontainer/system/linux.go | 41 ---------------- libcontainer/utils/utils_unix.go | 81 +++++++------------------------- 2 files changed, 16 insertions(+), 106 deletions(-) diff --git a/libcontainer/system/linux.go b/libcontainer/system/linux.go index 27e89d635ca..7bbf92a3d30 100644 --- a/libcontainer/system/linux.go +++ b/libcontainer/system/linux.go @@ -6,9 +6,7 @@ import ( "fmt" "io" "os" - "runtime" "strconv" - "strings" "syscall" "unsafe" @@ -216,42 +214,3 @@ func SetLinuxPersonality(personality int) error { } return nil } - -func prepareAt(dir *os.File, path string) (int, string) { - if dir == nil { - return unix.AT_FDCWD, path - } - - // Rather than just filepath.Join-ing path here, do it manually so the - // error and handle correctly indicate cases like path=".." as being - // relative to the correct directory. The handle.Name() might end up being - // wrong but because this is (currently) only used in MkdirAllInRoot, that - // isn't a problem. - dirName := dir.Name() - if !strings.HasSuffix(dirName, "/") { - dirName += "/" - } - fullPath := dirName + path - - return int(dir.Fd()), fullPath -} - -func Openat(dir *os.File, path string, flags int, mode uint32) (*os.File, error) { - dirFd, fullPath := prepareAt(dir, path) - fd, err := unix.Openat(dirFd, path, flags, mode) - if err != nil { - return nil, &os.PathError{Op: "openat", Path: fullPath, Err: err} - } - runtime.KeepAlive(dir) - return os.NewFile(uintptr(fd), fullPath), nil -} - -func Mkdirat(dir *os.File, path string, mode uint32) error { - dirFd, fullPath := prepareAt(dir, path) - err := unix.Mkdirat(dirFd, path, mode) - if err != nil { - err = &os.PathError{Op: "mkdirat", Path: fullPath, Err: err} - } - runtime.KeepAlive(dir) - return err -} diff --git a/libcontainer/utils/utils_unix.go b/libcontainer/utils/utils_unix.go index 1f3439b78fb..6521114ba75 100644 --- a/libcontainer/utils/utils_unix.go +++ b/libcontainer/utils/utils_unix.go @@ -3,7 +3,6 @@ package utils import ( - "errors" "fmt" "math" "os" @@ -14,8 +13,6 @@ import ( "sync" _ "unsafe" // for go:linkname - "github.com/opencontainers/runc/libcontainer/system" - securejoin "github.com/cyphar/filepath-securejoin" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" @@ -299,23 +296,23 @@ func IsLexicallyInRoot(root, path string) bool { // This means that the path also must not contain ".." elements, otherwise an // error will occur. // -// This is a somewhat less safe alternative to -// , but it should -// detect attempts to trick us into creating directories outside of the root. -// We should migrate to securejoin.MkdirAll once it is merged. +// This uses securejoin.MkdirAllHandle under the hood, but it has special +// handling if unsafePath has already been scoped within the rootfs (this is +// needed for a lot of runc callers and fixing this would require reworking a +// lot of path logic). func MkdirAllInRootOpen(root, unsafePath string, mode uint32) (_ *os.File, Err error) { - // If the path is already "within" the root, use it verbatim. - fullPath := unsafePath - if !IsLexicallyInRoot(root, unsafePath) { - var err error - fullPath, err = securejoin.SecureJoin(root, unsafePath) + // If the path is already "within" the root, get the path relative to the + // root and use that as the unsafe path. This is necessary because a lot of + // MkdirAllInRootOpen callers have already done SecureJoin, and refactoring + // all of them to stop using these SecureJoin'd paths would require a fair + // amount of work. + // TODO(cyphar): Do the refactor to libpathrs once it's ready. + if IsLexicallyInRoot(root, unsafePath) { + subPath, err := filepath.Rel(root, unsafePath) if err != nil { return nil, err } - } - subPath, err := filepath.Rel(root, fullPath) - if err != nil { - return nil, err + unsafePath = subPath } // Check for any silly mode bits. @@ -323,59 +320,13 @@ func MkdirAllInRootOpen(root, unsafePath string, mode uint32) (_ *os.File, Err e return nil, fmt.Errorf("tried to include non-mode bits in MkdirAll mode: 0o%.3o", mode) } - currentDir, err := os.OpenFile(root, unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + rootDir, err := os.OpenFile(root, unix.O_DIRECTORY|unix.O_CLOEXEC, 0) if err != nil { return nil, fmt.Errorf("open root handle: %w", err) } - defer func() { - if Err != nil { - currentDir.Close() - } - }() - - for _, part := range strings.Split(subPath, string(filepath.Separator)) { - switch part { - case "", ".": - // Skip over no-op components. - continue - case "..": - return nil, fmt.Errorf("possible breakout detected: found %q component in SecureJoin subpath %s", part, subPath) - } + defer rootDir.Close() - nextDir, err := system.Openat(currentDir, part, unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) - switch { - case err == nil: - // Update the currentDir. - _ = currentDir.Close() - currentDir = nextDir - - case errors.Is(err, unix.ENOTDIR): - // This might be a symlink or some other random file. Either way, - // error out. - return nil, fmt.Errorf("cannot mkdir in %s/%s: %w", currentDir.Name(), part, unix.ENOTDIR) - - case errors.Is(err, os.ErrNotExist): - // Luckily, mkdirat will not follow trailing symlinks, so this is - // safe to do as-is. - if err := system.Mkdirat(currentDir, part, mode); err != nil { - return nil, err - } - // Open the new directory. There is a race here where an attacker - // could swap the directory with a different directory, but - // MkdirAll's fuzzy semantics mean we don't care about that. - nextDir, err := system.Openat(currentDir, part, unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) - if err != nil { - return nil, fmt.Errorf("open newly created directory: %w", err) - } - // Update the currentDir. - _ = currentDir.Close() - currentDir = nextDir - - default: - return nil, err - } - } - return currentDir, nil + return securejoin.MkdirAllHandle(rootDir, unsafePath, int(mode)) } // MkdirAllInRoot is a wrapper around MkdirAllInRootOpen which closes the