From 6fc191449109ea14bb7d61238f24a33fe08c651f Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Fri, 18 Jul 2025 13:18:53 +1000 Subject: [PATCH] internal: move utils.MkdirAllInRoot to internal/pathrs We will have more wrappers around filepath-securejoin, and so move them to their own specific package so that we can eventually use libpathrs fairly cleanly (by swapping out the implementation). Signed-off-by: Aleksa Sarai --- internal/pathrs/doc.go | 23 ++++++ internal/pathrs/mkdirall_securejoin.go | 97 ++++++++++++++++++++++++++ internal/pathrs/path.go | 34 +++++++++ internal/pathrs/path_test.go | 53 ++++++++++++++ libcontainer/rootfs_linux.go | 13 ++-- libcontainer/utils/utils_unix.go | 83 ---------------------- 6 files changed, 214 insertions(+), 89 deletions(-) create mode 100644 internal/pathrs/doc.go create mode 100644 internal/pathrs/mkdirall_securejoin.go create mode 100644 internal/pathrs/path.go create mode 100644 internal/pathrs/path_test.go diff --git a/internal/pathrs/doc.go b/internal/pathrs/doc.go new file mode 100644 index 000000000..496ca5951 --- /dev/null +++ b/internal/pathrs/doc.go @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: Apache-2.0 +/* + * Copyright (C) 2024-2025 Aleksa Sarai + * 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 provides wrappers around filepath-securejoin to add the +// minimum set of features needed from libpathrs that are not provided by +// filepath-securejoin, with the eventual goal being that these can be used to +// ease the transition by converting them stubs when enabling libpathrs builds. +package pathrs diff --git a/internal/pathrs/mkdirall_securejoin.go b/internal/pathrs/mkdirall_securejoin.go new file mode 100644 index 000000000..7fec8d21e --- /dev/null +++ b/internal/pathrs/mkdirall_securejoin.go @@ -0,0 +1,97 @@ +// SPDX-License-Identifier: Apache-2.0 +/* + * Copyright (C) 2024-2025 Aleksa Sarai + * 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" + + securejoin "github.com/cyphar/filepath-securejoin" + "github.com/sirupsen/logrus" + "golang.org/x/sys/unix" +) + +// MkdirAllInRootOpen attempts to make +// +// path, _ := securejoin.SecureJoin(root, unsafePath) +// os.MkdirAll(path, mode) +// os.Open(path) +// +// safer against attacks where components in the path are changed between +// SecureJoin returning and MkdirAll (or Open) being called. In particular, we +// try to detect any symlink components in the path while we are doing the +// MkdirAll. +// +// NOTE: If unsafePath is a subpath of root, we assume that you have already +// called SecureJoin and so we use the provided path verbatim without resolving +// any symlinks (this is done in a way that avoids symlink-exchange races). +// This means that the path also must not contain ".." elements, otherwise an +// error will occur. +// +// This uses securejoin.MkdirAllHandle under the hood, but it has special +// 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 + } + + // Check for any silly mode bits. + if mode&^0o7777 != 0 { + return nil, fmt.Errorf("tried to include non-mode bits in MkdirAll mode: 0o%.3o", mode) + } + // Linux (and thus os.MkdirAll) silently ignores the suid and sgid bits if + // passed. While it would make sense to return an error in that case (since + // the user has asked for a mode that won't be applied), for compatibility + // reasons we have to ignore these bits. + if ignoredBits := mode &^ 0o1777; ignoredBits != 0 { + logrus.Warnf("MkdirAll called with no-op mode bits that are ignored by Linux: 0o%.3o", ignoredBits) + mode &= 0o1777 + } + + rootDir, err := os.OpenFile(root, unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + return nil, fmt.Errorf("open root handle: %w", err) + } + defer rootDir.Close() + + return securejoin.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 +} diff --git a/internal/pathrs/path.go b/internal/pathrs/path.go new file mode 100644 index 000000000..1ee7c795d --- /dev/null +++ b/internal/pathrs/path.go @@ -0,0 +1,34 @@ +// SPDX-License-Identifier: Apache-2.0 +/* + * Copyright (C) 2024-2025 Aleksa Sarai + * 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 ( + "strings" +) + +// IsLexicallyInRoot is shorthand for strings.HasPrefix(path+"/", root+"/"), +// but properly handling the case where path or root have a "/" suffix. +// +// NOTE: The return value only make sense if the path is already mostly cleaned +// (i.e., doesn't contain "..", ".", nor unneeded "/"s). +func IsLexicallyInRoot(root, path string) bool { + root = strings.TrimRight(root, "/") + path = strings.TrimRight(path, "/") + return strings.HasPrefix(path+"/", root+"/") +} diff --git a/internal/pathrs/path_test.go b/internal/pathrs/path_test.go new file mode 100644 index 000000000..19d577fba --- /dev/null +++ b/internal/pathrs/path_test.go @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: Apache-2.0 +/* + * Copyright (C) 2024-2025 Aleksa Sarai + * 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 "testing" + +func TestIsLexicallyInRoot(t *testing.T) { + for _, test := range []struct { + name string + root, path string + expected bool + }{ + {"Equal1", "/foo", "/foo", true}, + {"Equal2", "/bar/baz", "/bar/baz", true}, + {"Equal3", "/bar/baz/", "/bar/baz/", true}, + {"Root", "/", "/foo/bar", true}, + {"Root-Equal", "/", "/", true}, + {"InRoot-Basic1", "/foo/bar", "/foo/bar/baz/abcd", true}, + {"InRoot-Basic2", "/a/b/c/d", "/a/b/c/d/e/f/g/h", true}, + {"InRoot-Long", "/var/lib/docker/container/1234abcde/rootfs", "/var/lib/docker/container/1234abcde/rootfs/a/b/c", true}, + {"InRoot-TrailingSlash1", "/foo/bar/", "/foo/bar", true}, + {"InRoot-TrailingSlash2", "/foo/", "/foo/bar/baz/boop", true}, + {"NotInRoot-Basic1", "/foo", "/bar", false}, + {"NotInRoot-Basic2", "/foo", "/bar", false}, + {"NotInRoot-Basic3", "/foo/bar/baz", "/foo/boo/baz/abc", false}, + {"NotInRoot-Long", "/var/lib/docker/container/1234abcde/rootfs", "/a/b/c", false}, + {"NotInRoot-Tricky1", "/foo/bar", "/foo/bara", false}, + {"NotInRoot-Tricky2", "/foo/bar", "/foo/ba/r", false}, + } { + t.Run(test.name, func(t *testing.T) { + got := IsLexicallyInRoot(test.root, test.path) + if test.expected != got { + t.Errorf("IsLexicallyInRoot(%q, %q) = %v (expected %v)", test.root, test.path, got, test.expected) + } + }) + } +} diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index aa1f68b94..dddd709b8 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -25,6 +25,7 @@ import ( devices "github.com/opencontainers/cgroups/devices/config" "github.com/opencontainers/cgroups/fs2" "github.com/opencontainers/runc/internal/linux" + "github.com/opencontainers/runc/internal/pathrs" "github.com/opencontainers/runc/libcontainer/configs" "github.com/opencontainers/runc/libcontainer/utils" ) @@ -327,7 +328,7 @@ func mountCgroupV1(m *configs.Mount, c *mountConfig) error { // inside the tmpfs, so we don't want to resolve symlinks). subsystemPath := filepath.Join(c.root, b.Destination) subsystemName := filepath.Base(b.Destination) - if err := utils.MkdirAllInRoot(c.root, subsystemPath, 0o755); err != nil { + if err := pathrs.MkdirAllInRoot(c.root, subsystemPath, 0o755); err != nil { return err } if err := utils.WithProcfd(c.root, b.Destination, func(dstFd string) error { @@ -520,7 +521,7 @@ func createMountpoint(rootfs string, m mountEntry) (string, error) { } // Make the parent directory. destDir, destBase := filepath.Split(dest) - destDirFd, err := utils.MkdirAllInRootOpen(rootfs, destDir, 0o755) + destDirFd, err := pathrs.MkdirAllInRootOpen(rootfs, destDir, 0o755) if err != nil { return "", fmt.Errorf("make parent dir of file bind-mount: %w", err) } @@ -557,7 +558,7 @@ func createMountpoint(rootfs string, m mountEntry) (string, error) { } } - if err := utils.MkdirAllInRoot(rootfs, dest, 0o755); err != nil { + if err := pathrs.MkdirAllInRoot(rootfs, dest, 0o755); err != nil { return "", err } return dest, nil @@ -576,7 +577,7 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { // TODO: This won't be necessary once we switch to libpathrs and we can // stop all of these symlink-exchange attacks. dest := filepath.Clean(m.Destination) - if !utils.IsLexicallyInRoot(rootfs, dest) { + if !pathrs.IsLexicallyInRoot(rootfs, dest) { // Do not use securejoin as it resolves symlinks. dest = filepath.Join(rootfs, dest) } @@ -590,7 +591,7 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { } else if !fi.IsDir() { return fmt.Errorf("filesystem %q must be mounted on ordinary directory", m.Device) } - if err := utils.MkdirAllInRoot(rootfs, dest, 0o755); err != nil { + if err := pathrs.MkdirAllInRoot(rootfs, dest, 0o755); err != nil { return err } // Selinux kernels do not support labeling of /proc or /sys. @@ -956,7 +957,7 @@ func createDeviceNode(rootfs string, node *devices.Device, bind bool) error { if dest == rootfs { return fmt.Errorf("%w: mknod over rootfs", errRootfsToFile) } - if err := utils.MkdirAllInRoot(rootfs, filepath.Dir(dest), 0o755); err != nil { + if err := pathrs.MkdirAllInRoot(rootfs, filepath.Dir(dest), 0o755); err != nil { return err } if bind { diff --git a/libcontainer/utils/utils_unix.go b/libcontainer/utils/utils_unix.go index 3c32769dd..f2c1e1f2b 100644 --- a/libcontainer/utils/utils_unix.go +++ b/libcontainer/utils/utils_unix.go @@ -9,7 +9,6 @@ import ( "path/filepath" "runtime" "strconv" - "strings" "sync" _ "unsafe" // for go:linkname @@ -269,88 +268,6 @@ func ProcThreadSelfFd(fd uintptr) (string, ProcThreadSelfCloser) { return ProcThreadSelf("fd/" + strconv.FormatUint(uint64(fd), 10)) } -// IsLexicallyInRoot is shorthand for strings.HasPrefix(path+"/", root+"/"), -// but properly handling the case where path or root are "/". -// -// NOTE: The return value only make sense if the path doesn't contain "..". -func IsLexicallyInRoot(root, path string) bool { - if root != "/" { - root += "/" - } - if path != "/" { - path += "/" - } - return strings.HasPrefix(path, root) -} - -// MkdirAllInRootOpen attempts to make -// -// path, _ := securejoin.SecureJoin(root, unsafePath) -// os.MkdirAll(path, mode) -// os.Open(path) -// -// safer against attacks where components in the path are changed between -// SecureJoin returning and MkdirAll (or Open) being called. In particular, we -// try to detect any symlink components in the path while we are doing the -// MkdirAll. -// -// NOTE: If unsafePath is a subpath of root, we assume that you have already -// called SecureJoin and so we use the provided path verbatim without resolving -// any symlinks (this is done in a way that avoids symlink-exchange races). -// This means that the path also must not contain ".." elements, otherwise an -// error will occur. -// -// This uses securejoin.MkdirAllHandle under the hood, but it has special -// 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, Err 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 - } - - // Check for any silly mode bits. - if mode&^0o7777 != 0 { - return nil, fmt.Errorf("tried to include non-mode bits in MkdirAll mode: 0o%.3o", mode) - } - // Linux (and thus os.MkdirAll) silently ignores the suid and sgid bits if - // passed. While it would make sense to return an error in that case (since - // the user has asked for a mode that won't be applied), for compatibility - // reasons we have to ignore these bits. - if ignoredBits := mode &^ 0o1777; ignoredBits != 0 { - logrus.Warnf("MkdirAll called with no-op mode bits that are ignored by Linux: 0o%.3o", ignoredBits) - mode &= 0o1777 - } - - rootDir, err := os.OpenFile(root, unix.O_DIRECTORY|unix.O_CLOEXEC, 0) - if err != nil { - return nil, fmt.Errorf("open root handle: %w", err) - } - defer rootDir.Close() - - return securejoin.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 -} - // Openat is a Go-friendly openat(2) wrapper. func Openat(dir *os.File, path string, flags int, mode uint32) (*os.File, error) { dirFd := unix.AT_FDCWD