From dbdbaf80b2684557b0b7c58de2da5e33fb4f2795 Mon Sep 17 00:00:00 2001 From: Colin Shield Date: Fri, 9 Oct 2026 17:49:59 -0600 Subject: [PATCH 1/2] Prevent symlink-following chown in vcap-writable directories CreateJobPrerequisites runs as root on the host before launching a bpm container. Previously: 1. Additional volume paths were checked via os.Stat and paths were chowned via os.Chown, both of which follow symlinks. 2. Directory creation via createDirFor performed os.MkdirAll followed by os.Chown, which also followed symlinks. Because these paths reside in or can be placed in directories writable by the vcap user (such as ephemeral data directories or log directories), a compromised container could plant a symlink to an arbitrary host file or directory (e.g. /etc or /etc/shadow). On the next bpm restart, root-bpm would dereference the symlink and chown the host target to vcap. Fix: - Added safeio.MkdirAllChown and safeio.Lchown which use os.Lstat and os.Lchown to ensure symlinks are rejected and ownership changes do not follow symbolic links. - Updated CreateJobPrerequisites to verify additional volume paths with os.Lstat, rejecting symlink paths. - Replaced createDirFor and chownPaths with safeio.MkdirAllChown and safeio.Lchown. - Added comprehensive unit tests in safeio_test.go and adapter_test.go. [TNZGOV-18900] ai-assisted=yes --- src/bpm/runc/adapter/adapter.go | 13 +-- src/bpm/runc/adapter/adapter_test.go | 53 +++++++++ src/bpm/safeio/safeio.go | 52 ++++++++- src/bpm/safeio/safeio_test.go | 159 +++++++++++++++++++++++++++ 4 files changed, 266 insertions(+), 11 deletions(-) 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..f64a7010 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,30 @@ 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))) + }) + }) + 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..49e54090 100644 --- a/src/bpm/safeio/safeio.go +++ b/src/bpm/safeio/safeio.go @@ -15,9 +15,9 @@ // 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 at the leaf path cannot redirect the operation +// onto an arbitrary host file. package safeio import ( @@ -54,3 +54,49 @@ 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. +// If the path already exists and is a symlink, or if the leaf is a symlink, +// it refuses to follow it and returns an error. +func MkdirAllChown(path string, uid, gid int) error { + fi, err := os.Lstat(path) + if err == nil { + if fi.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("refusing to create directory through symlink at %s", path) + } + if !fi.IsDir() { + return fmt.Errorf("path %s exists and is not a directory", path) + } + } else if !os.IsNotExist(err) { + return err + } + + if err := os.MkdirAll(path, 0700); err != nil { + return err + } + + 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) +} + +// 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..c17dfed1 100644 --- a/src/bpm/safeio/safeio_test.go +++ b/src/bpm/safeio/safeio_test.go @@ -140,3 +140,162 @@ 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()) + + 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")) + }) + }) +}) + +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")) + }) + }) +}) From 939c37d4db29d4d29ecaad986033cb6f47ed1ab7 Mon Sep 17 00:00:00 2001 From: Colin Shield Date: Tue, 13 Oct 2026 18:37:52 -0600 Subject: [PATCH 2/2] Reject symlinks in every path component and add regression specs The previous change checked only the leaf of each path with os.Lstat before calling os.MkdirAll. MkdirAll resolves intermediate components through normal lookup, so a symlink planted at an intermediate component of a path inside a vcap-writable directory still redirects the operation: the leaf is created under the symlink's target and then chowned to vcap. MkdirAllChown now walks the path one component at a time from the filesystem root: Mkdirat, then Fstatat with AT_SYMLINK_NOFOLLOW to classify the component, then Openat with O_DIRECTORY|O_NOFOLLOW to descend. Ownership is applied with Fchown on the open directory descriptor, so the path cannot be swapped between the walk and the chown. Components are classified with an explicit S_IFLNK check rather than by errno, because with O_DIRECTORY set a symlink reports ENOTDIR on some platforms and ELOOP on others. This also rejects a legitimate symlink in the path prefix; callers must pass paths whose ancestors are real directories. Adds specs covering the intermediate-component case and relative paths, plus specs that drive CreateJobPrerequisites and assert that a host target outside the job tree is never chowned. Those require root to observe an ownership change and skip otherwise. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/bpm/runc/adapter/adapter_test.go | 101 +++++++++++++++++++++++++++ src/bpm/safeio/safeio.go | 79 +++++++++++++++------ src/bpm/safeio/safeio_test.go | 35 ++++++++++ 3 files changed, 193 insertions(+), 22 deletions(-) diff --git a/src/bpm/runc/adapter/adapter_test.go b/src/bpm/runc/adapter/adapter_test.go index f64a7010..da905abb 100644 --- a/src/bpm/runc/adapter/adapter_test.go +++ b/src/bpm/runc/adapter/adapter_test.go @@ -416,6 +416,107 @@ var _ = Describe("RuncAdapter", func() { }) }) + // 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 49e54090..ea907b7e 100644 --- a/src/bpm/safeio/safeio.go +++ b/src/bpm/safeio/safeio.go @@ -16,14 +16,18 @@ // 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, Lstat, and lchown -// so that a pre-planted symlink at the leaf path cannot redirect the operation -// onto an arbitrary host file. +// 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" ) @@ -57,34 +61,65 @@ func OpenAppendChown(path string, uid, gid int, perm os.FileMode) (*os.File, err // MkdirAllChown creates a directory (and any parent directories) with 0700 // permissions and chowns the leaf directory to the specified uid and gid. -// If the path already exists and is a symlink, or if the leaf is a symlink, -// it refuses to follow it and returns an error. +// +// 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 { - fi, err := os.Lstat(path) - if err == nil { - if fi.Mode()&os.ModeSymlink != 0 { - return fmt.Errorf("refusing to create directory through symlink at %s", path) + 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) } - if !fi.IsDir() { + 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) } - } else if !os.IsNotExist(err) { - return err - } - if err := os.MkdirAll(path, 0700); err != nil { - return err - } + // 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) + } - 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) + _ = unix.Close(parent) //nolint:errcheck + parent = child } - return os.Lchown(path, uid, gid) + return unix.Fchown(parent, uid, gid) } // Lchown changes the ownership of the named file or directory without following diff --git a/src/bpm/safeio/safeio_test.go b/src/bpm/safeio/safeio_test.go index c17dfed1..30ffc69b 100644 --- a/src/bpm/safeio/safeio_test.go +++ b/src/bpm/safeio/safeio_test.go @@ -152,6 +152,11 @@ var _ = Describe("MkdirAllChown", func() { 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() }) @@ -249,6 +254,36 @@ var _ = Describe("MkdirAllChown", func() { 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() {