merge #70 into cyphar/filepath-securejoin:main

Aleksa Sarai (3):
  pathrs-lite: rename procfs impl to purego suffix
  pathrs-lite: use black-box testing for API
  pathrs-lite: move purego implementation into subpackage

LGTMs: cyphar

Author: cyphar

pathrs-lite/doc.go

@@ -11,4 +11,6 @@
 
 // Package pathrs (pathrs-lite) is a less complete pure Go implementation of
 // some of the APIs provided by [libpathrs].
+//
+// [libpathrs]: https://github.com/cyphar/libpathrs
 package pathrs

pathrs-lite/internal/gopathrs/doc.go

@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: MPL-2.0
+
+//go:build linux
+
+// Copyright (C) 2024-2025 Aleksa Sarai <[email protected]>
+// Copyright (C) 2024-2025 SUSE LLC
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+// Package gopathrs is a less complete pure Go implementation of some of the
+// APIs provided by [libpathrs].
+//
+// [libpathrs]: https://github.com/cyphar/libpathrs
+package gopathrs

pathrs-lite/lookup_linux.go renamed to pathrs-lite/internal/gopathrs/lookup_linux.go

@@ -9,7 +9,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
-package pathrs
+package gopathrs
 
 import (
 	"errors"
@@ -166,11 +166,11 @@ func (s *symlinkStack) PopTopSymlink() (*os.File, string, bool) {
 	return tailEntry.dir, tailEntry.remainingPath, true
 }
 
-// partialLookupInRoot tries to lookup as much of the request path as possible
+// PartialLookupInRoot tries to lookup as much of the request path as possible
 // within the provided root (a-la RESOLVE_IN_ROOT) and opens the final existing
 // component of the requested path, returning a file handle to the final
 // existing component and a string containing the remaining path components.
-func partialLookupInRoot(root fd.Fd, unsafePath string) (*os.File, string, error) {
+func PartialLookupInRoot(root fd.Fd, unsafePath string) (*os.File, string, error) {
 	return lookupInRoot(root, unsafePath, true)
 }
 

pathrs-lite/lookup_linux_test.go renamed to pathrs-lite/internal/gopathrs/lookup_linux_test.go

@@ -9,7 +9,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
-package pathrs
+package gopathrs
 
 import (
 	"errors"
@@ -312,7 +312,7 @@ func tRunWrapper(t *testing.T) testutils.TRunFunc {
 func TestPartialLookupInRoot(t *testing.T) {
 	testutils.WithWithoutOpenat2(true, tRunWrapper(t), func(ti testutils.TestingT) {
 		t := ti.(*testing.T) //nolint:forcetypeassert // guaranteed to be true and in test code
-		testPartialLookup(t, partialLookupInRoot)
+		testPartialLookup(t, PartialLookupInRoot)
 	})
 }
 
@@ -325,7 +325,7 @@ func TestPartialLookupInRoot_BadInode(t *testing.T) {
 
 	testutils.WithWithoutOpenat2(true, tRunWrapper(t), func(ti testutils.TestingT) {
 		t := ti.(*testing.T) //nolint:forcetypeassert // guaranteed to be true and in test code
-		partialLookupFn := partialLookupInRoot
+		partialLookupFn := PartialLookupInRoot
 
 		tree := []string{
 			// Make sure we don't open "bad" inodes.
@@ -384,7 +384,7 @@ func newRacingLookupMeta(pauseCh chan struct{}) *racingLookupMeta {
 func (m *racingLookupMeta) checkPartialLookup(t *testing.T, rootDir fd.Fd, unsafePath string, skipErrs []error, allowedResults []lookupResult) {
 	// Similar to checkPartialLookup, but with extra logic for
 	// handling the lookup stopping partly through the lookup.
-	handle, remainingPath, err := partialLookupInRoot(rootDir, unsafePath)
+	handle, remainingPath, err := PartialLookupInRoot(rootDir, unsafePath)
 	var (
 		handleName string
 		realPath   string
@@ -427,7 +427,7 @@ func (m *racingLookupMeta) checkPartialLookup(t *testing.T, rootDir fd.Fd, unsaf
 	if realPath != handleName {
 		// It's possible for handle.Name() to be wrong because while it was
 		// correct when it was set, it might not match if the path was swapped
-		// afterwards (for both openat2 and partialLookupInRoot).
+		// afterwards (for both openat2 and PartialLookupInRoot).
 		m.badNameCount++
 	}
 
@@ -571,7 +571,7 @@ func TestPartialLookup_RacingRename(t *testing.T) {
 				// because there was a moment when this directory was inside
 				// the root, and the attacker moved it outside the root.
 				//
-				// Neither openat2 nor partialLookupInRoot will allow us to
+				// Neither openat2 nor PartialLookupInRoot will allow us to
 				// walk into ".." in this case (escaping the root), and we
 				// would catch that if it did happen.
 				lookupResult{handlePath: "../outsideroot", remainingPath: "c/d/e", fileType: unix.S_IFDIR},

pathrs-lite/mkdir_linux.go renamed to pathrs-lite/internal/gopathrs/mkdir_linux.go

@@ -9,7 +9,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
-package pathrs
+package gopathrs
 
 import (
 	"errors"
@@ -23,9 +23,12 @@ import (
 	"github.com/cyphar/filepath-securejoin/pathrs-lite/internal/fd"
 	"github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat"
 	"github.com/cyphar/filepath-securejoin/pathrs-lite/internal/linux"
+	"github.com/cyphar/filepath-securejoin/pathrs-lite/internal/procfs"
 )
 
-var errInvalidMode = errors.New("invalid permission mode")
+// ErrInvalidMode is returned from [MkdirAll] when the requested mode is
+// invalid.
+var ErrInvalidMode = errors.New("invalid permission mode")
 
 // modePermExt is like os.ModePerm except that it also includes the set[ug]id
 // and sticky bits.
@@ -45,11 +48,11 @@ func toUnixMode(mode os.FileMode) (uint32, error) {
 	}
 	// We don't allow file type bits.
 	if mode&os.ModeType != 0 {
-		return 0, fmt.Errorf("%w %+.3o (%s): type bits not permitted", errInvalidMode, mode, mode)
+		return 0, fmt.Errorf("%w %+.3o (%s): type bits not permitted", ErrInvalidMode, mode, mode)
 	}
 	// We don't allow other unknown modes.
 	if mode&^modePermExt != 0 || sysMode&unix.S_IFMT != 0 {
-		return 0, fmt.Errorf("%w %+.3o (%s): unknown mode bits", errInvalidMode, mode, mode)
+		return 0, fmt.Errorf("%w %+.3o (%s): unknown mode bits", ErrInvalidMode, mode, mode)
 	}
 	return sysMode, nil
 }
@@ -84,11 +87,11 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode os.FileMode) (_ *os.F
 	// users it seems more prudent to return an error so users notice that
 	// these bits will not be set.
 	if unixMode&^0o1777 != 0 {
-		return nil, fmt.Errorf("%w for mkdir %+.3o: suid and sgid are ignored by mkdir", errInvalidMode, mode)
+		return nil, fmt.Errorf("%w for mkdir %+.3o: suid and sgid are ignored by mkdir", ErrInvalidMode, mode)
 	}
 
 	// Try to open as much of the path as possible.
-	currentDir, remainingPath, err := partialLookupInRoot(root, unsafePath)
+	currentDir, remainingPath, err := PartialLookupInRoot(root, unsafePath)
 	defer func() {
 		if Err != nil {
 			_ = currentDir.Close()
@@ -117,7 +120,7 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode os.FileMode) (_ *os.F
 	// Re-open the path to match the O_DIRECTORY reopen loop later (so that we
 	// always return a non-O_PATH handle). We also check that we actually got a
 	// directory.
-	if reopenDir, err := Reopen(currentDir, unix.O_DIRECTORY|unix.O_CLOEXEC); errors.Is(err, unix.ENOTDIR) {
+	if reopenDir, err := procfs.ReopenFd(currentDir, unix.O_DIRECTORY|unix.O_CLOEXEC); errors.Is(err, unix.ENOTDIR) {
 		return nil, fmt.Errorf("cannot create subdirectories in %q: %w", currentDir.Name(), unix.ENOTDIR)
 	} else if err != nil {
 		return nil, fmt.Errorf("re-opening handle to %q: %w", currentDir.Name(), err)
@@ -207,40 +210,3 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode os.FileMode) (_ *os.F
 	}
 	return currentDir, nil
 }
-
-// MkdirAll is a race-safe alternative to the [os.MkdirAll] function,
-// where the new directory is guaranteed to be within the root directory (if an
-// attacker can move directories from inside the root to outside the root, the
-// created directory tree might be outside of the root but the key constraint
-// is that at no point will we walk outside of the directory tree we are
-// creating).
-//
-// Effectively, MkdirAll(root, unsafePath, mode) is equivalent to
-//
-//	path, _ := securejoin.SecureJoin(root, unsafePath)
-//	err := os.MkdirAll(path, mode)
-//
-// But is much safer. The above implementation is unsafe because if an attacker
-// can modify the filesystem tree between [SecureJoin] and [os.MkdirAll], it is
-// possible for MkdirAll to resolve unsafe symlink components and create
-// directories outside of the root.
-//
-// If you plan to open the directory after you have created it or want to use
-// an open directory handle as the root, you should use [MkdirAllHandle] instead.
-// This function is a wrapper around [MkdirAllHandle].
-//
-// [SecureJoin]: https://pkg.go.dev/github.com/cyphar/filepath-securejoin#SecureJoin
-func MkdirAll(root, unsafePath string, mode os.FileMode) error {
-	rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
-	if err != nil {
-		return err
-	}
-	defer rootDir.Close() //nolint:errcheck // close failures aren't critical here
-
-	f, err := MkdirAllHandle(rootDir, unsafePath, mode)
-	if err != nil {
-		return err
-	}
-	_ = f.Close()
-	return nil
-}

pathrs-lite/internal/gopathrs/mkdir_linux_test.go

@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: MPL-2.0
+
+//go:build linux
+
+// Copyright (C) 2024-2025 Aleksa Sarai <[email protected]>
+// Copyright (C) 2024-2025 SUSE LLC
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+package gopathrs_test
+
+import (
+	"fmt"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/sys/unix"
+
+	"github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gopathrs"
+)
+
+func TestMkdirAllHandle_InvalidMode(t *testing.T) { //nolint:revive // underscores are more readable for test helpers
+	for _, test := range []struct {
+		mode        os.FileMode
+		expectedErr error
+	}{
+		// unix.S_IS* bits are invalid.
+		{unix.S_ISUID | 0o777, gopathrs.ErrInvalidMode},
+		{unix.S_ISGID | 0o777, gopathrs.ErrInvalidMode},
+		{unix.S_ISVTX | 0o777, gopathrs.ErrInvalidMode},
+		{unix.S_ISUID | unix.S_ISGID | unix.S_ISVTX | 0o777, gopathrs.ErrInvalidMode},
+		// unix.S_IFMT bits are also invalid.
+		{unix.S_IFDIR | 0o777, gopathrs.ErrInvalidMode},
+		{unix.S_IFREG | 0o777, gopathrs.ErrInvalidMode},
+		{unix.S_IFIFO | 0o777, gopathrs.ErrInvalidMode},
+		// os.FileType bits are also invalid.
+		{os.ModeDir | 0o777, gopathrs.ErrInvalidMode},
+		{os.ModeNamedPipe | 0o777, gopathrs.ErrInvalidMode},
+		{os.ModeIrregular | 0o777, gopathrs.ErrInvalidMode},
+		// suid/sgid bits are silently ignored by mkdirat and so we return an
+		// error explicitly.
+		{os.ModeSetuid | 0o777, gopathrs.ErrInvalidMode},
+		{os.ModeSetgid | 0o777, gopathrs.ErrInvalidMode},
+		{os.ModeSetuid | os.ModeSetgid | os.ModeSticky | 0o777, gopathrs.ErrInvalidMode},
+		// Proper sticky bit should work.
+		{os.ModeSticky | 0o777, nil},
+		// Regular mode bits.
+		{0o777, nil},
+		{0o711, nil},
+	} {
+		test := test // copy iterator
+		t.Run(fmt.Sprintf("%s.%.3o", test.mode, test.mode), func(t *testing.T) {
+			root := t.TempDir()
+			rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
+			require.NoError(t, err, "open root")
+			defer rootDir.Close() //nolint:errcheck // test code
+
+			handle, err := gopathrs.MkdirAllHandle(rootDir, "a/b/c", test.mode)
+			require.ErrorIsf(t, err, test.expectedErr, "mkdirall %.3o (%s)", test.mode, test.mode)
+			if test.expectedErr == nil {
+				assert.NotNil(t, handle, "returned handle should be non-nil")
+				_ = handle.Close()
+			} else {
+				assert.Nil(t, handle, "returned handle should be nil")
+			}
+		})
+	}
+}

pathrs-lite/internal/gopathrs/open_linux.go

@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: MPL-2.0
+
+//go:build linux
+
+// Copyright (C) 2024-2025 Aleksa Sarai <[email protected]>
+// Copyright (C) 2024-2025 SUSE LLC
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+package gopathrs
+
+import (
+	"os"
+)
+
+// OpenatInRoot is equivalent to [OpenInRoot], except that the root is provided
+// using an *[os.File] handle, to ensure that the correct root directory is used.
+func OpenatInRoot(root *os.File, unsafePath string) (*os.File, error) {
+	handle, err := completeLookupInRoot(root, unsafePath)
+	if err != nil {
+		return nil, &os.PathError{Op: "securejoin.OpenInRoot", Path: unsafePath, Err: err}
+	}
+	return handle, nil
+}

pathrs-lite/openat2_linux.go renamed to pathrs-lite/internal/gopathrs/openat2_linux.go

@@ -9,7 +9,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
-package pathrs
+package gopathrs
 
 import (
 	"errors"

pathrs-lite/mkdir.go

@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: MPL-2.0
+
+//go:build linux
+
+// Copyright (C) 2024-2025 Aleksa Sarai <[email protected]>
+// Copyright (C) 2024-2025 SUSE LLC
+//
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+package pathrs
+
+import (
+	"os"
+
+	"golang.org/x/sys/unix"
+)
+
+// MkdirAll is a race-safe alternative to the [os.MkdirAll] function,
+// where the new directory is guaranteed to be within the root directory (if an
+// attacker can move directories from inside the root to outside the root, the
+// created directory tree might be outside of the root but the key constraint
+// is that at no point will we walk outside of the directory tree we are
+// creating).
+//
+// Effectively, MkdirAll(root, unsafePath, mode) is equivalent to
+//
+//	path, _ := securejoin.SecureJoin(root, unsafePath)
+//	err := os.MkdirAll(path, mode)
+//
+// But is much safer. The above implementation is unsafe because if an attacker
+// can modify the filesystem tree between [SecureJoin] and [os.MkdirAll], it is
+// possible for MkdirAll to resolve unsafe symlink components and create
+// directories outside of the root.
+//
+// If you plan to open the directory after you have created it or want to use
+// an open directory handle as the root, you should use [MkdirAllHandle] instead.
+// This function is a wrapper around [MkdirAllHandle].
+//
+// [SecureJoin]: https://pkg.go.dev/github.com/cyphar/filepath-securejoin#SecureJoin
+func MkdirAll(root, unsafePath string, mode os.FileMode) error {
+	rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
+	if err != nil {
+		return err
+	}
+	defer rootDir.Close() //nolint:errcheck // close failures aren't critical here
+
+	f, err := MkdirAllHandle(rootDir, unsafePath, mode)
+	if err != nil {
+		return err
+	}
+	_ = f.Close()
+	return nil
+}