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