Prevent symlink-following chown in vcap-writable directories

src/bpm/runc/adapter/adapter.go

@@ -90,11 +90,13 @@ func (a *RuncAdapter) CreateJobPrerequisites(
 			continue
 		}
 
-		fi, err := os.Stat(vol.Path)
+		fi, err := os.Lstat(vol.Path)
 		if os.IsNotExist(err) {
 			dirsToCreate = append(dirsToCreate, vol.Path)
 		} else if err != nil {
 			return nil, nil, err
+		} else if fi.Mode()&os.ModeSymlink != 0 {
+			return nil, nil, fmt.Errorf("refusing to configure additional volume at symlink %s", vol.Path)
 		} else if fi.IsDir() && fi.Mode() != 0700 {
 			if err := os.Chmod(vol.Path, 0700); err != nil {
 				return nil, nil, err
@@ -168,17 +170,12 @@ func createDirs(dirs []string, user specs.User) error {
 }
 
 func createDirFor(path string, uid, gid int) error {
-	err := os.MkdirAll(path, 0700)
-	if err != nil {
-		return err
-	}
-
-	return os.Chown(path, uid, gid)
+	return safeio.MkdirAllChown(path, uid, gid)
 }
 
 func chownPaths(paths []string, user specs.User) error {
 	for _, path := range paths {
-		err := os.Chown(path, int(user.UID), int(user.GID))
+		err := safeio.Lchown(path, int(user.UID), int(user.GID))
 		if err != nil {
 			return err
 		}

src/bpm/runc/adapter/adapter_test.go

@@ -208,6 +208,35 @@ var _ = Describe("RuncAdapter", func() {
 			})
 		})
 
+		Context("when an additional volume provided is a symlink", func() {
+			var (
+				sentinelFile string
+				symlinkPath  string
+			)
+
+			BeforeEach(func() {
+				sentinelFile = filepath.Join(systemRoot, "sentinel.target")
+				Expect(os.WriteFile(sentinelFile, []byte("protected"), 0600)).To(Succeed())
+
+				symlinkPath = filepath.Join(systemRoot, "symlink-vol")
+				Expect(os.Symlink(sentinelFile, symlinkPath)).To(Succeed())
+
+				procCfg.AdditionalVolumes = append(procCfg.AdditionalVolumes, config.Volume{
+					Path: symlinkPath,
+				})
+			})
+
+			It("refuses to configure the volume and does not modify the target", func() {
+				_, _, err := runcAdapter.CreateJobPrerequisites(bpmCfg, procCfg, user)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring("refusing to configure additional volume at symlink"))
+
+				contents, readErr := os.ReadFile(sentinelFile)
+				Expect(readErr).NotTo(HaveOccurred())
+				Expect(string(contents)).To(Equal("protected"))
+			})
+		})
+
 		Context("when a volume should be mounted only", func() {
 			BeforeEach(func() {
 				procCfg.AdditionalVolumes = append(procCfg.AdditionalVolumes, config.Volume{
@@ -363,6 +392,131 @@ var _ = Describe("RuncAdapter", func() {
 			})
 		})
 
+		Context("when a job directory is a symlink to an existing directory", func() {
+			var sentinelDir string
+
+			BeforeEach(func() {
+				sentinelDir = filepath.Join(systemRoot, "sentinel-dir")
+				Expect(os.MkdirAll(sentinelDir, 0755)).To(Succeed())
+
+				// Plant a symlink at the temp directory pointing to sentinel
+				tempDirParent := filepath.Dir(bpmCfg.TempDir().External())
+				Expect(os.MkdirAll(tempDirParent, 0700)).To(Succeed())
+				Expect(os.Symlink(sentinelDir, bpmCfg.TempDir().External())).To(Succeed())
+			})
+
+			It("refuses to follow the symlink and does not alter the target", func() {
+				_, _, err := runcAdapter.CreateJobPrerequisites(bpmCfg, procCfg, user)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring("refusing to create directory through symlink"))
+
+				info, statErr := os.Lstat(sentinelDir)
+				Expect(statErr).NotTo(HaveOccurred())
+				Expect(info.Mode() & os.ModePerm).To(Equal(os.FileMode(0755)))
+			})
+		})
+
+		// TNZGOV-18900 (CWE-59): CreateJobPrerequisites runs as root on the host.
+		// Several of the paths it chowns live in directories that are bind-mounted
+		// writable into the container and owned by vcap, so a compromised container
+		// can pre-plant a symlink and redirect root's chown onto an arbitrary host
+		// file — a container escape. These tests reproduce the escape against the
+		// real entry point and assert the host target's ownership is never changed.
+		//
+		// The escape is only observable when the chown can actually change an owner,
+		// which requires root; the whole suite already runs as root in CI (other
+		// cases assert Uid == 200). We skip explicitly when not root so a rootless
+		// run reports "skipped" rather than a misleading pass.
+		Describe("SECURITY: symlink-following chown (TNZGOV-18900)", func() {
+			var hostTarget, hostSentinel string
+
+			ownerUID := func(path string) uint32 {
+				info, err := os.Stat(path)
+				Expect(err).NotTo(HaveOccurred())
+				return info.Sys().(*syscall.Stat_t).Uid
+			}
+
+			BeforeEach(func() {
+				if os.Getuid() != 0 {
+					Skip("must run as root to observe the ownership change that constitutes the escape")
+				}
+
+				// Stand-in for a sensitive host location (e.g. /etc) that lives
+				// OUTSIDE the job tree and is owned by root.
+				hostTarget = filepath.Join(systemRoot, "host-sensitive")
+				Expect(os.MkdirAll(hostTarget, 0700)).To(Succeed())
+				hostSentinel = filepath.Join(hostTarget, "shadow")
+				Expect(os.WriteFile(hostSentinel, []byte("root:!locked:"), 0600)).To(Succeed())
+
+				Expect(ownerUID(hostTarget)).To(Equal(uint32(0)))
+			})
+
+			Context("when the attacker plants a symlink at a created job directory (leaf)", func() {
+				BeforeEach(func() {
+					// vcap owns the job data dir inside the container, so it can
+					// replace TempDir with a symlink to the host target.
+					tempDirParent := filepath.Dir(bpmCfg.TempDir().External())
+					Expect(os.MkdirAll(tempDirParent, 0700)).To(Succeed())
+					Expect(os.Symlink(hostTarget, bpmCfg.TempDir().External())).To(Succeed())
+				})
+
+				It("refuses to follow it and does not chown the host target to vcap", func() {
+					_, _, err := runcAdapter.CreateJobPrerequisites(bpmCfg, procCfg, user)
+					Expect(ownerUID(hostTarget)).To(Equal(uint32(0)), "host target was chowned to vcap — CONTAINER ESCAPE")
+					Expect(err).To(HaveOccurred())
+					Expect(err.Error()).To(ContainSubstring("refusing to create directory through symlink"))
+
+					contents, readErr := os.ReadFile(hostSentinel)
+					Expect(readErr).NotTo(HaveOccurred())
+					Expect(string(contents)).To(Equal("root:!locked:"))
+				})
+			})
+
+			Context("when the attacker plants a symlink at an additional volume (leaf)", func() {
+				BeforeEach(func() {
+					volParent := filepath.Join(systemRoot, "vcap-writable")
+					Expect(os.MkdirAll(volParent, 0770)).To(Succeed())
+					volPath := filepath.Join(volParent, "vol")
+					Expect(os.Symlink(hostTarget, volPath)).To(Succeed())
+
+					procCfg.AdditionalVolumes = append(procCfg.AdditionalVolumes, config.Volume{Path: volPath})
+				})
+
+				It("refuses to configure it and does not chown the host target to vcap", func() {
+					_, _, err := runcAdapter.CreateJobPrerequisites(bpmCfg, procCfg, user)
+					Expect(ownerUID(hostTarget)).To(Equal(uint32(0)), "host target was chowned to vcap — CONTAINER ESCAPE")
+					Expect(err).To(HaveOccurred())
+					Expect(err.Error()).To(ContainSubstring("refusing to configure additional volume at symlink"))
+
+				})
+			})
+
+			Context("when the attacker plants a symlink at an intermediate component of an additional volume", func() {
+				BeforeEach(func() {
+					// A nested volume path below a vcap-writable directory. The
+					// attacker controls an intermediate component, not the leaf —
+					// the case a leaf-only guard misses.
+					volParent := filepath.Join(systemRoot, "vcap-writable")
+					Expect(os.MkdirAll(volParent, 0770)).To(Succeed())
+					Expect(os.Symlink(hostTarget, filepath.Join(volParent, "sub"))).To(Succeed())
+
+					procCfg.AdditionalVolumes = append(procCfg.AdditionalVolumes, config.Volume{
+						Path: filepath.Join(volParent, "sub", "leaf"),
+					})
+				})
+
+				It("refuses to follow the intermediate symlink and does not create or chown under the target", func() {
+					_, _, err := runcAdapter.CreateJobPrerequisites(bpmCfg, procCfg, user)
+					Expect(err).To(HaveOccurred())
+					Expect(err.Error()).To(ContainSubstring("refusing to create directory through symlink"))
+
+					Expect(ownerUID(hostTarget)).To(Equal(uint32(0)), "host target was chowned to vcap — CONTAINER ESCAPE")
+					_, statErr := os.Lstat(filepath.Join(hostTarget, "leaf"))
+					Expect(os.IsNotExist(statErr)).To(BeTrue(), "leaf was created under the host target through the intermediate symlink")
+				})
+			})
+		})
+
 		Context("when the user requests an ephemeral disk", func() {
 			BeforeEach(func() {
 				procCfg.EphemeralDisk = true

src/bpm/safeio/safeio.go

@@ -15,15 +15,19 @@
 
 // Package safeio provides helpers for performing privileged file operations
 // against paths that live in directories writable by less-privileged users
-// (e.g. job log directories). The helpers use O_NOFOLLOW + fchown so that a
-// pre-planted symlink at the leaf path cannot redirect the operation onto an
-// arbitrary host file.
+// (e.g. job log directories). The helpers use O_NOFOLLOW, Lstat, and lchown
+// so that a pre-planted symlink cannot redirect the operation onto an
+// arbitrary host file. MkdirAllChown checks every component of the path;
+// OpenAppendChown and Lchown operate on an existing parent and so guard the
+// leaf only.
 package safeio
 
 import (
 	"errors"
 	"fmt"
 	"os"
+	"path/filepath"
+	"strings"
 
 	"golang.org/x/sys/unix"
 )
@@ -54,3 +58,80 @@ func OpenAppendChown(path string, uid, gid int, perm os.FileMode) (*os.File, err
 
 	return f, nil
 }
+
+// MkdirAllChown creates a directory (and any parent directories) with 0700
+// permissions and chowns the leaf directory to the specified uid and gid.
+//
+// The path is walked one component at a time from the filesystem root, and
+// every component is opened with O_NOFOLLOW. A symlink anywhere in the path —
+// not just at the leaf — is refused. Checking only the leaf is not sufficient:
+// an attacker who can plant a symlink at an intermediate component redirects
+// the whole subtree, so MkdirAll would create the leaf under the symlink's
+// target and the subsequent chown would hand that target to uid/gid.
+//
+// The final chown is issued against the open directory descriptor (fchown(2)),
+// so the path cannot be swapped between the walk and the ownership change.
+//
+// Because every component is checked, a legitimate symlink in the path prefix
+// is rejected too. Callers must pass paths whose ancestors are real
+// directories.
+func MkdirAllChown(path string, uid, gid int) error {
+	if !filepath.IsAbs(path) {
+		return fmt.Errorf("refusing to create directory at relative path %s", path)
+	}
+
+	// O_PATH would be enough to walk, but it makes O_NOFOLLOW return a
+	// descriptor to the symlink itself rather than ELOOP, so use a real
+	// directory open instead.
+	parent, err := unix.Open("/", unix.O_RDONLY|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
+	if err != nil {
+		return fmt.Errorf("open /: %w", err)
+	}
+	defer func() { _ = unix.Close(parent) }() //nolint:errcheck
+
+	for _, component := range strings.Split(strings.Trim(filepath.Clean(path), "/"), "/") {
+		if err := unix.Mkdirat(parent, component, 0700); err != nil && !errors.Is(err, unix.EEXIST) {
+			return fmt.Errorf("mkdir %s: %w", path, err)
+		}
+
+		// Classify the component before opening it. Errno alone is not
+		// portable here: with O_DIRECTORY set, a symlink reports ENOTDIR on
+		// some platforms and ELOOP on others.
+		var st unix.Stat_t
+		if err := unix.Fstatat(parent, component, &st, unix.AT_SYMLINK_NOFOLLOW); err != nil {
+			return fmt.Errorf("stat %s: %w", path, err)
+		}
+		switch st.Mode & unix.S_IFMT {
+		case unix.S_IFLNK:
+			return fmt.Errorf("refusing to create directory through symlink at %s", path)
+		case unix.S_IFDIR:
+		default:
+			return fmt.Errorf("path %s exists and is not a directory", path)
+		}
+
+		// O_NOFOLLOW closes the window between the check above and the open.
+		child, err := unix.Openat(parent, component, unix.O_RDONLY|unix.O_DIRECTORY|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0)
+		if err != nil {
+			return fmt.Errorf("open %s: %w", path, err)
+		}
+
+		_ = unix.Close(parent) //nolint:errcheck
+		parent = child
+	}
+
+	return unix.Fchown(parent, uid, gid)
+}
+
+// Lchown changes the ownership of the named file or directory without following
+// symbolic links. If the path is a symlink, it refuses to chown and returns an error.
+func Lchown(path string, uid, gid int) error {
+	fi, err := os.Lstat(path)
+	if err != nil {
+		return err
+	}
+	if fi.Mode()&os.ModeSymlink != 0 {
+		return fmt.Errorf("refusing to chown symlink at %s", path)
+	}
+
+	return os.Lchown(path, uid, gid)
+}