fix/batches: prevent workspace path injection (#1372)

Author: cbrnrd

internal/batches/docker/mount.go

@@ -0,0 +1,24 @@
+package docker
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/sourcegraph/sourcegraph/lib/errors"
+)
+
+// BindMount returns a Docker bind mount specification after checking that its
+// values cannot inject fields into Docker's comma-delimited mount grammar.
+func BindMount(source, target string, readOnly bool) (string, error) {
+	for name, value := range map[string]string{"source": source, "target": target} {
+		if value == "" || strings.ContainsAny(value, ",\r\n\x00") {
+			return "", errors.Newf("invalid Docker mount %s %q", name, value)
+		}
+	}
+
+	mount := fmt.Sprintf("type=bind,source=%s,target=%s", source, target)
+	if readOnly {
+		mount += ",ro"
+	}
+	return mount, nil
+}

internal/batches/docker/mount_test.go

@@ -0,0 +1,56 @@
+package docker
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestBindMount(t *testing.T) {
+	tests := []struct {
+		name     string
+		source   string
+		target   string
+		readOnly bool
+		want     string
+		wantErr  bool
+	}{
+		{
+			name:   "writable",
+			source: "/tmp/workspace",
+			target: "/work",
+			want:   "type=bind,source=/tmp/workspace,target=/work",
+		},
+		{
+			name:     "read-only",
+			source:   "/tmp/archive",
+			target:   "/tmp/archive",
+			readOnly: true,
+			want:     "type=bind,source=/tmp/archive,target=/tmp/archive,ro",
+		},
+		{
+			name:    "source injection",
+			source:  "/tmp/archive,source=/etc",
+			target:  "/tmp/archive",
+			wantErr: true,
+		},
+		{
+			name:    "target injection",
+			source:  "/tmp/archive",
+			target:  "/tmp/archive,source=/etc",
+			wantErr: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			got, err := BindMount(test.source, test.target, test.readOnly)
+			if test.wantErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			require.Equal(t, test.want, got)
+		})
+	}
+}

internal/batches/executor/run_steps.go

@@ -21,6 +21,7 @@ import (
 	"github.com/sourcegraph/sourcegraph/lib/batches/template"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 
+	"github.com/sourcegraph/src-cli/internal/batches/docker"
 	"github.com/sourcegraph/src-cli/internal/batches/log"
 	"github.com/sourcegraph/src-cli/internal/batches/repozip"
 	"github.com/sourcegraph/src-cli/internal/batches/util"
@@ -354,7 +355,7 @@ func executeSingleStep(
 	if err := validateContainerTempPath(containerTemp); err != nil {
 		return bytes.Buffer{}, bytes.Buffer{}, errors.Wrap(err, "validating run script target")
 	}
-	runScriptMount, err := dockerBindMount(runScriptFile, containerTemp)
+	runScriptMount, err := docker.BindMount(runScriptFile, containerTemp, true)
 	if err != nil {
 		return bytes.Buffer{}, bytes.Buffer{}, errors.Wrap(err, "creating run script mount")
 	}
@@ -373,7 +374,7 @@ func executeSingleStep(
 	}
 
 	for target, source := range filesToMount {
-		mountArg, err := dockerBindMount(source.Name(), target)
+		mountArg, err := docker.BindMount(source.Name(), target, true)
 		if err != nil {
 			return bytes.Buffer{}, bytes.Buffer{}, errors.Wrap(err, "creating files mount")
 		}
@@ -386,7 +387,7 @@ func executeSingleStep(
 		if err != nil {
 			return bytes.Buffer{}, bytes.Buffer{}, err
 		}
-		mountArg, err := dockerBindMount(workspaceFilePath, mount.Mountpoint)
+		mountArg, err := docker.BindMount(workspaceFilePath, mount.Mountpoint, true)
 		if err != nil {
 			return bytes.Buffer{}, bytes.Buffer{}, errors.Wrap(err, "creating host mount")
 		}
@@ -581,15 +582,6 @@ func validateContainerTempPath(tempfile string) error {
 	return nil
 }
 
-func dockerBindMount(source, target string) (string, error) {
-	for name, value := range map[string]string{"source": source, "target": target} {
-		if value == "" || strings.ContainsAny(value, ",\r\n\x00") {
-			return "", errors.Newf("invalid Docker mount %s %q", name, value)
-		}
-	}
-	return fmt.Sprintf("type=bind,source=%s,target=%s,ro", source, target), nil
-}
-
 // createFilesToMount creates temporary files with the contents of Step.Files
 // that are to be mounted into the container that executes the step.
 func createFilesToMount(tempDir string, step batcheslib.Step, stepContext *template.StepContext) (map[string]*os.File, func(), error) {

internal/batches/executor/run_steps_test.go

@@ -88,11 +88,6 @@ func TestProbeImageForShellRejectsMountInjection(t *testing.T) {
 	require.Contains(t, err.Error(), "mktemp returned invalid path")
 }
 
-func TestDockerBindMountRejectsMountGrammar(t *testing.T) {
-	_, err := dockerBindMount("/tmp/script", "/tmp/x,source=/var/run/docker.sock")
-	require.Error(t, err)
-}
-
 func TestCreateFilesToMount_RejectsCommaInTargetPath(t *testing.T) {
 	step := batcheslib.Step{
 		Files: map[string]string{

internal/batches/workspace/bind_workspace.go

@@ -13,6 +13,7 @@ import (
 	batcheslib "github.com/sourcegraph/sourcegraph/lib/batches"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 
+	"github.com/sourcegraph/src-cli/internal/batches/docker"
 	"github.com/sourcegraph/src-cli/internal/batches/graphql"
 	"github.com/sourcegraph/src-cli/internal/batches/repozip"
 	"github.com/sourcegraph/src-cli/internal/batches/util"
@@ -77,6 +78,10 @@ func (wc *dockerBindWorkspaceCreator) unzipToWorkspace(ctx context.Context, repo
 
 func (wc *dockerBindWorkspaceCreator) copyToWorkspace(ctx context.Context, w *dockerBindWorkspace, files map[string]string) error {
 	for name, src := range files {
+		if err := validateWorkspaceFileName(name); err != nil {
+			return err
+		}
+
 		srcStat, err := os.Stat(src)
 		if err != nil {
 			return err
@@ -91,7 +96,7 @@ func (wc *dockerBindWorkspaceCreator) copyToWorkspace(ctx context.Context, w *do
 			return err
 		}
 
-		destPath := path.Join(w.dir, name)
+		destPath := filepath.Join(w.dir, filepath.FromSlash(name))
 
 		destFile, err := prepareCopyDestinationFile(srcStat, destPath)
 		if err != nil {
@@ -136,12 +141,29 @@ func (w *dockerBindWorkspace) Close(ctx context.Context) error {
 }
 
 func (w *dockerBindWorkspace) DockerRunOpts(ctx context.Context, target string) ([]string, error) {
+	mount, err := docker.BindMount(w.dir, target, false)
+	if err != nil {
+		return nil, err
+	}
 	return []string{
 		"--mount",
-		fmt.Sprintf("type=bind,source=%s,target=%s", w.dir, target),
+		mount,
 	}, nil
 }
 
+func validateWorkspaceFileName(name string) error {
+	clean := path.Clean(name)
+	if path.IsAbs(name) || clean == ".." || strings.HasPrefix(clean, "../") {
+		return errors.Newf("workspace file path %q is outside the workspace", name)
+	}
+
+	native := filepath.Clean(filepath.FromSlash(name))
+	if filepath.IsAbs(native) || filepath.VolumeName(native) != "" || native == ".." || strings.HasPrefix(native, ".."+string(os.PathSeparator)) {
+		return errors.Newf("workspace file path %q is outside the workspace", name)
+	}
+	return nil
+}
+
 func (w *dockerBindWorkspace) WorkDir() *string { return &w.dir }
 
 func (w *dockerBindWorkspace) Diff(ctx context.Context) ([]byte, error) {

internal/batches/workspace/bind_workspace_test.go

@@ -143,6 +143,45 @@ func TestDockerBindWorkspaceCreator_Create(t *testing.T) {
 	})
 }
 
+func TestCopyToWorkspaceRejectsPathTraversal(t *testing.T) {
+	root := t.TempDir()
+	workspaceDir := filepath.Join(root, "workspace")
+	if err := os.Mkdir(workspaceDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+	victimDir := filepath.Join(root, "victim")
+	if err := os.Mkdir(victimDir, 0700); err != nil {
+		t.Fatal(err)
+	}
+	before, err := os.Stat(victimDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	source := filepath.Join(root, "source")
+	if err := os.WriteFile(source, []byte("attacker content"), 0600); err != nil {
+		t.Fatal(err)
+	}
+
+	creator := &dockerBindWorkspaceCreator{}
+	workspace := &dockerBindWorkspace{dir: workspaceDir}
+	err = creator.copyToWorkspace(context.Background(), workspace, map[string]string{
+		"../victim/.gitignore": source,
+	})
+	if err == nil || !strings.Contains(err.Error(), "outside the workspace") {
+		t.Fatalf("expected path traversal error, got %v", err)
+	}
+	if _, err := os.Stat(filepath.Join(victimDir, ".gitignore")); !os.IsNotExist(err) {
+		t.Fatalf("file was written outside the workspace: %v", err)
+	}
+	info, err := os.Stat(victimDir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got, want := info.Mode().Perm(), before.Mode().Perm(); got != want {
+		t.Fatalf("outside directory permissions changed: got %o, want %o", got, want)
+	}
+}
+
 func TestPrepareGitRepoRemovesUntrustedGitMetadata(t *testing.T) {
 	dir := t.TempDir()
 	dotGit := filepath.Join(dir, ".git")

internal/batches/workspace/volume_workspace.go

@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"os"
 	"sort"
-	"strings"
 
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 
@@ -112,6 +111,10 @@ git commit --quiet --all --allow-empty -m src-action-exec
 func (wc *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context, w *dockerVolumeWorkspace, zip string) error {
 	// We want to mount that temporary file into a Docker container that has the
 	// workspace volume attached, and unzip it into the volume.
+	zipMount, err := docker.BindMount(zip, "/tmp/zip", true)
+	if err != nil {
+		return errors.Wrap(err, "creating archive mount")
+	}
 
 	// We need to keep a temporary file in the volume before unzipping for the
 	// permissions to persist because... reasons. Rather than reading the
@@ -157,7 +160,7 @@ func (wc *dockerVolumeWorkspaceCreator) unzipRepoIntoVolume(ctx context.Context,
 		"--rm",
 		"--init",
 		"--workdir", "/work",
-		"--mount", "type=bind,source=" + zip + ",target=/tmp/zip,ro",
+		"--mount", zipMount,
 	}, w.dockerRunOptsWithUser(w.uidGid, "/work")...)
 	opts = append(
 		opts,
@@ -177,6 +180,7 @@ 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`
 
 	opts := append([]string{
 		"run",
@@ -192,22 +196,34 @@ func (wc *dockerVolumeWorkspaceCreator) copyFilesIntoVolumes(ctx context.Context
 	}
 	sort.Strings(names)
 
-	var copyCmds []string
-	for _, name := range names {
+	var copyArgs []string
+	for i, name := range names {
+		if err := validateWorkspaceFileName(name); err != nil {
+			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 err != nil {
+			return errors.Wrap(err, "creating additional file mount")
+		}
 		opts = append(opts, []string{
-			"--mount", "type=bind,source=" + localPath + ",target=/tmp/" + name + ",ro",
+			"--mount", mount,
 		}...)
 
-		copyCmds = append(copyCmds, "cp /tmp/"+name+" /work/"+name)
+		copyArgs = append(copyArgs, mountTarget, "/work/"+name)
 	}
 
 	opts = append(
 		opts,
 		DockerVolumeWorkspaceImage,
 		"sh", "-c",
-		strings.Join(copyCmds, " && ")+";",
+		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))
@@ -327,12 +343,17 @@ func (w *dockerVolumeWorkspace) runScript(ctx context.Context, target, script st
 		return nil, errors.Wrap(err, "generating run options")
 	}
 
+	scriptMount, err := docker.BindMount(name, "/run.sh", true)
+	if err != nil {
+		return nil, errors.Wrap(err, "creating run script mount")
+	}
+
 	opts := append([]string{
 		"run",
 		"--rm",
 		"--init",
 		"--workdir", target,
-		"--mount", "type=bind,source=" + name + ",target=/run.sh,ro",
+		"--mount", scriptMount,
 	}, common...)
 	opts = append(opts, DockerVolumeWorkspaceImage, "sh", "/run.sh")