Skip to content

Commit

Permalink
utils: switch to securejoin.MkdirAllHandle
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
cyphar committed Sep 3, 2024
1 parent 1d308c7 commit dd827f7
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 106 deletions.
41 changes: 0 additions & 41 deletions libcontainer/system/linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ import (
"fmt"
"io"
"os"
"runtime"
"strconv"
"strings"
"syscall"
"unsafe"

Expand Down Expand Up @@ -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
}
81 changes: 16 additions & 65 deletions libcontainer/utils/utils_unix.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
package utils

import (
"errors"
"fmt"
"math"
"os"
Expand All @@ -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"
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit dd827f7

Please sign in to comment.