Merge pull request opencontainers#4400 from cyphar/mkdirall-suidsgid-…

…bits

utils: mkdirall: fix handling of suid/sgid bits

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

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=

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 {

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"}]'

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