From 198ff76de59a600ce900497fd4a6131ee4448c48 Mon Sep 17 00:00:00 2001 From: Doug Davis Date: Wed, 21 Jan 2015 11:08:19 -0800 Subject: [PATCH] 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)