proc: export ProcThreadSelf and ProcSelfFdReadlink

These are quite useful for protecting against a variety of attacks, and
mirror the general API from libpathrs but a little bit less
fully-featured.

Signed-off-by: Aleksa Sarai <[email protected]>

Author: cyphar

CHANGELOG.md

@@ -7,6 +7,43 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ## [Unreleased] ##
 
 ### Added ###
+- Some minimal parts of the safe `procfs` API have now been exposed. At the
+  moment these include:
+
+   * `ProcThreadSelf` which allows you to get a safe handle `O_PATH` to a
+     subpath in `/proc/thread-self` (you can upgrade it to a proper handle with
+     `Reopen` like any other `O_PATH` handle). The returned
+     `ProcThreadSelfCloser` needs to be called after you completely finish
+     using the handle (this is necessary because Go is multi-threaded and
+     `ProcThreadSelf` references `/proc/thread-self` which may disappear if we
+     do not `runtime.LockOSThread` -- `ProcThreadSelfCloser` is currently
+     equivalent to `runtime.UnlockOSThread`).
+
+     Note that you cannot operate on any `procfs` symlinks (most notably
+     magiclinks) using this API. At the moment `filepath-securejoin` does not
+     support this feature (but [libpathrs][] does).
+
+   * `ProcSelfFdReadlink` lets you get the in-kernel path representation of a
+     file descriptor (think `readlink("/proc/self/fd/...")`). This is
+     equivalent to doing a `readlinkat(fd, "", ...)` of
+     `ProcThreadSelf("fd/%d")`, except that we verify that there aren't any
+     tricky overmounts that could fool the process.
+
+     Please be aware that the returned string is simply a snapshot at that
+     particular moment, and an attacker could move the file being pointed to.
+     In addition, complex namespace configurations could result in non-sensical
+     or confusing paths to be returned. The value received from this function
+     should only be used as secondary verification of some security property,
+     not as proof that a particular handle has a particular path.
+
+  The procfs handle used internally by the API is the same as the rest of
+  `filepath-securejoin` (for privileged programs this is usually a private
+  in-process `procfs` instance created with `fsopen(2)`).
+
+  As before, this is intended as a stop-gap before users migrate to
+  [libpathrs][], which provides a far more extensive safe `procfs` API and is
+  generally more robust.
+
 - Previously, the hardened procfs implementation (used internally within
   `Reopen` and `Open(at)InRoot`) only protected against overmount attacks on
   systems with `openat2(2)` (Linux 5.6) or systems with `fsopen(2)` or
@@ -20,7 +57,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   This was considered a reasonable trade-off, as the long-term intention was to
   get all users to just switch to [libpathrs][] if they wanted to use the safe
   `procfs` API (which had more extensive protections, and is what these new
-  protections in `filepath-securejoin` are based on).
+  protections in `filepath-securejoin` are based on). However, as the API
+  is now being exported it seems unwise to advertise the API as "safe" if we do
+  not protect against known attacks.
 
   The procfs API will now be more protected against attackers on systems
   lacking the aforementioned protections. However, the most comprehensive of

lookup_linux.go

@@ -190,7 +190,7 @@ func lookupInRoot(root *os.File, unsafePath string, partial bool) (Handle *os.Fi
 	// root is some magic-link like /proc/$pid/root, in which case we want to
 	// make sure when we do checkProcSelfFdPath that we are using the correct
 	// root path.
-	logicalRootPath, err := procSelfFdReadlink(root)
+	logicalRootPath, err := ProcSelfFdReadlink(root)
 	if err != nil {
 		return nil, "", fmt.Errorf("get real root path: %w", err)
 	}

lookup_linux_test.go

@@ -52,7 +52,7 @@ func checkPartialLookup(t *testing.T, partialLookupFn partialLookupFunc, rootDir
 	assert.Equal(t, expected.remainingPath, remainingPath, "remaining path")
 
 	// Check the handle path.
-	gotPath, err := procSelfFdReadlink(handle)
+	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.
@@ -371,9 +371,9 @@ func (m *racingLookupMeta) checkPartialLookup(t *testing.T, rootDir *os.File, un
 	if handle != nil {
 		handleName = handle.Name()
 
-		// Get the "proper" name from procSelfFdReadlink.
+		// Get the "proper" name from ProcSelfFdReadlink.
 		m.pauseCh <- struct{}{}
-		realPath, err = procSelfFdReadlink(handle)
+		realPath, err = ProcSelfFdReadlink(handle)
 		<-m.pauseCh
 		require.NoError(t, err, "get real path of returned handle")
 

mkdir_linux_test.go

@@ -46,7 +46,7 @@ var mkdirAll_MkdirAllHandle mkdirAllFunc = func(t *testing.T, root, unsafePath s
 	require.NoError(t, err)
 
 	// Now double-check that the handle is correct.
-	gotPath, err := procSelfFdReadlink(handle)
+	gotPath, err := ProcSelfFdReadlink(handle)
 	require.NoError(t, err, "get real path of returned handle")
 	assert.Equal(t, expectedPath, gotPath, "wrong final path from MkdirAllHandle")
 	// Also check that the f.Name() is correct while we're at it (this is

open_linux_test.go

@@ -62,13 +62,13 @@ func checkReopen(t *testing.T, handle *os.File, flags int, expectedErr error) {
 	require.NoError(t, err)
 
 	// Get the original handle path.
-	handlePath, err := procSelfFdReadlink(handle)
+	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)
+	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()")
@@ -102,7 +102,7 @@ func checkOpenInRoot(t *testing.T, openInRootFn openInRootFunc, root, unsafePath
 	require.NoError(t, err)
 
 	// Check the handle path.
-	gotPath, err := procSelfFdReadlink(handle)
+	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.

procfs_linux.go

@@ -210,24 +210,30 @@ var hasProcThreadSelf = sync_OnceValue(func() bool {
 
 var errUnsafeProcfs = errors.New("unsafe procfs detected")
 
-type procThreadSelfCloser func()
-
-// procThreadSelf returns a handle to /proc/thread-self/<subpath> (or an
-// equivalent handle on older kernels where /proc/thread-self doesn't exist).
-// Once finished with the handle, you must call the returned closer function
-// (runtime.UnlockOSThread). You must not pass the returned *os.File to other
-// Go threads or use the handle after calling the closer.
-//
-// This is similar to ProcThreadSelf from runc, but with extra hardening
-// applied and using *os.File.
+// ProcThreadSelfCloser is a callback that needs to be called when you are done
+// operating on an [os.File] fetched using [ProcThreadSelf].
 //
+// [os.File]: https://pkg.go.dev/os#File
+type ProcThreadSelfCloser func()
+
 // NOTE: THIS IS NOT YET SAFE TO EXPORT. The non-openat2(2) case is just using
 // a plain openat(2), which is not entirely safe against overmount attacks.
 // Yes, if we are using fsopen(2) or open_tree(2) (without AT_RECURSIVE), then
 // this is safe, but we shouldn't make less privileged users (or users on older
 // kernels) incorrectly assume this is safe. libpathrs does it correctly, and
 // it's best to leave it to them.
-func procThreadSelf(procRoot *os.File, subpath string) (_ *os.File, _ procThreadSelfCloser, Err error) {
+func procThreadSelf(procRoot *os.File, subpath string) (_ *os.File, _ ProcThreadSelfCloser, Err error) {
+	// If called from the external API, procRoot will be nil, so just get the
+	// global root handle. It's also possible one of our tests calls this with
+	// nil by accident, so we should handle the case anyway.
+	if procRoot == nil {
+		root, err := getProcRoot()
+		if err != nil {
+			return nil, nil, err
+		}
+		procRoot = root
+	}
+
 	// We need to lock our thread until the caller is done with the handle
 	// because between getting the handle and using it we could get interrupted
 	// by the Go runtime and hit the case where the underlying thread is
@@ -267,6 +273,18 @@ func procThreadSelf(procRoot *os.File, subpath string) (_ *os.File, _ procThread
 	return handle, runtime.UnlockOSThread, nil
 }
 
+// ProcThreadSelf returns a handle to /proc/thread-self/<subpath> (or an
+// equivalent handle on older kernels where /proc/thread-self doesn't exist).
+// Once finished with the handle, you must call the returned closer function
+// (runtime.UnlockOSThread). You must not pass the returned *os.File to other
+// Go threads or use the handle after calling the closer.
+//
+// This is similar to ProcThreadSelf from runc, but with extra hardening
+// applied and using *os.File.
+func ProcThreadSelf(subpath string) (*os.File, ProcThreadSelfCloser, error) {
+	return procThreadSelf(nil, subpath)
+}
+
 // STATX_MNT_ID_UNIQUE is provided in golang.org/x/[email protected], but in order to
 // avoid bumping the requirement for a single constant we can just define it
 // ourselves.
@@ -372,7 +390,10 @@ func rawProcSelfFdReadlink(fd int) (string, error) {
 	return doRawProcSelfFdReadlink(procRoot, fd)
 }
 
-func procSelfFdReadlink(f *os.File) (string, error) {
+// ProcSelfFdReadlink gets the real path of the given file by looking at
+// readlink(/proc/thread-self/fd/$n), with the same safety protections as
+// [ProcThreadSelf] (as well as some additional checks against overmounts).
+func ProcSelfFdReadlink(f *os.File) (string, error) {
 	return rawProcSelfFdReadlink(int(f.Fd()))
 }
 
@@ -404,7 +425,7 @@ func checkProcSelfFdPath(path string, file *os.File) error {
 	if err := isDeadInode(file); err != nil {
 		return err
 	}
-	actualPath, err := procSelfFdReadlink(file)
+	actualPath, err := ProcSelfFdReadlink(file)
 	if err != nil {
 		return fmt.Errorf("get path of handle: %w", err)
 	}

procfs_linux_test.go

@@ -77,8 +77,15 @@ func setupMountNamespace(t *testing.T) {
 	require.NoError(t, err)
 }
 
+func doProcThreadSelf(procRoot *os.File, subpath string) (*os.File, ProcThreadSelfCloser, error) {
+	if procRoot != nil {
+		return procThreadSelf(procRoot, subpath)
+	}
+	return ProcThreadSelf(subpath)
+}
+
 func testProcThreadSelf(t *testing.T, procRoot *os.File, subpath string, expectErr bool) {
-	handle, closer, err := procThreadSelf(procRoot, subpath)
+	handle, closer, err := doProcThreadSelf(procRoot, subpath)
 	if expectErr {
 		assert.ErrorIsf(t, err, errUnsafeProcfs, "should have detected /proc/thread-self/%s overmount", subpath)
 	} else if assert.NoErrorf(t, err, "/proc/thread-self/%s open should succeed", subpath) {
@@ -127,7 +134,9 @@ func testProcOvermountSubdir(t *testing.T, procRootFn procRootFunc, expectOvermo
 
 		procRoot, err := procRootFn()
 		require.NoError(t, err)
-		defer procRoot.Close() //nolint:errcheck // test code
+		if procRoot != nil {
+			defer procRoot.Close() //nolint:errcheck // test code
+		}
 
 		// For both tmpfs and procfs overmounts, we should catch them (with or
 		// without openat2, thanks to procfsLookupInRoot).
@@ -140,7 +149,7 @@ func testProcOvermountSubdir(t *testing.T, procRootFn procRootFunc, expectOvermo
 			symlinkOvermountErr = nil
 		}
 
-		procSelf, closer, err := procThreadSelf(procRoot, ".")
+		procSelf, closer, err := doProcThreadSelf(procRoot, ".")
 		require.NoError(t, err)
 		defer procSelf.Close() //nolint:errcheck // test code
 		defer closer()
@@ -155,22 +164,31 @@ func testProcOvermountSubdir(t *testing.T, procRootFn procRootFunc, expectOvermo
 		require.NoError(t, err)
 		defer procExe.Close() //nolint:errcheck // test code
 
+		// Get the handle that doProcThreadSelf is using internally when we
+		// test the external API.
+		testProcRoot := procRoot
+		if testProcRoot == nil {
+			testProcRoot, err = getProcRoot()
+			require.NoError(t, err)
+			// do not call testProcRoot.Close() -- it's a global handle
+		}
+
 		// no overmount
-		err = checkSubpathOvermount(procRoot, procCwd, "")
+		err = checkSubpathOvermount(testProcRoot, procCwd, "")
 		assert.NoError(t, err, "checking /proc/self/cwd with no overmount should succeed") //nolint:testifylint // this is an isolated operation so we can continue despite an error
-		err = checkSubpathOvermount(procRoot, procSelf, "cwd")
+		err = checkSubpathOvermount(testProcRoot, procSelf, "cwd")
 		assert.NoError(t, err, "checking /proc/self/cwd with no overmount should succeed") //nolint:testifylint // this is an isolated operation so we can continue despite an error
 		// basic overmount
-		err = checkSubpathOvermount(procRoot, procExe, "")
+		err = checkSubpathOvermount(testProcRoot, procExe, "")
 		assert.ErrorIs(t, err, symlinkOvermountErr, "unexpected /proc/self/exe overmount result") //nolint:testifylint // this is an isolated operation so we can continue despite an error
-		err = checkSubpathOvermount(procRoot, procSelf, "exe")
+		err = checkSubpathOvermount(testProcRoot, procSelf, "exe")
 		assert.ErrorIs(t, err, symlinkOvermountErr, "unexpected /proc/self/exe overmount result") //nolint:testifylint // this is an isolated operation so we can continue despite an error
 
 		// fd no overmount
-		_, err = doRawProcSelfFdReadlink(procRoot, 1)
+		_, err = doRawProcSelfFdReadlink(testProcRoot, 1)
 		assert.NoError(t, err, "checking /proc/self/fd/1 with no overmount should succeed") //nolint:testifylint // this is an isolated operation so we can continue despite an error
 		// fd overmount
-		link, err := doRawProcSelfFdReadlink(procRoot, 0)
+		link, err := doRawProcSelfFdReadlink(testProcRoot, 0)
 		assert.ErrorIs(t, err, symlinkOvermountErr, "unexpected /proc/self/fd/0 overmount result: got link %q", link) //nolint:testifylint // this is an isolated operation so we can continue despite an error
 	})
 }
@@ -224,6 +242,29 @@ func TestProcOvermountSubdir_doGetProcRoot_Mocked(t *testing.T) {
 	})
 }
 
+func TestProcOvermountSubdir_ProcThreadSelf(t *testing.T) {
+	withWithoutOpenat2(t, true, func(t *testing.T) {
+		testForceProcThreadSelf(t, func(t *testing.T) {
+			dummyGetRoot := func() (*os.File, error) { return nil, nil } //nolint:nilnil // intentional
+			// Use the same overmounts policy as doGetProcRoot.
+			testProcOvermountSubdir(t, dummyGetRoot, !hasNewMountAPI())
+		})
+	})
+}
+
+// Because of the introduction of protections against /proc overmounts,
+// ProcThreadSelf will not be called in actual tests unless we have a basic
+// test here.
+func TestProcThreadSelf(t *testing.T) {
+	withWithoutOpenat2(t, true, func(t *testing.T) {
+		handle, closer, err := ProcThreadSelf("stat")
+		require.NoError(t, err, "ProcThreadSelf(stat)")
+		require.NotNil(t, handle, "ProcThreadSelf(stat)")
+		defer closer()
+		defer handle.Close() //nolint:errcheck // test code
+	})
+}
+
 func canFsOpen() bool {
 	f, err := fsopen("tmpfs", 0)
 	if f != nil {

procfs_lookup_linux.go

@@ -69,7 +69,7 @@ func procfsLookupInRoot(procRoot *os.File, unsafePath string) (Handle *os.File,
 		//       nd_jump_link() so RESOLVE_NO_MAGICLINKS allows it.
 		//
 		// NOTE: We MUST NOT use RESOLVE_IN_ROOT here, as openat2File uses
-		//       procSelfFdReadlink to clean up the returned f.Name() if we use
+		//       ProcSelfFdReadlink to clean up the returned f.Name() if we use
 		//       RESOLVE_IN_ROOT (which would lead to an infinite recursion).
 		//
 		// TODO: It would be nice to have RESOLVE_NO_DOTDOT, purely for