fix/batches: prevent command injection from repository paths

Author: cbrnrd

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!),

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")
 	}
 }