merge #52 into cyphar/filepath-securejoin:main

Aleksa Sarai (7):
  proc: add pure-resolver sanity tests
  proc: export ProcThreadSelf and ProcSelfFdReadlink
  proc: switch to much safer resolver for non-openat2 systems
  proc: split verifyProcRoot
  proc: checkSymlinkOvermount -> checkSubpathOvermount
  proc: add more info for statx failures
  tests: reduce number of rounds for race tests

LGTMs: cyphar

Author: cyphar

CHANGELOG.md

@@ -6,6 +6,80 @@ 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
+  `open_tree(2)` (Linux 4.18) and programs with privileges to use them (with
+  some caveats about locked mounts that probably affect very few users). For
+  other users, an attacker with the ability to create malicious mounts (on most
+  systems, a sysadmin) could trick you into operating on files you didn't
+  expect. This attack only really makes sense in the context of container
+  runtime implementations.
+
+  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). 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
+  these protections effectively rely on [`statx(STATX_MNT_ID)`][statx.2] (Linux
+  5.8). On older kernel versions, there is no effective protection (there is
+  some minimal protection against non-`procfs` filesystem components but a
+  sufficiently clever attacker can work around those). In addition,
+  `STATX_MNT_ID` is vulnerable to mount ID reuse attacks by sufficiently
+  motivated and privileged attackers -- this problem is mitigated with
+  `STATX_MNT_ID_UNIQUE` (Linux 6.8) but that raises the minimum kernel version
+  for more protection.
+
+  The fact that these protections are quite limited despite needing a fair bit
+  of extra code to handle was one of the primary reasons we did not initially
+  implement this in `filepath-securejoin` ([libpathrs][] supports all of this,
+  of course).
+
+[libpathrs]: https://github.com/openSUSE/libpathrs
+[statx.2]: https://www.man7.org/linux/man-pages/man2/statx.2.html
+
 ## [0.4.1] - 2025-01-28 ##
 
 ### Fixed ###

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")
 
@@ -558,20 +558,34 @@ func TestPartialLookup_RacingRename(t *testing.T) {
 				go doRenameExchangeLoop(pauseCh, exitCh, rootDir, test.subPathA, test.subPathB)
 
 				// Do several runs to try to catch bugs.
-				const testRuns = 50000
+				const (
+					testRuns     = 3000
+					minPassCount = 10
+				)
 				m := newRacingLookupMeta(pauseCh)
-				for i := 0; i < testRuns; i++ {
+				doneRuns := 0
+				for ; doneRuns < testRuns || m.passOkCount < minPassCount; doneRuns++ {
 					m.checkPartialLookup(t, rootDir, test.unsafePath, test.skipErrs, test.allowedResults)
+					// Make sure we don't infinite loop here.
+					if doneRuns >= 50*testRuns {
+						break
+					}
 				}
 
 				pct := func(count int) string {
-					return fmt.Sprintf("%d(%.3f%%)", count, 100.0*float64(count)/float64(testRuns))
+					return fmt.Sprintf("%d(%.3f%%)", count, 100.0*float64(count)/float64(doneRuns))
+				}
+
+				// No passing runs is a bit unfortunate, but some of our tests
+				// can do that and failing here would just lead to flaky tests.
+				if m.passOkCount == 0 {
+					t.Logf("WARNING: NO PASSING RUNS!")
 				}
 
 				// Output some stats.
 				t.Logf("after %d runs: passOk=%s passErr=%s skip=%s fail=%s (+badErr=%s)",
 					// runs and breakdown of path-related (pass, fail) as well as skipped runs
-					testRuns, pct(m.passOkCount), pct(m.passErrCount), pct(m.skipCount), pct(m.failCount),
+					doneRuns, pct(m.passOkCount), pct(m.passErrCount), pct(m.skipCount), pct(m.failCount),
 					// failures due to incorrect errors (rather than bad paths)
 					pct(m.badErrCount))
 				t.Logf("  badHandleName=%s fixRemainingPath=%s",

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
@@ -404,9 +404,13 @@ func TestMkdirAllHandle_RacingRename(t *testing.T) { //nolint:revive // undersco
 				}(rootCh)
 
 				// Do several runs to try to catch bugs.
-				const testRuns = 2000
+				const (
+					testRuns     = 800
+					minPassCount = 10
+				)
 				m := newRacingMkdirMeta()
-				for i := 0; i < testRuns; i++ {
+				doneRuns := 0
+				for ; doneRuns < testRuns || m.passOkCount < minPassCount; doneRuns++ {
 					root := createTree(t, treeSpec...)
 
 					rootCh <- root
@@ -417,15 +421,26 @@ func TestMkdirAllHandle_RacingRename(t *testing.T) { //nolint:revive // undersco
 					// Clean up the root after each run so we don't exhaust all
 					// space in the tmpfs.
 					_ = os.RemoveAll(root)
+
+					// Make sure we don't infinite loop here.
+					if doneRuns >= 50*testRuns {
+						break
+					}
 				}
 
 				pct := func(count int) string {
-					return fmt.Sprintf("%d(%.3f%%)", count, 100.0*float64(count)/float64(testRuns))
+					return fmt.Sprintf("%d(%.3f%%)", count, 100.0*float64(count)/float64(doneRuns))
+				}
+
+				// No passing runs is a bit unfortunate, but some of our tests
+				// can do that and failing here would just lead to flaky tests.
+				if m.passOkCount == 0 {
+					t.Logf("WARNING: NO PASSING RUNS!")
 				}
 
 				// Output some stats.
 				t.Logf("after %d runs: passOk=%s passErr=%s fail=%s",
-					testRuns, pct(m.passOkCount), pct(m.passErrCount), pct(m.failCount))
+					doneRuns, pct(m.passOkCount), pct(m.passErrCount), pct(m.failCount))
 				if len(m.passErrCounts) > 0 {
 					t.Logf("  passErr breakdown:")
 					for err, count := range m.passErrCounts {
@@ -475,9 +490,13 @@ func TestMkdirAllHandle_RacingDelete(t *testing.T) { //nolint:revive // undersco
 				}(rootCh)
 
 				// Do several runs to try to catch bugs.
-				const testRuns = 2000
+				const (
+					testRuns     = 800
+					minPassCount = 10
+				)
 				m := newRacingMkdirMeta()
-				for i := 0; i < testRuns; i++ {
+				doneRuns := 0
+				for ; doneRuns < testRuns; doneRuns++ {
 					root := createTree(t, treeSpec...)
 
 					rootCh <- root
@@ -487,15 +506,26 @@ func TestMkdirAllHandle_RacingDelete(t *testing.T) { //nolint:revive // undersco
 					// Clean up the root after each run so we don't exhaust all
 					// space in the tmpfs.
 					_ = os.RemoveAll(root + "/..")
+
+					// Make sure we don't infinite loop here.
+					if doneRuns >= 50*testRuns {
+						break
+					}
 				}
 
 				pct := func(count int) string {
-					return fmt.Sprintf("%d(%.3f%%)", count, 100.0*float64(count)/float64(testRuns))
+					return fmt.Sprintf("%d(%.3f%%)", count, 100.0*float64(count)/float64(doneRuns))
+				}
+
+				// No passing runs is a bit unfortunate, but some of our tests
+				// can do that and failing here would just lead to flaky tests.
+				if m.passOkCount == 0 {
+					t.Logf("WARNING: NO PASSING RUNS!")
 				}
 
 				// Output some stats.
 				t.Logf("after %d runs: passOk=%s passErr=%s fail=%s",
-					testRuns, pct(m.passOkCount), pct(m.passErrCount), pct(m.failCount))
+					doneRuns, pct(m.passOkCount), pct(m.passErrCount), pct(m.failCount))
 				if len(m.passErrCounts) > 0 {
 					t.Logf("  passErr breakdown:")
 					for err, count := range m.passErrCounts {

open_linux.go

@@ -88,7 +88,7 @@ func Reopen(handle *os.File, flags int) (*os.File, error) {
 	// [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 {
+	if err := checkSubpathOvermount(procRoot, procFdDir, fdStr); err != nil {
 		return nil, fmt.Errorf("check safety of /proc/thread-self/fd/%s magiclink: %w", fdStr, err)
 	}
 

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.