fix/batches: prevent command injection from repository paths

Author: cbrnrd

internal/batches/workspace/volume_workspace.go

@@ -1,14 +1,16 @@
 package workspace
 
 import (
+	"archive/tar"
 	"bytes"
 	"context"
 	"crypto/rand"
 	"encoding/hex"
 	"fmt"
+	"io"
 	"os"
+	"path"
 	"sort"
-	"strings"
 
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 
@@ -178,41 +180,97 @@ func (wc *dockerVolumeWorkspaceCreator) copyFilesIntoVolumes(ctx context.Context
 		return nil
 	}
 
+	archive, err := wc.archiveAdditionalFiles(files)
+	if err != nil {
+		return err
+	}
+	defer os.Remove(archive)
+
 	opts := append([]string{
 		"run",
 		"--rm",
 		"--init",
 		"--workdir", "/work",
+		"--mount", "type=bind,source=" + archive + ",target=/tmp/additional-files.tar,ro",
 	}, 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 copyCmds []string
 	for _, name := range names {
-		localPath := files[name]
-		opts = append(opts, []string{
-			"--mount", "type=bind,source=" + localPath + ",target=/tmp/" + name + ",ro",
-		}...)
+		if name == "" || path.IsAbs(name) || path.Clean(name) != name || name == ".." || len(name) >= 3 && name[:3] == "../" {
+			return "", errors.Errorf("invalid additional file path %q", name)
+		}
 
-		copyCmds = append(copyCmds, "cp /tmp/"+name+" /work/"+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 {
+			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,
-		DockerVolumeWorkspaceImage,
-		"sh", "-c",
-		strings.Join(copyCmds, " && ")+";",
-	)
+		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)
+		}
+	}
 
-	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,12 +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/.gitignore,ro",
-					"--mount", "type=bind,source="+archiveWithAdditionalFiles.mockAdditionalFilePaths["another-file"]+",target=/tmp/another-file,ro",
 					DockerVolumeWorkspaceImage,
-					"sh", "-c", "cp /tmp/.gitignore /work/.gitignore && cp /tmp/another-file /work/another-file;",
+					"tar", "-xf", "/tmp/additional-files.tar", "-C", "/work",
 				),
 				expect.NewGlob(
 					expect.Success,
@@ -385,6 +383,46 @@ func TestVolumeWorkspaceCreator(t *testing.T) {
 	}
 }
 
+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)
+	}
+
+	wc := &dockerVolumeWorkspaceCreator{tempDir: t.TempDir()}
+	archive, err := wc.archiveAdditionalFiles(map[string]string{maliciousName: source})
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.Remove(archive)
+
+	f, err := os.Open(archive)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.Close()
+
+	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)
+	}
+}
+
 func TestVolumeWorkspace_Close(t *testing.T) {
 	ctx := context.Background()
 	w := &dockerVolumeWorkspace{volume: volumeID}