diff --git a/pkg/skaffold/docker/dockertest.go b/pkg/skaffold/docker/dockertest.go index 833c51b713b..aca97eefee2 100644 --- a/pkg/skaffold/docker/dockertest.go +++ b/pkg/skaffold/docker/dockertest.go @@ -106,6 +106,14 @@ func (f *fakeImageFetcher) fetch(_ context.Context, image string, _ Config) (*v1 }, }, }, nil + case "parentpath:onbuild": + return &v1.ConfigFile{ + Config: v1.Config{ + OnBuild: []string{ + "COPY ../secret.txt /secret.txt", + }, + }, + }, nil case "library/ruby:2.3.0": return nil, fmt.Errorf("retrieving image \"library/ruby:2.3.0\": unsupported MediaType: \"application/vnd.docker.distribution.manifest.v1+prettyjws\", see https://github.com/google/go-containerregistry/issues/377") } diff --git a/pkg/skaffold/docker/parse.go b/pkg/skaffold/docker/parse.go index af4229ca1e2..7b732f54609 100644 --- a/pkg/skaffold/docker/parse.go +++ b/pkg/skaffold/docker/parse.go @@ -208,13 +208,22 @@ func expandBuildArgs(nodes []*parser.Node, buildArgs map[string]*string) error { func expandSrcGlobPatterns(workspace string, cpCmds []*copyCommand) ([]FromTo, error) { var fts []FromTo + workspaceAbs, err := filepath.Abs(workspace) + if err != nil { + return nil, fmt.Errorf("resolving build context %q: %w", workspace, err) + } + for _, cpCmd := range cpCmds { matchesOne := false for _, p := range cpCmd.srcs { - path := filepath.Join(workspace, p) + path, relPath, err := resolveCopySourceInWorkspace(workspace, workspaceAbs, p) + if err != nil { + return nil, err + } + if _, err := os.Stat(path); err == nil { - fts = append(fts, FromTo{From: filepath.Clean(p), To: cpCmd.dest, ToIsDir: cpCmd.destIsDir, StartLine: cpCmd.startLine, EndLine: cpCmd.endLine}) + fts = append(fts, FromTo{From: relPath, To: cpCmd.dest, ToIsDir: cpCmd.destIsDir, StartLine: cpCmd.startLine, EndLine: cpCmd.endLine}) matchesOne = true continue } @@ -228,10 +237,17 @@ func expandSrcGlobPatterns(workspace string, cpCmds []*copyCommand) ([]FromTo, e } for _, f := range files { - rel, err := filepath.Rel(workspace, f) + fileAbs, err := filepath.Abs(f) + if err != nil { + return nil, fmt.Errorf("resolving copy source %q: %w", f, err) + } + rel, err := filepath.Rel(workspaceAbs, fileAbs) if err != nil { return nil, fmt.Errorf("getting relative path of %s", f) } + if !isRelativePathInWorkspace(rel) { + return nil, fmt.Errorf("copy source %q resolves outside build context %q", p, workspace) + } fts = append(fts, FromTo{From: rel, To: cpCmd.dest, ToIsDir: cpCmd.destIsDir, StartLine: cpCmd.startLine, EndLine: cpCmd.endLine}) } @@ -248,6 +264,32 @@ func expandSrcGlobPatterns(workspace string, cpCmds []*copyCommand) ([]FromTo, e return fts, nil } +func resolveCopySourceInWorkspace(workspace, workspaceAbs, src string) (string, string, error) { + src = filepath.Clean(src) + if filepath.IsAbs(src) { + return "", "", fmt.Errorf("copy source %q resolves outside build context %q", src, workspace) + } + + sourceAbs, err := filepath.Abs(filepath.Join(workspace, src)) + if err != nil { + return "", "", fmt.Errorf("resolving copy source %q: %w", src, err) + } + + rel, err := filepath.Rel(workspaceAbs, sourceAbs) + if err != nil { + return "", "", fmt.Errorf("getting relative path of %s", sourceAbs) + } + if !isRelativePathInWorkspace(rel) { + return "", "", fmt.Errorf("copy source %q resolves outside build context %q", src, workspace) + } + + return sourceAbs, rel, nil +} + +func isRelativePathInWorkspace(rel string) bool { + return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) && !filepath.IsAbs(rel) +} + func extractCopyCommands(ctx context.Context, nodes []*parser.Node, onlyLastImage bool, cfg Config) ([]*copyCommand, error) { stages := map[string]bool{ "scratch": true, diff --git a/pkg/skaffold/docker/parse_test.go b/pkg/skaffold/docker/parse_test.go index be46cd4e9b1..48869be1e64 100644 --- a/pkg/skaffold/docker/parse_test.go +++ b/pkg/skaffold/docker/parse_test.go @@ -116,6 +116,43 @@ func TestReadCopyCmdsFromDockerfile(t *testing.T) { } } +func TestReadCopyCmdsFromDockerfileRejectsSourcesOutsideWorkspace(t *testing.T) { + tests := []struct { + description string + dockerfile string + }{ + { + description: "direct copy parent path", + dockerfile: "FROM nginx\nCOPY ../secret.txt /secret.txt", + }, + { + description: "onbuild copy parent path", + dockerfile: "FROM parentpath:onbuild", + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + imageFetcher := fakeImageFetcher{} + t.Override(&RetrieveImage, imageFetcher.fetch) + + tmp := t.NewTempDir() + tmp.Mkdir("workspace/project") + tmp.Write("workspace/secret.txt", "secret") + + dockerfilePath := tmp.Path("workspace/project/Dockerfile") + err := os.WriteFile(dockerfilePath, []byte(test.dockerfile), 0644) + if err != nil { + t.Error(err) + } + + cfg := mockConfig{mode: config.RunModes.Build} + _, err = ReadCopyCmdsFromDockerfile(context.Background(), false, dockerfilePath, tmp.Path("workspace/project"), make(map[string]*string), cfg) + t.CheckErrorContains("resolves outside build context", err) + }) + } +} + func TestRemoveExtraBuildArgs(t *testing.T) { tests := []struct { description string