From bfc3a4192ae5723e401470688cdae59b95bd61f1 Mon Sep 17 00:00:00 2001 From: cyphar Date: Sat, 10 May 2014 16:38:47 +1000 Subject: [PATCH 1/3] daemon: container: ensure cp cannot traverse outside container rootfs This patch fixes the bug that allowed cp to copy files outside of the containers rootfs, by passing a relative path (such as ../../../../../../../../etc/shadow). This is fixed by first converting the path to an absolute path (relative to /) and then appending it to the container's rootfs before continuing. Docker-DCO-1.1-Signed-off-by: Aleksa Sarai (github: cyphar) --- AUTHORS | 1 + daemon/container.go | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/AUTHORS b/AUTHORS index adfcfaa851..b8c58ab09a 100644 --- a/AUTHORS +++ b/AUTHORS @@ -6,6 +6,7 @@ Aanand Prasad Aaron Feng Abel Muiño +Aleksa Sarai Alexander Larsson Alexey Shamrin Alex Gaynor diff --git a/daemon/container.go b/daemon/container.go index 7b6b65494e..7250b442a6 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -745,8 +745,13 @@ func (container *Container) Copy(resource string) (io.ReadCloser, error) { if err := container.Mount(); err != nil { return nil, err } + var filter []string + + // Ensure path is local to container basefs + resource = path.Join("/", resource) basePath := path.Join(container.basefs, resource) + stat, err := os.Stat(basePath) if err != nil { container.Unmount() From 79ca77f3e80d983cf72aa131c1b59c77c60270b0 Mon Sep 17 00:00:00 2001 From: cyphar Date: Sat, 10 May 2014 22:51:45 +1000 Subject: [PATCH 2/3] integration-cli: cp: added tests for cp This patch adds integration tests for the copying of resources from a container, to ensure that regressions in the security of resource copying can be easily discovered. Docker-DCO-1.1-Signed-off-by: Aleksa Sarai (github: cyphar) --- integration-cli/docker_cli_cp_test.go | 208 ++++++++++++++++++++++++++ 1 file changed, 208 insertions(+) create mode 100644 integration-cli/docker_cli_cp_test.go diff --git a/integration-cli/docker_cli_cp_test.go b/integration-cli/docker_cli_cp_test.go new file mode 100644 index 0000000000..b5a70a45ed --- /dev/null +++ b/integration-cli/docker_cli_cp_test.go @@ -0,0 +1,208 @@ +package main + +import ( + "fmt" + "io/ioutil" + "os" + "path/filepath" + "testing" +) + +const ( + cpTestPathParent = "/some" + cpTestPath = "/some/path" + cpTestName = "test" + cpFullPath = "/some/path/test" + + cpContainerContents = "holla, i am the container" + cpHostContents = "hello, i am the host" +) + +// Test for #5656 +// Check that garbage paths don't escape the container's rootfs +func TestCpGarbagePath(t *testing.T) { + out, exitCode, err := cmd(t, "run", "-d", "busybox", "/bin/sh", "-c", "mkdir -p '"+cpTestPath+"' && echo -n '"+cpContainerContents+"' > "+cpFullPath) + if err != nil || exitCode != 0 { + t.Fatal("failed to create a container", out, err) + } + + cleanedContainerID := stripTrailingCharacters(out) + defer deleteContainer(cleanedContainerID) + + out, _, err = cmd(t, "wait", cleanedContainerID) + if err != nil || stripTrailingCharacters(out) != "0" { + t.Fatal("failed to set up container", out, err) + } + + if err := os.MkdirAll(cpTestPath, os.ModeDir); err != nil { + t.Fatal(err) + } + + hostFile, err := os.Create(cpFullPath) + if err != nil { + t.Fatal(err) + } + defer hostFile.Close() + defer os.RemoveAll(cpTestPathParent) + + fmt.Fprintf(hostFile, "%s", cpHostContents) + + tmpdir, err := ioutil.TempDir("", "docker-integration") + if err != nil { + t.Fatal(err) + } + + tmpname := filepath.Join(tmpdir, cpTestName) + defer os.RemoveAll(tmpdir) + + path := filepath.Join("../../../../../../../../../../../../", cpFullPath) + + _, _, err = cmd(t, "cp", cleanedContainerID+":"+path, tmpdir) + if err != nil { + t.Fatalf("couldn't copy from garbage path: %s:%s %s", cleanedContainerID, path, err) + } + + file, _ := os.Open(tmpname) + defer file.Close() + + test, err := ioutil.ReadAll(file) + if err != nil { + t.Fatal(err) + } + + if string(test) == cpHostContents { + t.Errorf("output matched host file -- garbage path can escape container rootfs") + } + + if string(test) != cpContainerContents { + t.Errorf("output doesn't match the input for garbage path") + } + + logDone("cp - garbage paths relative to container's rootfs") +} + +// Check that relative paths are relative to the container's rootfs +func TestCpRelativePath(t *testing.T) { + out, exitCode, err := cmd(t, "run", "-d", "busybox", "/bin/sh", "-c", "mkdir -p '"+cpTestPath+"' && echo -n '"+cpContainerContents+"' > "+cpFullPath) + if err != nil || exitCode != 0 { + t.Fatal("failed to create a container", out, err) + } + + cleanedContainerID := stripTrailingCharacters(out) + defer deleteContainer(cleanedContainerID) + + out, _, err = cmd(t, "wait", cleanedContainerID) + if err != nil || stripTrailingCharacters(out) != "0" { + t.Fatal("failed to set up container", out, err) + } + + if err := os.MkdirAll(cpTestPath, os.ModeDir); err != nil { + t.Fatal(err) + } + + hostFile, err := os.Create(cpFullPath) + if err != nil { + t.Fatal(err) + } + defer hostFile.Close() + defer os.RemoveAll(cpTestPathParent) + + fmt.Fprintf(hostFile, "%s", cpHostContents) + + tmpdir, err := ioutil.TempDir("", "docker-integration") + + if err != nil { + t.Fatal(err) + } + + tmpname := filepath.Join(tmpdir, cpTestName) + defer os.RemoveAll(tmpdir) + + path, _ := filepath.Rel("/", cpFullPath) + + _, _, err = cmd(t, "cp", cleanedContainerID+":"+path, tmpdir) + if err != nil { + t.Fatalf("couldn't copy from relative path: %s:%s %s", cleanedContainerID, path, err) + } + + file, _ := os.Open(tmpname) + defer file.Close() + + test, err := ioutil.ReadAll(file) + if err != nil { + t.Fatal(err) + } + + if string(test) == cpHostContents { + t.Errorf("output matched host file -- relative path can escape container rootfs") + } + + if string(test) != cpContainerContents { + t.Errorf("output doesn't match the input for relative path") + } + + logDone("cp - relative paths relative to container's rootfs") +} + +// Check that absolute paths are relative to the container's rootfs +func TestCpAbsolutePath(t *testing.T) { + out, exitCode, err := cmd(t, "run", "-d", "busybox", "/bin/sh", "-c", "mkdir -p '"+cpTestPath+"' && echo -n '"+cpContainerContents+"' > "+cpFullPath) + if err != nil || exitCode != 0 { + t.Fatal("failed to create a container", out, err) + } + + cleanedContainerID := stripTrailingCharacters(out) + defer deleteContainer(cleanedContainerID) + + out, _, err = cmd(t, "wait", cleanedContainerID) + if err != nil || stripTrailingCharacters(out) != "0" { + t.Fatal("failed to set up container", out, err) + } + + if err := os.MkdirAll(cpTestPath, os.ModeDir); err != nil { + t.Fatal(err) + } + + hostFile, err := os.Create(cpFullPath) + if err != nil { + t.Fatal(err) + } + defer hostFile.Close() + defer os.RemoveAll(cpTestPathParent) + + fmt.Fprintf(hostFile, "%s", cpHostContents) + + tmpdir, err := ioutil.TempDir("", "docker-integration") + + if err != nil { + t.Fatal(err) + } + + tmpname := filepath.Join(tmpdir, cpTestName) + defer os.RemoveAll(tmpdir) + + path := cpFullPath + + _, _, err = cmd(t, "cp", cleanedContainerID+":"+path, tmpdir) + if err != nil { + t.Fatalf("couldn't copy from absolute path: %s:%s %s", cleanedContainerID, path, err) + } + + file, _ := os.Open(tmpname) + defer file.Close() + + test, err := ioutil.ReadAll(file) + if err != nil { + t.Fatal(err) + } + + if string(test) == cpHostContents { + t.Errorf("output matched host file -- absolute path can escape container rootfs") + } + + if string(test) != cpContainerContents { + t.Errorf("output doesn't match the input for absolute path") + } + + logDone("cp - absolute paths relative to container's rootfs") +} From 0fb507dc2328c5c364a2cd1701a155efb1767a1a Mon Sep 17 00:00:00 2001 From: cyphar Date: Mon, 12 May 2014 06:49:46 +1000 Subject: [PATCH 3/3] daemon: *: refactored container resource path generation This patch is a preventative patch, it fixes possible future vulnerabilities regarding unsantised paths. Due to several recent vulnerabilities, wherein the docker daemon could be fooled into accessing data from the host (rather than a container), this patch was created to try and mitigate future possible vulnerabilities in the same vein. Docker-DCO-1.1-Signed-off-by: Aleksa Sarai (github: cyphar) --- daemon/container.go | 36 ++++++++++++++++++++++-------------- daemon/volumes.go | 4 ++-- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/daemon/container.go b/daemon/container.go index 7250b442a6..4f10cc4e93 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -9,6 +9,7 @@ import ( "log" "os" "path" + "path/filepath" "strings" "sync" "syscall" @@ -89,7 +90,7 @@ func (container *Container) Inject(file io.Reader, pth string) error { defer container.Unmount() // Return error if path exists - destPath := path.Join(container.basefs, pth) + destPath := container.getResourcePath(pth) if _, err := os.Stat(destPath); err == nil { // Since err is nil, the path could be stat'd and it exists return fmt.Errorf("%s exists", pth) @@ -101,7 +102,7 @@ func (container *Container) Inject(file io.Reader, pth string) error { } // Make sure the directory exists - if err := os.MkdirAll(path.Join(container.basefs, path.Dir(pth)), 0755); err != nil { + if err := os.MkdirAll(container.getResourcePath(path.Dir(pth)), 0755); err != nil { return err } @@ -170,6 +171,16 @@ func (container *Container) WriteHostConfig() (err error) { return ioutil.WriteFile(container.hostConfigPath(), data, 0666) } +func (container *Container) getResourcePath(path string) string { + cleanPath := filepath.Join("/", path) + return filepath.Join(container.basefs, cleanPath) +} + +func (container *Container) getRootResourcePath(path string) string { + cleanPath := filepath.Join("/", path) + return filepath.Join(container.root, cleanPath) +} + func populateCommand(c *Container, env []string) error { var ( en *execdriver.Network @@ -344,7 +355,7 @@ func (container *Container) StderrLogPipe() io.ReadCloser { } func (container *Container) buildHostnameFile() error { - container.HostnamePath = path.Join(container.root, "hostname") + container.HostnamePath = container.getRootResourcePath("hostname") if container.Config.Domainname != "" { return ioutil.WriteFile(container.HostnamePath, []byte(fmt.Sprintf("%s.%s\n", container.Config.Hostname, container.Config.Domainname)), 0644) } @@ -356,7 +367,7 @@ func (container *Container) buildHostnameAndHostsFiles(IP string) error { return err } - container.HostsPath = path.Join(container.root, "hosts") + container.HostsPath = container.getRootResourcePath("hosts") extraContent := make(map[string]string) @@ -674,7 +685,7 @@ func (container *Container) Unmount() error { } func (container *Container) logPath(name string) string { - return path.Join(container.root, fmt.Sprintf("%s-%s.log", container.ID, name)) + return container.getRootResourcePath(fmt.Sprintf("%s-%s.log", container.ID, name)) } func (container *Container) ReadLog(name string) (io.Reader, error) { @@ -682,11 +693,11 @@ func (container *Container) ReadLog(name string) (io.Reader, error) { } func (container *Container) hostConfigPath() string { - return path.Join(container.root, "hostconfig.json") + return container.getRootResourcePath("hostconfig.json") } func (container *Container) jsonPath() string { - return path.Join(container.root, "config.json") + return container.getRootResourcePath("config.json") } // This method must be exported to be used from the lxc template @@ -748,10 +759,7 @@ func (container *Container) Copy(resource string) (io.ReadCloser, error) { var filter []string - // Ensure path is local to container basefs - resource = path.Join("/", resource) - basePath := path.Join(container.basefs, resource) - + basePath := container.getResourcePath(resource) stat, err := os.Stat(basePath) if err != nil { container.Unmount() @@ -849,7 +857,7 @@ func (container *Container) setupContainerDns() error { } else if len(daemon.config.DnsSearch) > 0 { dnsSearch = daemon.config.DnsSearch } - container.ResolvConfPath = path.Join(container.root, "resolv.conf") + container.ResolvConfPath = container.getRootResourcePath("resolv.conf") return resolvconf.Build(container.ResolvConfPath, dns, dnsSearch) } else { container.ResolvConfPath = "/etc/resolv.conf" @@ -987,12 +995,12 @@ func (container *Container) setupWorkingDirectory() error { if container.Config.WorkingDir != "" { container.Config.WorkingDir = path.Clean(container.Config.WorkingDir) - pthInfo, err := os.Stat(path.Join(container.basefs, container.Config.WorkingDir)) + pthInfo, err := os.Stat(container.getResourcePath(container.Config.WorkingDir)) if err != nil { if !os.IsNotExist(err) { return err } - if err := os.MkdirAll(path.Join(container.basefs, container.Config.WorkingDir), 0755); err != nil { + if err := os.MkdirAll(container.getResourcePath(container.Config.WorkingDir), 0755); err != nil { return err } } diff --git a/daemon/volumes.go b/daemon/volumes.go index ac01c6a982..eac743b2d9 100644 --- a/daemon/volumes.go +++ b/daemon/volumes.go @@ -94,11 +94,11 @@ func applyVolumesFrom(container *Container) error { if _, exists := container.Volumes[volPath]; exists { continue } - stat, err := os.Stat(filepath.Join(c.basefs, volPath)) + stat, err := os.Stat(c.getResourcePath(volPath)) if err != nil { return err } - if err := createIfNotExists(filepath.Join(container.basefs, volPath), stat.IsDir()); err != nil { + if err := createIfNotExists(container.getResourcePath(volPath), stat.IsDir()); err != nil { return err } container.Volumes[volPath] = id