diff --git a/lookup_linux.go b/lookup_linux.go index da8efad..16acb03 100644 --- a/lookup_linux.go +++ b/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() diff --git a/open_linux.go b/open_linux.go index 39ea1ba..52dce76 100644 --- a/open_linux.go +++ b/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 } diff --git a/procfs_linux.go b/procfs_linux.go index 3c08285..ef0f44a 100644 --- a/procfs_linux.go +++ b/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) { diff --git a/procfs_linux_test.go b/procfs_linux_test.go index 947f387..87b5d4a 100644 --- a/procfs_linux_test.go +++ b/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