From a258c4fdbaadc8436be79eaed6348e8e5db8c3bf Mon Sep 17 00:00:00 2001
From: Aleksa Sarai <cyphar@cyphar.com>
Date: Fri, 28 Jun 2024 16:51:48 +1000
Subject: [PATCH] procfs: add tests for our safe /proc helpers
This ensures we test all of the fallbacks even if we don't use them in
practice when doing MkdirAll in our test suite. This raises the test
coverage to 77.7%.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
.github/workflows/ci.yml | 4 ++
procfs_linux.go | 18 ++---
procfs_linux_test.go | 144 +++++++++++++++++++++++++++++++++++++++
util_linux_test.go | 6 ++
4 files changed, 164 insertions(+), 8 deletions(-)
create mode 100644 procfs_linux_test.go
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 67a117b..1e819e3 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -35,3 +35,7 @@ jobs:
go-version: ${{ matrix.go-version }}
- name: unit tests
run: go test -v -cover ./...
+ # TODO: Merge the code coverage stats from both runs...
+ - name: unit tests (root)
+ if: ${{ ! startsWith(matrix.os, 'windows-') }}
+ run: sudo go test -v -cover ./...
diff --git a/procfs_linux.go b/procfs_linux.go
index cafb806..b2c54b5 100644
--- a/procfs_linux.go
+++ b/procfs_linux.go
@@ -214,12 +214,7 @@ type procThreadSelfCloser func()
//
// This is similar to ProcThreadSelf from runc, but with extra hardening
// applied and using *os.File.
-func procThreadSelf(subpath string) (_ *os.File, _ procThreadSelfCloser, Err error) {
- procRoot, err := getProcRoot()
- if err != nil {
- return nil, nil, err
- }
-
+func procThreadSelf(procRoot *os.File, subpath string) (_ *os.File, _ procThreadSelfCloser, Err error) {
haveProcThreadSelfOnce.Do(func() {
// If the kernel doesn't support thread-self, it doesn't matter which
// /proc handle we use.
@@ -256,7 +251,10 @@ func procThreadSelf(subpath string) (_ *os.File, _ procThreadSelfCloser, Err err
}
// Grab the handle.
- var handle *os.File
+ var (
+ handle *os.File
+ err error
+ )
if hasOpenat2() {
// We prefer being able to use RESOLVE_NO_XDEV if we can, to be
// absolutely sure we are operating on a clean /proc handle that
@@ -293,7 +291,11 @@ func procThreadSelf(subpath string) (_ *os.File, _ procThreadSelfCloser, Err err
}
func rawProcSelfFdReadlink(fd int) (string, error) {
- procSelfFd, closer, err := procThreadSelf("fd/")
+ procRoot, err := getProcRoot()
+ if err != nil {
+ return "", err
+ }
+ procSelfFd, closer, err := procThreadSelf(procRoot, "fd/")
if err != nil {
return "", fmt.Errorf("get safe /proc/thread-self/fd handle: %w", err)
}
diff --git a/procfs_linux_test.go b/procfs_linux_test.go
new file mode 100644
index 0000000..3fde230
--- /dev/null
+++ b/procfs_linux_test.go
@@ -0,0 +1,144 @@
+//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 (
+ "errors"
+ "os"
+ "path"
+ "runtime"
+ "testing"
+
+ "github.com/stretchr/testify/require"
+ "golang.org/x/sys/unix"
+)
+
+func setupProcOvermount(t *testing.T) {
+ requireRoot(t)
+
+ // Lock our thread because we need to create a custom mount namespace. Each
+ // test run is run in its own goroutine (this is not _explicitly_
+ // guaranteed by Go but t.FailNow() uses Goexit, which means it has to be
+ // true in practice) so locking the test to this thread means the other
+ // tests will run on different goroutines.
+ //
+ // There is no UnlockOSThread() here, to ensure that the Go runtime will
+ // kill this thread once this goroutine returns (ensuring no other
+ // goroutines run in this context).
+ runtime.LockOSThread()
+
+ // New mount namespace (we are multi-threaded with a shared fs so we need
+ // CLONE_FS to split us from the other threads in the Go process).
+ err := unix.Unshare(unix.CLONE_FS | unix.CLONE_NEWNS)
+ require.Nil(t, err, "new mount namespace")
+
+ // Private /.
+ err = unix.Mount("", "/", "", unix.MS_PRIVATE|unix.MS_REC, "")
+ require.Nil(t, err)
+
+ // Figure out what /proc/thread-self is for this system.
+ procRoot, err := unsafeHostProcRoot()
+ require.Nil(t, err, "get real host /proc during setup")
+ defer procRoot.Close()
+
+ procThreadSelf, closer, err := procThreadSelf(procRoot, ".")
+ require.Nil(t, err, "get real host /proc/thread-self during setup")
+ procThreadSelfPath := procThreadSelf.Name()
+ _ = procThreadSelf.Close()
+ closer() // LockOSThread stacks, so we can call this safely.
+
+ // Create some overmounts on /proc.
+ for _, mount := range []struct {
+ source, targetSubPath, fsType string
+ flags uintptr
+ }{
+ // A tmpfs on top of /proc/thread-self/fdinfo to check whether
+ // verifyProcRoot() works on old kernels.
+ {"tmpfs", "fdinfo", "tmpfs", 0},
+ // A bind-mount of noop-write real procfs file on top of
+ // /proc/thread-self/attr/current so we can test whether
+ // verifyProcRoot() works for the file case.
+ //
+ // We don't use procThreadSelf for files in filepath-securejoin, but
+ // this is to test the runc-equivalent behaviour for when this logic is
+ // moved to libpathrs.
+ {"/proc/self/sched", "attr/current", "", unix.MS_BIND},
+ // TODO: Add a test for bind-mounting on top of magic-links. We can't
+ // detect this at the moment (and maybe in the fd case this will
+ // be blocked by the kernel...) but it'd be nice to have the
+ // problem written down.
+ // TODO: Add a test for mounting on top of /proc/self or
+ // /proc/thread-self. This should be detected with openat2.
+ } {
+ target := path.Join(procThreadSelfPath, mount.targetSubPath)
+ err := unix.Mount(mount.source, target, mount.fsType, mount.flags, "")
+ require.Nilf(t, err, "mount(%s, [%s/]%s, %s, 0x%x)", mount.source, procThreadSelfPath, mount.targetSubPath, mount.fsType, mount.flags)
+ }
+}
+
+type procRootFunc func() (*os.File, error)
+
+func testProcThreadSelf(t *testing.T, procRoot *os.File, subpath string, expectErr bool) {
+ handle, closer, err := procThreadSelf(procRoot, subpath)
+ if expectErr {
+ if err == nil || !errors.Is(err, errUnsafeProcfs) {
+ t.Errorf("should have detected /proc/thread-self/%s overmount: %v", subpath, err)
+ }
+ } else {
+ require.Nil(t, err)
+ _ = handle.Close()
+ closer() // LockOSThread stacks, so we can call this safely.
+ }
+}
+
+func testProcOvermount(t *testing.T, procRootFn procRootFunc, expectOvermounts bool) {
+ setupProcOvermount(t)
+
+ procRoot, err := procRootFn()
+ require.Nil(t, err)
+ defer procRoot.Close()
+
+ // We expect to always detect tmpfs overmounts if we have a /proc with
+ // overmounts.
+ detectFdinfo := expectOvermounts
+ // We only expect to detect procfs bind-mounts if there are /proc
+ // overmounts and we have openat2.
+ detectAttrCurrent := expectOvermounts && hasOpenat2()
+
+ testProcThreadSelf(t, procRoot, "fdinfo", detectFdinfo)
+ testProcThreadSelf(t, procRoot, "attr/current", detectAttrCurrent)
+}
+
+func TestProcOvermount_unsafeHostProcRoot(t *testing.T) {
+ withWithoutOpenat2(t, func(t *testing.T) {
+ // If we use the host /proc directly, we should see overmounts.
+ testProcOvermount(t, unsafeHostProcRoot, true)
+ })
+}
+
+func TestProcOvermount_newPrivateProcMount(t *testing.T) {
+ if !hasPrivateMounts() {
+ t.Skip("test requires fsopen/open_tree support")
+ }
+ withWithoutOpenat2(t, func(t *testing.T) {
+ // If we create our own procfs, the overmounts shouldn't appear.
+ testProcOvermount(t, newPrivateProcMount, false)
+ })
+}
+
+func TestProcOvermount_clonePrivateProcMount(t *testing.T) {
+ if !hasPrivateMounts() {
+ t.Skip("test requires fsopen/open_tree support")
+ }
+ withWithoutOpenat2(t, func(t *testing.T) {
+ // If we use open_tree(2), we don't use AT_RECURSIVE when running in
+ // this test (because the overmounts are not locked mounts) and so we
+ // don't expect to see overmounts.
+ // TODO: Explicitly test AT_RECURSIVE
+ testProcOvermount(t, clonePrivateProcMount, false)
+ })
+}
diff --git a/util_linux_test.go b/util_linux_test.go
index 34f6d65..4482010 100644
--- a/util_linux_test.go
+++ b/util_linux_test.go
@@ -16,6 +16,12 @@ import (
"github.com/stretchr/testify/require"
)
+func requireRoot(t *testing.T) {
+ if os.Geteuid() != 0 {
+ t.Skip("test requires root")
+ }
+}
+
func withWithoutOpenat2(t *testing.T, testFn func(t *testing.T)) {
for _, useOpenat2 := range []bool{true, false} {
useOpenat2 := useOpenat2 // copy iterator