diff --git a/src/bpm/runc/adapter/adapter.go b/src/bpm/runc/adapter/adapter.go index 6e640c9a..03cdf0f1 100644 --- a/src/bpm/runc/adapter/adapter.go +++ b/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 } diff --git a/src/bpm/runc/adapter/adapter_test.go b/src/bpm/runc/adapter/adapter_test.go index 11adb329..da905abb 100644 --- a/src/bpm/runc/adapter/adapter_test.go +++ b/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 diff --git a/src/bpm/safeio/safeio.go b/src/bpm/safeio/safeio.go index b337ae2d..ea907b7e 100644 --- a/src/bpm/safeio/safeio.go +++ b/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) +} diff --git a/src/bpm/safeio/safeio_test.go b/src/bpm/safeio/safeio_test.go index 909f2fcc..30ffc69b 100644 --- a/src/bpm/safeio/safeio_test.go +++ b/src/bpm/safeio/safeio_test.go @@ -140,3 +140,197 @@ var _ = Describe("OpenAppendChown", func() { }) }) }) + +var _ = Describe("MkdirAllChown", func() { + var ( + tmpDir string + uid, gid int + ) + + BeforeEach(func() { + var err error + tmpDir, err = os.MkdirTemp("", "safeio-mkdir") + Expect(err).NotTo(HaveOccurred()) + + // MkdirAllChown rejects symlinks in every path component, and on some + // platforms the temp root itself sits behind one. + tmpDir, err = filepath.EvalSymlinks(tmpDir) + Expect(err).NotTo(HaveOccurred()) + + uid = os.Getuid() + gid = os.Getgid() + }) + + AfterEach(func() { + Expect(os.RemoveAll(tmpDir)).To(Succeed()) + }) + + Context("when the directory does not exist", func() { + It("creates the directory with 0700 permissions and correct ownership", func() { + dirPath := filepath.Join(tmpDir, "sub", "newdir") + + err := safeio.MkdirAllChown(dirPath, uid, gid) + Expect(err).NotTo(HaveOccurred()) + + info, err := os.Stat(dirPath) + Expect(err).NotTo(HaveOccurred()) + Expect(info.IsDir()).To(BeTrue()) + Expect(info.Mode() & os.ModePerm).To(Equal(os.FileMode(0700))) + Expect(info.Sys().(*syscall.Stat_t).Uid).To(Equal(uint32(uid))) + Expect(info.Sys().(*syscall.Stat_t).Gid).To(Equal(uint32(gid))) + }) + }) + + Context("when the directory already exists", func() { + It("ensures ownership without error", func() { + dirPath := filepath.Join(tmpDir, "existingdir") + Expect(os.Mkdir(dirPath, 0700)).To(Succeed()) + + err := safeio.MkdirAllChown(dirPath, uid, gid) + Expect(err).NotTo(HaveOccurred()) + + info, err := os.Stat(dirPath) + Expect(err).NotTo(HaveOccurred()) + Expect(info.IsDir()).To(BeTrue()) + }) + }) + + Context("when the path is a symlink to an existing directory", func() { + var ( + targetDir string + linkPath string + ) + + BeforeEach(func() { + targetDir = filepath.Join(tmpDir, "target-dir") + linkPath = filepath.Join(tmpDir, "symlink-dir") + + Expect(os.Mkdir(targetDir, 0755)).To(Succeed()) + Expect(os.Symlink(targetDir, linkPath)).To(Succeed()) + }) + + It("refuses to follow the symlink and returns an error", func() { + err := safeio.MkdirAllChown(linkPath, uid, gid) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("refusing to create directory through symlink")) + + // Target directory should retain original permissions + info, err := os.Lstat(targetDir) + Expect(err).NotTo(HaveOccurred()) + Expect(info.Mode() & os.ModePerm).To(Equal(os.FileMode(0755))) + }) + }) + + Context("when the path is a symlink to a non-existent path", func() { + var ( + targetDir string + linkPath string + ) + + BeforeEach(func() { + targetDir = filepath.Join(tmpDir, "nonexistent-target") + linkPath = filepath.Join(tmpDir, "dangling-link") + + Expect(os.Symlink(targetDir, linkPath)).To(Succeed()) + }) + + It("refuses to follow the symlink and does not create the target", func() { + err := safeio.MkdirAllChown(linkPath, uid, gid) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("refusing to create directory through symlink")) + + _, err = os.Lstat(targetDir) + Expect(os.IsNotExist(err)).To(BeTrue()) + }) + }) + + Context("when the path exists as a regular file", func() { + It("returns an error", func() { + filePath := filepath.Join(tmpDir, "regular-file") + Expect(os.WriteFile(filePath, []byte("data"), 0600)).To(Succeed()) + + err := safeio.MkdirAllChown(filePath, uid, gid) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("exists and is not a directory")) + }) + }) + + Context("when an intermediate component is a symlink", func() { + var protectedDir string + + BeforeEach(func() { + protectedDir = filepath.Join(tmpDir, "protected") + Expect(os.MkdirAll(protectedDir, 0755)).To(Succeed()) + + writable := filepath.Join(tmpDir, "writable") + Expect(os.MkdirAll(writable, 0777)).To(Succeed()) + Expect(os.Symlink(protectedDir, filepath.Join(writable, "a"))).To(Succeed()) + }) + + It("refuses to follow the symlink and does not create the leaf under the target", func() { + err := safeio.MkdirAllChown(filepath.Join(tmpDir, "writable", "a", "b"), uid, gid) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("refusing to create directory through symlink")) + + _, statErr := os.Lstat(filepath.Join(protectedDir, "b")) + Expect(os.IsNotExist(statErr)).To(BeTrue(), "leaf was created through the intermediate symlink") + }) + }) + + Context("when the path is relative", func() { + It("returns an error", func() { + err := safeio.MkdirAllChown("relative/path", uid, gid) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("relative path")) + }) + }) +}) + +var _ = Describe("Lchown", func() { + var ( + tmpDir string + uid, gid int + ) + + BeforeEach(func() { + var err error + tmpDir, err = os.MkdirTemp("", "safeio-lchown") + Expect(err).NotTo(HaveOccurred()) + + uid = os.Getuid() + gid = os.Getgid() + }) + + AfterEach(func() { + Expect(os.RemoveAll(tmpDir)).To(Succeed()) + }) + + Context("when the path is a regular file", func() { + It("successfully changes ownership", func() { + filePath := filepath.Join(tmpDir, "file.txt") + Expect(os.WriteFile(filePath, []byte("test"), 0600)).To(Succeed()) + + err := safeio.Lchown(filePath, uid, gid) + Expect(err).NotTo(HaveOccurred()) + + info, err := os.Stat(filePath) + Expect(err).NotTo(HaveOccurred()) + Expect(info.Sys().(*syscall.Stat_t).Uid).To(Equal(uint32(uid))) + Expect(info.Sys().(*syscall.Stat_t).Gid).To(Equal(uint32(gid))) + }) + }) + + Context("when the path is a symlink", func() { + It("refuses to chown and returns an error without altering the target", func() { + target := filepath.Join(tmpDir, "target.txt") + link := filepath.Join(tmpDir, "link.txt") + + Expect(os.WriteFile(target, []byte("protected"), 0600)).To(Succeed()) + Expect(os.Symlink(target, link)).To(Succeed()) + + err := safeio.Lchown(link, uid, gid) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("refusing to chown symlink")) + }) + }) +})