procfs: use readlink(fd, "") for magic-links

By operating on the magic-link directly, we (in theory) should be safe
against a racing mount even when using unsafeHostProcRoot(). There's not
much we can do about Reopen, but at least the core lookup logic should
be safe against race attacks.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

lookup_linux.go

@@ -283,7 +283,9 @@ func lookupInRoot(root *os.File, unsafePath string, partial bool) (Handle *os.Fi
 
 			switch st.Mode() & os.ModeType {
 			case os.ModeSymlink:
-				// readlinkat implies AT_EMPTY_PATH.
+				// readlinkat implies AT_EMPTY_PATH since Linux 2.6.38. See
+				// Linux commit 65cfc6722361 ("readlinkat(), fchownat() and
+				// fstatat() with empty relative pathnames").
 				linkDest, err := readlinkatFile(nextDir, "")
 				// We don't need the handle anymore.
 				_ = nextDir.Close()

open_linux.go

@@ -9,6 +9,7 @@ package securejoin
 import (
 	"fmt"
 	"os"
+	"strconv"
 
 	"golang.org/x/sys/unix"
 )
@@ -65,15 +66,36 @@ func Reopen(handle *os.File, flags int) (*os.File, error) {
 		return nil, err
 	}
 
+	// We can't operate on /proc/thread-self/fd/$n directly when doing a
+	// re-open, so we need to open /proc/thread-self/fd and then open a single
+	// final component.
+	procFdDir, closer, err := procThreadSelf(procRoot, "fd/")
+	if err != nil {
+		return nil, fmt.Errorf("get safe /proc/thread-self/fd handle: %w", err)
+	}
+	defer procFdDir.Close()
+	defer closer()
+
+	// Try to detect if there is a mount on top of the magic-link we are about
+	// to open. If we are using unsafeHostProcRoot(), this could change after
+	// we check it (and there's nothing we can do about that) but for
+	// privateProcRoot() this should be guaranteed to be safe (at least since
+	// Linux 5.12[1], when anonymous mount namespaces were completely isolated
+	// from external mounts including mount propagation events).
+	//
+	// [1]: Linux commit ee2e3f50629f ("mount: fix mounting of detached mounts
+	// onto targets that reside on shared mounts").
+	fdStr := strconv.Itoa(int(handle.Fd()))
+	if err := checkSymlinkOvermount(procRoot, procFdDir, fdStr); err != nil {
+		return nil, fmt.Errorf("check safety of /proc/thread-self/fd/%s magiclink: %w", fdStr, err)
+	}
+
 	flags |= unix.O_CLOEXEC
-	fdPath := fmt.Sprintf("fd/%d", handle.Fd())
-	return doProcSelfMagiclink(procRoot, fdPath, func(procDirHandle *os.File, base string) (*os.File, error) {
-		// Rather than just wrapping openatFile, open-code it so we can copy
-		// handle.Name().
-		reopenFd, err := unix.Openat(int(procDirHandle.Fd()), base, flags, 0)
-		if err != nil {
-			return nil, fmt.Errorf("reopen fd %d: %w", handle.Fd(), err)
-		}
-		return os.NewFile(uintptr(reopenFd), handle.Name()), nil
-	})
+	// Rather than just wrapping openatFile, open-code it so we can copy
+	// handle.Name().
+	reopenFd, err := unix.Openat(int(procFdDir.Fd()), fdStr, flags, 0)
+	if err != nil {
+		return nil, fmt.Errorf("reopen fd %d: %w", handle.Fd(), err)
+	}
+	return os.NewFile(uintptr(reopenFd), handle.Name()), nil
 }

procfs_linux.go

@@ -10,7 +10,6 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"path/filepath"
 	"runtime"
 	"strconv"
 	"sync"
@@ -286,14 +285,14 @@ func procThreadSelf(procRoot *os.File, subpath string) (_ *os.File, _ procThread
 		//       procSelfFdReadlink to clean up the returned f.Name() if we use
 		//       RESOLVE_IN_ROOT (which would lead to an infinite recursion).
 		handle, err = openat2File(procRoot, threadSelf+subpath, &unix.OpenHow{
-			Flags:   unix.O_PATH | unix.O_CLOEXEC,
+			Flags:   unix.O_PATH | unix.O_NOFOLLOW | unix.O_CLOEXEC,
 			Resolve: unix.RESOLVE_BENEATH | unix.RESOLVE_NO_XDEV | unix.RESOLVE_NO_MAGICLINKS,
 		})
 		if err != nil {
 			return nil, nil, fmt.Errorf("%w: %w", errUnsafeProcfs, err)
 		}
 	} else {
-		handle, err = openatFile(procRoot, threadSelf+subpath, unix.O_PATH|unix.O_CLOEXEC, 0)
+		handle, err = openatFile(procRoot, threadSelf+subpath, unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
 		if err != nil {
 			return nil, nil, fmt.Errorf("%w: %w", errUnsafeProcfs, err)
 		}
@@ -333,7 +332,7 @@ func hasStatxMountId() bool {
 	return hasStatxMountIdBool
 }
 
-func checkSymlinkOvermount(dir *os.File, path string) error {
+func checkSymlinkOvermount(procRoot *os.File, dir *os.File, path string) error {
 	// If we don't have statx(STATX_MNT_ID*) support, we can't do anything.
 	if !hasStatxMountId() {
 		return nil
@@ -347,7 +346,7 @@ func checkSymlinkOvermount(dir *os.File, path string) error {
 	)
 
 	// Get the mntId of our procfs handle.
-	err := unix.Statx(int(dir.Fd()), "", unix.AT_EMPTY_PATH, int(wantStxMask), &stx)
+	err := unix.Statx(int(procRoot.Fd()), "", unix.AT_EMPTY_PATH, int(wantStxMask), &stx)
 	if err != nil {
 		return &os.PathError{Op: "statx", Path: dir.Name(), Err: err}
 	}
@@ -360,7 +359,7 @@ func checkSymlinkOvermount(dir *os.File, path string) error {
 
 	// Get the mntId of the target symlink.
 	stx = unix.Statx_t{}
-	err = unix.Statx(int(dir.Fd()), path, unix.AT_SYMLINK_NOFOLLOW, int(wantStxMask), &stx)
+	err = unix.Statx(int(dir.Fd()), path, unix.AT_SYMLINK_NOFOLLOW|unix.AT_EMPTY_PATH, int(wantStxMask), &stx)
 	if err != nil {
 		return &os.PathError{Op: "statx", Path: dir.Name() + "/" + path, Err: err}
 	}
@@ -381,37 +380,33 @@ func checkSymlinkOvermount(dir *os.File, path string) error {
 	return nil
 }
 
-func doProcSelfMagiclink[T any](procRoot *os.File, subPath string, fn func(procDirHandle *os.File, base string) (T, error)) (T, error) {
-	// We cannot operate on the magic-link directly with a handle, we need to
-	// create a handle to the parent of the magic-link and then do
-	// single-component operations on it.
-	dir, base := filepath.Dir(subPath), filepath.Base(subPath)
-
-	procDirHandle, closer, err := procThreadSelf(procRoot, dir)
+func doRawProcSelfFdReadlink(procRoot *os.File, fd int) (string, error) {
+	fdPath := fmt.Sprintf("fd/%d", fd)
+	procFdLink, closer, err := procThreadSelf(procRoot, fdPath)
 	if err != nil {
-		return *new(T), fmt.Errorf("get safe /proc/thread-self/%s handle: %w", dir, err)
+		return "", fmt.Errorf("get safe /proc/thread-self/%s handle: %w", fdPath, err)
 	}
-	defer procDirHandle.Close()
+	defer procFdLink.Close()
 	defer closer()
 
-	// Try to detect if there is a mount on top of the symlink we are about to
-	// read. If we are using unsafeHostProcRoot(), this could change after we
-	// check it (and there's nothing we can do about that) but for
-	// privateProcRoot() this should be guaranteed to be safe (at least since
-	// Linux 5.12[1], when anonymous mount namespaces were completely isolated
-	// from external mounts including mount propagation events).
+	// Try to detect if there is a mount on top of the magic-link. Since we use the handle directly
+	// provide to the closure. If the closure uses the handle directly, this
+	// should be safe in general (a mount on top of the path afterwards would
+	// not affect the handle itself) and will definitely be safe if we are
+	// using privateProcRoot() (at least since Linux 5.12[1], when anonymous
+	// mount namespaces were completely isolated from external mounts including
+	// mount propagation events).
 	//
 	// [1]: Linux commit ee2e3f50629f ("mount: fix mounting of detached mounts
 	// onto targets that reside on shared mounts").
-	if err := checkSymlinkOvermount(procDirHandle, base); err != nil {
-		return *new(T), fmt.Errorf("check safety of %s proc magiclink: %w", subPath, err)
+	if err := checkSymlinkOvermount(procRoot, procFdLink, ""); err != nil {
+		return "", fmt.Errorf("check safety of /proc/thread-self/fd/%d magiclink: %w", fd, err)
 	}
-	return fn(procDirHandle, base)
-}
 
-func doRawProcSelfFdReadlink(procRoot *os.File, fd int) (string, error) {
-	fdPath := fmt.Sprintf("fd/%d", fd)
-	return doProcSelfMagiclink(procRoot, fdPath, readlinkatFile)
+	// readlinkat implies AT_EMPTY_PATH since Linux 2.6.38. See Linux commit
+	// 65cfc6722361 ("readlinkat(), fchownat() and fstatat() with empty
+	// relative pathnames").
+	return readlinkatFile(procFdLink, "")
 }
 
 func rawProcSelfFdReadlink(fd int) (string, error) {

procfs_linux_test.go

@@ -136,17 +136,30 @@ func testProcOvermountSubdir(t *testing.T, procRootFn procRootFunc, expectOvermo
 			symlinkOvermountErr = nil
 		}
 
-		// Check that /proc/self/exe and .
-		procThreadSelf, closer, err := procThreadSelf(procRoot, ".")
+		procSelf, closer, err := procThreadSelf(procRoot, ".")
 		require.NoError(t, err)
-		defer procThreadSelf.Close()
+		defer procSelf.Close()
 		defer closer()
 
+		// Open these paths directly to emulate a non-openat2 handle that
+		// didn't detect a bind-mount to check that checkSymlinkOvermount works
+		// properly for AT_EMPTY_PATH checks as well.
+		procCwd, err := openatFile(procSelf, "cwd", unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
+		require.NoError(t, err)
+		defer procCwd.Close()
+		procExe, err := openatFile(procSelf, "exe", unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
+		require.NoError(t, err)
+		defer procExe.Close()
+
 		// no overmount
-		err = checkSymlinkOvermount(procThreadSelf, "cwd")
+		err = checkSymlinkOvermount(procRoot, procCwd, "")
+		assert.NoError(t, err, "checking /proc/self/cwd with no overmount should succeed")
+		err = checkSymlinkOvermount(procRoot, procSelf, "cwd")
 		assert.NoError(t, err, "checking /proc/self/cwd with no overmount should succeed")
 		// basic overmount
-		err = checkSymlinkOvermount(procThreadSelf, "exe")
+		err = checkSymlinkOvermount(procRoot, procExe, "")
+		assert.ErrorIs(t, err, symlinkOvermountErr, "unexpected /proc/self/exe overmount result")
+		err = checkSymlinkOvermount(procRoot, procSelf, "exe")
 		assert.ErrorIs(t, err, symlinkOvermountErr, "unexpected /proc/self/exe overmount result")
 
 		// fd no overmount