diff --git a/go.mod b/go.mod index df040a685f8..35c9a16c47c 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( github.com/cilium/ebpf v0.12.3 github.com/containerd/console v1.0.4 github.com/coreos/go-systemd/v22 v22.5.0 - github.com/cyphar/filepath-securejoin v0.3.1 + github.com/cyphar/filepath-securejoin v0.3.2 github.com/docker/go-units v0.5.0 github.com/godbus/dbus/v5 v5.1.0 github.com/moby/sys/mountinfo v0.7.1 diff --git a/go.sum b/go.sum index 30eac24d227..365c720661b 100644 --- a/go.sum +++ b/go.sum @@ -9,8 +9,8 @@ github.com/coreos/go-systemd/v22 v22.5.0 h1:RrqgGjYQKalulkV8NGVIfkXQf6YYmOyiJKk8 github.com/coreos/go-systemd/v22 v22.5.0/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSVTIJ3seZv2GcEnc= github.com/cpuguy83/go-md2man/v2 v2.0.2 h1:p1EgwI/C7NhT0JmVkwCD2ZBK8j4aeHQX2pMHHBfMQ6w= github.com/cpuguy83/go-md2man/v2 v2.0.2/go.mod h1:tgQtvFlXSQOSOSIRvRPT7W67SCa46tRHOmNcaadrF8o= -github.com/cyphar/filepath-securejoin v0.3.1 h1:1V7cHiaW+C+39wEfpH6XlLBQo3j/PciWFrgfCLS8XrE= -github.com/cyphar/filepath-securejoin v0.3.1/go.mod h1:F7i41x/9cBF7lzCrVsYs9fuzwRZm4NQsGTBdpp6mETc= +github.com/cyphar/filepath-securejoin v0.3.2 h1:QhZu5AxQ+o1XZH0Ye05YzvJ0kAdK6VQc0z9NNMek7gc= +github.com/cyphar/filepath-securejoin v0.3.2/go.mod h1:F7i41x/9cBF7lzCrVsYs9fuzwRZm4NQsGTBdpp6mETc= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= diff --git a/libcontainer/utils/utils_unix.go b/libcontainer/utils/utils_unix.go index 6521114ba75..cc84597a7ce 100644 --- a/libcontainer/utils/utils_unix.go +++ b/libcontainer/utils/utils_unix.go @@ -319,6 +319,14 @@ func MkdirAllInRootOpen(root, unsafePath string, mode uint32) (_ *os.File, Err e 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 { diff --git a/tests/integration/mounts.bats b/tests/integration/mounts.bats index 5dafb655c92..11fb2cfc63e 100644 --- a/tests/integration/mounts.bats +++ b/tests/integration/mounts.bats @@ -199,6 +199,41 @@ function test_mount_order() { [ "$status" -eq 0 ] } +# CVE-2023-27561 CVE-2019-19921 +@test "runc run [/proc is a symlink]" { + # Make /proc in the container a symlink. + rm -rf rootfs/proc + mkdir -p rootfs/bad-proc + ln -sf /bad-proc rootfs/proc + # This should fail. + runc run test_busybox + [ "$status" -ne 0 ] + [[ "$output" == *"must be mounted on ordinary directory"* ]] +} + +# https://github.com/opencontainers/runc/issues/4401 +@test "runc run [setgid / + mkdirall]" { + mkdir rootfs/setgid + chmod '=7755' rootfs/setgid + + update_config '.mounts += [{ + type: "tmpfs", + source: "tmpfs", + destination: "/setgid/a/b/c", + options: ["ro", "nodev", "nosuid"] + }]' + update_config '.process.args |= ["true"]' + + runc run test_busybox + [ "$status" -eq 0 ] + + # Verify that the setgid bit is inherited. + [[ "$(stat -c %a rootfs/setgid)" == 7755 ]] + [[ "$(stat -c %a rootfs/setgid/a)" == 2755 ]] + [[ "$(stat -c %a rootfs/setgid/a/b)" == 2755 ]] + [[ "$(stat -c %a rootfs/setgid/a/b/c)" == 2755 ]] +} + @test "runc run [ro /sys/fs/cgroup mounts]" { # Without cgroup namespace. update_config '.linux.namespaces -= [{"type": "cgroup"}]' diff --git a/vendor/github.com/cyphar/filepath-securejoin/CHANGELOG.md b/vendor/github.com/cyphar/filepath-securejoin/CHANGELOG.md index 7436896e137..98172cedda7 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/CHANGELOG.md +++ b/vendor/github.com/cyphar/filepath-securejoin/CHANGELOG.md @@ -6,6 +6,24 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] ## +## [0.3.2] - 2024-09-13 ## + +### Changed ### +- Passing the `S_ISUID` or `S_ISGID` modes to `MkdirAllInRoot` will now return + an explicit error saying that those bits are ignored by `mkdirat(2)`. In the + past a different error was returned, but since the silent ignoring behaviour + is codified in the man pages a more explicit error seems apt. While silently + ignoring these bits would be the most compatible option, it could lead to + users thinking their code sets these bits when it doesn't. Programs that need + to deal with compatibility can mask the bits themselves. (#23, #25) + +### Fixed ### +- If a directory has `S_ISGID` set, then all child directories will have + `S_ISGID` set when created and a different gid will be used for any inode + created under the directory. Previously, the "expected owner and mode" + validation in `securejoin.MkdirAll` did not correctly handle this. We now + correctly handle this case. (#24, #25) + ## [0.3.1] - 2024-07-23 ## ### Changed ### @@ -127,7 +145,8 @@ This is our first release of `github.com/cyphar/filepath-securejoin`, containing a full implementation with a coverage of 93.5% (the only missing cases are the error cases, which are hard to mocktest at the moment). -[Unreleased]: https://github.com/cyphar/filepath-securejoin/compare/v0.3.1...HEAD +[Unreleased]: https://github.com/cyphar/filepath-securejoin/compare/v0.3.2...HEAD +[0.3.2]: https://github.com/cyphar/filepath-securejoin/compare/v0.3.1...v0.3.2 [0.3.1]: https://github.com/cyphar/filepath-securejoin/compare/v0.3.0...v0.3.1 [0.3.0]: https://github.com/cyphar/filepath-securejoin/compare/v0.2.5...v0.3.0 [0.2.5]: https://github.com/cyphar/filepath-securejoin/compare/v0.2.4...v0.2.5 diff --git a/vendor/github.com/cyphar/filepath-securejoin/VERSION b/vendor/github.com/cyphar/filepath-securejoin/VERSION index 9e11b32fcaa..d15723fbe8d 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/VERSION +++ b/vendor/github.com/cyphar/filepath-securejoin/VERSION @@ -1 +1 @@ -0.3.1 +0.3.2 diff --git a/vendor/github.com/cyphar/filepath-securejoin/mkdir_linux.go b/vendor/github.com/cyphar/filepath-securejoin/mkdir_linux.go index ad2bd7973ab..49ffdbe02bd 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/mkdir_linux.go +++ b/vendor/github.com/cyphar/filepath-securejoin/mkdir_linux.go @@ -46,6 +46,13 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err if mode&^0o7777 != 0 { return nil, fmt.Errorf("%w for mkdir 0o%.3o", errInvalidMode, mode) } + // On Linux, mkdirat(2) (and os.Mkdir) silently ignore the suid and sgid + // bits. We could also silently ignore them but since we have very few + // users it seems more prudent to return an error so users notice that + // these bits will not be set. + if mode&^0o1777 != 0 { + return nil, fmt.Errorf("%w for mkdir 0o%.3o: suid and sgid are ignored by mkdir", errInvalidMode, mode) + } // Try to open as much of the path as possible. currentDir, remainingPath, err := partialLookupInRoot(root, unsafePath) @@ -120,6 +127,17 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err expectedGid = uint32(unix.Getegid()) ) + // The setgid bit (S_ISGID = 0o2000) is inherited to child directories and + // affects the group of any inodes created in said directory, so if the + // starting directory has it set we need to adjust our expected mode and + // owner to match. + if st, err := fstatFile(currentDir); err != nil { + return nil, fmt.Errorf("failed to stat starting path for mkdir %q: %w", currentDir.Name(), err) + } else if st.Mode&unix.S_ISGID == unix.S_ISGID { + expectedMode |= unix.S_ISGID + expectedGid = st.Gid + } + // Create the remaining components. for _, part := range remainingParts { switch part { diff --git a/vendor/github.com/cyphar/filepath-securejoin/openat_linux.go b/vendor/github.com/cyphar/filepath-securejoin/openat_linux.go index 949fb5f2d82..ac083f218fe 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/openat_linux.go +++ b/vendor/github.com/cyphar/filepath-securejoin/openat_linux.go @@ -42,6 +42,10 @@ func fstatatFile(dir *os.File, path string, flags int) (unix.Stat_t, error) { return stat, nil } +func fstatFile(fd *os.File) (unix.Stat_t, error) { + return fstatatFile(fd, "", unix.AT_EMPTY_PATH) +} + func readlinkatFile(dir *os.File, path string) (string, error) { size := 4096 for { diff --git a/vendor/modules.txt b/vendor/modules.txt index 22becdb55fb..c340ef1e67d 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -24,7 +24,7 @@ github.com/coreos/go-systemd/v22/dbus # github.com/cpuguy83/go-md2man/v2 v2.0.2 ## explicit; go 1.11 github.com/cpuguy83/go-md2man/v2/md2man -# github.com/cyphar/filepath-securejoin v0.3.1 +# github.com/cyphar/filepath-securejoin v0.3.2 ## explicit; go 1.20 github.com/cyphar/filepath-securejoin # github.com/docker/go-units v0.5.0