Make container resource mounts unbindable

It's a common scenario for admins and/or monitoring applications to
mount in the daemon root dir into a container. When doing so all mounts
get coppied into the container, often with private references.
This can prevent removal of a container due to the various mounts that
must be configured before a container is started (for example, for
shared /dev/shm, or secrets) being leaked into another namespace,
usually with private references.

This is particularly problematic on older kernels (e.g. RHEL < 7.4)
where a mount may be active in another namespace and attempting to
remove a mountpoint which is active in another namespace fails.

This change moves all container resource mounts into a common directory
so that the directory can be made unbindable.
What this does is prevents sub-mounts of this new directory from leaking
into other namespaces when mounted with `rbind`... which is how all
binds are handled for containers.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

container/container.go

@@ -1022,14 +1022,23 @@ func (container *Container) InitializeStdio(iop *cio.DirectIO) (cio.IO, error) {
 	return &rio{IO: iop, sc: container.StreamConfig}, nil
 }
 
+// MountsResourcePath returns the path where mounts are stored for the given mount
+func (container *Container) MountsResourcePath(mount string) (string, error) {
+	return container.GetRootResourcePath(filepath.Join("mounts", mount))
+}
+
 // SecretMountPath returns the path of the secret mount for the container
-func (container *Container) SecretMountPath() string {
+func (container *Container) SecretMountPath() (string, error) {
-	return filepath.Join(container.Root, "secrets")
+	return container.MountsResourcePath("secrets")
 }
 
 // SecretFilePath returns the path to the location of a secret on the host.
-func (container *Container) SecretFilePath(secretRef swarmtypes.SecretReference) string {
+func (container *Container) SecretFilePath(secretRef swarmtypes.SecretReference) (string, error) {
-	return filepath.Join(container.SecretMountPath(), secretRef.SecretID)
+	secrets, err := container.SecretMountPath()
+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(secrets, secretRef.SecretID), nil
 }
 
 func getSecretTargetPath(r *swarmtypes.SecretReference) string {
@@ -1042,13 +1051,17 @@ func getSecretTargetPath(r *swarmtypes.SecretReference) string {
 
 // ConfigsDirPath returns the path to the directory where configs are stored on
 // disk.
-func (container *Container) ConfigsDirPath() string {
+func (container *Container) ConfigsDirPath() (string, error) {
-	return filepath.Join(container.Root, "configs")
+	return container.GetRootResourcePath("configs")
 }
 
 // ConfigFilePath returns the path to the on-disk location of a config.
-func (container *Container) ConfigFilePath(configRef swarmtypes.ConfigReference) string {
+func (container *Container) ConfigFilePath(configRef swarmtypes.ConfigReference) (string, error) {
-	return filepath.Join(container.ConfigsDirPath(), configRef.ConfigID)
+	configs, err := container.ConfigsDirPath()
+	if err != nil {
+		return "", err
+	}
+	return filepath.Join(configs, configRef.ConfigID), nil
 }
 
 // CreateDaemonEnvironment creates a new environment variable slice for this container.

container/container_unix.go

@@ -151,7 +151,7 @@ func (container *Container) CopyImagePathContent(v volume.Volume, destination st
 
 // ShmResourcePath returns path to shm
 func (container *Container) ShmResourcePath() (string, error) {
-	return container.GetRootResourcePath("shm")
+	return container.MountsResourcePath("shm")
 }
 
 // HasMountFor checks if path is a mountpoint
@@ -218,49 +218,61 @@ func (container *Container) IpcMounts() []Mount {
 }
 
 // SecretMounts returns the mounts for the secret path.
-func (container *Container) SecretMounts() []Mount {
+func (container *Container) SecretMounts() ([]Mount, error) {
 	var mounts []Mount
 	for _, r := range container.SecretReferences {
 		if r.File == nil {
 			continue
 		}
+		src, err := container.SecretFilePath(*r)
+		if err != nil {
+			return nil, err
+		}
 		mounts = append(mounts, Mount{
-			Source:      container.SecretFilePath(*r),
+			Source:      src,
 			Destination: getSecretTargetPath(r),
 			Writable:    false,
 		})
 	}
 
-	return mounts
+	return mounts, nil
 }
 
 // UnmountSecrets unmounts the local tmpfs for secrets
 func (container *Container) UnmountSecrets() error {
-	if _, err := os.Stat(container.SecretMountPath()); err != nil {
+	p, err := container.SecretMountPath()
+	if err != nil {
+		return err
+	}
+	if _, err := os.Stat(p); err != nil {
 		if os.IsNotExist(err) {
 			return nil
 		}
 		return err
 	}
 
-	return detachMounted(container.SecretMountPath())
+	return mount.RecursiveUnmount(p)
 }
 
 // ConfigMounts returns the mounts for configs.
-func (container *Container) ConfigMounts() []Mount {
+func (container *Container) ConfigMounts() ([]Mount, error) {
 	var mounts []Mount
 	for _, configRef := range container.ConfigReferences {
 		if configRef.File == nil {
 			continue
 		}
+		src, err := container.ConfigFilePath(*configRef)
+		if err != nil {
+			return nil, err
+		}
 		mounts = append(mounts, Mount{
-			Source:      container.ConfigFilePath(*configRef),
+			Source:      src,
 			Destination: configRef.File.Name,
 			Writable:    false,
 		})
 	}
 
-	return mounts
+	return mounts, nil
 }
 
 type conflictingUpdateOptions string

container/container_windows.go

@@ -54,22 +54,30 @@ func (container *Container) CreateSecretSymlinks() error {
 // SecretMounts returns the mount for the secret path.
 // All secrets are stored in a single mount on Windows. Target symlinks are
 // created for each secret, pointing to the files in this mount.
-func (container *Container) SecretMounts() []Mount {
+func (container *Container) SecretMounts() ([]Mount, error) {
 	var mounts []Mount
 	if len(container.SecretReferences) > 0 {
+		src, err := container.SecretMountPath()
+		if err != nil {
+			return nil, err
+		}
 		mounts = append(mounts, Mount{
-			Source:      container.SecretMountPath(),
+			Source:      src,
 			Destination: containerInternalSecretMountPath,
 			Writable:    false,
 		})
 	}
 
-	return mounts
+	return mounts, nil
 }
 
 // UnmountSecrets unmounts the fs for secrets
 func (container *Container) UnmountSecrets() error {
-	return os.RemoveAll(container.SecretMountPath())
+	p, err := container.SecretMountPath()
+	if err != nil {
+		return err
+	}
+	return os.RemoveAll(p)
 }
 
 // CreateConfigSymlinks creates symlinks to files in the config mount.
@@ -96,17 +104,21 @@ func (container *Container) CreateConfigSymlinks() error {
 // ConfigMounts returns the mount for configs.
 // All configs are stored in a single mount on Windows. Target symlinks are
 // created for each config, pointing to the files in this mount.
-func (container *Container) ConfigMounts() []Mount {
+func (container *Container) ConfigMounts() ([]Mount, error) {
 	var mounts []Mount
 	if len(container.ConfigReferences) > 0 {
+		src, err := container.ConfigsDirPath()
+		if err != nil {
+			return nil, err
+		}
 		mounts = append(mounts, Mount{
-			Source:      container.ConfigsDirPath(),
+			Source:      src,
 			Destination: containerInternalConfigsDirPath,
 			Writable:    false,
 		})
 	}
 
-	return mounts
+	return mounts, nil
 }
 
 // DetachAndUnmount unmounts all volumes.

daemon/container_operations_unix.go

@@ -165,7 +165,10 @@ func (daemon *Daemon) setupSecretDir(c *container.Container) (setupErr error) {
 		return nil
 	}
 
-	localMountPath := c.SecretMountPath()
+	localMountPath, err := c.SecretMountPath()
+	if err != nil {
+		return errors.Wrap(err, "error getting secrets mount dir")
+	}
 	logrus.Debugf("secrets: setting up secret dir: %s", localMountPath)
 
 	// retrieve possible remapped range start for root UID, GID
@@ -204,7 +207,10 @@ func (daemon *Daemon) setupSecretDir(c *container.Container) (setupErr error) {
 
 		// secrets are created in the SecretMountPath on the host, at a
 		// single level
-		fPath := c.SecretFilePath(*s)
+		fPath, err := c.SecretFilePath(*s)
+		if err != nil {
+			return errors.Wrap(err, "error getting secret file path")
+		}
 		if err := idtools.MkdirAllAndChown(filepath.Dir(fPath), 0700, rootIDs); err != nil {
 			return errors.Wrap(err, "error creating secret mount path")
 		}
@@ -250,7 +256,10 @@ func (daemon *Daemon) setupConfigDir(c *container.Container) (setupErr error) {
 		return nil
 	}
 
-	localPath := c.ConfigsDirPath()
+	localPath, err := c.ConfigsDirPath()
+	if err != nil {
+		return err
+	}
 	logrus.Debugf("configs: setting up config dir: %s", localPath)
 
 	// retrieve possible remapped range start for root UID, GID
@@ -279,7 +288,10 @@ func (daemon *Daemon) setupConfigDir(c *container.Container) (setupErr error) {
 			continue
 		}
 
-		fPath := c.ConfigFilePath(*configRef)
+		fPath, err := c.ConfigFilePath(*configRef)
+		if err != nil {
+			return err
+		}
 
 		log := logrus.WithFields(logrus.Fields{"name": configRef.File.Name, "path": fPath})
 
@@ -379,3 +391,22 @@ func (daemon *Daemon) initializeNetworkingPaths(container *container.Container,
 	container.ResolvConfPath = nc.ResolvConfPath
 	return nil
 }
+
+func (daemon *Daemon) setupContainerMountsRoot(c *container.Container) error {
+	// get the root mount path so we can make it unbindable
+	p, err := c.MountsResourcePath("")
+	if err != nil {
+		return err
+	}
+
+	if err := idtools.MkdirAllAndChown(p, 0700, daemon.idMappings.RootPair()); err != nil {
+		return err
+	}
+
+	if err := mount.MakeUnbindable(p); err != nil {
+		// Setting unbindable is a precaution and is not neccessary for correct operation.
+		// Do not error out if this fails.
+		logrus.WithError(err).WithField("resource", p).WithField("container", c.ID).Warn("Error setting container resource mounts to unbindable, this may cause mount leakages, preventing removal of this container.")
+	}
+	return nil
+}

daemon/container_operations_windows.go

@@ -21,7 +21,10 @@ func (daemon *Daemon) setupConfigDir(c *container.Container) (setupErr error) {
 		return nil
 	}
 
-	localPath := c.ConfigsDirPath()
+	localPath, err := c.ConfigsDirPath()
+	if err != nil {
+		return err
+	}
 	logrus.Debugf("configs: setting up config dir: %s", localPath)
 
 	// create local config root
@@ -48,7 +51,10 @@ func (daemon *Daemon) setupConfigDir(c *container.Container) (setupErr error) {
 			continue
 		}
 
-		fPath := c.ConfigFilePath(*configRef)
+		fPath, err := c.ConfigFilePath(*configRef)
+		if err != nil {
+			return err
+		}
 
 		log := logrus.WithFields(logrus.Fields{"name": configRef.File.Name, "path": fPath})
 
@@ -94,7 +100,10 @@ func (daemon *Daemon) setupSecretDir(c *container.Container) (setupErr error) {
 		return nil
 	}
 
-	localMountPath := c.SecretMountPath()
+	localMountPath, err := c.SecretMountPath()
+	if err != nil {
+		return err
+	}
 	logrus.Debugf("secrets: setting up secret dir: %s", localMountPath)
 
 	// create local secret root
@@ -123,7 +132,10 @@ func (daemon *Daemon) setupSecretDir(c *container.Container) (setupErr error) {
 
 		// secrets are created in the SecretMountPath on the host, at a
 		// single level
-		fPath := c.SecretFilePath(*s)
+		fPath, err := c.SecretFilePath(*s)
+		if err != nil {
+			return err
+		}
 		logrus.WithFields(logrus.Fields{
 			"name": s.File.Name,
 			"path": fPath,

daemon/oci_linux.go

@@ -812,6 +812,10 @@ func (daemon *Daemon) createSpec(c *container.Container) (*specs.Spec, error) {
 		return nil, fmt.Errorf("linux seccomp: %v", err)
 	}
 
+	if err := daemon.setupContainerMountsRoot(c); err != nil {
+		return nil, err
+	}
+
 	if err := daemon.setupIpcDirs(c); err != nil {
 		return nil, err
 	}
@@ -839,11 +843,17 @@ func (daemon *Daemon) createSpec(c *container.Container) (*specs.Spec, error) {
 	}
 	ms = append(ms, tmpfsMounts...)
 
-	if m := c.SecretMounts(); m != nil {
+	secretMounts, err := c.SecretMounts()
-		ms = append(ms, m...)
+	if err != nil {
+		return nil, err
 	}
+	ms = append(ms, secretMounts...)
 
-	ms = append(ms, c.ConfigMounts()...)
+	configMounts, err := c.ConfigMounts()
+	if err != nil {
+		return nil, err
+	}
+	ms = append(ms, configMounts...)
 
 	sort.Sort(mounts(ms))
 	if err := setMounts(daemon, &s, c, ms); err != nil {

daemon/oci_windows.go

@@ -93,12 +93,20 @@ func (daemon *Daemon) createSpec(c *container.Container) (*specs.Spec, error) {
 		}
 	}
 
-	if m := c.SecretMounts(); m != nil {
+	secretMounts, err := c.SecretMounts()
-		mounts = append(mounts, m...)
+	if err != nil {
+		return nil, err
+	}
+	if secretMounts != nil {
+		mounts = append(mounts, secretMounts...)
 	}
 
-	if m := c.ConfigMounts(); m != nil {
+	configMounts, err := c.ConfigMounts()
-		mounts = append(mounts, m...)
+	if err != nil {
+		return nil, err
+	}
+	if configMounts != nil {
+		mounts = append(mounts, configMounts...)
 	}
 
 	for _, mount := range mounts {

daemon/start.go

@@ -9,6 +9,7 @@ import (
 	containertypes "github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/container"
 	"github.com/docker/docker/errdefs"
+	"github.com/docker/docker/pkg/mount"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
@@ -231,6 +232,10 @@ func (daemon *Daemon) Cleanup(container *container.Container) {
 		logrus.Warnf("%s cleanup: failed to unmount secrets: %s", container.ID, err)
 	}
 
+	if err := mount.RecursiveUnmount(container.Root); err != nil {
+		logrus.WithError(err).WithField("container", container.ID).Warn("Error while cleaning up container resource mounts.")
+	}
+
 	for _, eConfig := range container.ExecCommands.Commands() {
 		daemon.unregisterExecCommand(container, eConfig)
 	}

integration/container/mounts_linux_test.go

@@ -0,0 +1,84 @@
+package container
+
+import (
+	"bytes"
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/api/types/container"
+	"github.com/docker/docker/api/types/mount"
+	"github.com/docker/docker/integration-cli/daemon"
+	"github.com/docker/docker/pkg/stdcopy"
+)
+
+func TestContainerShmNoLeak(t *testing.T) {
+	t.Parallel()
+	d := daemon.New(t, "docker", "dockerd", daemon.Config{})
+	client, err := d.NewClient()
+	if err != nil {
+		t.Fatal(err)
+	}
+	d.StartWithBusybox(t)
+	defer d.Stop(t)
+
+	ctx := context.Background()
+	cfg := container.Config{
+		Image: "busybox",
+		Cmd:   []string{"top"},
+	}
+
+	ctr, err := client.ContainerCreate(ctx, &cfg, nil, nil, "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer client.ContainerRemove(ctx, ctr.ID, types.ContainerRemoveOptions{Force: true})
+
+	if err := client.ContainerStart(ctx, ctr.ID, types.ContainerStartOptions{}); err != nil {
+		t.Fatal(err)
+	}
+
+	// this should recursively bind mount everything in the test daemons root
+	// except of course we are hoping that the previous containers /dev/shm mount did not leak into this new container
+	hc := container.HostConfig{
+		Mounts: []mount.Mount{
+			{
+				Type:        mount.TypeBind,
+				Source:      d.Root,
+				Target:      "/testdaemonroot",
+				BindOptions: &mount.BindOptions{Propagation: mount.PropagationRPrivate}},
+		},
+	}
+	cfg.Cmd = []string{"/bin/sh", "-c", fmt.Sprintf("mount | grep testdaemonroot | grep containers | grep %s", ctr.ID)}
+	cfg.AttachStdout = true
+	cfg.AttachStderr = true
+	ctrLeak, err := client.ContainerCreate(ctx, &cfg, &hc, nil, "")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	attach, err := client.ContainerAttach(ctx, ctrLeak.ID, types.ContainerAttachOptions{
+		Stream: true,
+		Stdout: true,
+		Stderr: true,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err := client.ContainerStart(ctx, ctrLeak.ID, types.ContainerStartOptions{}); err != nil {
+		t.Fatal(err)
+	}
+
+	buf := bytes.NewBuffer(nil)
+
+	if _, err := stdcopy.StdCopy(buf, buf, attach.Reader); err != nil {
+		t.Fatal(err)
+	}
+
+	out := bytes.TrimSpace(buf.Bytes())
+	if !bytes.Equal(out, []byte{}) {
+		t.Fatalf("mount leaked: %s", string(out))
+	}
+}