Don't bind host docker daemon socket #52

vicamo · 2023-04-24T13:19:49Z

vicamo commented

2023-04-24 13:19:49 +00:00

First-time contributor

Binding host docker daemon socket into executor containers can be a terrible security hole. Don't do this unless explicitly specified through customized mounts.

To exploit this issue, run following test job:

on:
  pull_request:
    branches:
      - 'main'

jobs:
  test:
    runs-on: ubuntu-latest
    container: docker:git
    steps:
      - name: Bind host rootfs
        run: |
          docker run --detach --name i_am_almighty -v /:/host-rootfs ubuntu:latest /bin/tail -f /dev/null
          docker exec i_am_almighty id
          docker exec i_am_almighty ls -la /host-rootfs

Binding host docker daemon socket into executor containers can be a terrible security hole. Don't do this unless explicitly specified through customized mounts. To exploit this issue, run following test job: ```yml on: pull_request: branches: - 'main' jobs: test: runs-on: ubuntu-latest container: docker:git steps: - name: Bind host rootfs run: | docker run --detach --name i_am_almighty -v /:/host-rootfs ubuntu:latest /bin/tail -f /dev/null docker exec i_am_almighty id docker exec i_am_almighty ls -la /host-rootfs ```

vicamo added 1 commit 2023-04-24 13:19:50 +00:00

Don't bind host docker daemon socket

Binding host docker daemon socket into executor containers can be a
terrible security hole. Don't do this unless explicitly specified
through customized mounts.

Signed-off-by: You-Sheng Yang <vicamo@gmail.com>

checks / check and test (pull_request) Has been cancelled

Details

c50c8bdd92

vicamo commented

2023-04-24 14:21:53 +00:00

First-time contributor

See also discussions on upstream nektos/act project.

See also [discussions](https://github.com/nektos/act/pull/1760) on upstream nektos/act project.

wolfogre commented

2023-04-25 02:59:42 +00:00

I see. I noticed that the outcome of the discussion is keeping unchanged to act. What do you think about act runner?

wolfogre added the

kind

bug

label 2023-04-25 03:00:11 +00:00

vicamo commented

2023-04-26 06:42:01 +00:00

First-time contributor

I see. I noticed that the outcome of the discussion is keeping unchanged to act. What do you think about act runner?

I think, based on the replies of upstream nektos/act maintainers, I was rushed into a premature pull request like this. It appears the problem lies in an upper level that Gitea's act_runner must be able to be configured as a spwaner to ephemeral execution environments to address such privilege escalation problems all at once. So far nektos/act doesn't have that, and the Draft: LXC support for self-hosted runners PR might be a closest attempt/viable solution. docker-machine is supported by GitLab, but Docker.com deprecated it already. Before ephemeral executor being possible, Gitea ACT backend should not ever be used in public/production environments in my opinion. Converting this into an issue or precondition of Gitea ACT backend instead might be a good idea at this moment.

It's also possible if Gitea decides to disable host docker socket binding as part of its CI container spec (don't know if this is already covered), then this PR can be useful.

> I see. I noticed that the outcome of the discussion is keeping unchanged to act. What do you think about act runner? I think, based on the replies of upstream nektos/act maintainers, I was rushed into a premature pull request like this. It appears the problem lies in an upper level that Gitea's act_runner must be able to be configured as a spwaner to ephemeral execution environments to address such privilege escalation problems all at once. So far nektos/act doesn't have that, and the [Draft: LXC support for self-hosted runners](https://github.com/nektos/act/pull/1682) PR might be a closest attempt/viable solution. `docker-machine` is supported by GitLab, but Docker.com deprecated it already. Before ephemeral executor being possible, Gitea ACT backend should not ever be used in public/production environments in my opinion. Converting this into an issue or precondition of Gitea ACT backend instead might be a good idea at this moment. It's also possible if Gitea decides to disable host docker socket binding as part of its CI container spec (don't know if this is already covered), then this PR can be useful.

vicamo commented

2023-04-26 06:56:24 +00:00

First-time contributor

It's also possible if Gitea decides to disable host docker socket binding as part of its CI container spec (don't know if this is already covered), then this PR can be useful.

The reason upstream nektos/act has host docker daemon bound is that: 1) GitHub Actions Runner has it, 2) nektos/act is meant for running GitHub Actions yml locally. Neither of them is true for Gitea. Gitea may enforce a more restricted executor environment than GitHub does. And if Gitea takes this way, probably the "self-hosted" feature should be disabled from Gitea's act fork as well.

> It's also possible if Gitea decides to disable host docker socket binding as part of its CI container spec (don't know if this is already covered), then this PR can be useful. The reason upstream nektos/act has host docker daemon bound is that: 1) GitHub Actions Runner has it, 2) nektos/act is meant for running GitHub Actions yml locally. Neither of them is true for Gitea. Gitea may enforce a more restricted executor environment than GitHub does. And if Gitea takes this way, probably the "self-hosted" feature should be disabled from Gitea's act fork as well.

lunny referenced this pull request from gitea/act_runner

2023-04-30 10:43:00 +00:00

Gitea act_runner is HIGHLY insecure due to binding of docker.sock into all containers (= root on host) #167

lunny commented

2023-04-30 11:07:27 +00:00

I think we should fix it but we also need to allow users to add the mapping explicitly.

vitalif commented

2023-04-30 11:28:14 +00:00

First-time contributor

I think we should fix it but we also need to allow users to add the mapping explicitly.

If you always allow it then it basically cancels all the protection %) it should be disabled if privileged option is disabled in runner config. Also you should remove bind mounts from jobs.container.volumes to remove job access to host FS.

> I think we should fix it but we also need to allow users to add the mapping explicitly. If you always allow it then it basically cancels all the protection %) it should be disabled if privileged option is disabled in runner config. Also you should remove bind mounts from `jobs.container.volumes` to remove job access to host FS.

vitalif commented

2023-04-30 11:28:57 +00:00

First-time contributor

So this PR is not enough %)

vitalif referenced this pull request from gitea/act_runner

2023-04-30 22:32:53 +00:00

Gitea act_runner is HIGHLY insecure due to binding of docker.sock into all containers (= root on host) #167

vicamo commented

2023-05-01 08:59:28 +00:00

First-time contributor

I think we should fix it but we also need to allow users to add the mapping explicitly.

If you always allow it then it basically cancels all the protection %) it should be disabled if privileged option is disabled in runner config. Also you should remove bind mounts from jobs.container.volumes to remove job access to host FS.

Do Gitea CI really have to support mounting user-specified volumes? In my opinion, host docker daemon socket should never be exposed to job containers even when privileged flags is on. For users trying to build docker images or taking actions that require a docker daemon, docker:dind service container should be used, and the only volume shared in between should be work dir. This is also how GitLab CI runner works if I remember correctly.

And there are two more hard-coded volumes being mounted:

volume name act-toolcache, target /toolcache:
- should really be /opt/hostedtoolcache as there is an upstream branch https://github.com/nektos/act/tree/fix-toolcache pointed out. Yet that branch is not yet merged in upstream. Don't know why.
- automatically created? and then reused between all job containers? It's mounted rw, then it's also a security hole.
volume name rc.jobContainerName() + "-env", target /var/run/act (from LinuxContainerEnvironmentExtensions.GetActPath()):
- created automatically by docker CLI. Not sure if it might be shared between job containers of the same name of two different sessions. If so, also a security hole here.

> > I think we should fix it but we also need to allow users to add the mapping explicitly. > > If you always allow it then it basically cancels all the protection %) it should be disabled if privileged option is disabled in runner config. Also you should remove bind mounts from `jobs.container.volumes` to remove job access to host FS. Do Gitea CI really have to support mounting user-specified volumes? In my opinion, host docker daemon socket should never be exposed to job containers even when privileged flags is on. For users trying to build docker images or taking actions that require a docker daemon, `docker:dind` service container should be used, and the only volume shared in between should be work dir. This is also how GitLab CI runner works if I remember correctly. And there are two more hard-coded volumes being mounted: * volume name **act-toolcache**, target `/toolcache`: - should really be `/opt/hostedtoolcache` as there is an upstream branch https://github.com/nektos/act/tree/fix-toolcache pointed out. Yet that branch is not yet merged in upstream. Don't know why. - automatically created? and then reused between all job containers? It's mounted rw, then it's also a security hole. * volume name `rc.jobContainerName() + "-env"`, target `/var/run/act` (from `LinuxContainerEnvironmentExtensions.GetActPath()`): - created automatically by docker CLI. Not sure if it might be shared between job containers of the same name of two different sessions. If so, also a security hole here.

lunny commented

2023-05-01 14:48:40 +00:00

I think we should fix it but we also need to allow users to add the mapping explicitly.

If you always allow it then it basically cancels all the protection %) it should be disabled if privileged option is disabled in runner config. Also you should remove bind mounts from jobs.container.volumes to remove job access to host FS.

Do Gitea CI really have to support mounting user-specified volumes? In my opinion, host docker daemon socket should never be exposed to job containers even when privileged flags is on. For users trying to build docker images or taking actions that require a docker daemon, docker:dind service container should be used, and the only volume shared in between should be work dir. This is also how GitLab CI runner works if I remember correctly.

And there are two more hard-coded volumes being mounted:

volume name act-toolcache, target /toolcache:

should really be /opt/hostedtoolcache as there is an upstream branch https://github.com/nektos/act/tree/fix-toolcache pointed out. Yet that branch is not yet merged in upstream. Don't know why.

automatically created? and then reused between all job containers? It's mounted rw, then it's also a security hole.

volume name rc.jobContainerName() + "-env", target /var/run/act (from LinuxContainerEnvironmentExtensions.GetActPath()):

created automatically by docker CLI. Not sure if it might be shared between job containers of the same name of two different sessions. If so, also a security hole here.

I think yes. There are different scenes. For a company/team internal usage, this maybe not a real problem.

> > > I think we should fix it but we also need to allow users to add the mapping explicitly. > > > > If you always allow it then it basically cancels all the protection %) it should be disabled if privileged option is disabled in runner config. Also you should remove bind mounts from `jobs.container.volumes` to remove job access to host FS. > > Do Gitea CI really have to support mounting user-specified volumes? In my opinion, host docker daemon socket should never be exposed to job containers even when privileged flags is on. For users trying to build docker images or taking actions that require a docker daemon, `docker:dind` service container should be used, and the only volume shared in between should be work dir. This is also how GitLab CI runner works if I remember correctly. > > And there are two more hard-coded volumes being mounted: > * volume name **act-toolcache**, target `/toolcache`: > - should really be `/opt/hostedtoolcache` as there is an upstream branch https://github.com/nektos/act/tree/fix-toolcache pointed out. Yet that branch is not yet merged in upstream. Don't know why. > - automatically created? and then reused between all job containers? It's mounted rw, then it's also a security hole. > * volume name `rc.jobContainerName() + "-env"`, target `/var/run/act` (from `LinuxContainerEnvironmentExtensions.GetActPath()`): > - created automatically by docker CLI. Not sure if it might be shared between job containers of the same name of two different sessions. If so, also a security hole here. I think yes. There are different scenes. For a company/team internal usage, this maybe not a real problem.

ChristopherHX commented

2023-05-01 19:19:49 +00:00

First-time contributor

Sounds like you want to remove all mounts, here a patch for act. Which makes it impossible to add any kind of mounts. By just removing them before sending the request to docker.

diff --git a/pkg/container/docker_run.go b/pkg/container/docker_run.go
index 7be7448..35c6219 100644
--- a/pkg/container/docker_run.go
+++ b/pkg/container/docker_run.go
@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"path"
 	"path/filepath"
 	"regexp"
 	"runtime"
@@ -444,6 +445,8 @@ func (cr *containerReference) create(capAdd []string, capDrop []string) common.E
 		if err != nil {
 			return err
 		}
+		hostConfig.Binds = nil
+		hostConfig.Mounts = nil
 
 		resp, err := cr.cli.ContainerCreate(ctx, config, hostConfig, nil, platSpecs, input.Name)
 		if err != nil {
@@ -753,7 +756,7 @@ func (cr *containerReference) copyContent(dstPath string, files ...*FileEntry) c
 		for _, file := range files {
 			logger.Debugf("Writing entry to tarball %s len:%d", file.Name, len(file.Body))
 			hdr := &tar.Header{
-				Name: file.Name,
+				Name: path.Join(dstPath, file.Name),
 				Mode: file.Mode,
 				Size: int64(len(file.Body)),
 				Uid:  cr.UID,
@@ -771,7 +774,7 @@ func (cr *containerReference) copyContent(dstPath string, files ...*FileEntry) c
 		}
 
 		logger.Debugf("Extracting content to '%s'", dstPath)
-		err := cr.cli.CopyToContainer(ctx, cr.id, dstPath, &buf, types.CopyToContainerOptions{})
+		err := cr.cli.CopyToContainer(ctx, cr.id, "/", &buf, types.CopyToContainerOptions{})
 		if err != nil {
 			return fmt.Errorf("failed to copy content to container: %w", err)
 		}

Together with https://github.com/nektos/act/pull/1783 and a list of act-runner labels with docker://, only other docker flags can make concerning things.

Without opt out this security hardening breaks other workflows. DIND is not really supported for act job container.

Sounds like you want to remove all mounts, here a patch for act. Which makes it impossible to add any kind of mounts. By just removing them before sending the request to docker. ```diff diff --git a/pkg/container/docker_run.go b/pkg/container/docker_run.go index 7be7448..35c6219 100644 --- a/pkg/container/docker_run.go +++ b/pkg/container/docker_run.go @@ -10,6 +10,7 @@ import ( "fmt" "io" "os" + "path" "path/filepath" "regexp" "runtime" @@ -444,6 +445,8 @@ func (cr *containerReference) create(capAdd []string, capDrop []string) common.E if err != nil { return err } + hostConfig.Binds = nil + hostConfig.Mounts = nil resp, err := cr.cli.ContainerCreate(ctx, config, hostConfig, nil, platSpecs, input.Name) if err != nil { @@ -753,7 +756,7 @@ func (cr *containerReference) copyContent(dstPath string, files ...*FileEntry) c for _, file := range files { logger.Debugf("Writing entry to tarball %s len:%d", file.Name, len(file.Body)) hdr := &tar.Header{ - Name: file.Name, + Name: path.Join(dstPath, file.Name), Mode: file.Mode, Size: int64(len(file.Body)), Uid: cr.UID, @@ -771,7 +774,7 @@ func (cr *containerReference) copyContent(dstPath string, files ...*FileEntry) c } logger.Debugf("Extracting content to '%s'", dstPath) - err := cr.cli.CopyToContainer(ctx, cr.id, dstPath, &buf, types.CopyToContainerOptions{}) + err := cr.cli.CopyToContainer(ctx, cr.id, "/", &buf, types.CopyToContainerOptions{}) if err != nil { return fmt.Errorf("failed to copy content to container: %w", err) } ``` Together with https://github.com/nektos/act/pull/1783 and a list of act-runner labels with `docker://`, only other docker flags can make concerning things. Without opt out this security hardening breaks other workflows. DIND is not really supported for act job container.

vicamo commented

2023-05-02 08:11:30 +00:00

First-time contributor

Sounds like you want to remove all mounts, here a patch for act. Which makes it impossible to add any kind of mounts. By just removing them before sending the request to docker.

I'd like to remove any hard-coded mounts in the source code as possible, especially read-write ones who may live across the job container's life time.

In addition, volume mounts, additional docker options should not be allowed, unless the job was executed inside an ephemeral VM. And since nektos/act is not capable of launching ephemeral VM yet, these features should be disabled when Gitea Actions is launched as is.

Together with https://github.com/nektos/act/pull/1783 and a list of act-runner labels with docker://, only other docker flags can make concerning things.

Without opt out this security hardening breaks other workflows. DIND is not really supported for act job container.

I think DIND is currently supported through services container? Except that act currently binds host docker daemon socket into job containers, so docker client would connect to host docker daemon directly.

> Sounds like you want to remove all mounts, here a patch for act. Which makes it impossible to add any kind of mounts. By just removing them before sending the request to docker. I'd like to remove any hard-coded mounts in the source code as possible, especially read-write ones who may live across the job container's life time. In addition, volume mounts, additional docker options should not be allowed, unless the job was executed inside an ephemeral VM. And since nektos/act is not capable of launching ephemeral VM yet, these features should be disabled when Gitea Actions is launched as is. > Together with https://github.com/nektos/act/pull/1783 and a list of act-runner labels with `docker://`, only other docker flags can make concerning things. > > Without opt out this security hardening breaks other workflows. DIND is not really supported for act job container. I think DIND is currently supported through services container? Except that act currently binds host docker daemon socket into job containers, so docker client would connect to host docker daemon directly.

lunny commented

2023-10-30 06:02:45 +00:00

I think this is outdated. Now there an option from config.

	if rc.Config.ContainerDaemonSocket != "-" {
		daemonPath := getDockerDaemonSocketMountPath(rc.Config.ContainerDaemonSocket)
		binds = append(binds, fmt.Sprintf("%s:%s", daemonPath, "/var/run/docker.sock"))
	}

I think this is outdated. Now there an option from config. ```go if rc.Config.ContainerDaemonSocket != "-" { daemonPath := getDockerDaemonSocketMountPath(rc.Config.ContainerDaemonSocket) binds = append(binds, fmt.Sprintf("%s:%s", daemonPath, "/var/run/docker.sock")) } ```

lunny closed this pull request

2023-11-01 01:32:49 +00:00

vicamo deleted branch for-upstream/dont-bind-host-docker-socket

2023-12-22 09:05:42 +00:00

checks / check and test (pull_request) Has been cancelled

Details