diff --git a/internal/pathrs/path.go b/internal/pathrs/path.go index 1ee7c795d..709c3ae3f 100644 --- a/internal/pathrs/path.go +++ b/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 "../../../..//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) +} diff --git a/internal/pathrs/path_test.go b/internal/pathrs/path_test.go index 19d577fba..b7fdde6b9 100644 --- a/internal/pathrs/path_test.go +++ b/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) + } + } +} diff --git a/libcontainer/apparmor/apparmor_linux.go b/libcontainer/apparmor/apparmor_linux.go index 2cde88bae..9bb4fb2cd 100644 --- a/libcontainer/apparmor/apparmor_linux.go +++ b/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 diff --git a/libcontainer/factory_linux.go b/libcontainer/factory_linux.go index b799e4a98..70d935c0d 100644 --- a/libcontainer/factory_linux.go +++ b/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 } diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 9f2af0a21..f6f58987f 100644 --- a/libcontainer/rootfs_linux.go +++ b/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 } diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go index bffd3762c..957011b45 100644 --- a/libcontainer/specconv/spec_linux.go +++ b/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) } } diff --git a/libcontainer/utils/utils.go b/libcontainer/utils/utils.go index 88291a0fe..7943d89f6 100644 --- a/libcontainer/utils/utils.go +++ b/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 "../../../..//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 diff --git a/libcontainer/utils/utils_test.go b/libcontainer/utils/utils_test.go index 4b5fd833c..5953118ef 100644 --- a/libcontainer/utils/utils_test.go +++ b/libcontainer/utils/utils_test.go @@ -70,70 +70,3 @@ func TestWriteJSON(t *testing.T) { t.Errorf("expected to write %s but was %s", expected, b.String()) } } - -func TestCleanPath(t *testing.T) { - path := CleanPath("") - if path != "" { - t.Errorf("expected to receive empty string and received %s", path) - } - - path = CleanPath("rootfs") - if path != "rootfs" { - t.Errorf("expected to receive 'rootfs' and received %s", path) - } - - path = CleanPath("../../../var") - if path != "var" { - t.Errorf("expected to receive 'var' and received %s", path) - } - - path = CleanPath("/../../../var") - if path != "/var" { - t.Errorf("expected to receive '/var' and received %s", path) - } - - path = CleanPath("/foo/bar/") - if path != "/foo/bar" { - t.Errorf("expected to receive '/foo/bar' and received %s", path) - } - - path = CleanPath("/foo/bar/../") - if path != "/foo" { - t.Errorf("expected to receive '/foo' and received %s", path) - } -} - -func TestStripRoot(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 := StripRoot(test.root, test.path) - if got != test.out { - t.Errorf("StripRoot(%q, %q) -- got %q, expected %q", test.root, test.path, got, test.out) - } - } -} diff --git a/libcontainer/utils/utils_unix.go b/libcontainer/utils/utils_unix.go index a457a09b0..37ec59939 100644 --- a/libcontainer/utils/utils_unix.go +++ b/libcontainer/utils/utils_unix.go @@ -13,10 +13,11 @@ import ( _ "unsafe" // for go:linkname securejoin "github.com/cyphar/filepath-securejoin" - "github.com/opencontainers/runc/internal/linux" - "github.com/opencontainers/runc/internal/pathrs" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" + + "github.com/opencontainers/runc/internal/linux" + "github.com/opencontainers/runc/internal/pathrs" ) var ( @@ -153,7 +154,7 @@ func NewSockPair(name string) (parent, child *os.File, err error) { // the passed closure (the file handle will be freed once the closure returns). func WithProcfd(root, unsafePath string, fn func(procfd string) error) error { // Remove the root then forcefully resolve inside the root. - unsafePath = StripRoot(root, unsafePath) + unsafePath = pathrs.LexicallyStripRoot(root, unsafePath) fullPath, err := securejoin.SecureJoin(root, unsafePath) if err != nil { return fmt.Errorf("resolving path inside rootfs failed: %w", err)