Repository: moby/moby (language: go)

Issue:
Process capabilities cannot be retained when starting a container as non-root with `--security-opt=no-new-privileges`
### Description

When using docker as a runtime in kubernetes, the capabilities specified in the container's security context (in the pod yaml manifests) are not respected if running as **non-root** user:
```yaml
    securityContext:
      allowPrivilegeEscalation: false
      capabilities:
        add:
        - NET_BIND_SERVICE
        drop:
        - ALL
      privileged: false
      runAsGroup: 107
      runAsNonRoot: true
      runAsUser: 107
```
```bash
$ k exec -ti virt-launcher-testvm-XXXX -- bash
bash-5.1$ grep Cap /proc/1/status 
CapInh:	0000000000000000
CapPrm:	0000000000000000 # permitted caps zeroed
CapEff:	0000000000000000 # effective caps zeroed
CapBnd:	0000000000000400 # cap_net_bind_service
CapAmb:	0000000000000000
````

In KubeVirt project we had several similar issues reported: https://github.com/kubevirt/kubevirt/issues/9465

This can be easily reproduced with minikube. Other runtimes (containerd and crio) handle the capabilities correctly:
```
CapInh:	0000000000000000
CapPrm:	0000000000000400 # cap_net_bind_service
CapEff:	0000000000000400 # cap_net_bind_service
CapBnd:	0000000000000400 # cap_net_bind_service
CapAmb:	0000000000000000
```

I briefly looked at the sources. Though I am not 100% confident that this snippet is actually causing the problem, but the bellow code looked suspicious to me:
https://github.com/moby/moby/blob/c651a535588402f27d8f1a1408c794fc3289f39f/oci/oci.go#L31-L35

It was introduced by this commit https://github.com/moby/moby/commit/349aeeab7c5e878ddc6c0c95a0a937476816c43e (and refactored in https://github.com/moby/moby/commit/0d9a37d0c249e871af0e667317be3169054a989f). 

### Reproduce

```bash
$ minikube start --driver=kvm2
$ k create -f https://github.com/kubevirt/kubevirt/releases/download/v0.59.0/kubevirt-operator.yaml
$ k create -f https://github.com/kubevirt/kubevirt/releases/download/v0.59.0/kubevirt-cr.yaml
$ wget https://kubevirt.io/labs/manifests/vm.yaml
$ vim vm.yaml # add annotation `kubevirt.io/keep-launcher-alive-after-failure: "true"`
$ k create -f vm.yaml
$ k edit vm testvm # set `running: true`
$ k logs -f virt-launcher-testvm-XXXX
...
{"component":"virt-launcher","level":"error","msg":"failed to start virtqemud","pos":"libvirt_helper.go:250","reason":"fork/exec /usr/sbin/virtqemud: errno 0","timestamp":"2023-05-08T09:34:32.370373Z"}
panic: fork/exec /usr/sbin/virtqemud: errno 0
...
$ k exec -ti virt-launcher-testvm-XXXX -- bash
bash-5.1$ grep Cap /proc/1/status 
CapInh:	0000000000000000
CapPrm:	0000000000000000 # permitted caps zeroed
CapEff:	0000000000000000 # effective caps zeroed
CapBnd:	0000000000000400 # cap_net_bind_service
CapAmb:	0000000000000000
```

### Expected behavior

Effective/permitted caps should be set correctly:
```
CapPrm:	0000000000000400
CapEff:	0000000000000400
```

### docker version

```bash
Client:
 Version:           20.10.23
 API version:       1.41
 Go version:        go1.18.10
 Git commit:        7155243
 Built:             Thu Jan 19 17:30:35 2023
 OS/Arch:           linux/amd64
 Context:           default
 Experimental:      true

Server: Docker Engine - Community
 Engine:
  Version:          20.10.23
  API version:      1.41 (minimum version 1.12)
  Go version:       go1.18.10
  Git commit:       6051f14
  Built:            Thu Jan 19 17:36:08 2023
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          v1.7.0
  GitCommit:        1fbd70374134b891f97ce19c70b6e50c7b9f4e0d
 runc:
  Version:          1.1.5
  GitCommit:        f19387a6bec4944c770f7668ab51c4348d9c2f38
 docker-init:
  Version:          0.19.0
  GitCommit:        de40ad0
```


### docker info

```bash
Client:
 Context:    default
 Debug Mode: false

Server:
 Containers: 34
  Running: 28
  Paused: 0
  Stopped: 6
 Images: 14
 Server Version: 20.10.23
 Storage Driver: overlay2
  Backing Filesystem: extfs
  Supports d_type: true
  Native Overlay Diff: true
  userxattr: false
 Logging Driver: json-file
 Cgroup Driver: cgroupfs
 Cgroup Version: 1
 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: 1fbd70374134b891f97ce19c70b6e50c7b9f4e0d
 runc version: f19387a6bec4944c770f7668ab51c4348d9c2f38
 init version: de40ad0
 Security Options:
  seccomp
   Profile: default
 Kernel Version: 5.10.57
 Operating System: Buildroot 2021.02.12
 OSType: linux
 Architecture: x86_64
 CPUs: 2
 Total Memory: 3.22GiB
 Name: minikube
 ID: 462Q:TJOC:6UQE:VT5O:7XAO:AS3J:5M6Q:VOT3:HXV2:HTVP:4TFY:4W7K
 Docker Root Dir: /var/lib/docker
 Debug Mode: false
 Registry: https://index.docker.io/v1/
 Labels:
  provider=kvm2
 Experimental: false
 Insecure Registries:
  10.96.0.0/12
  127.0.0.0/8
 Live Restore Enabled: false
 Product License: Community Engine

WARNING: No blkio throttle.read_bps_device support
WARNING: No blkio throttle.write_bps_device support
WARNING: No blkio throttle.read_iops_device support
WARNING: No blkio throttle.write_iops_device support
```


### Additional Info

This can also be reproduced without KubeVirt:
```
$ k apply -f - <<EOF
apiVersion: v1
kind: Pod
metadata:
  name: sleeper
spec:
  restartPolicy: Never
  terminationGracePeriodSeconds: 30
  containers:
  - name: sleeper
    image: busybox
    securityContext:
      allowPrivilegeEscalation: false
      capabilities:
        add:
        - NET_BIND_SERVICE
        drop:
        - ALL
      privileged: false
      runAsGroup: 107
      runAsNonRoot: true
      runAsUser: 107
    command:
    - /bin/sh
    - "-euxc"
    - |
      sleep infinity
EOF
$ k exec -ti sleeper -- sh
~ $ ps aux
PID   USER     TIME  COMMAND
    1 107       0:00 /bin/sh -euxc sleep infinity 
   13 107       0:00 sh
   19 107       0:00 ps aux
~ $ grep Cap /proc/1/status
CapInh:	0000000000000000
CapPrm:	0000000000000000
CapEff:	0000000000000000
CapBnd:	0000000000000400
CapAmb:	0000000000000000
```


Discussion / hints:
> I briefly looked at the sources. Though I am not 100% confident that this snippet is actually causing the problem, but the bellow code looked suspicious to me:

Those changes were part of docker 20.10.14; https://docs.docker.com/engine/release-notes/20.10/#201014 to address [CVE-2022-24769](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2022-24769)

/cc @samuelkarp 


> It was introduced by this commit 0d9a37d.

Sorry, I was wrong. That commit only refactored the existing code with the effective and permitted caps drop. I think the real commit is this one 349aeeab7c5
```go
	// if non root drop capabilities in the way execve does
	if s.Process.User.UID != 0 {
		s.Process.Capabilities.Effective = []string{}
		s.Process.Capabilities.Permitted = []string{}
	}
```

>  committed on Jun 16, 2018 

Looks pretty old...
What version of Kubernetes or cri-dockerd are you using? I don't think they're in the loop here, but a full context is helpful.
I was able to reproduce the problem on minikube with Kubernetes `v1.26.3`:
```
$ /usr/bin/cri-dockerd --version
cri-dockerd 0.3.1 (9a87d6a)
```
Forgot to mention that the binary has the capabilities set:
```
bash-5.1$ getcap /usr/bin/virt-launcher-monitor
/usr/bin/virt-launcher-monitor cap_net_bind_service=ep
```
Clearing out the effective and permitted capability sets for non-root users was introduced in #36587. https://github.com/moby/moby/pull/36587#issuecomment-391336084 notes that the `docker run --no-new-privileges` option stops file capabilities from working, which, if I'm not mistaken, is equivalent to `securityContext.allowPrivilegeEscalation=false` in a Pod spec.

Notably, there was work to change cri-containerd to implement the same capability-set-clearing behaviour as moby with lots of agreement that it should happen ([though it hasn't quite happened yet](https://github.com/containerd/containerd/pull/4669#issuecomment-1133156942)) which disproves the claim that moby's behaviour is wrong and the other projects are correct.
Hi @corhere, thank you for the clarification. This now makes more sense to me. 

> **allowPrivilegeEscalation**: Controls whether a process can gain more privileges than its parent process. **This bool directly controls whether the no_new_privs flag gets set on the container process**.

> With **no_new_privs** set, execve() promises not to grant the privilege to do anything that could not have been done without the execve call. For example, the setuid and setgid bits will no longer change the uid or gid; **file capabilities will not add to the permitted set**, and LSMs will not relax constraints after execve.

So apparently we now have different behavior in k8s depending on the container runtime.
Hi @corhere , @justincormack 
I am not quit getting why would you want to clear the capabilities before the `execve`.  `--no-new-privileges` should not stop file capabilities working, only those which are not explicitly requested imho.

Can you help me understand the reasoning behind this? I think docker should keep the capabilities requested by the user.

Fix patch:
diff --git a/integration/internal/container/ops.go b/integration/internal/container/ops.go
index fbc3b6b33bdbb..7f173ae0b9923 100644
--- a/integration/internal/container/ops.go
+++ b/integration/internal/container/ops.go
@@ -255,3 +255,21 @@ func WithCDIDevices(cdiDeviceNames ...string) func(*TestContainerConfig) {
 		c.HostConfig.DeviceRequests = append(c.HostConfig.DeviceRequests, request)
 	}
 }
+
+func WithCapability(capabilities ...string) func(*TestContainerConfig) {
+	return func(c *TestContainerConfig) {
+		c.HostConfig.CapAdd = append(c.HostConfig.CapAdd, capabilities...)
+	}
+}
+
+func WithDropCapability(capabilities ...string) func(*TestContainerConfig) {
+	return func(c *TestContainerConfig) {
+		c.HostConfig.CapDrop = append(c.HostConfig.CapDrop, capabilities...)
+	}
+}
+
+func WithSecurityOpt(opt string) func(*TestContainerConfig) {
+	return func(c *TestContainerConfig) {
+		c.HostConfig.SecurityOpt = append(c.HostConfig.SecurityOpt, opt)
+	}
+}
diff --git a/oci/oci.go b/oci/oci.go
index 864ccf5b60c41..45ed7979ee827 100644
--- a/oci/oci.go
+++ b/oci/oci.go
@@ -23,19 +23,10 @@ func SetCapabilities(s *specs.Spec, caplist []string) error {
 	if s.Process == nil {
 		s.Process = &specs.Process{}
 	}
-	// setUser has already been executed here
-	if s.Process.User.UID == 0 {
-		s.Process.Capabilities = &specs.LinuxCapabilities{
-			Effective: caplist,
-			Bounding:  caplist,
-			Permitted: caplist,
-		}
-	} else {
-		// Do not set Effective and Permitted capabilities for non-root users,
-		// to match what execve does.
-		s.Process.Capabilities = &specs.LinuxCapabilities{
-			Bounding: caplist,
-		}
+	s.Process.Capabilities = &specs.LinuxCapabilities{
+		Effective: caplist,
+		Bounding:  caplist,
+		Permitted: caplist,
 	}
 	return nil
 }


Test patch:
diff --git a/integration/capabilities/capabilities_linux_test.go b/integration/capabilities/capabilities_linux_test.go
new file mode 100644
index 0000000000000..272f3dcdb7c5c
--- /dev/null
+++ b/integration/capabilities/capabilities_linux_test.go
@@ -0,0 +1,108 @@
+package capabilities
+
+import (
+	"bytes"
+	"context"
+	"io"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/integration/internal/container"
+	"github.com/docker/docker/pkg/stdcopy"
+	"github.com/docker/docker/testutil/fakecontext"
+
+	"gotest.tools/v3/assert"
+	"gotest.tools/v3/poll"
+)
+
+func TestNoNewPrivileges(t *testing.T) {
+	defer setupTest(t)()
+
+	withFileCapability := `
+		FROM debian:bullseye-slim
+		RUN apt-get update && apt-get install -y libcap2-bin --no-install-recommends
+		RUN setcap CAP_DAC_OVERRIDE=+eip /bin/cat
+		RUN echo "hello" > /txt && chown 0:0 /txt && chmod 700 /txt
+		RUN useradd -u 1500 test
+	`
+	imageTag := "captest"
+
+	source := fakecontext.New(t, "", fakecontext.WithDockerfile(withFileCapability))
+	defer source.Close()
+
+	client := testEnv.APIClient()
+
+	// Build image
+	ctx := context.TODO()
+	resp, err := client.ImageBuild(ctx,
+		source.AsTarReader(t),
+		types.ImageBuildOptions{
+			Tags: []string{imageTag},
+		})
+	assert.NilError(t, err)
+	_, err = io.Copy(io.Discard, resp.Body)
+	assert.NilError(t, err)
+	resp.Body.Close()
+
+	testCases := []struct {
+		doc            string
+		opts           []func(*container.TestContainerConfig)
+		stdOut, stdErr string
+	}{
+		{
+			doc: "CapabilityRequested=true",
+			opts: []func(*container.TestContainerConfig){
+				container.WithUser("test"),
+				container.WithCapability("CAP_DAC_OVERRIDE"),
+			},
+			stdOut: "hello",
+		},
+		{
+			doc: "CapabilityRequested=false",
+			opts: []func(*container.TestContainerConfig){
+				container.WithUser("test"),
+				container.WithDropCapability("CAP_DAC_OVERRIDE"),
+			},
+			stdErr: "exec /bin/cat: operation not permitted",
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.doc, func(t *testing.T) {
+			// Run the container with the image
+			opts := append(tc.opts,
+				container.WithImage(imageTag),
+				container.WithCmd("/bin/cat", "/txt"),
+				container.WithSecurityOpt("no-new-privileges=true"),
+			)
+			cid := container.Run(ctx, t, client, opts...)
+			poll.WaitOn(t, container.IsInState(ctx, client, cid, "exited"), poll.WithDelay(100*time.Millisecond))
+
+			// Assert on outputs
+			logReader, err := client.ContainerLogs(ctx, cid, types.ContainerLogsOptions{
+				ShowStdout: true,
+				ShowStderr: true,
+			})
+			assert.NilError(t, err)
+			defer logReader.Close()
+
+			var actualStdout, actualStderr bytes.Buffer
+			_, err = stdcopy.StdCopy(&actualStdout, &actualStderr, logReader)
+			assert.NilError(t, err)
+
+			stdOut := strings.TrimSpace(actualStdout.String())
+			stdErr := strings.TrimSpace(actualStderr.String())
+			if stdOut != tc.stdOut {
+				t.Fatalf("test produced invalid output: %q, expected %q. Stderr:%q", stdOut, tc.stdOut, stdErr)
+			}
+			if stdErr != tc.stdErr {
+				t.Fatalf("test produced invalid error: %q, expected %q. Stdout:%q", stdErr, tc.stdErr, stdOut)
+
+			}
+		})
+	}
+
+}
diff --git a/integration/capabilities/main_linux_test.go b/integration/capabilities/main_linux_test.go
new file mode 100644
index 0000000000000..0f074dd15603e
--- /dev/null
+++ b/integration/capabilities/main_linux_test.go
@@ -0,0 +1,33 @@
+package capabilities
+
+import (
+	"fmt"
+	"os"
+	"testing"
+
+	"github.com/docker/docker/testutil/environment"
+)
+
+var testEnv *environment.Execution
+
+func TestMain(m *testing.M) {
+	var err error
+	testEnv, err = environment.New()
+	if err != nil {
+		fmt.Println(err)
+		os.Exit(1)
+	}
+	err = environment.EnsureFrozenImagesLinux(testEnv)
+	if err != nil {
+		fmt.Println(err)
+		os.Exit(1)
+	}
+
+	testEnv.Print()
+	os.Exit(m.Run())
+}
+
+func setupTest(t *testing.T) func() {
+	environment.ProtectAll(t, testEnv)
+	return func() { testEnv.Clean(t) }
+}