Merge pull request #4985 from cyphar/hallucinated-paths

pathrs: add "hallucination" helpers for SecureJoin magic

Author: lifubang

internal/pathrs/mkdirall.go

@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: Apache-2.0
+/*
+ * Copyright (C) 2024-2025 Aleksa Sarai <[email protected]>
+ * Copyright (C) 2024-2025 SUSE LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package pathrs
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+)
+
+// MkdirAllParentInRoot is like [MkdirAllInRoot] except that it only creates
+// the parent directory of the target path, returning the trailing component so
+// the caller has more flexibility around constructing the final inode.
+//
+// Callers need to be very careful operating on the trailing path, as trivial
+// mistakes like following symlinks can cause security bugs. Most people
+// should probably just use [MkdirAllInRoot] or [CreateInRoot].
+func MkdirAllParentInRoot(root, unsafePath string, mode os.FileMode) (*os.File, string, error) {
+	// MkdirAllInRoot also does hallucinateUnsafePath, but we need to do it
+	// here first because when we split unsafePath into (dir, file) components
+	// we want to be doing so with the hallucinated path (so that trailing
+	// dangling symlinks are treated correctly).
+	unsafePath, err := hallucinateUnsafePath(root, unsafePath)
+	if err != nil {
+		return nil, "", fmt.Errorf("failed to construct hallucinated target path: %w", err)
+	}
+
+	dirPath, filename := filepath.Split(unsafePath)
+	if filepath.Join("/", filename) == "/" {
+		return nil, "", fmt.Errorf("create parent dir in root subpath %q has bad trailing component %q", unsafePath, filename)
+	}
+
+	dirFd, err := MkdirAllInRoot(root, dirPath, mode)
+	return dirFd, filename, err
+}

internal/pathrs/mkdirall_pathrslite.go

@@ -21,14 +21,13 @@ package pathrs
 import (
 	"fmt"
 	"os"
-	"path/filepath"
 
 	"github.com/cyphar/filepath-securejoin/pathrs-lite"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
 )
 
-// MkdirAllInRootOpen attempts to make
+// MkdirAllInRoot attempts to make
 //
 //	path, _ := securejoin.SecureJoin(root, unsafePath)
 //	os.MkdirAll(path, mode)
@@ -49,19 +48,10 @@ import (
 // handling if unsafePath has already been scoped within the rootfs (this is
 // needed for a lot of runc callers and fixing this would require reworking a
 // lot of path logic).
-func MkdirAllInRootOpen(root, unsafePath string, mode os.FileMode) (*os.File, error) {
-	// If the path is already "within" the root, get the path relative to the
-	// root and use that as the unsafe path. This is necessary because a lot of
-	// MkdirAllInRootOpen callers have already done SecureJoin, and refactoring
-	// all of them to stop using these SecureJoin'd paths would require a fair
-	// amount of work.
-	// TODO(cyphar): Do the refactor to libpathrs once it's ready.
-	if IsLexicallyInRoot(root, unsafePath) {
-		subPath, err := filepath.Rel(root, unsafePath)
-		if err != nil {
-			return nil, err
-		}
-		unsafePath = subPath
+func MkdirAllInRoot(root, unsafePath string, mode os.FileMode) (*os.File, error) {
+	unsafePath, err := hallucinateUnsafePath(root, unsafePath)
+	if err != nil {
+		return nil, fmt.Errorf("failed to construct hallucinated target path: %w", err)
 	}
 
 	// Check for any silly mode bits.
@@ -87,13 +77,3 @@ func MkdirAllInRootOpen(root, unsafePath string, mode os.FileMode) (*os.File, er
 		return pathrs.MkdirAllHandle(rootDir, unsafePath, mode)
 	})
 }
-
-// MkdirAllInRoot is a wrapper around MkdirAllInRootOpen which closes the
-// returned handle, for callers that don't need to use it.
-func MkdirAllInRoot(root, unsafePath string, mode os.FileMode) error {
-	f, err := MkdirAllInRootOpen(root, unsafePath, mode)
-	if err == nil {
-		_ = f.Close()
-	}
-	return err
-}

internal/pathrs/path.go

@@ -19,7 +19,11 @@
 package pathrs
 
 import (
+	"os"
+	"path/filepath"
 	"strings"
+
+	securejoin "github.com/cyphar/filepath-securejoin"
 )
 
 // IsLexicallyInRoot is shorthand for strings.HasPrefix(path+"/", root+"/"),
@@ -32,3 +36,81 @@ func IsLexicallyInRoot(root, path string) bool {
 	path = strings.TrimRight(path, "/")
 	return strings.HasPrefix(path+"/", root+"/")
 }
+
+// LexicallyCleanPath makes a path safe for use with filepath.Join. This is
+// done by not only cleaning the path, but also (if the path is relative)
+// adding a leading '/' and cleaning it (then removing the leading '/'). This
+// ensures that a path resulting from prepending another path will always
+// resolve to lexically be a subdirectory of the prefixed path. This is all
+// done lexically, so paths that include symlinks won't be safe as a result of
+// using CleanPath.
+func LexicallyCleanPath(path string) string {
+	// Deal with empty strings nicely.
+	if path == "" {
+		return ""
+	}
+
+	// Ensure that all paths are cleaned (especially problematic ones like
+	// "/../../../../../" which can cause lots of issues).
+
+	if filepath.IsAbs(path) {
+		return filepath.Clean(path)
+	}
+
+	// If the path isn't absolute, we need to do more processing to fix paths
+	// such as "../../../../<etc>/some/path". We also shouldn't convert absolute
+	// paths to relative ones.
+	path = filepath.Clean(string(os.PathSeparator) + path)
+	// This can't fail, as (by definition) all paths are relative to root.
+	path, _ = filepath.Rel(string(os.PathSeparator), path)
+
+	return path
+}
+
+// LexicallyStripRoot returns the passed path, stripping the root path if it
+// was (lexicially) inside it. Note that both passed paths will always be
+// treated as absolute, and the returned path will also always be absolute. In
+// addition, the paths are cleaned before stripping the root.
+func LexicallyStripRoot(root, path string) string {
+	// Make the paths clean and absolute.
+	root, path = LexicallyCleanPath("/"+root), LexicallyCleanPath("/"+path)
+	switch {
+	case path == root:
+		path = "/"
+	case root == "/":
+		// do nothing
+	default:
+		path = strings.TrimPrefix(path, root+"/")
+	}
+	return LexicallyCleanPath("/" + path)
+}
+
+// hallucinateUnsafePath creates a new unsafePath which has all symlinks
+// (including dangling symlinks) fully resolved and any non-existent components
+// treated as though they are real. This is effectively just a wrapper around
+// [securejoin.SecureJoin] that strips the root. This path *IS NOT* safe to use
+// as-is, you *MUST* operate on the returned path with pathrs-lite.
+//
+// The reason for this methods is that in previous runc versions, we would
+// tolerate nonsense paths with dangling symlinks as path components.
+// pathrs-lite does not support this, so instead we have to emulate this
+// behaviour by doing SecureJoin *purely to get a semi-reasonable path to use*
+// and then we use pathrs-lite to operate on the path safely.
+//
+// It would be quite difficult to emulate this in a race-free way in
+// pathrs-lite, so instead we use [securejoin.SecureJoin] to simply produce a
+// new candidate path for operations like [MkdirAllInRoot] so they can then
+// operate on the new unsafePath as if it was what the user requested.
+//
+// If unsafePath is already lexically inside root, it is stripped before
+// re-resolving it (this is done to ensure compatibility with legacy callers
+// within runc that call SecureJoin before calling into pathrs).
+func hallucinateUnsafePath(root, unsafePath string) (string, error) {
+	unsafePath = LexicallyStripRoot(root, unsafePath)
+	weirdPath, err := securejoin.SecureJoin(root, unsafePath)
+	if err != nil {
+		return "", err
+	}
+	unsafePath = LexicallyStripRoot(root, weirdPath)
+	return unsafePath, nil
+}

internal/pathrs/path_test.go

@@ -51,3 +51,70 @@ func TestIsLexicallyInRoot(t *testing.T) {
 		})
 	}
 }
+
+func TestLexicallyCleanPath(t *testing.T) {
+	path := LexicallyCleanPath("")
+	if path != "" {
+		t.Errorf("expected to receive empty string and received %s", path)
+	}
+
+	path = LexicallyCleanPath("rootfs")
+	if path != "rootfs" {
+		t.Errorf("expected to receive 'rootfs' and received %s", path)
+	}
+
+	path = LexicallyCleanPath("../../../var")
+	if path != "var" {
+		t.Errorf("expected to receive 'var' and received %s", path)
+	}
+
+	path = LexicallyCleanPath("/../../../var")
+	if path != "/var" {
+		t.Errorf("expected to receive '/var' and received %s", path)
+	}
+
+	path = LexicallyCleanPath("/foo/bar/")
+	if path != "/foo/bar" {
+		t.Errorf("expected to receive '/foo/bar' and received %s", path)
+	}
+
+	path = LexicallyCleanPath("/foo/bar/../")
+	if path != "/foo" {
+		t.Errorf("expected to receive '/foo' and received %s", path)
+	}
+}
+
+func TestLexicallyStripRoot(t *testing.T) {
+	for _, test := range []struct {
+		root, path, out string
+	}{
+		// Works with multiple components.
+		{"/a/b", "/a/b/c", "/c"},
+		{"/hello/world", "/hello/world/the/quick-brown/fox", "/the/quick-brown/fox"},
+		// '/' must be a no-op.
+		{"/", "/a/b/c", "/a/b/c"},
+		// Must be the correct order.
+		{"/a/b", "/a/c/b", "/a/c/b"},
+		// Must be at start.
+		{"/abc/def", "/foo/abc/def/bar", "/foo/abc/def/bar"},
+		// Must be a lexical parent.
+		{"/foo/bar", "/foo/barSAMECOMPONENT", "/foo/barSAMECOMPONENT"},
+		// Must only strip the root once.
+		{"/foo/bar", "/foo/bar/foo/bar/baz", "/foo/bar/baz"},
+		// Deal with .. in a fairly sane way.
+		{"/foo/bar", "/foo/bar/../baz", "/foo/baz"},
+		{"/foo/bar", "../../../../../../foo/bar/baz", "/baz"},
+		{"/foo/bar", "/../../../../../../foo/bar/baz", "/baz"},
+		{"/foo/bar/../baz", "/foo/baz/bar", "/bar"},
+		{"/foo/bar/../baz", "/foo/baz/../bar/../baz/./foo", "/foo"},
+		// All paths are made absolute before stripping.
+		{"foo/bar", "/foo/bar/baz/bee", "/baz/bee"},
+		{"/foo/bar", "foo/bar/baz/beef", "/baz/beef"},
+		{"foo/bar", "foo/bar/baz/beets", "/baz/beets"},
+	} {
+		got := LexicallyStripRoot(test.root, test.path)
+		if got != test.out {
+			t.Errorf("LexicallyStripRoot(%q, %q) -- got %q, expected %q", test.root, test.path, got, test.out)
+		}
+	}
+}

internal/pathrs/root_pathrslite.go

@@ -19,9 +19,7 @@
 package pathrs
 
 import (
-	"fmt"
 	"os"
-	"path/filepath"
 
 	"github.com/cyphar/filepath-securejoin/pathrs-lite"
 	"golang.org/x/sys/unix"
@@ -50,12 +48,7 @@ func OpenInRoot(root, subpath string, flags int) (*os.File, error) {
 // include it in the passed flags. The fileMode argument uses unix.* mode bits,
 // *not* os.FileMode.
 func CreateInRoot(root, subpath string, flags int, fileMode uint32) (*os.File, error) {
-	dir, filename := filepath.Split(subpath)
-	if filepath.Join("/", filename) == "/" {
-		return nil, fmt.Errorf("create in root subpath %q has bad trailing component %q", subpath, filename)
-	}
-
-	dirFd, err := MkdirAllInRootOpen(root, dir, 0o755)
+	dirFd, filename, err := MkdirAllParentInRoot(root, subpath, 0o755)
 	if err != nil {
 		return nil, err
 	}

libcontainer/apparmor/apparmor_linux.go

@@ -9,7 +9,6 @@ import (
 	"golang.org/x/sys/unix"
 
 	"github.com/opencontainers/runc/internal/pathrs"
-	"github.com/opencontainers/runc/libcontainer/utils"
 )
 
 var (
@@ -29,7 +28,7 @@ func isEnabled() bool {
 }
 
 func setProcAttr(attr, value string) error {
-	attr = utils.CleanPath(attr)
+	attr = pathrs.LexicallyCleanPath(attr)
 	attrSubPath := "attr/apparmor/" + attr
 	if _, err := os.Stat("/proc/self/" + attrSubPath); errors.Is(err, os.ErrNotExist) {
 		// fall back to the old convention

libcontainer/criu_linux.go

@@ -25,6 +25,7 @@ import (
 	"google.golang.org/protobuf/proto"
 
 	"github.com/opencontainers/cgroups"
+	"github.com/opencontainers/runc/internal/pathrs"
 	"github.com/opencontainers/runc/libcontainer/configs"
 	"github.com/opencontainers/runc/libcontainer/utils"
 )
@@ -555,16 +556,22 @@ func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error {
 	umounts := []string{}
 	defer func() {
 		for _, u := range umounts {
-			_ = utils.WithProcfd(c.config.Rootfs, u, func(procfd string) error {
-				if e := unix.Unmount(procfd, unix.MNT_DETACH); e != nil {
-					if e != unix.EINVAL {
+			mntFile, err := pathrs.OpenInRoot(c.config.Rootfs, u, unix.O_PATH)
+			if err != nil {
+				logrus.Warnf("Error during cleanup unmounting %s: open handle: %v", u, err)
+				continue
+			}
+			_ = utils.WithProcfdFile(mntFile, func(procfd string) error {
+				if err := unix.Unmount(procfd, unix.MNT_DETACH); err != nil {
+					if err != unix.EINVAL {
 						// Ignore EINVAL as it means 'target is not a mount point.'
 						// It probably has already been unmounted.
-						logrus.Warnf("Error during cleanup unmounting of %s (%s): %v", procfd, u, e)
+						logrus.Warnf("Error during cleanup unmounting of %s (%s): %v", procfd, u, err)
 					}
 				}
 				return nil
 			})
+			_ = mntFile.Close()
 		}
 	}()
 	// Now go through all mounts and create the required mountpoints.

libcontainer/factory_linux.go

@@ -11,10 +11,10 @@ import (
 
 	"github.com/opencontainers/cgroups"
 	"github.com/opencontainers/cgroups/manager"
+	"github.com/opencontainers/runc/internal/pathrs"
 	"github.com/opencontainers/runc/libcontainer/configs"
 	"github.com/opencontainers/runc/libcontainer/configs/validate"
 	"github.com/opencontainers/runc/libcontainer/intelrdt"
-	"github.com/opencontainers/runc/libcontainer/utils"
 )
 
 const (
@@ -211,7 +211,7 @@ func validateID(id string) error {
 
 	}
 
-	if string(os.PathSeparator)+id != utils.CleanPath(string(os.PathSeparator)+id) {
+	if string(os.PathSeparator)+id != pathrs.LexicallyCleanPath(string(os.PathSeparator)+id) {
 		return ErrInvalidID
 	}