diff --git a/internal/batches/workspace/volume_workspace.go b/internal/batches/workspace/volume_workspace.go index 011ffb436c..a2c9427b5e 100644 --- a/internal/batches/workspace/volume_workspace.go +++ b/internal/batches/workspace/volume_workspace.go @@ -1,12 +1,15 @@ package workspace import ( + "archive/tar" "bytes" "context" "crypto/rand" "encoding/hex" "fmt" + "io" "os" + "path" "sort" "github.com/sourcegraph/sourcegraph/lib/errors" @@ -180,55 +183,105 @@ func (wc *dockerVolumeWorkspaceCreator) copyFilesIntoVolumes(ctx context.Context if len(files) == 0 { return nil } - const copyScript = `while test "$#" -gt 0; do cp "$1" "$2" || exit; shift 2; done` + + archive, err := wc.archiveAdditionalFiles(files) + if err != nil { + return err + } + defer os.Remove(archive) + archiveMount, err := docker.BindMount(archive, "/tmp/additional-files.tar", true) + if err != nil { + return errors.Wrap(err, "creating additional files archive mount") + } opts := append([]string{ "run", "--rm", "--init", "--workdir", "/work", + "--mount", archiveMount, }, w.dockerRunOptsWithUser(w.uidGid, "/work")...) - // We sort these so our tests don't break. Sorry. + opts = append( + opts, + DockerVolumeWorkspaceImage, + "tar", "-xf", "/tmp/additional-files.tar", "-C", "/work", + ) + + if out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput(); err != nil { + return errors.Wrapf(err, "additional files output:\n\n%s\n\n", string(out)) + } + return nil +} + +func (wc *dockerVolumeWorkspaceCreator) archiveAdditionalFiles(files map[string]string) (archivePath string, err error) { + f, err := os.CreateTemp(wc.tempDir, "src-additional-files-*.tar") + if err != nil { + return "", errors.Wrap(err, "creating additional files archive") + } + archivePath = f.Name() + defer func() { + if err != nil { + f.Close() + os.Remove(archivePath) + } + }() + + tw := tar.NewWriter(f) var names []string for name := range files { names = append(names, name) } sort.Strings(names) - var copyArgs []string - for i, name := range names { + for _, name := range names { if err := validateWorkspaceFileName(name); err != nil { - return err + return "", err } - localPath := files[name] - // Names originate from the Sourcegraph instance. Keep them out of both - // Docker's comma-delimited mount grammar and the shell program. - mountTarget := fmt.Sprintf("/tmp/src-additional-file-%d", i) - mount, err := docker.BindMount(localPath, mountTarget, true) + if name == "" || path.Clean(name) != name { + return "", errors.Errorf("invalid additional file path %q", name) + } + + file, err := os.Open(files[name]) + if err != nil { + return "", errors.Wrapf(err, "opening additional file %q", name) + } + info, err := file.Stat() if err != nil { - return errors.Wrap(err, "creating additional file mount") + file.Close() + return "", errors.Wrapf(err, "stating additional file %q", name) + } + if !info.Mode().IsRegular() { + file.Close() + return "", errors.Errorf("additional file %q is not a regular file", name) } - opts = append(opts, []string{ - "--mount", mount, - }...) - copyArgs = append(copyArgs, mountTarget, "/work/"+name) + header, err := tar.FileInfoHeader(info, "") + if err != nil { + file.Close() + return "", errors.Wrapf(err, "creating archive header for additional file %q", name) + } + header.Name = name + if err := tw.WriteHeader(header); err != nil { + file.Close() + return "", errors.Wrapf(err, "writing archive header for additional file %q", name) + } + if _, err := io.Copy(tw, file); err != nil { + file.Close() + return "", errors.Wrapf(err, "archiving additional file %q", name) + } + if err := file.Close(); err != nil { + return "", errors.Wrapf(err, "closing additional file %q", name) + } } - opts = append( - opts, - DockerVolumeWorkspaceImage, - "sh", "-c", - copyScript, - "copy-additional-files", - ) - opts = append(opts, copyArgs...) - - if out, err := exec.CommandContext(ctx, "docker", opts...).CombinedOutput(); err != nil { - return errors.Wrapf(err, "unzip output:\n\n%s\n\n", string(out)) + if err := tw.Close(); err != nil { + return "", errors.Wrap(err, "closing additional files archive") } - return nil + if err := f.Close(); err != nil { + return "", errors.Wrap(err, "closing additional files archive file") + } + return archivePath, nil } // dockerVolumeWorkspace workspaces are placed on Docker volumes (surprise!), diff --git a/internal/batches/workspace/volume_workspace_test.go b/internal/batches/workspace/volume_workspace_test.go index cc879614ac..f23b5e5312 100644 --- a/internal/batches/workspace/volume_workspace_test.go +++ b/internal/batches/workspace/volume_workspace_test.go @@ -1,7 +1,9 @@ package workspace import ( + "archive/tar" "context" + "io" "os" "path/filepath" "strings" @@ -39,13 +41,10 @@ func TestVolumeWorkspaceCreator(t *testing.T) { mockAdditionalFilePaths: map[string]string{}, } for _, name := range []string{".gitignore", "another-file"} { - // Since we don't read the files and mock the Docker commands, - // we don't need to create them. - path := filepath.Join(os.TempDir(), "additional-file"+name) - // Instead we create a real-looking path that we sanitize so - // it doesn't trip up the globbing expecations below: - path = strings.ReplaceAll(path, string(os.PathSeparator), "-") - + path := filepath.Join(t.TempDir(), "additional-file"+name) + if err := os.WriteFile(path, []byte(name), 0600); err != nil { + t.Fatal(err) + } archiveWithAdditionalFiles.mockAdditionalFilePaths[name] = path } @@ -336,15 +335,11 @@ func TestVolumeWorkspaceCreator(t *testing.T) { expect.Success, "docker", "run", "--rm", "--init", "--workdir", "/work", + "--mount", "type=bind,source=*,target=/tmp/additional-files.tar,ro", "--user", "0:0", "--mount", "type=volume,source="+volumeID+",target=/work", - "--mount", "type=bind,source="+archiveWithAdditionalFiles.mockAdditionalFilePaths[".gitignore"]+",target=/tmp/src-additional-file-0,ro", - "--mount", "type=bind,source="+archiveWithAdditionalFiles.mockAdditionalFilePaths["another-file"]+",target=/tmp/src-additional-file-1,ro", DockerVolumeWorkspaceImage, - "sh", "-c", `while test "$#" -gt 0; do cp "$1" "$2" || exit; shift 2; done`, - "copy-additional-files", - "/tmp/src-additional-file-0", "/work/.gitignore", - "/tmp/src-additional-file-1", "/work/another-file", + "tar", "-xf", "/tmp/additional-files.tar", "-C", "/work", ), expect.NewGlob( expect.Success, @@ -388,53 +383,54 @@ func TestVolumeWorkspaceCreator(t *testing.T) { } } -func TestCopyFilesIntoVolumesDoesNotInterpolateNames(t *testing.T) { - const maliciousName = "x,ro,type=bind,source=/var/run/docker.sock,target=/h1sock;touch /work/injected;/.gitignore" +func TestArchiveAdditionalFilesTreatsRepositoryPathsAsData(t *testing.T) { + const maliciousName = "x;touch${IFS}/tmp/pwned,source=.,target=/x/.gitignore" + source := filepath.Join(t.TempDir(), "additional-file") + if err := os.WriteFile(source, []byte("contents"), 0600); err != nil { + t.Fatal(err) + } - expect.Commands( - t, - expect.NewGlob( - expect.Success, - "docker", "run", "--rm", "--init", "--workdir", "/work", - "--user", "0:0", - "--mount", "type=volume,source="+volumeID+",target=/work", - "--mount", "type=bind,source=/tmp/additional-file,target=/tmp/src-additional-file-0,ro", - DockerVolumeWorkspaceImage, - "sh", "-c", `while test "$#" -gt 0; do cp "$1" "$2" || exit; shift 2; done`, - "copy-additional-files", - "/tmp/src-additional-file-0", "/work/"+maliciousName, - ), - ) + wc := &dockerVolumeWorkspaceCreator{tempDir: t.TempDir()} + archive, err := wc.archiveAdditionalFiles(map[string]string{maliciousName: source}) + if err != nil { + t.Fatal(err) + } + defer os.Remove(archive) - wc := &dockerVolumeWorkspaceCreator{} - w := &dockerVolumeWorkspace{volume: volumeID} - err := wc.copyFilesIntoVolumes(context.Background(), w, map[string]string{ - maliciousName: "/tmp/additional-file", - }) + f, err := os.Open(archive) if err != nil { - t.Fatalf("unexpected error: %v", err) + t.Fatal(err) } -} + defer f.Close() -func TestCopyFilesIntoVolumesRejectsUnsafePaths(t *testing.T) { - tests := map[string]map[string]string{ - "workspace traversal": { - "../etc/.gitignore": "/tmp/additional-file", - }, - "mount source injection": { - ".gitignore": "/tmp/additional-file,source=/etc", - }, + r := tar.NewReader(f) + header, err := r.Next() + if err != nil { + t.Fatal(err) + } + if header.Name != maliciousName { + t.Fatalf("unexpected archived path: have=%q want=%q", header.Name, maliciousName) + } + contents, err := io.ReadAll(r) + if err != nil { + t.Fatal(err) } + if string(contents) != "contents" { + t.Fatalf("unexpected archived contents: %q", contents) + } + if _, err := r.Next(); err != io.EOF { + t.Fatalf("unexpected second archive entry: %v", err) + } +} - for name, files := range tests { - t.Run(name, func(t *testing.T) { - expect.Commands(t) - wc := &dockerVolumeWorkspaceCreator{} - w := &dockerVolumeWorkspace{volume: volumeID} - if err := wc.copyFilesIntoVolumes(context.Background(), w, files); err == nil { - t.Fatal("expected unsafe path to be rejected") - } - }) +func TestCopyFilesIntoVolumesRejectsWorkspaceTraversal(t *testing.T) { + expect.Commands(t) + wc := &dockerVolumeWorkspaceCreator{} + w := &dockerVolumeWorkspace{volume: volumeID} + if err := wc.copyFilesIntoVolumes(context.Background(), w, map[string]string{ + "../etc/.gitignore": "/tmp/additional-file", + }); err == nil { + t.Fatal("expected workspace traversal to be rejected") } }