From 42716dcf5c986e4cbb51f480f2782c05e5bd0b41 Mon Sep 17 00:00:00 2001 From: Phil Estes Date: Sat, 22 Jul 2017 22:11:09 -0400 Subject: [PATCH] Only chown network files within container metadata If the user specifies a mountpath from the host, we should not be attempting to chown files outside the daemon's metadata directory (represented by `daemon.repository` at init time). This forces users who want to use user namespaces to handle the ownership needs of any external files mounted as network files (/etc/resolv.conf, /etc/hosts, /etc/hostname) separately from the daemon. In all other volume/bind mount situations we have taken this same line--we don't chown host file content. Docker-DCO-1.1-Signed-off-by: Phil Estes --- daemon/volumes_unix.go | 9 ++- .../docker_api_containers_unix_test.go | 77 +++++++++++++++++++ integration-cli/docker_cli_run_test.go | 15 ++++ 3 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 integration-cli/docker_api_containers_unix_test.go diff --git a/daemon/volumes_unix.go b/daemon/volumes_unix.go index 0a4cbf8493..a88e0f1530 100644 --- a/daemon/volumes_unix.go +++ b/daemon/volumes_unix.go @@ -86,8 +86,13 @@ func (daemon *Daemon) setupMounts(c *container.Container) ([]container.Mount, er // remapped root (user namespaces) rootIDs := daemon.idMappings.RootPair() for _, mount := range netMounts { - if err := os.Chown(mount.Source, rootIDs.UID, rootIDs.GID); err != nil { - return nil, err + // we should only modify ownership of network files within our own container + // metadata repository. If the user specifies a mount path external, it is + // up to the user to make sure the file has proper ownership for userns + if strings.Index(mount.Source, daemon.repository) == 0 { + if err := os.Chown(mount.Source, rootIDs.UID, rootIDs.GID); err != nil { + return nil, err + } } } return append(mounts, netMounts...), nil diff --git a/integration-cli/docker_api_containers_unix_test.go b/integration-cli/docker_api_containers_unix_test.go new file mode 100644 index 0000000000..4964f52644 --- /dev/null +++ b/integration-cli/docker_api_containers_unix_test.go @@ -0,0 +1,77 @@ +// +build !windows + +package main + +import ( + "io/ioutil" + "os" + "path/filepath" + + "github.com/docker/docker/api/types" + containertypes "github.com/docker/docker/api/types/container" + mounttypes "github.com/docker/docker/api/types/mount" + networktypes "github.com/docker/docker/api/types/network" + "github.com/docker/docker/client" + "github.com/docker/docker/integration-cli/checker" + "github.com/docker/docker/pkg/ioutils" + "github.com/docker/docker/pkg/system" + "github.com/go-check/check" + "github.com/stretchr/testify/assert" + "golang.org/x/net/context" +) + +func (s *DockerSuite) TestContainersAPINetworkMountsNoChown(c *check.C) { + // chown only applies to Linux bind mounted volumes; must be same host to verify + testRequires(c, DaemonIsLinux, SameHostDaemon) + + tmpDir, err := ioutils.TempDir("", "test-network-mounts") + c.Assert(err, checker.IsNil) + defer os.RemoveAll(tmpDir) + + // make tmp dir readable by anyone to allow userns process to mount from + err = os.Chmod(tmpDir, 0755) + c.Assert(err, checker.IsNil) + // create temp files to use as network mounts + tmpNWFileMount := filepath.Join(tmpDir, "nwfile") + + err = ioutil.WriteFile(tmpNWFileMount, []byte("network file bind mount"), 0644) + c.Assert(err, checker.IsNil) + + config := containertypes.Config{ + Image: "busybox", + } + hostConfig := containertypes.HostConfig{ + Mounts: []mounttypes.Mount{ + { + Type: "bind", + Source: tmpNWFileMount, + Target: "/etc/resolv.conf", + }, + { + Type: "bind", + Source: tmpNWFileMount, + Target: "/etc/hostname", + }, + { + Type: "bind", + Source: tmpNWFileMount, + Target: "/etc/hosts", + }, + }, + } + + cli, err := client.NewEnvClient() + c.Assert(err, checker.IsNil) + defer cli.Close() + + ctrCreate, err := cli.ContainerCreate(context.Background(), &config, &hostConfig, &networktypes.NetworkingConfig{}, "") + c.Assert(err, checker.IsNil) + // container will exit immediately because of no tty, but we only need the start sequence to test the condition + err = cli.ContainerStart(context.Background(), ctrCreate.ID, types.ContainerStartOptions{}) + c.Assert(err, checker.IsNil) + + // check that host-located bind mount network file did not change ownership when the container was started + statT, err := system.Stat(tmpNWFileMount) + c.Assert(err, checker.IsNil) + assert.Equal(c, uint32(0), statT.UID(), "bind mounted network file should not change ownership from root") +} diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index 6ac10e70e9..67225235b2 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -3115,6 +3115,11 @@ func (s *DockerSuite) TestRunNetworkFilesBindMount(c *check.C) { filename := createTmpFile(c, expected) defer os.Remove(filename) + // for user namespaced test runs, the temp file must be accessible to unprivileged root + if err := os.Chmod(filename, 0646); err != nil { + c.Fatalf("error modifying permissions of %s: %v", filename, err) + } + nwfiles := []string{"/etc/resolv.conf", "/etc/hosts", "/etc/hostname"} for i := range nwfiles { @@ -3132,6 +3137,11 @@ func (s *DockerSuite) TestRunNetworkFilesBindMountRO(c *check.C) { filename := createTmpFile(c, "test123") defer os.Remove(filename) + // for user namespaced test runs, the temp file must be accessible to unprivileged root + if err := os.Chmod(filename, 0646); err != nil { + c.Fatalf("error modifying permissions of %s: %v", filename, err) + } + nwfiles := []string{"/etc/resolv.conf", "/etc/hosts", "/etc/hostname"} for i := range nwfiles { @@ -3149,6 +3159,11 @@ func (s *DockerSuite) TestRunNetworkFilesBindMountROFilesystem(c *check.C) { filename := createTmpFile(c, "test123") defer os.Remove(filename) + // for user namespaced test runs, the temp file must be accessible to unprivileged root + if err := os.Chmod(filename, 0646); err != nil { + c.Fatalf("error modifying permissions of %s: %v", filename, err) + } + nwfiles := []string{"/etc/resolv.conf", "/etc/hosts", "/etc/hostname"} for i := range nwfiles {