From 96f72c6c429942d9dceffc4a508be5a554b0fec3 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Wed, 10 Jul 2024 16:52:39 +1000 Subject: [PATCH 1/5] procfs: make procSelfFdReadlink more generic with generics In order to add a reopen primitive, it'll be necessary to do very similar checks to procSelfFdReadlink but with slightly different types. By refactoring procSelfFdReadlink with generics we can use the same codepath for both operations. Signed-off-by: Aleksa Sarai --- procfs_linux.go | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/procfs_linux.go b/procfs_linux.go index ba6a6dd..daac3f0 100644 --- a/procfs_linux.go +++ b/procfs_linux.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "os" + "path/filepath" "runtime" "strconv" "sync" @@ -380,12 +381,17 @@ func checkSymlinkOvermount(dir *os.File, path string) error { return nil } -func doRawProcSelfFdReadlink(procRoot *os.File, fd int) (string, error) { - procSelfFd, closer, err := procThreadSelf(procRoot, "fd/") +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) if err != nil { - return "", fmt.Errorf("get safe /proc/thread-self/fd handle: %w", err) + return *new(T), fmt.Errorf("get safe /proc/thread-self/%s handle: %w", dir, err) } - defer procSelfFd.Close() + defer procDirHandle.Close() defer closer() // Try to detect if there is a mount on top of the symlink we are about to @@ -397,12 +403,15 @@ func doRawProcSelfFdReadlink(procRoot *os.File, fd int) (string, error) { // // [1]: Linux commit ee2e3f50629f ("mount: fix mounting of detached mounts // onto targets that reside on shared mounts"). - fdStr := strconv.Itoa(fd) - if err := checkSymlinkOvermount(procSelfFd, fdStr); err != nil { - return "", fmt.Errorf("check safety of fd %d proc magiclink: %w", fd, err) + if err := checkSymlinkOvermount(procDirHandle, base); err != nil { + return *new(T), fmt.Errorf("check safety of %s proc magiclink: %w", subPath, err) } - // Get the contents of the link. - return readlinkatFile(procSelfFd, fdStr) + return fn(procDirHandle, base) +} + +func doRawProcSelfFdReadlink(procRoot *os.File, fd int) (string, error) { + fdPath := fmt.Sprintf("fd/%d", fd) + return doProcSelfMagiclink(procRoot, fdPath, readlinkatFile) } func rawProcSelfFdReadlink(fd int) (string, error) { From 1e6990b9ef813d8d80a050c6826dae0fce495be4 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Wed, 10 Jul 2024 16:54:03 +1000 Subject: [PATCH 2/5] open: add Open(at)InRoot and Reopen These are very thin wrappers around the existing partialLookupInRoot and doProcSelfMagiclink infrastructure to allow user to get a much safer API for operating on paths than SecureJoin. In principle, with very careful usage, these operations should be sufficient to make any program safe against races. In theory we could disable the symlink stack when doing OpenInRoot (because the symlink stack is only relevant for partial lookups), but for now we'll just keep the codepaths the same. Also, add a note to SecureJoin to tell users that you should probably use OpenInRoot if you can. This API is based on the libpathrs base API. Signed-off-by: Aleksa Sarai --- join.go | 6 ++++ open_linux.go | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 open_linux.go diff --git a/join.go b/join.go index 324ab1b..bd86a48 100644 --- a/join.go +++ b/join.go @@ -41,6 +41,12 @@ func IsNotExist(err error) bool { // replaced with symlinks on the filesystem) after this function has returned. // Such a symlink race is necessarily out-of-scope of SecureJoin. // +// NOTE: Due to the above limitation, Linux users are strongly encouraged to +// use OpenInRoot instead, which does safely protect against these kinds of +// attacks. There is no way to solve this problem with SecureJoinVFS because +// the API is fundamentally wrong (you cannot return a "safe" path string and +// guarantee it won't be modified afterwards). +// // Volume names in unsafePath are always discarded, regardless if they are // provided via direct input or when evaluating symlinks. Therefore: // diff --git a/open_linux.go b/open_linux.go new file mode 100644 index 0000000..2170061 --- /dev/null +++ b/open_linux.go @@ -0,0 +1,83 @@ +//go:build linux + +// Copyright (C) 2024 SUSE LLC. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package securejoin + +import ( + "fmt" + "os" + + "golang.org/x/sys/unix" +) + +// OpenatInRoot is equivalent to OpenInRoot, except that the root is provided +// using an *os.File handle, to ensure that the correct root directory is used. +func OpenatInRoot(root *os.File, unsafePath string) (*os.File, error) { + handle, remainingPath, err := partialLookupInRoot(root, unsafePath) + if err != nil { + return nil, err + } + if remainingPath != "" { + _ = handle.Close() + return nil, &os.PathError{Op: "securejoin.OpenInRoot", Path: unsafePath, Err: unix.ENOENT} + } + return handle, nil +} + +// OpenInRoot safely opens the provided unsafePath within the root. +// Effectively, OpenInRoot(root, unsafePath) is equivalent to +// +// path, _ := securejoin.SecureJoin(root, unsafePath) +// handle, err := os.OpenFile(path, unix.O_PATH|unix.O_CLOEXEC) +// +// But is much safer. The above implementation is unsafe because if an attacker +// can modify the filesystem tree between SecureJoin and OpenFile, it is +// possible for the returned file to be outside of the root. +// +// Note that the returned handle is an O_PATH handle, meaning that only a very +// limited set of operations will work on the handle. This is done to avoid +// accidentally opening an untrusted file that could cause issues (such as a +// disconnected TTY that could cause a DoS, or some other issue). In order to +// use the returned handle, you can "upgrade" it to a proper handle using +// Reopen. +func OpenInRoot(root, unsafePath string) (*os.File, error) { + rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return nil, err + } + defer rootDir.Close() + return OpenatInRoot(rootDir, unsafePath) +} + +// Reopen takes an *os.File handle and re-opens it through /proc/self/fd. +// Reopen(file, flags) is effectively equivalent to +// +// fdPath := fmt.Sprintf("/proc/self/fd/%d", file.Fd()) +// os.OpenFile(fdPath, flags|unix.O_CLOEXEC) +// +// But with some extra hardenings to ensure that we are not tricked by a +// maliciously-configured /proc mount. While this attack scenario is not +// common, in container runtimes it is possible for higher-level runtimes to be +// tricked into configuring an unsafe /proc that can be used to attack file +// operations. See CVE-2019-19921 for more details. +func Reopen(handle *os.File, flags int) (*os.File, error) { + procRoot, err := getProcRoot() + if err != nil { + return nil, 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 + }) +} From 975d7b33c8803451621a3d67c35f220ddbdbf1e8 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Wed, 10 Jul 2024 16:57:05 +1000 Subject: [PATCH 3/5] open: add OpenInRoot and Reopen tests These are mostly based on the partialOpen tests, but making sure we don't return any partial resolutions. Signed-off-by: Aleksa Sarai --- open_linux_test.go | 384 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 384 insertions(+) create mode 100644 open_linux_test.go diff --git a/open_linux_test.go b/open_linux_test.go new file mode 100644 index 0000000..eb794c0 --- /dev/null +++ b/open_linux_test.go @@ -0,0 +1,384 @@ +//go:build linux + +// Copyright (C) 2024 SUSE LLC. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package securejoin + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" +) + +type openInRootFunc func(root, unsafePath string) (*os.File, error) + +type openResult struct { + handlePath string + err error + fileType uint32 +} + +// O_LARGEFILE is automatically added by the kernel when opening files on +// 64-bit machines. Unfortunately, it is architecture-dependent and +// unix.O_LARGEFILE is 0 (presumably to avoid users setting it). So we need to +// initialise it at init. +var O_LARGEFILE = 0x8000 + +func init() { + switch runtime.GOARCH { + case "arm", "arm64": + O_LARGEFILE = 0x20000 + case "mips", "mips64", "mips64le", "mips64p32", "mips64p32le": + O_LARGEFILE = 0x2000 + case "ppc", "ppc64", "ppc64le": + O_LARGEFILE = 0x10000 + case "sparc", "sparc64": + O_LARGEFILE = 0x40000 + default: + // 0x8000 is the default flag in asm-generic. + } +} + +func checkReopen(t *testing.T, handle *os.File, flags int, expectedErr error) { + newHandle, err := Reopen(handle, flags) + if newHandle != nil { + defer newHandle.Close() + } + if expectedErr != nil { + if assert.Error(t, err) { + assert.ErrorIs(t, err, expectedErr) + } else { + t.Errorf("unexpected handle %q", handle.Name()) + } + return + } + assert.NoError(t, err) + + // Get the original handle path. + handlePath, err := procSelfFdReadlink(handle) + require.NoError(t, err, "get real path of original handle") + // Make sure the handle matches the readlink path. + assert.Equal(t, handlePath, handle.Name(), "handle.Name() matching real original handle path") + + // Check that the new and old handle have the same path. + newHandlePath, err := procSelfFdReadlink(newHandle) + require.NoError(t, err, "get real path of reopened handle") + assert.Equal(t, handlePath, newHandlePath, "old and reopen handle paths") + assert.Equal(t, handle.Name(), newHandle.Name(), "old and reopen handle.Name()") + + // Check the fd flags. + newHandleFdFlags, err := unix.FcntlInt(newHandle.Fd(), unix.F_GETFD, 0) + require.NoError(t, err, "fcntl(F_GETFD)") + assert.Equal(t, unix.FD_CLOEXEC, newHandleFdFlags&unix.FD_CLOEXEC, "FD_CLOEXEC flag must be set") + + // Check the file handle flags. + newHandleStatusFlags, err := unix.FcntlInt(newHandle.Fd(), unix.F_GETFL, 0) + require.NoError(t, err, "fcntl(F_GETFL)") + flags &^= unix.O_CLOEXEC // O_CLOEXEC is checked by F_GETFD + newHandleStatusFlags &^= O_LARGEFILE // ignore the O_LARGEFILE flag + assert.Equal(t, flags, newHandleStatusFlags, "re-opened handle status flags must match re-open flags (%+x)") +} + +func checkOpenInRoot(t *testing.T, openInRootFn openInRootFunc, root, unsafePath string, expected openResult) { + handle, err := openInRootFn(root, unsafePath) + if handle != nil { + defer handle.Close() + } + if expected.err != nil { + if assert.Error(t, err) { + assert.ErrorIs(t, err, expected.err) + } else { + t.Errorf("unexpected handle %q", handle.Name()) + } + return + } + assert.NoError(t, err) + + // Check the handle path. + gotPath, err := procSelfFdReadlink(handle) + require.NoError(t, err, "get real path of returned handle") + assert.Equal(t, expected.handlePath, gotPath, "real handle path") + // Make sure the handle matches the readlink path. + assert.Equal(t, gotPath, handle.Name(), "handle.Name() matching real handle path") + + // Check the handle type. + unixStat, err := fstat(handle) + require.NoError(t, err, "fstat handle") + assert.Equal(t, expected.fileType, unixStat.Mode&unix.S_IFMT, "handle S_IFMT type") + + // Check that re-opening produces a handle with the same path. + switch expected.fileType { + case unix.S_IFDIR: + checkReopen(t, handle, unix.O_RDONLY, nil) + checkReopen(t, handle, unix.O_DIRECTORY, nil) + case unix.S_IFREG: + checkReopen(t, handle, unix.O_RDWR, nil) + checkReopen(t, handle, unix.O_DIRECTORY, unix.ENOTDIR) + // Only files and directories are safe to open this way. Use O_PATH for + // everything else. + default: + checkReopen(t, handle, unix.O_PATH, nil) + checkReopen(t, handle, unix.O_PATH|unix.O_DIRECTORY, unix.ENOTDIR) + } +} + +func testOpenInRoot(t *testing.T, openInRootFn openInRootFunc) { + tree := []string{ + "dir a", + "dir b/c/d/e/f", + "file b/c/file", + "symlink e /b/c/d/e", + "symlink b-file b/c/file", + // Dangling symlinks. + "symlink a-fake1 a/fake", + "symlink a-fake2 a/fake/foo/bar/..", + "symlink a-fake3 a/fake/../../b", + "dir c", + "symlink c/a-fake1 a/fake", + "symlink c/a-fake2 a/fake/foo/bar/..", + "symlink c/a-fake3 a/fake/../../b", + // Test non-lexical symlinks. + "dir target", + "dir link1", + "symlink link1/target_abs /target", + "symlink link1/target_rel ../target", + "dir link2", + "symlink link2/link1_abs /link1", + "symlink link2/link1_rel ../link1", + "dir link3", + "symlink link3/target_abs /link2/link1_rel/target_rel", + "symlink link3/target_rel ../link2/link1_rel/target_rel", + "symlink link3/deep_dangling1 ../link2/link1_rel/target_rel/nonexist", + "symlink link3/deep_dangling2 ../link2/link1_rel/target_rel/nonexist", + // Deep dangling symlinks (with single components). + "dir dangling", + "symlink dangling/a b/c", + "dir dangling/b", + "symlink dangling/b/c ../c", + "symlink dangling/c d/e", + "dir dangling/d", + "symlink dangling/d/e ../e", + "symlink dangling/e f/../g", + "dir dangling/f", + "symlink dangling/g h/i/j/nonexistent", + "dir dangling/h/i/j", + // Deep dangling symlink using a non-dir component. + "dir dangling-file", + "symlink dangling-file/a b/c", + "dir dangling-file/b", + "symlink dangling-file/b/c ../c", + "symlink dangling-file/c d/e", + "dir dangling-file/d", + "symlink dangling-file/d/e ../e", + "symlink dangling-file/e f/../g", + "dir dangling-file/f", + "symlink dangling-file/g h/i/j/file/foo", + "dir dangling-file/h/i/j", + "file dangling-file/h/i/j/file", + // Some "bad" inodes that a regular user can create. + "fifo b/fifo", + "sock b/sock", + // Symlink loops. + "dir loop", + "symlink loop/basic-loop1 basic-loop1", + "symlink loop/basic-loop2 /loop/basic-loop2", + "symlink loop/basic-loop3 ../loop/basic-loop3", + "dir loop/a", + "symlink loop/a/link ../b/link", + "dir loop/b", + "symlink loop/b/link /loop/c/link", + "dir loop/c", + "symlink loop/c/link /loop/d/link", + "symlink loop/d e", + "dir loop/e", + "symlink loop/e/link ../a/link", + "symlink loop/link a/link", + } + + root := createTree(t, tree...) + + for name, test := range map[string]struct { + unsafePath string + expected openResult + }{ + // Complete lookups. + "complete-dir1": {"a", openResult{handlePath: "/a", fileType: unix.S_IFDIR}}, + "complete-dir2": {"b/c/d/e/f", openResult{handlePath: "/b/c/d/e/f", fileType: unix.S_IFDIR}}, + "complete-fifo": {"b/fifo", openResult{handlePath: "/b/fifo", fileType: unix.S_IFIFO}}, + "complete-sock": {"b/sock", openResult{handlePath: "/b/sock", fileType: unix.S_IFSOCK}}, + // Partial lookups. + "partial-dir-basic": {"a/b/c/d/e/f/g/h", openResult{err: unix.ENOENT}}, + "partial-dir-dotdot": {"a/foo/../bar/baz", openResult{err: unix.ENOENT}}, + // Complete lookups of non-lexical symlinks. + "nonlexical-basic-complete": {"target", openResult{handlePath: "/target", fileType: unix.S_IFDIR}}, + "nonlexical-basic-partial": {"target/foo", openResult{err: unix.ENOENT}}, + "nonlexical-basic-partial-dotdot": {"target/../target/foo/bar/../baz", openResult{err: unix.ENOENT}}, + "nonlexical-level1-abs-complete": {"link1/target_abs", openResult{handlePath: "/target", fileType: unix.S_IFDIR}}, + "nonlexical-level1-abs-partial": {"link1/target_abs/foo", openResult{err: unix.ENOENT}}, + "nonlexical-level1-abs-partial-dotdot": {"link1/target_abs/../target/foo/bar/../baz", openResult{err: unix.ENOENT}}, + "nonlexical-level1-rel-complete": {"link1/target_rel", openResult{handlePath: "/target", fileType: unix.S_IFDIR}}, + "nonlexical-level1-rel-partial": {"link1/target_rel/foo", openResult{err: unix.ENOENT}}, + "nonlexical-level1-rel-partial-dotdot": {"link1/target_rel/../target/foo/bar/../baz", openResult{err: unix.ENOENT}}, + "nonlexical-level2-abs-abs-complete": {"link2/link1_abs/target_abs", openResult{handlePath: "/target", fileType: unix.S_IFDIR}}, + "nonlexical-level2-abs-abs-partial": {"link2/link1_abs/target_abs/foo", openResult{err: unix.ENOENT}}, + "nonlexical-level2-abs-abs-partial-dotdot": {"link2/link1_abs/target_abs/../target/foo/bar/../baz", openResult{err: unix.ENOENT}}, + "nonlexical-level2-abs-rel-complete": {"link2/link1_abs/target_rel", openResult{handlePath: "/target", fileType: unix.S_IFDIR}}, + "nonlexical-level2-abs-rel-partial": {"link2/link1_abs/target_rel/foo", openResult{err: unix.ENOENT}}, + "nonlexical-level2-abs-rel-partial-dotdot": {"link2/link1_abs/target_rel/../target/foo/bar/../baz", openResult{err: unix.ENOENT}}, + "nonlexical-level2-abs-open-complete": {"link2/link1_abs/../target", openResult{handlePath: "/target", fileType: unix.S_IFDIR}}, + "nonlexical-level2-abs-open-partial": {"link2/link1_abs/../target/foo", openResult{err: unix.ENOENT}}, + "nonlexical-level2-abs-open-partial-dotdot": {"link2/link1_abs/../target/../target/foo/bar/../baz", openResult{err: unix.ENOENT}}, + "nonlexical-level2-rel-abs-complete": {"link2/link1_rel/target_abs", openResult{handlePath: "/target", fileType: unix.S_IFDIR}}, + "nonlexical-level2-rel-abs-partial": {"link2/link1_rel/target_abs/foo", openResult{err: unix.ENOENT}}, + "nonlexical-level2-rel-abs-partial-dotdot": {"link2/link1_rel/target_abs/../target/foo/bar/../baz", openResult{err: unix.ENOENT}}, + "nonlexical-level2-rel-rel-complete": {"link2/link1_rel/target_rel", openResult{handlePath: "/target", fileType: unix.S_IFDIR}}, + "nonlexical-level2-rel-rel-partial": {"link2/link1_rel/target_rel/foo", openResult{err: unix.ENOENT}}, + "nonlexical-level2-rel-rel-partial-dotdot": {"link2/link1_rel/target_rel/../target/foo/bar/../baz", openResult{err: unix.ENOENT}}, + "nonlexical-level2-rel-open-complete": {"link2/link1_rel/../target", openResult{handlePath: "/target", fileType: unix.S_IFDIR}}, + "nonlexical-level2-rel-open-partial": {"link2/link1_rel/../target/foo", openResult{err: unix.ENOENT}}, + "nonlexical-level2-rel-open-partial-dotdot": {"link2/link1_rel/../target/../target/foo/bar/../baz", openResult{err: unix.ENOENT}}, + "nonlexical-level3-abs-complete": {"link3/target_abs", openResult{handlePath: "/target", fileType: unix.S_IFDIR}}, + "nonlexical-level3-abs-partial": {"link3/target_abs/foo", openResult{err: unix.ENOENT}}, + "nonlexical-level3-abs-partial-dotdot": {"link3/target_abs/../target/foo/bar/../baz", openResult{err: unix.ENOENT}}, + "nonlexical-level3-rel-complete": {"link3/target_rel", openResult{handlePath: "/target", fileType: unix.S_IFDIR}}, + "nonlexical-level3-rel-partial": {"link3/target_rel/foo", openResult{err: unix.ENOENT}}, + "nonlexical-level3-rel-partial-dotdot": {"link3/target_rel/../target/foo/bar/../baz", openResult{err: unix.ENOENT}}, + // Partial lookups due to hitting a non-directory. + "partial-nondir-dot": {"b/c/file/.", openResult{err: unix.ENOENT}}, + "partial-nondir-dotdot1": {"b/c/file/..", openResult{err: unix.ENOENT}}, + "partial-nondir-dotdot2": {"b/c/file/../foo/bar", openResult{err: unix.ENOENT}}, + "partial-nondir-symlink-dot": {"b-file/.", openResult{err: unix.ENOENT}}, + "partial-nondir-symlink-dotdot1": {"b-file/..", openResult{err: unix.ENOENT}}, + "partial-nondir-symlink-dotdot2": {"b-file/../foo/bar", openResult{err: unix.ENOENT}}, + "partial-fifo-dot": {"b/fifo/.", openResult{err: unix.ENOENT}}, + "partial-fifo-dotdot1": {"b/fifo/..", openResult{err: unix.ENOENT}}, + "partial-fifo-dotdot2": {"b/fifo/../foo/bar", openResult{err: unix.ENOENT}}, + "partial-sock-dot": {"b/sock/.", openResult{err: unix.ENOENT}}, + "partial-sock-dotdot1": {"b/sock/..", openResult{err: unix.ENOENT}}, + "partial-sock-dotdot2": {"b/sock/../foo/bar", openResult{err: unix.ENOENT}}, + // Dangling symlinks are treated as though they are non-existent. + "dangling1-inroot-trailing": {"a-fake1", openResult{err: unix.ENOENT}}, + "dangling1-inroot-partial": {"a-fake1/foo", openResult{err: unix.ENOENT}}, + "dangling1-inroot-partial-dotdot": {"a-fake1/../bar/baz", openResult{err: unix.ENOENT}}, + "dangling1-sub-trailing": {"c/a-fake1", openResult{err: unix.ENOENT}}, + "dangling1-sub-partial": {"c/a-fake1/foo", openResult{err: unix.ENOENT}}, + "dangling1-sub-partial-dotdot": {"c/a-fake1/../bar/baz", openResult{err: unix.ENOENT}}, + "dangling2-inroot-trailing": {"a-fake2", openResult{err: unix.ENOENT}}, + "dangling2-inroot-partial": {"a-fake2/foo", openResult{err: unix.ENOENT}}, + "dangling2-inroot-partial-dotdot": {"a-fake2/../bar/baz", openResult{err: unix.ENOENT}}, + "dangling2-sub-trailing": {"c/a-fake2", openResult{err: unix.ENOENT}}, + "dangling2-sub-partial": {"c/a-fake2/foo", openResult{err: unix.ENOENT}}, + "dangling2-sub-partial-dotdot": {"c/a-fake2/../bar/baz", openResult{err: unix.ENOENT}}, + "dangling3-inroot-trailing": {"a-fake3", openResult{err: unix.ENOENT}}, + "dangling3-inroot-partial": {"a-fake3/foo", openResult{err: unix.ENOENT}}, + "dangling3-inroot-partial-dotdot": {"a-fake3/../bar/baz", openResult{err: unix.ENOENT}}, + "dangling3-sub-trailing": {"c/a-fake3", openResult{err: unix.ENOENT}}, + "dangling3-sub-partial": {"c/a-fake3/foo", openResult{err: unix.ENOENT}}, + "dangling3-sub-partial-dotdot": {"c/a-fake3/../bar/baz", openResult{err: unix.ENOENT}}, + // Tricky dangling symlinks. + "dangling-tricky1-trailing": {"link3/deep_dangling1", openResult{err: unix.ENOENT}}, + "dangling-tricky1-partial": {"link3/deep_dangling1/foo", openResult{err: unix.ENOENT}}, + "dangling-tricky1-partial-dotdot": {"link3/deep_dangling1/..", openResult{err: unix.ENOENT}}, + "dangling-tricky2-trailing": {"link3/deep_dangling2", openResult{err: unix.ENOENT}}, + "dangling-tricky2-partial": {"link3/deep_dangling2/foo", openResult{err: unix.ENOENT}}, + "dangling-tricky2-partial-dotdot": {"link3/deep_dangling2/..", openResult{err: unix.ENOENT}}, + // Really deep dangling links. + "deep-dangling1": {"dangling/a", openResult{err: unix.ENOENT}}, + "deep-dangling2": {"dangling/b/c", openResult{err: unix.ENOENT}}, + "deep-dangling3": {"dangling/c", openResult{err: unix.ENOENT}}, + "deep-dangling4": {"dangling/d/e", openResult{err: unix.ENOENT}}, + "deep-dangling5": {"dangling/e", openResult{err: unix.ENOENT}}, + "deep-dangling6": {"dangling/g", openResult{err: unix.ENOENT}}, + "deep-dangling-fileasdir1": {"dangling-file/a", openResult{err: unix.ENOENT}}, + "deep-dangling-fileasdir2": {"dangling-file/b/c", openResult{err: unix.ENOENT}}, + "deep-dangling-fileasdir3": {"dangling-file/c", openResult{err: unix.ENOENT}}, + "deep-dangling-fileasdir4": {"dangling-file/d/e", openResult{err: unix.ENOENT}}, + "deep-dangling-fileasdir5": {"dangling-file/e", openResult{err: unix.ENOENT}}, + "deep-dangling-fileasdir6": {"dangling-file/g", openResult{err: unix.ENOENT}}, + // Symlink loops. + "loop": {"loop/link", openResult{err: unix.ELOOP}}, + "loop-basic1": {"loop/basic-loop1", openResult{err: unix.ELOOP}}, + "loop-basic2": {"loop/basic-loop2", openResult{err: unix.ELOOP}}, + "loop-basic3": {"loop/basic-loop3", openResult{err: unix.ELOOP}}, + } { + test := test // copy iterator + // Update the handlePath to be inside our root. + if test.expected.handlePath != "" { + test.expected.handlePath = filepath.Join(root, test.expected.handlePath) + } + t.Run(name, func(t *testing.T) { + checkOpenInRoot(t, openInRootFn, root, test.unsafePath, test.expected) + }) + } +} + +func TestOpenInRoot(t *testing.T) { + withWithoutOpenat2(t, true, func(t *testing.T) { + testOpenInRoot(t, OpenInRoot) + }) +} + +func TestOpenInRootHandle(t *testing.T) { + withWithoutOpenat2(t, true, func(t *testing.T) { + testOpenInRoot(t, func(root, unsafePath string) (*os.File, error) { + rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return nil, err + } + defer rootDir.Close() + + return OpenatInRoot(rootDir, unsafePath) + }) + }) +} + +func TestOpenInRoot_BadInode(t *testing.T) { + requireRoot(t) // mknod + + withWithoutOpenat2(t, true, func(t *testing.T) { + tree := []string{ + // Make sure we don't open "bad" inodes. + "dir foo", + "char foo/whiteout 0 0", + "block foo/whiteout-blk 0 0", + } + + root := createTree(t, tree...) + + rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + require.NoError(t, err) + defer rootDir.Close() + + for name, test := range map[string]struct { + unsafePath string + expected openResult + }{ + // Complete lookups. + "char-trailing": {"foo/whiteout", openResult{handlePath: "/foo/whiteout", fileType: unix.S_IFCHR}}, + "blk-trailing": {"foo/whiteout-blk", openResult{handlePath: "/foo/whiteout-blk", fileType: unix.S_IFBLK}}, + // Partial lookups due to hitting a non-directory. + "char-dot": {"foo/whiteout/.", openResult{err: unix.ENOENT}}, + "char-dotdot1": {"foo/whiteout/..", openResult{err: unix.ENOENT}}, + "char-dotdot2": {"foo/whiteout/../foo/bar", openResult{err: unix.ENOENT}}, + "blk-dot": {"foo/whiteout-blk/.", openResult{err: unix.ENOENT}}, + "blk-dotdot1": {"foo/whiteout-blk/..", openResult{err: unix.ENOENT}}, + "blk-dotdot2": {"foo/whiteout-blk/../foo/bar", openResult{err: unix.ENOENT}}, + } { + test := test // copy iterator + // Update the handlePath to be inside our root. + if test.expected.handlePath != "" { + test.expected.handlePath = filepath.Join(root, test.expected.handlePath) + } + t.Run(name, func(t *testing.T) { + checkOpenInRoot(t, OpenInRoot, root, test.unsafePath, test.expected) + }) + } + }) +} From ebb9f1f1dba03d9eef1384d34401da8e1b5413f2 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Thu, 11 Jul 2024 02:09:04 +1000 Subject: [PATCH 4/5] mkdirall: switch away from O_PATH for mkdir loop We need to read the list of dirents during the loop, and re-opening and closing the fd each time doesn't really make much sense. O_DIRECTORY should stop us from opening any "bad" inodes, so we might as well make MkdirAllHandle always return an O_DIRECTORY and we can eliminate all of the unneeded reopens. We need to duplicate the handle at the start to make sure we return a non-O_PATH even if we are asked to MkdirAll an existing directory. We can just re-use Reopen now that we have it. Signed-off-by: Aleksa Sarai --- mkdir_linux.go | 45 ++++++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/mkdir_linux.go b/mkdir_linux.go index ee47410..05e0bde 100644 --- a/mkdir_linux.go +++ b/mkdir_linux.go @@ -74,11 +74,15 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err return nil, fmt.Errorf("finding existing subpath of %q: %w", unsafePath, err) } - // Check that we actually got a directory. - if st, err := currentDir.Stat(); err != nil { - return nil, fmt.Errorf("stat existing subpath handle %q: %w", currentDir.Name(), err) - } else if !st.IsDir() { + // Re-open the path to match the O_DIRECTORY reopen loop later (so that we + // always return a non-O_PATH handle). We also check that we actually got a + // directory. + if reopenDir, err := Reopen(currentDir, unix.O_DIRECTORY|unix.O_CLOEXEC); errors.Is(err, unix.ENOTDIR) { return nil, fmt.Errorf("cannot create subdirectories in %q: %w", currentDir.Name(), unix.ENOTDIR) + } else if err != nil { + return nil, fmt.Errorf("re-opening handle to %q: %w", currentDir.Name(), err) + } else { + currentDir = reopenDir } remainingParts := strings.Split(remainingPath, string(filepath.Separator)) @@ -135,15 +139,16 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err return nil, err } - // Get a handle to the next component. + // Get a handle to the next component. O_DIRECTORY means we don't need + // to use O_PATH. var nextDir *os.File if hasOpenat2() { nextDir, err = openat2File(currentDir, part, &unix.OpenHow{ - Flags: unix.O_PATH | unix.O_NOFOLLOW | unix.O_DIRECTORY | unix.O_CLOEXEC, + Flags: unix.O_NOFOLLOW | unix.O_DIRECTORY | unix.O_CLOEXEC, Resolve: unix.RESOLVE_BENEATH | unix.RESOLVE_NO_SYMLINKS | unix.RESOLVE_NO_XDEV, }) } else { - nextDir, err = openatFile(currentDir, part, unix.O_PATH|unix.O_NOFOLLOW|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + nextDir, err = openatFile(currentDir, part, unix.O_NOFOLLOW|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) } if err != nil { return nil, err @@ -167,24 +172,18 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err if stat.Uid != expectedUid || stat.Gid != expectedGid { return nil, fmt.Errorf("%w: newly created directory %q has incorrect owner %d:%d (expected %d:%d)", errPossibleAttack, currentDir.Name(), stat.Uid, stat.Gid, expectedUid, expectedGid) } - // Re-open our O_PATH handle for the directory with O_RDONLY so we - // can check if it the directory is empty. This is safe to do with - // open(2) because there is no way for "." to be replaced or - // mounted over. - if dir, err := openatFile(currentDir, ".", unix.O_RDONLY|unix.O_NOFOLLOW|unix.O_DIRECTORY|unix.O_CLOEXEC, 0); err != nil { - return nil, fmt.Errorf("%w: re-open newly created directory %q: %w", errPossibleAttack, currentDir.Name(), err) - } else { - // We only need to check for a single entry, and we should get EOF - // if the directory is empty. - _, err := dir.Readdirnames(1) - _ = dir.Close() - if !errors.Is(err, io.EOF) { - if err == nil { - err = fmt.Errorf("%w: newly created directory %q is non-empty", errPossibleAttack, currentDir.Name()) - } - return nil, fmt.Errorf("check if newly created directory %q is empty: %w", currentDir.Name(), err) + // Check that the directory is empty. We only need to check for + // a single entry, and we should get EOF if the directory is + // empty. + _, err := currentDir.Readdirnames(1) + if !errors.Is(err, io.EOF) { + if err == nil { + err = fmt.Errorf("%w: newly created directory %q is non-empty", errPossibleAttack, currentDir.Name()) } + return nil, fmt.Errorf("check if newly created directory %q is empty: %w", currentDir.Name(), err) } + // Reset the offset. + _, _ = currentDir.Seek(0, unix.SEEK_SET) } } return currentDir, nil From 0a923e53d44862a9d632d940ffaf2292e7ee7461 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Thu, 11 Jul 2024 18:12:18 +1000 Subject: [PATCH 5/5] README: update to describe and strongly recommend new APIs Signed-off-by: Aleksa Sarai --- README.md | 139 ++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 114 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 4eca0f2..253956f 100644 --- a/README.md +++ b/README.md @@ -2,31 +2,24 @@ [![Build Status](https://github.com/cyphar/filepath-securejoin/actions/workflows/ci.yml/badge.svg)](https://github.com/cyphar/filepath-securejoin/actions/workflows/ci.yml) -An implementation of `SecureJoin`, a [candidate for inclusion in the Go -standard library][go#20126]. The purpose of this function is to be a "secure" -alternative to `filepath.Join`, and in particular it provides certain -guarantees that are not provided by `filepath.Join`. - -> **NOTE**: This code is *only* safe if you are not at risk of other processes -> modifying path components after you've used `SecureJoin`. If it is possible -> for a malicious process to modify path components of the resolved path, then -> you will be vulnerable to some fairly trivial TOCTOU race conditions. [There -> are some Linux kernel patches I'm working on which might allow for a better -> solution.][lwn-obeneath] -> -> In addition, with a slightly modified API it might be possible to use -> `O_PATH` and verify that the opened path is actually the resolved one -- but -> I have not done that yet. I might add it in the future as a helper function -> to help users verify the path (we can't just return `/proc/self/fd/` -> because that doesn't always work transparently for all users). - -This is the function prototype: +### Old API ### -```go -func SecureJoin(root, unsafePath string) (string, error) -``` +This library was originally just an implementation of `SecureJoin` which was +[intended to be included in the Go standard library][go#20126] as a safer +`filepath.Join` that would restrict the path lookup to be inside a root +directory. + +The implementation was based on code that existed in several container +runtimes. Unfortunately, this API is **fundamentally unsafe** against attackers +that can modify path components after `SecureJoin` returns and before the +caller uses the path, allowing for some fairly trivial TOCTOU attacks. + +`SecureJoin` (and `SecureJoinVFS`) are still provided by this library to +support legacy users, but new users are strongly suggested to avoid using +`SecureJoin` and instead use the [new api](#new-api) or switch to +[libpathrs][libpathrs]. -This library **guarantees** the following: +With the above limitations in mind, this library guarantees the following: * If no error is set, the resulting string **must** be a child path of `root` and will not contain any symlink path components (they will all be @@ -47,7 +40,7 @@ This library **guarantees** the following: A (trivial) implementation of this function on GNU/Linux systems could be done with the following (note that this requires root privileges and is far more opaque than the implementation in this library, and also requires that -`readlink` is inside the `root` path): +`readlink` is inside the `root` path and is trustworthy): ```go package securejoin @@ -70,9 +63,105 @@ func SecureJoin(root, unsafePath string) (string, error) { } ``` -[lwn-obeneath]: https://lwn.net/Articles/767547/ +[libpathrs]: https://github.com/openSUSE/libpathrs [go#20126]: https://github.com/golang/go/issues/20126 +### New API ### + +While we recommend users switch to [libpathrs][libpathrs] as soon as it has a +stable release, some methods implemented by libpathrs have been ported to this +library to ease the transition. These APIs are only supported on Linux. + +These APIs are implemented such that `filepath-securejoin` will +opportunistically use certain newer kernel APIs that make these operations far +more secure. In particular: + +* All of the lookup operations will use [`openat2`][openat2.2] on new enough + kernels (Linux 5.6 or later) to restrict lookups through magic-links and + bind-mounts (for certain operations) and to make use of `RESOLVE_IN_ROOT` to + efficiently resolve symlinks within a rootfs. + +* The APIs provide hardening against a malicious `/proc` mount to either detect + or avoid being tricked by a `/proc` that is not legitimate. This is done + using [`openat2`][openat2.2] for all users, and privileged users will also be + further protected by using [`fsopen`][fsopen.2] and [`open_tree`][open_tree.2] + (Linux 4.18 or later). + +[openat2.2]: https://www.man7.org/linux/man-pages/man2/openat2.2.html +[fsopen.2]: https://github.com/brauner/man-pages-md/blob/main/fsopen.md +[open_tree.2]: https://github.com/brauner/man-pages-md/blob/main/open_tree.md + +#### `OpenInRoot` #### + +```go +func OpenInRoot(root, unsafePath string) (*os.File, error) +func OpenatInRoot(root *os.File, unsafePath string) (*os.File, error) +func Reopen(handle *os.File, flags int) (*os.File, error) +``` + +`OpenInRoot` is a much safer version of + +```go +path, err := securejoin.SecureJoin(root, unsafePath) +file, err := os.OpenFile(path, unix.O_PATH|unix.O_CLOEXEC) +``` + +that protects against various race attacks that could lead to serious security +issues, depending on the application. Note that the returned `*os.File` is an +`O_PATH` file descriptor, which is quite restricted. Callers will probably need +to use `Reopen` to get a more usable handle (this split is done to provide +useful features like PTY spawning and to avoid users accidentally opening bad +inodes that could cause a DoS). + +Callers need to be careful in how they use the returned `*os.File`. Usually it +is only safe to operate on the handle directly, and it is very easy to create a +security issue. [libpathrs][libpathrs] provides far more helpers to make using +these handles safer -- there is currently no plan to port them to +`filepath-securejoin`. + +`OpenatInRoot` is like `OpenInRoot` except that the root is provided using an +`*os.File`. This allows you to ensure that multiple `OpenatInRoot` (or +`MkdirAllHandle`) calls are operating on the same rootfs. + +> **NOTE**: Unlike `SecureJoin`, `OpenInRoot` will error out as soon as it hits +> a dangling symlink or non-existent path. This is in contrast to `SecureJoin` +> which treated non-existent components as though they were real directories, +> and would allow for partial resolution of dangling symlinks. These behaviours +> are at odds with how Linux treats non-existent paths and dangling symlinks, +> and so these are no longer allowed. + +#### `MkdirAll` #### + +```go +func MkdirAll(root, unsafePath string, mode int) error +func MkdirAllHandle(root *os.File, unsafePath string, mode int) (*os.File, error) +``` + +`MkdirAll` is a much safer version of + +```go +path, err := securejoin.SecureJoin(root, unsafePath) +err = os.MkdirAll(path, mode) +``` + +that protects against the same kinds of races that `OpenInRoot` protects +against. + +`MkdirAllHandle` is like `MkdirAll` except that the root is provided using an +`*os.File` (the reason for this is the same as with `OpenatInRoot`) and an +`*os.File` of the final created directory is returned (this directory is +guaranteed to be effectively identical to the directory created by +`MkdirAllHandle`, which is not possible to ensure by just using `OpenatInRoot` +after `MkdirAll`). + +> **NOTE**: Unlike `SecureJoin`, `MkdirAll` will error out as soon as it hits +> a dangling symlink or non-existent path. This is in contrast to `SecureJoin` +> which treated non-existent components as though they were real directories, +> and would allow for partial resolution of dangling symlinks. These behaviours +> are at odds with how Linux treats non-existent paths and dangling symlinks, +> and so these are no longer allowed. This means that `MkdirAll` will not +> create non-existent directories referenced by a dangling symlink. + ### License ### The license of this project is the same as Go, which is a BSD 3-clause license