Merge pull request #4393 from cyphar/mkdirall-inroot-securejoin

utils: switch to securejoin.MkdirAllHandle

go.mod

@@ -7,7 +7,7 @@ require (
 	github.com/cilium/ebpf v0.12.3
 	github.com/containerd/console v1.0.4
 	github.com/coreos/go-systemd/v22 v22.5.0
-	github.com/cyphar/filepath-securejoin v0.2.4
+	github.com/cyphar/filepath-securejoin v0.3.1
 	github.com/docker/go-units v0.5.0
 	github.com/godbus/dbus/v5 v5.1.0
 	github.com/moby/sys/mountinfo v0.7.1
@@ -21,7 +21,7 @@ require (
 	github.com/urfave/cli v1.22.14
 	github.com/vishvananda/netlink v1.1.0
 	golang.org/x/net v0.24.0
-	golang.org/x/sys v0.19.0
+	golang.org/x/sys v0.22.0
 	google.golang.org/protobuf v1.33.0
 )
 

go.sum

@@ -9,8 +9,8 @@ github.com/coreos/go-systemd/v22 v22.5.0 h1:RrqgGjYQKalulkV8NGVIfkXQf6YYmOyiJKk8
 github.com/coreos/go-systemd/v22 v22.5.0/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSVTIJ3seZv2GcEnc=
 github.com/cpuguy83/go-md2man/v2 v2.0.2 h1:p1EgwI/C7NhT0JmVkwCD2ZBK8j4aeHQX2pMHHBfMQ6w=
 github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o=
-github.com/cyphar/filepath-securejoin v0.2.4 h1:Ugdm7cg7i6ZK6x3xDF1oEu1nfkyfH53EtKeQYTC3kyg=
-github.com/cyphar/filepath-securejoin v0.2.4/go.mod h1:aPGpWjXOXUn2NCNjFvBE6aRxGGx79pTxQpKOJNYHHl4=
+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=
@@ -58,8 +58,9 @@ github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpE
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
 github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
-github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk=
 github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo=
+github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg=
+github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
 github.com/syndtr/gocapability v0.0.0-20200815063812-42c35b437635 h1:kdXcSzyDtseVEc4yCz2qF8ZrQvIDBJLl4S1c3GCXmoI=
 github.com/syndtr/gocapability v0.0.0-20200815063812-42c35b437635/go.mod h1:hkRG7XYTFWNJGYcbNJQlaLq0fg1yr4J4t/NcTQtrfww=
 github.com/urfave/cli v1.22.14 h1:ebbhrRiGK2i4naQJr+1Xj92HXZCrK7MsyTS/ob3HnAk=
@@ -77,8 +78,8 @@ golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c/go.mod h1:h1NjWce9XRLGQEsW7w
 golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.19.0 h1:q5f1RH2jigJ1MoAWp2KTp3gm5zAGFUTarQZ5U386+4o=
-golang.org/x/sys v0.19.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
+golang.org/x/sys v0.22.0 h1:RI27ohtqKCnwULzJLqkv897zojh5/DwS/ENaMzUOaWI=
+golang.org/x/sys v0.22.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
 google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw=
 google.golang.org/protobuf v1.28.1/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I=

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,83 +296,37 @@ 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
-// <https://github.com/cyphar/filepath-securejoin/pull/13>, 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.
 	if mode&^0o7777 != 0 {
 		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