libcontainer: move CleanPath and StripRoot to internal/pathrs

These helpers will be needed for the compatibility code added in future
patches in this series, but because "internal/pathrs" is imported by
"libcontainer/utils" we need to move them so that we can avoid circular
dependencies.

Because the old functions were in a non-internal package it is possible
some downstreams use them, so add some wrappers but mark them as
deprecated.

Signed-off-by: Aleksa Sarai <[email protected]>

Author: cyphar

internal/pathrs/path.go

@@ -19,6 +19,8 @@
 package pathrs
 
 import (
+	"os"
+	"path/filepath"
 	"strings"
 )
 
@@ -32,3 +34,51 @@ 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)
+}

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)
+		}
+	}
+}

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/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
 	}
 

libcontainer/rootfs_linux.go

@@ -91,7 +91,7 @@ func (m mountEntry) srcStatfs() (*unix.Statfs_t, error) {
 // needsSetupDev returns true if /dev needs to be set up.
 func needsSetupDev(config *configs.Config) bool {
 	for _, m := range config.Mounts {
-		if m.Device == "bind" && utils.CleanPath(m.Destination) == "/dev" {
+		if m.Device == "bind" && pathrs.LexicallyCleanPath(m.Destination) == "/dev" {
 			return false
 		}
 	}
@@ -257,7 +257,7 @@ func finalizeRootfs(config *configs.Config) (err error) {
 		if m.Flags&unix.MS_RDONLY != unix.MS_RDONLY {
 			continue
 		}
-		if m.Device == "tmpfs" || utils.CleanPath(m.Destination) == "/dev" {
+		if m.Device == "tmpfs" || pathrs.LexicallyCleanPath(m.Destination) == "/dev" {
 			if err := remountReadonly(m); err != nil {
 				return err
 			}
@@ -506,7 +506,7 @@ func statfsToMountFlags(st unix.Statfs_t) int {
 var errRootfsToFile = errors.New("config tries to change rootfs to file")
 
 func (m *mountEntry) createOpenMountpoint(rootfs string) (Err error) {
-	unsafePath := utils.StripRoot(rootfs, m.Destination)
+	unsafePath := pathrs.LexicallyStripRoot(rootfs, m.Destination)
 	dstFile, err := pathrs.OpenInRoot(rootfs, unsafePath, unix.O_PATH)
 	defer func() {
 		if dstFile != nil && Err != nil {
@@ -553,7 +553,7 @@ func (m *mountEntry) createOpenMountpoint(rootfs string) (Err error) {
 		if err != nil {
 			return err
 		}
-		unsafePath = utils.StripRoot(rootfs, newUnsafePath)
+		unsafePath = pathrs.LexicallyStripRoot(rootfs, newUnsafePath)
 
 		if dstIsFile {
 			dstFile, err = pathrs.CreateInRoot(rootfs, unsafePath, unix.O_CREAT|unix.O_EXCL|unix.O_NOFOLLOW, 0o644)
@@ -952,7 +952,7 @@ func createDevices(config *configs.Config) error {
 	for _, node := range config.Devices {
 
 		// The /dev/ptmx device is setup by setupPtmx()
-		if utils.CleanPath(node.Path) == "/dev/ptmx" {
+		if pathrs.LexicallyCleanPath(node.Path) == "/dev/ptmx" {
 			continue
 		}
 
@@ -1392,7 +1392,7 @@ func reopenAfterMount(rootfs string, f *os.File, flags int) (_ *os.File, Err err
 	if !pathrs.IsLexicallyInRoot(rootfs, fullPath) {
 		return nil, fmt.Errorf("mountpoint %q is outside of rootfs %q", fullPath, rootfs)
 	}
-	unsafePath := utils.StripRoot(rootfs, fullPath)
+	unsafePath := pathrs.LexicallyStripRoot(rootfs, fullPath)
 	reopened, err := pathrs.OpenInRoot(rootfs, unsafePath, flags)
 	if err != nil {
 		return nil, fmt.Errorf("re-open mountpoint %q: %w", unsafePath, err)
@@ -1432,7 +1432,7 @@ func (m *mountEntry) mountPropagate(rootfs string, mountLabel string) error {
 	// operations on it. We need to set up files in "/dev", and other tmpfs
 	// mounts may need to be chmod-ed after mounting. These mounts will be
 	// remounted ro later in finalizeRootfs(), if necessary.
-	if m.Device == "tmpfs" || utils.CleanPath(m.Destination) == "/dev" {
+	if m.Device == "tmpfs" || pathrs.LexicallyCleanPath(m.Destination) == "/dev" {
 		flags &= ^unix.MS_RDONLY
 	}
 

libcontainer/specconv/spec_linux.go

@@ -16,17 +16,17 @@ import (
 
 	systemdDbus "github.com/coreos/go-systemd/v22/dbus"
 	dbus "github.com/godbus/dbus/v5"
+	"github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/sirupsen/logrus"
+	"golang.org/x/sys/unix"
+
 	"github.com/opencontainers/cgroups"
 	devices "github.com/opencontainers/cgroups/devices/config"
 	"github.com/opencontainers/runc/internal/linux"
+	"github.com/opencontainers/runc/internal/pathrs"
 	"github.com/opencontainers/runc/libcontainer/configs"
 	"github.com/opencontainers/runc/libcontainer/internal/userns"
 	"github.com/opencontainers/runc/libcontainer/seccomp"
-	libcontainerUtils "github.com/opencontainers/runc/libcontainer/utils"
-	"github.com/opencontainers/runtime-spec/specs-go"
-	"github.com/sirupsen/logrus"
-
-	"golang.org/x/sys/unix"
 )
 
 var (
@@ -802,7 +802,7 @@ func CreateCgroupConfig(opts *CreateOpts, defaultDevs []*devices.Device) (*cgrou
 		if useSystemdCgroup {
 			myCgroupPath = spec.Linux.CgroupsPath
 		} else {
-			myCgroupPath = libcontainerUtils.CleanPath(spec.Linux.CgroupsPath)
+			myCgroupPath = pathrs.LexicallyCleanPath(spec.Linux.CgroupsPath)
 		}
 	}
 

libcontainer/utils/utils.go

@@ -4,11 +4,11 @@ package utils
 import (
 	"encoding/json"
 	"io"
-	"os"
-	"path/filepath"
 	"strings"
 
 	"golang.org/x/sys/unix"
+
+	"github.com/opencontainers/runc/internal/pathrs"
 )
 
 const (
@@ -37,53 +37,6 @@ func WriteJSON(w io.Writer, v any) error {
 	return err
 }
 
-// CleanPath 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 CleanPath(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
-}
-
-// StripRoot 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 StripRoot(root, path string) string {
-	// Make the paths clean and absolute.
-	root, path = CleanPath("/"+root), CleanPath("/"+path)
-	switch {
-	case path == root:
-		path = "/"
-	case root == "/":
-		// do nothing
-	default:
-		path = strings.TrimPrefix(path, root+"/")
-	}
-	return CleanPath("/" + path)
-}
-
 // SearchLabels searches through a list of key=value pairs for a given key,
 // returning its value, and the binary flag telling whether the key exist.
 func SearchLabels(labels []string, key string) (string, bool) {
@@ -114,3 +67,23 @@ func Annotations(labels []string) (bundle string, userAnnotations map[string]str
 	}
 	return bundle, userAnnotations
 }
+
+// CleanPath 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.
+//
+// Deprecated: This function has been moved to internal/pathrs and this wrapper
+// will be removed in runc 1.5.
+var CleanPath = pathrs.LexicallyCleanPath
+
+// StripRoot 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.
+//
+// Deprecated: This function has been moved to internal/pathrs and this wrapper
+// will be removed in runc 1.5.
+var StripRoot = pathrs.LexicallyStripRoot