From 66843cd4236d8e326a9920477ed29b0bd3dbc44a Mon Sep 17 00:00:00 2001 From: Alexandr Morozov Date: Wed, 26 Nov 2014 23:00:13 -0800 Subject: [PATCH] Change path breakout detection logic in archive package Fixes #9375 Signed-off-by: Alexandr Morozov Conflicts: integration-cli/docker_cli_cp_test.go removed extra test Upstream-commit: 994e4a1c69c5f48eeba679437fdd7b7ed7ab0fc5 Component: engine --- .../integration-cli/docker_cli_cp_test.go | 38 +++++++++++++++++++ components/engine/pkg/archive/archive.go | 9 +++-- components/engine/pkg/archive/diff.go | 12 +++--- 3 files changed, 51 insertions(+), 8 deletions(-) diff --git a/components/engine/integration-cli/docker_cli_cp_test.go b/components/engine/integration-cli/docker_cli_cp_test.go index aecc68edb4..a5e16bb214 100644 --- a/components/engine/integration-cli/docker_cli_cp_test.go +++ b/components/engine/integration-cli/docker_cli_cp_test.go @@ -371,3 +371,41 @@ func TestCpUnprivilegedUser(t *testing.T) { logDone("cp - unprivileged user") } + +func TestCpToDot(t *testing.T) { + out, exitCode, err := dockerCmd(t, "run", "-d", "busybox", "/bin/sh", "-c", "echo lololol > /test") + if err != nil || exitCode != 0 { + t.Fatal("failed to create a container", out, err) + } + + cleanedContainerID := stripTrailingCharacters(out) + defer deleteContainer(cleanedContainerID) + + out, _, err = dockerCmd(t, "wait", cleanedContainerID) + if err != nil || stripTrailingCharacters(out) != "0" { + t.Fatal("failed to set up container", out, err) + } + + tmpdir, err := ioutil.TempDir("", "docker-integration") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpdir) + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + defer os.Chdir(cwd) + if err := os.Chdir(tmpdir); err != nil { + t.Fatal(err) + } + _, _, err = dockerCmd(t, "cp", cleanedContainerID+":/test", ".") + if err != nil { + t.Fatalf("couldn't docker cp to \".\" path: %s", err) + } + content, err := ioutil.ReadFile("./test") + if string(content) != "lololol\n" { + t.Fatal("Wrong content in copied file %q, should be %q", content, "lololol\n") + } + logDone("cp - to dot path") +} diff --git a/components/engine/pkg/archive/archive.go b/components/engine/pkg/archive/archive.go index 13f447ca49..74c6014506 100644 --- a/components/engine/pkg/archive/archive.go +++ b/components/engine/pkg/archive/archive.go @@ -473,10 +473,13 @@ loop: } } - // Prevent symlink breakout path := filepath.Join(dest, hdr.Name) - if !strings.HasPrefix(path, dest) { - return breakoutError(fmt.Errorf("%q is outside of %q", path, dest)) + rel, err := filepath.Rel(dest, path) + if err != nil { + return err + } + if strings.HasPrefix(rel, "..") { + return breakoutError(fmt.Errorf("%q is outside of %q", hdr.Name, dest)) } // If path exits we almost always just want to remove and replace it diff --git a/components/engine/pkg/archive/diff.go b/components/engine/pkg/archive/diff.go index e5467637bc..80bb197468 100644 --- a/components/engine/pkg/archive/diff.go +++ b/components/engine/pkg/archive/diff.go @@ -81,12 +81,14 @@ func UnpackLayer(dest string, layer ArchiveReader) error { } path := filepath.Join(dest, hdr.Name) - base := filepath.Base(path) - - // Prevent symlink breakout - if !strings.HasPrefix(path, dest) { - return breakoutError(fmt.Errorf("%q is outside of %q", path, dest)) + rel, err := filepath.Rel(dest, path) + if err != nil { + return err } + if strings.HasPrefix(rel, "..") { + return breakoutError(fmt.Errorf("%q is outside of %q", hdr.Name, dest)) + } + base := filepath.Base(path) if strings.HasPrefix(base, ".wh.") { originalBase := base[len(".wh."):]