merge #4174 into opencontainers/runc:main

Rodrigo Campos (3):
  Makefile: Fix runc-dmz removal
  contrib/cmd/memfd-bind: Mention runc-dmz needs RUNC_DMZ=true
  libct/dmz: Require RUNC_DMZ=true to opt-in

LGTMs: AkihiroSuda cyphar

Author: cyphar

Makefile

@@ -79,7 +79,7 @@ recvtty sd-helper seccompagent fs-idmap memfd-bind pidfd-kill remap-rootfs:
 
 .PHONY: clean
 clean:
-	rm -f runc runc-* libcontainer/dmz/runc-dmz
+	rm -f runc runc-* libcontainer/dmz/binary/runc-dmz
 	rm -f contrib/cmd/recvtty/recvtty
 	rm -f contrib/cmd/sd-helper/sd-helper
 	rm -f contrib/cmd/seccompagent/seccompagent

README.md

@@ -68,7 +68,7 @@ make BUILDTAGS=""
 | Build Tag     | Feature                               | Enabled by Default | Dependencies        |
 |---------------|---------------------------------------|--------------------|---------------------|
 | `seccomp`     | Syscall filtering using `libseccomp`. | yes                | `libseccomp`        |
-| `!runc_nodmz` | Reduce memory usage for CVE-2019-5736 protection by using a small C binary, [see `memfd-bind` for more details][contrib-memfd-bind]. `runc_nodmz` disables this feature and causes runc to use a different protection mechanism which will further increases memory usage temporarily during container startup. This feature can also be disabled at runtime by setting the `RUNC_DMZ=legacy` environment variable. | yes ||
+| `!runc_nodmz` | Reduce memory usage for CVE-2019-5736 protection by using a small C binary, [see `memfd-bind` for more details][contrib-memfd-bind]. `runc_nodmz` disables this **experimental feature** and causes runc to use a different protection mechanism which will further increases memory usage temporarily during container startup. To enable this feature you also need to set the `RUNC_DMZ=true` environment variable. | yes ||
 | `runc_dmz_selinux_nocompat` | Disables a SELinux DMZ workaround (new distros should set this). See [dmz README] for details. | no ||
 
 The following build tags were used earlier, but are now obsoleted:

contrib/cmd/memfd-bind/README.md

@@ -36,12 +36,12 @@ much memory usage they can use:
 
 * `runc-dmz` is (depending on which libc it was compiled with) between 10kB and
   1MB in size, and a copy is created once per process spawned inside a
-  container by runc (both the pid1 and every `runc exec`). There are
-  circumstances where using `runc-dmz` will fail in ways that runc cannot
-  predict ahead of time (such as restrictive LSMs applied to containers), in
-  which case users can disable it with the `RUNC_DMZ=legacy` setting.
-  `runc-dmz` also requires an additional `execve` over the other options,
-  though since the binary is so small the cost is probably not even noticeable.
+  container by runc (both the pid1 and every `runc exec`). The `RUNC_DMZ=true`
+  environment variable needs to be set to opt-in. There are circumstances where
+  using `runc-dmz` will fail in ways that runc cannot predict ahead of time (such
+  as restrictive LSMs applied to containers).  `runc-dmz` also requires an
+  additional `execve` over the other options, though since the binary is so small
+  the cost is probably not even noticeable.
 
 * The classic method of making a copy of the entire `runc` binary during
   container process setup takes up about 10MB per process spawned inside the

libcontainer/dmz/dmz_linux.go

@@ -7,7 +7,9 @@ import (
 	"bytes"
 	"debug/elf"
 	"embed"
+	"fmt"
 	"os"
+	"strconv"
 	"sync"
 
 	"github.com/sirupsen/logrus"
@@ -43,11 +45,19 @@ var (
 // If the runc-dmz binary is not embedded into the runc binary, Binary will
 // return ErrNoDmzBinary as the error.
 func Binary(tmpDir string) (*os.File, error) {
-	// Setting RUNC_DMZ=legacy disables this dmz method.
-	if os.Getenv("RUNC_DMZ") == "legacy" {
-		logrus.Debugf("RUNC_DMZ=legacy set -- switching back to classic /proc/self/exe cloning")
+	// Only RUNC_DMZ=true enables runc_dmz.
+	runcDmz := os.Getenv("RUNC_DMZ")
+	if runcDmz == "" {
+		logrus.Debugf("RUNC_DMZ is not set -- switching back to classic /proc/self/exe cloning")
 		return nil, ErrNoDmzBinary
 	}
+	if dmzEnabled, err := strconv.ParseBool(runcDmz); err == nil && !dmzEnabled {
+		logrus.Debugf("RUNC_DMZ is false -- switching back to classic /proc/self/exe cloning")
+		return nil, ErrNoDmzBinary
+	} else if err != nil {
+		return nil, fmt.Errorf("parsing RUNC_DMZ: %w", err)
+	}
+
 	runcDmzBinaryOnce.Do(func() {
 		runcDmzBinary, _ = runcDmzFs.ReadFile("binary/runc-dmz")
 		// Verify that our embedded binary has a standard ELF header.

tests/integration/exec.bats

@@ -323,14 +323,14 @@ function check_exec_debug() {
 	[ "$status" -eq 0 ]
 }
 
-@test "RUNC_DMZ=legacy runc exec [execve error]" {
+@test "runc exec [execve error]" {
 	cat <<EOF >rootfs/run.sh
 #!/mmnnttbb foo bar
 sh
 EOF
 	chmod +x rootfs/run.sh
-	RUNC_DMZ=legacy runc run -d --console-socket "$CONSOLE_SOCKET" test_busybox
-	RUNC_DMZ=legacy runc exec -t test_busybox /run.sh
+	runc run -d --console-socket "$CONSOLE_SOCKET" test_busybox
+	runc exec -t test_busybox /run.sh
 	[ "$status" -ne 0 ]
 
 	# After the sync socket closed, we should not send error to parent

tests/integration/run.bats

@@ -127,20 +127,20 @@ function teardown() {
 	[ "${lines[0]}" = "410" ]
 }
 
-@test "runc run [runc-dmz]" {
-	runc --debug run test_hello
+@test "RUNC_DMZ=true runc run [runc-dmz]" {
+	RUNC_DMZ=true runc --debug run test_hello
 	[ "$status" -eq 0 ]
 	[[ "$output" = *"Hello World"* ]]
 	# We use runc-dmz if we can.
 	[[ "$output" = *"runc-dmz: using runc-dmz"* ]]
 }
 
-@test "runc run [cap_sys_ptrace -> /proc/self/exe clone]" {
+@test "RUNC_DMZ=true runc run [cap_sys_ptrace -> /proc/self/exe clone]" {
 	# Add CAP_SYS_PTRACE to the bounding set, the minimum needed to indicate a
 	# container process _could_ get CAP_SYS_PTRACE.
 	update_config '.process.capabilities.bounding += ["CAP_SYS_PTRACE"]'
 
-	runc --debug run test_hello
+	RUNC_DMZ=true runc --debug run test_hello
 	[ "$status" -eq 0 ]
 	[[ "$output" = *"Hello World"* ]]
 	if [ "$EUID" -ne 0 ] && is_kernel_gte 4.10; then
@@ -154,8 +154,8 @@ function teardown() {
 	fi
 }
 
-@test "RUNC_DMZ=legacy runc run [/proc/self/exe clone]" {
-	RUNC_DMZ=legacy runc --debug run test_hello
+@test "runc run [/proc/self/exe clone]" {
+	runc --debug run test_hello
 	[ "$status" -eq 0 ]
 	[[ "$output" = *"Hello World"* ]]
 	[[ "$output" = *"runc-dmz: using /proc/self/exe clone"* ]]
@@ -231,14 +231,14 @@ function teardown() {
 	grep -E '^boottime\s+1337\s+3141519$' <<<"$output"
 }
 
-@test "runc run [exec error]" {
+@test "RUNC_DMZ=true runc run [exec error]" {
 	cat <<EOF >rootfs/run.sh
 #!/mmnnttbb foo bar
 sh
 EOF
 	chmod +x rootfs/run.sh
 	update_config '.process.args = [ "/run.sh" ]'
-	runc run test_hello
+	RUNC_DMZ=true runc run test_hello
 
 	# Ensure that the output contains the right error message. For runc-dmz, both
 	# nolibc and libc have the same formatting string (but libc will print the
@@ -248,14 +248,14 @@ EOF
 	[[ "$output" = *"exec /run.sh: "* ]]
 }
 
-@test "RUNC_DMZ=legacy runc run [execve error]" {
+@test "runc run [execve error]" {
 	cat <<EOF >rootfs/run.sh
 #!/mmnnttbb foo bar
 sh
 EOF
 	chmod +x rootfs/run.sh
 	update_config '.process.args = [ "/run.sh" ]'
-	RUNC_DMZ=legacy runc run test_hello
+	runc run test_hello
 	[ "$status" -ne 0 ]
 
 	# After the sync socket closed, we should not send error to parent

tests/integration/selinux.bats

@@ -39,15 +39,14 @@ function teardown() {
 }
 
 # https://github.com/opencontainers/runc/issues/4057
-@test "runc run (custom selinux label)" {
+@test "runc run (custom selinux label, RUNC_DMZ=true)" {
 	update_config '	  .process.selinuxLabel |= "system_u:system_r:container_t:s0:c4,c5"
 			| .process.args = ["/bin/true"]'
-	runc run tst
+	RUNC_DMZ=true runc run tst
 	[ "$status" -eq 0 ]
 }
 
-@test "runc run (custom selinux label, RUNC_DMZ=legacy)" {
-	export RUNC_DMZ=legacy
+@test "runc run (custom selinux label)" {
 	update_config '	  .process.selinuxLabel |= "system_u:system_r:container_t:s0:c4,c5"
 			| .process.args = ["/bin/true"]'
 	runc run tst