From eaa5192856c1ad09614318e88030554b96bb6e81 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Mon, 18 Dec 2017 16:02:23 -0500 Subject: [PATCH] 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 --- container/container.go | 29 +++++--- container/container_unix.go | 30 +++++--- container/container_windows.go | 26 +++++-- daemon/container_operations_unix.go | 39 ++++++++-- daemon/container_operations_windows.go | 20 ++++-- daemon/oci_linux.go | 16 ++++- daemon/oci_windows.go | 16 +++-- daemon/start.go | 5 ++ integration/container/mounts_linux_test.go | 84 ++++++++++++++++++++++ 9 files changed, 226 insertions(+), 39 deletions(-) create mode 100644 integration/container/mounts_linux_test.go diff --git a/container/container.go b/container/container.go index dd11a11543..f19f9893f4 100644 --- a/container/container.go +++ b/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 { - return filepath.Join(container.Root, "secrets") +func (container *Container) SecretMountPath() (string, error) { + 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 { - return filepath.Join(container.SecretMountPath(), secretRef.SecretID) +func (container *Container) SecretFilePath(secretRef swarmtypes.SecretReference) (string, error) { + 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 { - return filepath.Join(container.Root, "configs") +func (container *Container) ConfigsDirPath() (string, error) { + return container.GetRootResourcePath("configs") } // ConfigFilePath returns the path to the on-disk location of a config. -func (container *Container) ConfigFilePath(configRef swarmtypes.ConfigReference) string { - return filepath.Join(container.ConfigsDirPath(), configRef.ConfigID) +func (container *Container) ConfigFilePath(configRef swarmtypes.ConfigReference) (string, error) { + 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. diff --git a/container/container_unix.go b/container/container_unix.go index f030232060..63ad2389c4 100644 --- a/container/container_unix.go +++ b/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 diff --git a/container/container_windows.go b/container/container_windows.go index 92b50a6eb6..94a4e1c0b0 100644 --- a/container/container_windows.go +++ b/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. diff --git a/daemon/container_operations_unix.go b/daemon/container_operations_unix.go index 9fe899b623..a123df3a9a 100644 --- a/daemon/container_operations_unix.go +++ b/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 +} diff --git a/daemon/container_operations_windows.go b/daemon/container_operations_windows.go index 51762a2441..c762fc99cb 100644 --- a/daemon/container_operations_windows.go +++ b/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, diff --git a/daemon/oci_linux.go b/daemon/oci_linux.go index 757de85366..53dc276565 100644 --- a/daemon/oci_linux.go +++ b/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 { - ms = append(ms, m...) + secretMounts, err := c.SecretMounts() + 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 { diff --git a/daemon/oci_windows.go b/daemon/oci_windows.go index c7c94f327a..9f135b87af 100644 --- a/daemon/oci_windows.go +++ b/daemon/oci_windows.go @@ -93,12 +93,20 @@ func (daemon *Daemon) createSpec(c *container.Container) (*specs.Spec, error) { } } - if m := c.SecretMounts(); m != nil { - mounts = append(mounts, m...) + secretMounts, err := c.SecretMounts() + if err != nil { + return nil, err + } + if secretMounts != nil { + mounts = append(mounts, secretMounts...) } - if m := c.ConfigMounts(); m != nil { - mounts = append(mounts, m...) + configMounts, err := c.ConfigMounts() + if err != nil { + return nil, err + } + if configMounts != nil { + mounts = append(mounts, configMounts...) } for _, mount := range mounts { diff --git a/daemon/start.go b/daemon/start.go index 008fb050d4..af9bfda014 100644 --- a/daemon/start.go +++ b/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) } diff --git a/integration/container/mounts_linux_test.go b/integration/container/mounts_linux_test.go new file mode 100644 index 0000000000..8c13258c30 --- /dev/null +++ b/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)) + } +}