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 <cyphar@cyphar.com>

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
-}

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
+// This uses securejoin.MkdirAllHandle under the hood, but it has special
-// <https://github.com/cyphar/filepath-securejoin/pull/13>, but it should
+// handling if unsafePath has already been scoped within the rootfs (this is
-// detect attempts to trick us into creating directories outside of the root.
+// needed for a lot of runc callers and fixing this would require reworking a
-// We should migrate to securejoin.MkdirAll once it is merged.
+// 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.
+	// If the path is already "within" the root, get the path relative to the
-	fullPath := unsafePath
+	// root and use that as the unsafe path. This is necessary because a lot of
-	if !IsLexicallyInRoot(root, unsafePath) {
+	// MkdirAllInRootOpen callers have already done SecureJoin, and refactoring
-		var err error
+	// all of them to stop using these SecureJoin'd paths would require a fair
-		fullPath, err = securejoin.SecureJoin(root, unsafePath)
+	// 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
 		}
-	}
+		unsafePath = subPath
-	subPath, err := filepath.Rel(root, fullPath)
-	if err != nil {
-		return nil, err
 	}
 
 	// 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() {
+	defer rootDir.Close()
-		if Err != nil {
-			currentDir.Close()
-		}
-	}()
 
-	for _, part := range strings.Split(subPath, string(filepath.Separator)) {
+	return securejoin.MkdirAllHandle(rootDir, unsafePath, int(mode))
-		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)
-		}
-
-		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
 }
 
 // MkdirAllInRoot is a wrapper around MkdirAllInRootOpen which closes the