rootfs: switch to fd-based handling of mountpoint targets

An attacker could race with us during mount configuration in order to
trick us into mounting over an unexpected path. This would bypass
checkProcMount() and would allow for security profiles to be left
unapplied by mounting over /proc/self/attr/... (or even more serious
outcomes such as killing the entire system by tricking runc into writing
strings to /proc/sysrq-trigger).

This is a larger issue with our current mount infrastructure, and the
ideal solution would be to rewrite it all to be fd-based (which would
also allow us to support the "new" mount API, which also avoids a bunch
of other issues with mount(8)). However, such a rewrite is not really
workable as a security fix, so this patch is a bit of a compromise
approach to fix the issue while also moving us a bit towards that
eventual end-goal.

The core issue in CVE-2025-52881 is that we currently use the (insecure)
SecureJoin to re-resolve mountpoint target paths multiple times during
mounting. Rather than generating a string from createMountpoint(), we
instead open an *os.File handle to the target mountpoint directly and
then operate on that handle. This will make it easier to remove
utils.WithProcfd() and rework mountViaFds() in the future.

The only real issue we need to work around is that we need to re-open
the mount target after doing the mount in order to get a handle to the
mountpoint -- pathrs.Reopen() doesn't work in this case (it just
re-opens the inode under the mountpoint) so we need to do a naive
re-open using the full path. Note that if we used move_mount(2) this
wouldn't be a problem because we would have a handle to the mountpoint
itself.

Note that this is still somewhat of a temporary solution -- ideally
mountViaFds would use *os.File directly to let us avoid some other
issues with using bare /proc/... paths, as well as also letting us more
easily use the new mount API on modern kernels.

Fixes: GHSA-cgrx-mc8f-2prm CVE-2025-52881
Co-developed-by: lifubang <[email protected]>
Signed-off-by: Aleksa Sarai <[email protected]>

Author: cyphar

internal/pathrs/root_pathrslite.go

@@ -0,0 +1,71 @@
+// 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"
+
+	"github.com/cyphar/filepath-securejoin/pathrs-lite"
+	"golang.org/x/sys/unix"
+
+	"github.com/opencontainers/runc/internal/linux"
+)
+
+// OpenInRoot opens the given path inside the root with the provided flags. It
+// is effectively shorthand for [securejoin.OpenInRoot] followed by
+// [securejoin.Reopen].
+func OpenInRoot(root, subpath string, flags int) (*os.File, error) {
+	handle, err := pathrs.OpenInRoot(root, subpath)
+	if err != nil {
+		return nil, err
+	}
+	defer handle.Close()
+	return pathrs.Reopen(handle, flags)
+}
+
+// CreateInRoot creates a new file inside a root (as well as any missing parent
+// directories) and returns a handle to said file. This effectively has
+// open(O_CREAT|O_NOFOLLOW) semantics. If you want the creation to use O_EXCL,
+// 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)
+	if err != nil {
+		return nil, err
+	}
+	defer dirFd.Close()
+
+	// We know that the filename does not have any "/" components, and that
+	// dirFd is inside the root. O_NOFOLLOW will stop us from following
+	// trailing symlinks, so this is safe to do. libpathrs's Root::create_file
+	// works the same way.
+	flags |= unix.O_CREAT | unix.O_NOFOLLOW
+	fd, err := linux.Openat(int(dirFd.Fd()), filename, flags, fileMode)
+	if err != nil {
+		return nil, err
+	}
+	return os.NewFile(uintptr(fd), root+"/"+subpath), nil
+}

libcontainer/criu_linux.go

@@ -582,8 +582,12 @@ func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error {
 		if isOnTmpfs(m.Destination, mounts) {
 			continue
 		}
-		if _, err := createMountpoint(c.config.Rootfs, mountEntry{Mount: m}); err != nil {
-			return fmt.Errorf("create criu restore mountpoint for %s mount: %w", m.Destination, err)
+		me := mountEntry{Mount: m}
+		if err := me.createOpenMountpoint(c.config.Rootfs); err != nil {
+			return fmt.Errorf("create criu restore mountpoint for %s mount: %w", me.Destination, err)
+		}
+		if me.dstFile != nil {
+			defer me.dstFile.Close()
 		}
 		// If the mount point is a bind mount, we need to mount
 		// it now so that runc can create the necessary mount
@@ -595,13 +599,20 @@ func (c *Container) prepareCriuRestoreMounts(mounts []*configs.Mount) error {
 		// because during initial container creation mounts are
 		// set up in the order they are configured.
 		if m.Device == "bind" {
-			if err := utils.WithProcfd(c.config.Rootfs, m.Destination, func(dstFd string) error {
+			if err := utils.WithProcfdFile(me.dstFile, func(dstFd string) error {
 				return mountViaFds(m.Source, nil, m.Destination, dstFd, "", unix.MS_BIND|unix.MS_REC, "")
 			}); err != nil {
 				return err
 			}
 			umounts = append(umounts, m.Destination)
 		}
+		if me.dstFile != nil {
+			// As this is being done in a loop, the defer earlier will be
+			// delayed until all mountpoints are handled -- for a config with
+			// many mountpoints this could result in a lot of open files. So we
+			// opportunistically close the file as well as deferring it.
+			_ = me.dstFile.Close()
+		}
 	}
 	return nil
 }