merge #55 into cyphar/filepath-securejoin:main

Aleksa Sarai (3):
  proc: add ProcPid helper
  proc: a few more lookup tests
  proc: remove safety warning comment

LGTMs: cyphar

Author: cyphar

CHANGELOG.md

@@ -23,6 +23,21 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
      magiclinks) using this API. At the moment `filepath-securejoin` does not
      support this feature (but [libpathrs][] does).
 
+   * `ProcPid` is very similar to `ProcThreadSelf`, except it lets you get
+     handles to subpaths of other processes. Unlike `ProcThreadSelf`, it is not
+     necessary to lock the goroutine to the current thread and so no
+     `ProcThreadSelfCloser` closure will be returned.
+
+     Please note that it is possible for you to be unable to access processes
+     in certain configurations (when using `fsopen(2)`, the internal procfs
+     mount will have `subset=pids,hidepids=traceable` mount options applied,
+     which will hide many other processes and any non-process-related top-level
+     files), but `ProcPid(os.Getpid(), ...)` (to access the current
+     thread-group leader) should always work.
+
+     Also note that if the target process dies, the handle you received from
+     `ProcPid` may start returning errors or blank data when you operate on it.
+
    * `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

procfs_linux.go

@@ -216,12 +216,6 @@ var errUnsafeProcfs = errors.New("unsafe procfs detected")
 // [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) {
 	// 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
@@ -285,6 +279,34 @@ func ProcThreadSelf(subpath string) (*os.File, ProcThreadSelfCloser, error) {
 	return procThreadSelf(nil, subpath)
 }
 
+// ProcPid returns a handle to /proc/$pid/<subpath> (pid can be a pid or tid).
+// You should not use this for the current thread, as special handling is
+// needed for /proc/thread-self (or /proc/self/task/<tid>) when dealing with
+// goroutine scheduling -- use [ProcThreadSelf] instead. This is mainly
+// intended for usage when operating on other processes.
+//
+// You should not try to operate on the top-level /proc handle (such as by
+// setting subpath to "../foo"). This will not work at all on non-openat2
+// systems, and when using an internal fsopen-based handle, the mount will have
+// subset=pids and hidepid=traceable set (which will restrict what PIDs can be
+// accessed with this API, as well as removing any non-PID procfs files).
+func ProcPid(pid int, subpath string) (*os.File, error) {
+	procRoot, err := getProcRoot()
+	if err != nil {
+		return nil, err
+	}
+
+	handle, err := procfsLookupInRoot(procRoot, strconv.Itoa(pid)+"/"+subpath)
+	if err != nil {
+		// TODO: Once we bump the minimum Go version to 1.20, we can use
+		// multiple %w verbs for this wrapping. For now we need to use a
+		// compatibility shim for older Go versions.
+		// err = fmt.Errorf("%w: %w", errUnsafeProcfs, err)
+		return nil, wrapBaseError(err, errUnsafeProcfs)
+	}
+	return handle, nil
+}
+
 // 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.

procfs_linux_test.go

@@ -252,16 +252,146 @@ func TestProcOvermountSubdir_ProcThreadSelf(t *testing.T) {
 	})
 }
 
+// isFsopenRoot returns whether the internal procfs handle is an fsopen root.
+func isFsopenRoot(t *testing.T) bool {
+	procRoot, err := getProcRoot()
+	require.NoError(t, err)
+	return procRoot.Name() == "fsmount:fscontext:proc"
+}
+
 // 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
+		t.Run("stat", func(t *testing.T) {
+			handle, closer, err := ProcThreadSelf("stat")
+			require.NoError(t, err, "ProcThreadSelf(stat)")
+			require.NotNil(t, handle, "ProcThreadSelf(stat) handle")
+			require.NotNil(t, closer, "ProcThreadSelf(stat) closer")
+			defer closer()
+			defer handle.Close() //nolint:errcheck // test code
+
+			realPath, err := ProcSelfFdReadlink(handle)
+			require.NoError(t, err)
+			wantPath := fmt.Sprintf("/%d/task/%d/stat", os.Getpid(), unix.Gettid())
+			if !isFsopenRoot(t) {
+				// The /proc prefix is only present when not using fsopen.
+				wantPath = "/proc" + wantPath
+			}
+			assert.Equal(t, wantPath, realPath, "final handle path")
+		})
+
+		t.Run("abspath", func(t *testing.T) {
+			handle, closer, err := ProcThreadSelf("/stat")
+			require.NoError(t, err, "ProcThreadSelf(/stat)")
+			require.NotNil(t, handle, "ProcThreadSelf(/stat) handle")
+			require.NotNil(t, closer, "ProcThreadSelf(/stat) closer")
+			defer closer()
+			defer handle.Close() //nolint:errcheck // test code
+
+			realPath, err := ProcSelfFdReadlink(handle)
+			require.NoError(t, err)
+			wantPath := fmt.Sprintf("/%d/task/%d/stat", os.Getpid(), unix.Gettid())
+			if !isFsopenRoot(t) {
+				// The /proc prefix is only present when not using fsopen.
+				wantPath = "/proc" + wantPath
+			}
+			assert.Equal(t, wantPath, realPath, "final handle path")
+		})
+
+		t.Run("wacky-abspath", func(t *testing.T) {
+			handle, closer, err := ProcThreadSelf("////./////stat")
+			require.NoError(t, err, "ProcThreadSelf(////./////stat)")
+			require.NotNil(t, handle, "ProcThreadSelf(////./////stat) handle")
+			require.NotNil(t, closer, "ProcThreadSelf(////./////stat) closer")
+			defer closer()
+			defer handle.Close() //nolint:errcheck // test code
+
+			realPath, err := ProcSelfFdReadlink(handle)
+			require.NoError(t, err)
+			wantPath := fmt.Sprintf("/%d/task/%d/stat", os.Getpid(), unix.Gettid())
+			if !isFsopenRoot(t) {
+				// The /proc prefix is only present when not using fsopen.
+				wantPath = "/proc" + wantPath
+			}
+			assert.Equal(t, wantPath, realPath, "final handle path")
+		})
+
+		t.Run("dotdot", func(t *testing.T) {
+			handle, closer, err := ProcThreadSelf("../../../../../../../../..")
+			require.Error(t, err, "ProcThreadSelf(../...)")
+			require.Nil(t, handle, "ProcThreadSelf(../...) handle")
+			require.Nil(t, closer, "ProcThreadSelf(../...) closer")
+		})
+
+		t.Run("wacky-dotdot", func(t *testing.T) {
+			handle, closer, err := ProcThreadSelf("/../../../../../../../../..")
+			require.Error(t, err, "ProcThreadSelf(/../...)")
+			require.Nil(t, handle, "ProcThreadSelf(/../...) handle")
+			require.Nil(t, closer, "ProcThreadSelf(/../...) closer")
+		})
+	})
+}
+
+func TestProcPid(t *testing.T) {
+	withWithoutOpenat2(t, true, func(t *testing.T) {
+		t.Run("pid1-stat", func(t *testing.T) {
+			handle, err := ProcPid(1, "stat")
+			require.NoError(t, err, "ProcPid(1, stat)")
+			require.NotNil(t, handle, "ProcPid(1, stat) handle")
+
+			realPath, err := ProcSelfFdReadlink(handle)
+			require.NoError(t, err)
+			wantPath := "/1/stat"
+			if !isFsopenRoot(t) {
+				// The /proc prefix is only present when not using fsopen.
+				wantPath = "/proc" + wantPath
+			}
+			assert.Equal(t, wantPath, realPath, "final handle path")
+		})
+
+		t.Run("pid1-stat-abspath", func(t *testing.T) {
+			handle, err := ProcPid(1, "/stat")
+			require.NoError(t, err, "ProcPid(1, /stat)")
+			require.NotNil(t, handle, "ProcPid(1, /stat) handle")
+
+			realPath, err := ProcSelfFdReadlink(handle)
+			require.NoError(t, err)
+			wantPath := "/1/stat"
+			if !isFsopenRoot(t) {
+				// The /proc prefix is only present when not using fsopen.
+				wantPath = "/proc" + wantPath
+			}
+			assert.Equal(t, wantPath, realPath, "final handle path")
+		})
+
+		t.Run("pid1-stat-wacky-abspath", func(t *testing.T) {
+			handle, err := ProcPid(1, "////.////stat")
+			require.NoError(t, err, "ProcPid(1, ////.////stat)")
+			require.NotNil(t, handle, "ProcPid(1, ////.////stat) handle")
+
+			realPath, err := ProcSelfFdReadlink(handle)
+			require.NoError(t, err)
+			wantPath := "/1/stat"
+			if !isFsopenRoot(t) {
+				// The /proc prefix is only present when not using fsopen.
+				wantPath = "/proc" + wantPath
+			}
+			assert.Equal(t, wantPath, realPath, "final handle path")
+		})
+
+		t.Run("dotdot", func(t *testing.T) {
+			handle, err := ProcPid(1, "../../../../../../../../..")
+			require.Error(t, err, "ProcPid(1, ../...)")
+			require.Nil(t, handle, "ProcPid(1, ../...) handle")
+		})
+
+		t.Run("wacky-dotdot", func(t *testing.T) {
+			handle, err := ProcPid(1, "/../../../../../../../../..")
+			require.Error(t, err, "ProcPid(1, /../...)")
+			require.Nil(t, handle, "ProcPid(1, /../...) handle")
+		})
 	})
 }