From 198ff76de59a600ce900497fd4a6131ee4448c48 Mon Sep 17 00:00:00 2001 From: Doug Davis Date: Wed, 21 Jan 2015 11:08:19 -0800 Subject: [PATCH 1/3] Add an API test for docker build -f Dockerfile I noticed that while we have tests to make sure that people don't specify a Dockerfile (via -f) that's outside of the build context when using the docker cli, we don't check on the server side to make sure that API users have the same check done. This would be a security risk. While in there I had to add a new util func for the tests to allow us to send content to the server that isn't json encoded - in this case a tarball Signed-off-by: Doug Davis --- builder/evaluator.go | 21 ++++++++---- integration-cli/docker_api_containers_test.go | 32 +++++++++++++++++++ integration-cli/docker_utils.go | 22 +++++++++---- 3 files changed, 61 insertions(+), 14 deletions(-) diff --git a/builder/evaluator.go b/builder/evaluator.go index 3149bd0df7..0a6122cf21 100644 --- a/builder/evaluator.go +++ b/builder/evaluator.go @@ -24,7 +24,7 @@ import ( "fmt" "io" "os" - "path" + "path/filepath" "strings" log "github.com/Sirupsen/logrus" @@ -169,12 +169,19 @@ func (b *Builder) Run(context io.Reader) (string, error) { // Reads a Dockerfile from the current context. It assumes that the // 'filename' is a relative path from the root of the context -func (b *Builder) readDockerfile(filename string) error { - filename = path.Join(b.contextPath, filename) +func (b *Builder) readDockerfile(origFile string) error { + filename := filepath.Join(b.contextPath, origFile) + + tmpDockerPath := filepath.Dir(filename) + string(os.PathSeparator) + tmpContextPath := filepath.Clean(b.contextPath) + string(os.PathSeparator) + + if !strings.HasPrefix(tmpDockerPath, tmpContextPath) { + return fmt.Errorf("Dockerfile (%s) must be within the build context", origFile) + } fi, err := os.Stat(filename) if os.IsNotExist(err) { - return fmt.Errorf("Cannot build a directory without a Dockerfile") + return fmt.Errorf("Cannot locate specified Dockerfile: %s", origFile) } if fi.Size() == 0 { return ErrDockerfileEmpty @@ -201,13 +208,13 @@ func (b *Builder) readDockerfile(filename string) error { // Note that this assumes the Dockerfile has been read into memory and // is now safe to be removed. - excludes, _ := utils.ReadDockerIgnore(path.Join(b.contextPath, ".dockerignore")) + excludes, _ := utils.ReadDockerIgnore(filepath.Join(b.contextPath, ".dockerignore")) if rm, _ := fileutils.Matches(".dockerignore", excludes); rm == true { - os.Remove(path.Join(b.contextPath, ".dockerignore")) + os.Remove(filepath.Join(b.contextPath, ".dockerignore")) b.context.(tarsum.BuilderContext).Remove(".dockerignore") } if rm, _ := fileutils.Matches(b.dockerfileName, excludes); rm == true { - os.Remove(path.Join(b.contextPath, b.dockerfileName)) + os.Remove(filepath.Join(b.contextPath, b.dockerfileName)) b.context.(tarsum.BuilderContext).Remove(b.dockerfileName) } diff --git a/integration-cli/docker_api_containers_test.go b/integration-cli/docker_api_containers_test.go index 4e945f5429..eb7d27c95a 100644 --- a/integration-cli/docker_api_containers_test.go +++ b/integration-cli/docker_api_containers_test.go @@ -299,3 +299,35 @@ func TestGetContainerStats(t *testing.T) { } logDone("container REST API - check GET containers/stats") } + +func TestBuildApiDockerfilePath(t *testing.T) { + // Test to make sure we stop people from trying to leave the + // build context when specifying the path to the dockerfile + buffer := new(bytes.Buffer) + tw := tar.NewWriter(buffer) + defer tw.Close() + + if err := tw.WriteHeader(&tar.Header{ + Name: "Dockerfile", + Size: 11, + }); err != nil { + t.Fatalf("failed to write tar file header: %v", err) + } + if _, err := tw.Write([]byte("FROM ubuntu")); err != nil { + t.Fatalf("failed to write tar file content: %v", err) + } + if err := tw.Close(); err != nil { + t.Fatalf("failed to close tar archive: %v", err) + } + + out, err := sockRequestRaw("POST", "/build?dockerfile=../Dockerfile", buffer, "application/x-tar") + if err == nil { + t.Fatalf("Build was supposed to fail") + } + + if !strings.Contains(string(out), "must be within the build context") { + t.Fatalf("Didn't complain about leaving build context") + } + + logDone("container REST API - check build w/bad Dockerfile path") +} diff --git a/integration-cli/docker_utils.go b/integration-cli/docker_utils.go index 3d66e9948f..35a97feca7 100644 --- a/integration-cli/docker_utils.go +++ b/integration-cli/docker_utils.go @@ -274,6 +274,15 @@ func daemonHost() string { } func sockRequest(method, endpoint string, data interface{}) ([]byte, error) { + jsonData := bytes.NewBuffer(nil) + if err := json.NewEncoder(jsonData).Encode(data); err != nil { + return nil, err + } + + return sockRequestRaw(method, endpoint, jsonData, "application/json") +} + +func sockRequestRaw(method, endpoint string, data io.Reader, ct string) ([]byte, error) { daemon := daemonHost() daemonUrl, err := url.Parse(daemon) if err != nil { @@ -296,17 +305,16 @@ func sockRequest(method, endpoint string, data interface{}) ([]byte, error) { client := httputil.NewClientConn(c, nil) defer client.Close() - jsonData := bytes.NewBuffer(nil) - if err := json.NewEncoder(jsonData).Encode(data); err != nil { - return nil, err - } - - req, err := http.NewRequest(method, endpoint, jsonData) - req.Header.Set("Content-Type", "application/json") + req, err := http.NewRequest(method, endpoint, data) if err != nil { return nil, fmt.Errorf("could not create new request: %v", err) } + if ct == "" { + ct = "application/json" + } + req.Header.Set("Content-Type", ct) + resp, err := client.Do(req) if err != nil { return nil, fmt.Errorf("could not perform request: %v", err) From 73d5baf585e3ef55864abeef43d45fe0b3a1c2bc Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Mon, 2 Feb 2015 16:17:12 -0500 Subject: [PATCH 2/3] builder: prevent Dockerfile to leave build context Signed-off-by: Tibor Vass --- api/client/commands.go | 47 +++++++------- builder/evaluator.go | 13 ++-- integration-cli/docker_api_containers_test.go | 43 +++++++++++-- integration-cli/docker_cli_build_test.go | 64 +++++++++++++++++++ 4 files changed, 132 insertions(+), 35 deletions(-) diff --git a/api/client/commands.go b/api/client/commands.go index 3cd110b4ec..d60c6d4827 100644 --- a/api/client/commands.go +++ b/api/client/commands.go @@ -39,6 +39,7 @@ import ( "github.com/docker/docker/pkg/parsers/filters" "github.com/docker/docker/pkg/promise" "github.com/docker/docker/pkg/signal" + "github.com/docker/docker/pkg/symlink" "github.com/docker/docker/pkg/term" "github.com/docker/docker/pkg/timeutils" "github.com/docker/docker/pkg/units" @@ -147,36 +148,36 @@ func (cli *DockerCli) CmdBuild(args ...string) error { return err } - var filename string // path to Dockerfile - var origDockerfile string // used for error msg + filename := *dockerfileName // path to Dockerfile if *dockerfileName == "" { // No -f/--file was specified so use the default - origDockerfile = api.DefaultDockerfileName - *dockerfileName = origDockerfile + *dockerfileName = api.DefaultDockerfileName filename = path.Join(absRoot, *dockerfileName) - } else { - origDockerfile = *dockerfileName - if filename, err = filepath.Abs(*dockerfileName); err != nil { - return err - } - - // Verify that 'filename' is within the build context - if !strings.HasSuffix(absRoot, string(os.PathSeparator)) { - absRoot += string(os.PathSeparator) - } - if !strings.HasPrefix(filename, absRoot) { - return fmt.Errorf("The Dockerfile (%s) must be within the build context (%s)", *dockerfileName, root) - } - - // Now reset the dockerfileName to be relative to the build context - *dockerfileName = filename[len(absRoot):] } - if _, err = os.Stat(filename); os.IsNotExist(err) { - return fmt.Errorf("Can not locate Dockerfile: %s", origDockerfile) + origDockerfile := *dockerfileName // used for error msg + + if filename, err = filepath.Abs(filename); err != nil { + return err } - var includes []string = []string{"."} + + // Verify that 'filename' is within the build context + filename, err = symlink.FollowSymlinkInScope(filename, absRoot) + if err != nil { + return fmt.Errorf("The Dockerfile (%s) must be within the build context (%s)", origDockerfile, root) + } + + // Now reset the dockerfileName to be relative to the build context + *dockerfileName, err = filepath.Rel(filename, absRoot) + if err != nil { + return err + } + + if _, err = os.Lstat(filename); os.IsNotExist(err) { + return fmt.Errorf("Cannot locate Dockerfile: %s", origDockerfile) + } + var includes = []string{"."} excludes, err := utils.ReadDockerIgnore(path.Join(root, ".dockerignore")) if err != nil { diff --git a/builder/evaluator.go b/builder/evaluator.go index 0a6122cf21..b76c7f29bb 100644 --- a/builder/evaluator.go +++ b/builder/evaluator.go @@ -32,6 +32,7 @@ import ( "github.com/docker/docker/daemon" "github.com/docker/docker/engine" "github.com/docker/docker/pkg/fileutils" + "github.com/docker/docker/pkg/symlink" "github.com/docker/docker/pkg/tarsum" "github.com/docker/docker/registry" "github.com/docker/docker/runconfig" @@ -170,16 +171,12 @@ func (b *Builder) Run(context io.Reader) (string, error) { // Reads a Dockerfile from the current context. It assumes that the // 'filename' is a relative path from the root of the context func (b *Builder) readDockerfile(origFile string) error { - filename := filepath.Join(b.contextPath, origFile) - - tmpDockerPath := filepath.Dir(filename) + string(os.PathSeparator) - tmpContextPath := filepath.Clean(b.contextPath) + string(os.PathSeparator) - - if !strings.HasPrefix(tmpDockerPath, tmpContextPath) { - return fmt.Errorf("Dockerfile (%s) must be within the build context", origFile) + filename, err := symlink.FollowSymlinkInScope(filepath.Join(b.contextPath, origFile), b.contextPath) + if err != nil { + return fmt.Errorf("The Dockerfile (%s) must be within the build context", origFile) } - fi, err := os.Stat(filename) + fi, err := os.Lstat(filename) if os.IsNotExist(err) { return fmt.Errorf("Cannot locate specified Dockerfile: %s", origFile) } diff --git a/integration-cli/docker_api_containers_test.go b/integration-cli/docker_api_containers_test.go index eb7d27c95a..5dce388381 100644 --- a/integration-cli/docker_api_containers_test.go +++ b/integration-cli/docker_api_containers_test.go @@ -307,13 +307,14 @@ func TestBuildApiDockerfilePath(t *testing.T) { tw := tar.NewWriter(buffer) defer tw.Close() + dockerfile := []byte("FROM busybox") if err := tw.WriteHeader(&tar.Header{ Name: "Dockerfile", - Size: 11, + Size: int64(len(dockerfile)), }); err != nil { t.Fatalf("failed to write tar file header: %v", err) } - if _, err := tw.Write([]byte("FROM ubuntu")); err != nil { + if _, err := tw.Write(dockerfile); err != nil { t.Fatalf("failed to write tar file content: %v", err) } if err := tw.Close(); err != nil { @@ -322,12 +323,46 @@ func TestBuildApiDockerfilePath(t *testing.T) { out, err := sockRequestRaw("POST", "/build?dockerfile=../Dockerfile", buffer, "application/x-tar") if err == nil { - t.Fatalf("Build was supposed to fail") + t.Fatalf("Build was supposed to fail: %s", out) } if !strings.Contains(string(out), "must be within the build context") { - t.Fatalf("Didn't complain about leaving build context") + t.Fatalf("Didn't complain about leaving build context: %s", out) } logDone("container REST API - check build w/bad Dockerfile path") } + +func TestBuildApiDockerfileSymlink(t *testing.T) { + // Test to make sure we stop people from trying to leave the + // build context when specifying a symlink as the path to the dockerfile + buffer := new(bytes.Buffer) + tw := tar.NewWriter(buffer) + defer tw.Close() + + if err := tw.WriteHeader(&tar.Header{ + Name: "Dockerfile", + Typeflag: tar.TypeSymlink, + Linkname: "/etc/passwd", + }); err != nil { + t.Fatalf("failed to write tar file header: %v", err) + } + if err := tw.Close(); err != nil { + t.Fatalf("failed to close tar archive: %v", err) + } + + out, err := sockRequestRaw("POST", "/build", buffer, "application/x-tar") + if err == nil { + t.Fatalf("Build was supposed to fail: %s", out) + } + + // The reason the error is "Cannot locate specified Dockerfile" is because + // in the builder, the symlink is resolved within the context, therefore + // Dockerfile -> /etc/passwd becomes etc/passwd from the context which is + // a nonexistent file. + if !strings.Contains(string(out), "Cannot locate specified Dockerfile: Dockerfile") { + t.Fatalf("Didn't complain about leaving build context: %s", out) + } + + logDone("container REST API - check build w/bad Dockerfile symlink path") +} diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index 2c04ba8f27..fad38317ef 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -4630,3 +4630,67 @@ func TestBuildFromOfficialNames(t *testing.T) { } logDone("build - from official names") } + +func TestBuildDockerfileOutsideContext(t *testing.T) { + name := "testbuilddockerfileoutsidecontext" + tmpdir, err := ioutil.TempDir("", name) + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpdir) + ctx := filepath.Join(tmpdir, "context") + if err := os.MkdirAll(ctx, 0755); err != nil { + t.Fatal(err) + } + if err := ioutil.WriteFile(filepath.Join(ctx, "Dockerfile"), []byte("FROM busybox"), 0644); err != nil { + t.Fatal(err) + } + wd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + defer os.Chdir(wd) + if err := os.Chdir(ctx); err != nil { + t.Fatal(err) + } + if err := ioutil.WriteFile(filepath.Join(tmpdir, "outsideDockerfile"), []byte("FROM busbox"), 0644); err != nil { + t.Fatal(err) + } + if err := os.Symlink("../outsideDockerfile", filepath.Join(ctx, "dockerfile1")); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(tmpdir, "outsideDockerfile"), filepath.Join(ctx, "dockerfile2")); err != nil { + t.Fatal(err) + } + if err := os.Link("../outsideDockerfile", filepath.Join(ctx, "dockerfile3")); err != nil { + t.Fatal(err) + } + if err := os.Link(filepath.Join(tmpdir, "outsideDockerfile"), filepath.Join(ctx, "dockerfile4")); err != nil { + t.Fatal(err) + } + for _, dockerfilePath := range []string{ + "../outsideDockerfile", + filepath.Join(ctx, "dockerfile1"), + filepath.Join(ctx, "dockerfile2"), + filepath.Join(ctx, "dockerfile3"), + filepath.Join(ctx, "dockerfile4"), + } { + out, _, err := runCommandWithOutput(exec.Command(dockerBinary, "build", "-t", name, "--no-cache", "-f", dockerfilePath, ".")) + if err == nil { + t.Fatalf("Expected error with %s. Out: %s", dockerfilePath, out) + } + deleteImages(name) + } + + os.Chdir(tmpdir) + + // Path to Dockerfile should be resolved relative to working directory, not relative to context. + // There is a Dockerfile in the context, but since there is no Dockerfile in the current directory, the following should fail + out, _, err := runCommandWithOutput(exec.Command(dockerBinary, "build", "-t", name, "--no-cache", "-f", "Dockerfile", ctx)) + if err == nil { + t.Fatalf("Expected error. Out: %s", out) + } + deleteImages(name) + + logDone("build - Dockerfile outside context") +} From 22e2254c744d1ed2d75f3cdc802e4347727dea6a Mon Sep 17 00:00:00 2001 From: Arnaud Porterie Date: Mon, 2 Feb 2015 23:42:18 -0800 Subject: [PATCH 3/3] Fix client-side validation of Dockerfile path Arguments to `filepath.Rel` were reversed, making all builder tests to fail. Signed-off-by: Arnaud Porterie --- api/client/commands.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/client/commands.go b/api/client/commands.go index d60c6d4827..c4ce5e01f3 100644 --- a/api/client/commands.go +++ b/api/client/commands.go @@ -169,7 +169,7 @@ func (cli *DockerCli) CmdBuild(args ...string) error { } // Now reset the dockerfileName to be relative to the build context - *dockerfileName, err = filepath.Rel(filename, absRoot) + *dockerfileName, err = filepath.Rel(absRoot, filename) if err != nil { return err }