merge #57 into cyphar/filepath-securejoin:main

Aleksa Sarai (1):
  procfs: remove caching of procfs root

LGTMs: cyphar

Author: cyphar

CHANGELOG.md

@@ -92,7 +92,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   implement this in `filepath-securejoin` ([libpathrs][] supports all of this,
   of course).
 
-[libpathrs]: https://github.com/openSUSE/libpathrs
+### Changed ###
+- The procfs root file descriptor is no longer cached for the lifetime of the
+  process. This kind of global file descriptor caching has caused security
+  issues in container runtimes before (see [CVE-2024-21626][] for an example),
+  and so it seems prudent to avoid it. This mirrors [a similar change made to
+  libpathrs][libpathrs-pr204].
+
+[libpathrs]: https://github.com/cyphar/libpathrs
+[libpathrs-pr204]: https://github.com/cyphar/libpathrs/pull/204
 [statx.2]: https://www.man7.org/linux/man-pages/man2/statx.2.html
 
 ## [0.4.1] - 2025-01-28 ##
@@ -262,7 +270,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   safe to start migrating to as we have extensive tests ensuring they behave
   correctly and are safe against various races and other attacks.
 
-[libpathrs]: https://github.com/openSUSE/libpathrs
+[libpathrs]: https://github.com/cyphar/libpathrs
 [open.2]: https://www.man7.org/linux/man-pages/man2/open.2.html
 
 ## [0.2.5] - 2024-05-03 ##

gocompat_generics_go121.go

@@ -26,7 +26,3 @@ func slices_Clone[S ~[]E, E any](slice S) S { //nolint:revive // name is meant t
 func sync_OnceValue[T any](f func() T) func() T { //nolint:revive // name is meant to mirror stdlib
 	return sync.OnceValue(f)
 }
-
-func sync_OnceValues[T1, T2 any](f func() (T1, T2)) func() (T1, T2) { //nolint:revive // name is meant to mirror stdlib
-	return sync.OnceValues(f)
-}

gocompat_generics_unsupported.go

@@ -93,32 +93,3 @@ func sync_OnceValue[T any](f func() T) func() T { //nolint:revive // name is mea
 		return result
 	}
 }
-
-// Copied from the Go 1.24 stdlib implementation.
-func sync_OnceValues[T1, T2 any](f func() (T1, T2)) func() (T1, T2) { //nolint:revive // name is meant to mirror stdlib
-	var (
-		once  sync.Once
-		valid bool
-		p     any
-		r1    T1
-		r2    T2
-	)
-	g := func() {
-		defer func() {
-			p = recover()
-			if !valid {
-				panic(p)
-			}
-		}()
-		r1, r2 = f()
-		f = nil
-		valid = true
-	}
-	return func() (T1, T2) {
-		once.Do(g)
-		if !valid {
-			panic(p)
-		}
-		return r1, r2
-	}
-}

open_linux.go

@@ -67,6 +67,7 @@ func Reopen(handle *os.File, flags int) (*os.File, error) {
 	if err != nil {
 		return nil, err
 	}
+	defer procRoot.Close() //nolint:errcheck // close failures aren't critical here
 
 	// 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

procfs_linux.go

@@ -189,7 +189,7 @@ func unsafeHostProcRoot() (_ *os.File, Err error) {
 	return procRoot, nil
 }
 
-func doGetProcRoot() (*os.File, error) {
+func getProcRoot() (*os.File, error) {
 	procRoot, err := privateProcRoot()
 	if err != nil {
 		// Fall back to using a /proc handle if making a private mount failed.
@@ -200,10 +200,6 @@ func doGetProcRoot() (*os.File, error) {
 	return procRoot, err
 }
 
-var getProcRoot = sync_OnceValues(func() (*os.File, error) {
-	return doGetProcRoot()
-})
-
 var hasProcThreadSelf = sync_OnceValue(func() bool {
 	return unix.Access("/proc/thread-self/", unix.F_OK) == nil
 })
@@ -226,6 +222,7 @@ func procThreadSelf(procRoot *os.File, subpath string) (_ *os.File, _ ProcThread
 			return nil, nil, err
 		}
 		procRoot = root
+		defer procRoot.Close() //nolint:errcheck // close failures aren't critical here
 	}
 
 	// We need to lock our thread until the caller is done with the handle
@@ -295,6 +292,7 @@ func ProcPid(pid int, subpath string) (*os.File, error) {
 	if err != nil {
 		return nil, err
 	}
+	defer procRoot.Close() //nolint:errcheck // close failures aren't critical here
 
 	handle, err := procfsLookupInRoot(procRoot, strconv.Itoa(pid)+"/"+subpath)
 	if err != nil {
@@ -407,6 +405,7 @@ func rawProcSelfFdReadlink(fd int) (string, error) {
 	if err != nil {
 		return "", err
 	}
+	defer procRoot.Close() //nolint:errcheck // close failures aren't critical here
 	return doRawProcSelfFdReadlink(procRoot, fd)
 }
 

procfs_linux_test.go

@@ -164,13 +164,14 @@ 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()
+			// If using the external API, we cannot get the procfs handled used
+			// directly (because it gets created anew for each operation), so
+			// instead just reuse procSelf for this.
+			testProcRoot, err = openatFile(procSelf, "../../..", unix.O_PATH, 0)
 			require.NoError(t, err)
-			// do not call testProcRoot.Close() -- it's a global handle
+			defer testProcRoot.Close() //nolint:errcheck // test code
 		}
 
 		// no overmount
@@ -222,22 +223,22 @@ func TestProcOvermountSubdir_clonePrivateProcMount(t *testing.T) {
 	})
 }
 
-func TestProcOvermountSubdir_doGetProcRoot(t *testing.T) {
+func TestProcOvermountSubdir_getProcRoot(t *testing.T) {
 	withWithoutOpenat2(t, true, func(t *testing.T) {
 		// We expect to not get overmounts if we have the new mount API.
 		// FIXME: It's possible to hit overmounts if there are locked mounts
 		// and we hit the AT_RECURSIVE case...
-		testProcOvermountSubdir(t, doGetProcRoot, !hasNewMountAPI())
+		testProcOvermountSubdir(t, getProcRoot, !hasNewMountAPI())
 	})
 }
 
-func TestProcOvermountSubdir_doGetProcRoot_Mocked(t *testing.T) {
+func TestProcOvermountSubdir_getProcRoot_Mocked(t *testing.T) {
 	if !hasNewMountAPI() {
 		t.Skip("test requires fsopen/open_tree support")
 	}
 	withWithoutOpenat2(t, true, func(t *testing.T) {
 		testForceGetProcRoot(t, func(t *testing.T, expectOvermounts bool) {
-			testProcOvermountSubdir(t, doGetProcRoot, expectOvermounts)
+			testProcOvermountSubdir(t, getProcRoot, expectOvermounts)
 		})
 	})
 }
@@ -246,7 +247,7 @@ 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.
+			// Use the same overmounts policy as getProcRoot.
 			testProcOvermountSubdir(t, dummyGetRoot, !hasNewMountAPI())
 		})
 	})
@@ -454,18 +455,18 @@ func TestProcOvermount_newPrivateProcMount(t *testing.T) {
 	testProcOvermount(t, newPrivateProcMount, true)
 }
 
-func TestProcOvermount_doGetProcRoot(t *testing.T) {
+func TestProcOvermount_getProcRoot(t *testing.T) {
 	privateProcMount := canFsOpen() && !testingForcePrivateProcRootOpenTree(nil)
-	testProcOvermount(t, doGetProcRoot, privateProcMount)
+	testProcOvermount(t, getProcRoot, privateProcMount)
 }
 
-func TestProcOvermount_doGetProcRoot_Mocked(t *testing.T) {
+func TestProcOvermount_getProcRoot_Mocked(t *testing.T) {
 	if !hasNewMountAPI() {
 		t.Skip("test requires fsopen/open_tree support")
 	}
 	testForceGetProcRoot(t, func(t *testing.T, _ bool) {
 		privateProcMount := canFsOpen() && !testingForcePrivateProcRootOpenTree(nil)
-		testProcOvermount(t, doGetProcRoot, privateProcMount)
+		testProcOvermount(t, getProcRoot, privateProcMount)
 	})
 }