From 457e1ffa4c78b188e7ff88ecdad90ad9f044c776 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Fri, 13 Sep 2024 15:24:15 +1000 Subject: [PATCH 1/4] tests: add regression test for CVE-2019-19921 / CVE-2023-27561 We reintroduced this once already because it is quite easy to miss this subtle aspect of proc mounting. The recent migration to securejoin.MkdirAllInRoot could have also inadvertently reintroduced this (though it didn't). Signed-off-by: Aleksa Sarai --- tests/integration/mounts.bats | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/integration/mounts.bats b/tests/integration/mounts.bats index 5dafb655c92..1dba80f9be6 100644 --- a/tests/integration/mounts.bats +++ b/tests/integration/mounts.bats @@ -199,6 +199,18 @@ 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"* ]] +} + @test "runc run [ro /sys/fs/cgroup mounts]" { # Without cgroup namespace. update_config '.linux.namespaces -= [{"type": "cgroup"}]' From 646efe70b11447d43e5587f34ed203dc4b65f1af Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Fri, 13 Sep 2024 14:33:48 +1000 Subject: [PATCH 2/4] utils: mkdirall: mask silently ignored mode bits to match os.MkdirAll It turns out that the suid and sgid mode bits are silently ignored by Linux (though the sticky bit is honoured), and some users are requesting mode bits that are ignored. While returning an error (as securejoin does) makes some sense, this is a regression. Ref: https://github.com/cyphar/filepath-securejoin/issues/23 Fixes: dd827f7b715a ("utils: switch to securejoin.MkdirAllHandle") Signed-off-by: Aleksa Sarai --- libcontainer/utils/utils_unix.go | 8 ++++++++ 1 file changed, 8 insertions(+) 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 { From 066b109e99b5795107f1762d9806827dc031f3c3 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Fri, 13 Sep 2024 17:22:20 +1000 Subject: [PATCH 3/4] vendor: update to github.com/cyphar/filepath-securejoin@v0.3.2 This includes a fix for the handling of S_ISGID directories. Signed-off-by: Aleksa Sarai --- go.mod | 2 +- go.sum | 4 ++-- .../cyphar/filepath-securejoin/CHANGELOG.md | 21 ++++++++++++++++++- .../cyphar/filepath-securejoin/VERSION | 2 +- .../cyphar/filepath-securejoin/mkdir_linux.go | 18 ++++++++++++++++ .../filepath-securejoin/openat_linux.go | 4 ++++ vendor/modules.txt | 2 +- 7 files changed, 47 insertions(+), 6 deletions(-) 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/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 From d8844e2939015f03a683c5f40f4111388d2f1244 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Fri, 13 Sep 2024 17:45:34 +1000 Subject: [PATCH 4/4] tests: integration: add setgid mkdirall test Signed-off-by: Aleksa Sarai --- tests/integration/mounts.bats | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/integration/mounts.bats b/tests/integration/mounts.bats index 1dba80f9be6..11fb2cfc63e 100644 --- a/tests/integration/mounts.bats +++ b/tests/integration/mounts.bats @@ -211,6 +211,29 @@ function test_mount_order() { [[ "$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"}]'