Repository: moby/moby (language: go)

Issue:
make /sys/fs/cgroup rw by default with cgroup2
**Description**

Make /sys/fs/cgroup rw by default within the container if cgroup2 is used. Currently running e.g. systemd within the container fails because it cannot create it's cgroups as /sys/fs/cgroup is mounted ro. Also now with cgroup2 being supported the old container escapes of v1 are no longer an issue.

For anyone wanting a q'n'd fix for this problem I've attached: https://github.com/moby/moby/pull/42043

**Steps to reproduce the issue:**
1.
2.
3.

**Describe the results you received:**


**Describe the results you expected:**
Applications within the container being able to create child cgroups and "manage" there own assigned scope.

**Additional information you deem important (e.g. issue happens only occasionally):**

**Output of `docker version`:**

```
Docker version 20.10.3, build 48d30b5b32
```

**Output of `docker info`:**

```
Client:
 Context:    default
 Debug Mode: false
 Plugins:
  app: Docker App (Docker Inc., v0.9.1-beta3)
  buildx: Build with BuildKit (Docker Inc., v0.5.1-tp-docker)

Server:
 Containers: 1
  Running: 1
  Paused: 0
  Stopped: 0
 Images: 57
 Server Version: 20.10.3
 Storage Driver: overlay2
  Backing Filesystem: extfs
  Supports d_type: true
  Native Overlay Diff: false
 Logging Driver: json-file
 Cgroup Driver: systemd
 Cgroup Version: 2
 Plugins:
  Volume: local
  Network: bridge host ipvlan macvlan null overlay
  Log: awslogs fluentd gcplogs gelf journald json-file local logentries splunk syslog
 Swarm: inactive
 Runtimes: io.containerd.runc.v2 io.containerd.runtime.v1.linux runc
 Default Runtime: runc
 Init Binary: docker-init
 containerd version: 269548fa27e0089a8b8278fc4fc781d7f65a939b.m
 runc version: 12644e614e25b05da6fd08a38ffa0cfe1903fdec
 init version: de40ad0
 Security Options:
  seccomp
   Profile: default
  cgroupns
 Kernel Version: 5.10.16-arch1-1
 Operating System: Arch Linux
 OSType: linux
 Architecture: x86_64
 CPUs: 16
 Total Memory: 27.11GiB
 Name: ArchBook
 ID: ARJ3:2WKK:VWM6:MEG5:SVQF:NKWZ:SVQQ:2KNQ:P7WI:MG3W:SGX6:3TX6
 Docker Root Dir: /var/lib/docker
 Debug Mode: false
 Username: agowa338
 Registry: https://index.docker.io/v1/
 Labels:
 Experimental: false
 Insecure Registries:
  127.0.0.0/8
 Live Restore Enabled: false
```

**Additional environment details (AWS, VirtualBox, physical, etc.):**

WIP: Make /sys/fs/cgroup rw
Fixes #42040 

**- What I did**

Makes /sys/fs/cgroup rw. This patch is an only intended as reference as I don't know how to make this change dependent upon the used cgroup version.

It should be rw for cgroup2 and ro for cgroup1 (because of container escapes).

**- How I did it**

**- How to verify it**

**- Description for the changelog**
Allow writing to /sys/fs/cgroup to allow containers to manage there own cgroup spaces and create child spaces. E.g. to run systemd within a container without unnecessary privileges.



Discussion / hints:
The workaround I found for myself:

```
--cgroupns=private --cgroup-parent=myservice.slice -v /sys/fs/cgroup/myservice.slice:/sys/fs/cgroup
```
/cc @AkihiroSuda @justincormack 
@AkihiroSuda You're right, a flag may be a better option, but I don't know the moby code base well enough to implement this. Therefore I changed the default and marked this PR as WIP so that it is clear what configuration I intend to have changed.

Also as to my knowledge having the v2 cgroups writeable should be fine, but a security assessment may prove useful here.
cc @giuseppe @kolyshkin @cyphar FYI

Since the cgroup the process is in is the same cgroup where the restrictions are set, I don't think it's at all safe to allow containers to write to cgroup files -- that would allow a container to modify its own restrictions. If we created a sub-cgroup that the container was placed inside, maybe that would be safe but I'm not convinced to be honest.

As an aside, I wonder if you can change the devices eBPF program *today* with a read-only mount -- IOW does `bpf(2)` actually stop you from operating on a read-only mounted cgroup? (And the devices cgroup is the strongest argument for not doing this -- a container being able to modify its own devices restrictions would be **very** bad.)
Today you cannot set eBPF (and afaik not even with this patch) as that is filtered (at least that's the error you get when you apply this patch and try to start systemd.

Ok, I thought docker already creates a sub-cgroup for the containers. That was my fault than. But having cgroup support within containers would still be very valuable.
If this doesn't even work with sub-cgroups, is there a change to get this possibility upstream into the linux kernel so that docker can use it?
Yes, `bpf(2)` is blocked by the default seccomp profile (unless your container has `CAP_SYS_ADMIN`) -- the reason I mentioned the devices eBPF is because I hadn't considered whether `BPF_PROG_ATTACH` actually obeys read-only mounts for attaching (and a quick look at the code tells me it doesn't, but I haven't tested it yet).

> Ok, I thought docker already creates a sub-cgroup for the containers. That was my fault than. But having cgroup support within containers would still be very valuable.

I would have to think about how we would implement this, since runc is doing cgroup setup and there isn't a nice way to tell runc to configure a cgroup and then move the program into a sub-cgroup...

> If this doesn't even work with sub-cgroups, is there a change to get this possibility upstream into the linux kernel so that docker can use it?

If containers were placed in a sub-cgroup such that the restricted cgroup is a parent, then this should be safe -- Tejun has said that cgroupv2 is safe for delegation, which tells me that the kernel is giving us a guarantee that this is safe.
So we depend upon adding a new feature to runc to allow to create sub-cgroup than?
Any updates? I see podman "solves" this by introducing a new option: `--security-opt unmask=/sys/fs/cgroup`

https://github.com/containers/podman/pull/9536
In re-reading this thread with fresh eyes, it occurs to me that we're discussing two separate things that I think could be disconnected in order to make some progress?

1. in order to enable this _by default_, we need more functionality/UX design in `runc` (harder)

2. in order to make it possible for users to opt into this (without the full "hammer" that is `--privileged`), I _think_ we just need to add a new security opt and check for it in the following block of code?
   https://github.com/moby/moby/blob/f0cec02a403496e2b1dd1aaf12b2530922e210db/daemon/oci_linux.go#L667-L675

(the argument for the latter without necessarily having the former is that there are use cases where the downsides listed above are not as bad, such as containers that are already mostly unconfined from a cgroups perspective -- certainly better than going all the way to privileged!)

Fix patch:
diff --git a/container/container.go b/container/container.go
index c16a353fa8dc4..a280c0dae3fb9 100644
--- a/container/container.go
+++ b/container/container.go
@@ -127,6 +127,7 @@ type SecurityOptions struct {
 	AppArmorProfile string
 	SeccompProfile  string
 	NoNewPrivileges bool
+	WritableCgroups *bool
 }
 
 type localLogCacheMeta struct {
diff --git a/daemon/daemon_unix.go b/daemon/daemon_unix.go
index de437a149b388..8463e086911a5 100644
--- a/daemon/daemon_unix.go
+++ b/daemon/daemon_unix.go
@@ -221,6 +221,11 @@ func parseSecurityOpt(securityOptions *container.SecurityOptions, config *contai
 			securityOptions.NoNewPrivileges = true
 			continue
 		}
+		if opt == "writable-cgroups" {
+			trueVal := true
+			securityOptions.WritableCgroups = &trueVal
+			continue
+		}
 		if opt == "disable" {
 			labelOpts = append(labelOpts, "disable")
 			continue
@@ -251,6 +256,12 @@ func parseSecurityOpt(securityOptions *container.SecurityOptions, config *contai
 				return fmt.Errorf("invalid --security-opt 2: %q", opt)
 			}
 			securityOptions.NoNewPrivileges = nnp
+		case "writable-cgroups":
+			writableCgroups, err := strconv.ParseBool(v)
+			if err != nil {
+				return fmt.Errorf("invalid --security-opt 2: %q", opt)
+			}
+			securityOptions.WritableCgroups = &writableCgroups
 		default:
 			return fmt.Errorf("invalid --security-opt 2: %q", opt)
 		}
diff --git a/daemon/oci_linux.go b/daemon/oci_linux.go
index 12e4d6c29da0f..cdc1477939efc 100644
--- a/daemon/oci_linux.go
+++ b/daemon/oci_linux.go
@@ -664,9 +664,21 @@ func withMounts(daemon *Daemon, daemonCfg *configStore, c *container.Container,
 			}
 		}
 
+		// if the user didn't specify otherwise, default to the value of privileged
+		writableCgroups := c.HostConfig.Privileged
+		if c.WritableCgroups != nil {
+			if daemonCfg.Rootless || daemon.idMapping.UIDMaps != nil {
+				// error if the user requested a configuration we can't explicitly support
+				return errors.New("option WritableCgroups conflicts with user namespaces and rootless mode")
+			}
+			writableCgroups = *c.WritableCgroups
+		}
 		// TODO: until a kernel/mount solution exists for handling remount in a user namespace,
 		// we must clear the readonly flag for the cgroups mount (@mrunalp concurs)
-		if uidMap := daemon.idMapping.UIDMaps; uidMap != nil || c.HostConfig.Privileged {
+		if daemon.idMapping.UIDMaps != nil {
+			writableCgroups = true
+		}
+		if writableCgroups {
 			for i, m := range s.Mounts {
 				if m.Type == "cgroup" {
 					clearReadOnly(&s.Mounts[i])


Test patch:
diff --git a/daemon/daemon_unix_test.go b/daemon/daemon_unix_test.go
index f673b16543494..ec987007e831f 100644
--- a/daemon/daemon_unix_test.go
+++ b/daemon/daemon_unix_test.go
@@ -161,6 +161,13 @@ func TestParseSecurityOpt(t *testing.T) {
 		})
 		assert.Error(t, err, `invalid --security-opt 2: "unknown=something"`)
 	})
+	t.Run("invalid cgroup option", func(t *testing.T) {
+		secOpts := &container.SecurityOptions{}
+		err := parseSecurityOpt(secOpts, &containertypes.HostConfig{
+			SecurityOpt: []string{"writable-cgroups=dang"},
+		})
+		assert.Error(t, err, `invalid --security-opt 2: "writable-cgroups=dang"`)
+	})
 }
 
 func TestParseNNPSecurityOptions(t *testing.T) {
diff --git a/integration/container/run_linux_test.go b/integration/container/run_linux_test.go
index 70c8713616ece..8af460e0b54ab 100644
--- a/integration/container/run_linux_test.go
+++ b/integration/container/run_linux_test.go
@@ -399,3 +399,92 @@ func TestSeccomp(t *testing.T) {
 		}
 	}
 }
+
+func TestCgroupRW(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType != "linux")
+	skip.If(t, testEnv.IsRootless, "can't test writable cgroups in rootless (permission denied)")
+	skip.If(t, testEnv.IsUserNamespace, "can't test writable cgroups in user namespaces (permission denied)")
+
+	ctx := setupTest(t)
+	apiClient := testEnv.APIClient()
+
+	type testCase struct {
+		name             string
+		ops              []func(*container.TestContainerConfig)
+		expectedErrMsg   string
+		expectedExitCode int
+	}
+	testCases := []testCase{
+		{
+			name: "nil",
+			ops:  nil,
+			// no err msg, because disabled-by-default
+			expectedExitCode: 1,
+		},
+		{
+			name: "writable",
+			ops:  []func(*container.TestContainerConfig){container.WithSecurityOpt("writable-cgroups")},
+			// no err msg, because this is correct key=bool
+			expectedExitCode: 0,
+		},
+		{
+			name: "writable=true",
+			ops:  []func(*container.TestContainerConfig){container.WithSecurityOpt("writable-cgroups=true")},
+			// no err msg, because this is correct key=value
+			expectedExitCode: 0,
+		},
+		{
+			name: "writable=false",
+			ops:  []func(*container.TestContainerConfig){container.WithSecurityOpt("writable-cgroups=false")},
+			// no err msg, because this is correct key=value
+			expectedExitCode: 1,
+		},
+		{
+			name:           "writeable=true",
+			ops:            []func(*container.TestContainerConfig){container.WithSecurityOpt("writeable-cgroups=true")},
+			expectedErrMsg: `Error response from daemon: invalid --security-opt 2: "writeable-cgroups=true"`,
+		},
+		{
+			name:           "writable=1",
+			ops:            []func(*container.TestContainerConfig){container.WithSecurityOpt("writable-cgroups=1")},
+			expectedErrMsg: `Error response from daemon: invalid --security-opt 2: "writable-cgroups=1"`,
+		},
+		{
+			name:           "writable=potato",
+			ops:            []func(*container.TestContainerConfig){container.WithSecurityOpt("writable-cgroups=potato")},
+			expectedErrMsg: `Error response from daemon: invalid --security-opt 2: "writable-cgroups=potato"`,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			config := container.NewTestConfig(tc.ops...)
+			resp, err := container.CreateFromConfig(ctx, apiClient, config)
+			if err != nil {
+				assert.Equal(t, tc.expectedErrMsg, err.Error())
+				return
+			}
+			// TODO check if ro or not
+			err = apiClient.ContainerStart(ctx, resp.ID, containertypes.StartOptions{})
+			assert.NilError(t, err)
+
+			res, err := container.Exec(ctx, apiClient, resp.ID, []string{"sh", "-ec", `
+				# see also "contrib/check-config.sh" for the same test
+				if [ "$(stat -f -c %t /sys/fs/cgroup 2> /dev/null)" = '63677270' ]; then
+					# nice, must be cgroupsv2
+					exec mkdir /sys/fs/cgroup/foo
+				else
+					# boo, must be cgroupsv1
+					exec mkdir /sys/fs/cgroup/pids/foo
+				fi
+			`})
+			assert.NilError(t, err)
+			if tc.expectedExitCode != 0 {
+				assert.Check(t, is.Contains(res.Stderr(), "Read-only file system"))
+			} else {
+				assert.Equal(t, res.Stderr(), "")
+			}
+			assert.Equal(t, res.Stdout(), "")
+			assert.Equal(t, tc.expectedExitCode, res.ExitCode)
+		})
+	}
+}