From 5c3df2a5087c250950f9ef36312abbbcf8d4a737 Mon Sep 17 00:00:00 2001 From: Carter Brainerd Date: Tue, 8 Sep 2026 11:12:13 -0400 Subject: [PATCH 1/3] fix/batches: prevent workspace path injection --- .../batches/workspace/volume_workspace.go | 17 ++++++--- .../workspace/volume_workspace_test.go | 37 +++++++++++++++++-- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/internal/batches/workspace/volume_workspace.go b/internal/batches/workspace/volume_workspace.go index b161090b77..96ea5ce5b4 100644 --- a/internal/batches/workspace/volume_workspace.go +++ b/internal/batches/workspace/volume_workspace.go @@ -8,7 +8,6 @@ import ( "fmt" "os" "sort" - "strings" "github.com/sourcegraph/sourcegraph/lib/errors" @@ -177,6 +176,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 +192,27 @@ 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 { 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) opts = append(opts, []string{ - "--mount", "type=bind,source=" + localPath + ",target=/tmp/" + name + ",ro", + "--mount", "type=bind,source=" + localPath + ",target=" + mountTarget + ",ro", }...) - 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)) diff --git a/internal/batches/workspace/volume_workspace_test.go b/internal/batches/workspace/volume_workspace_test.go index 9f6706741c..848ceec3e7 100644 --- a/internal/batches/workspace/volume_workspace_test.go +++ b/internal/batches/workspace/volume_workspace_test.go @@ -338,10 +338,13 @@ func TestVolumeWorkspaceCreator(t *testing.T) { "--workdir", "/work", "--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", + "--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", "cp /tmp/.gitignore /work/.gitignore && cp /tmp/another-file /work/another-file;", + "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", ), expect.NewGlob( expect.Success, @@ -385,6 +388,34 @@ 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" + + 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{} + w := &dockerVolumeWorkspace{volume: volumeID} + err := wc.copyFilesIntoVolumes(context.Background(), w, map[string]string{ + maliciousName: "/tmp/additional-file", + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + func TestVolumeWorkspace_Close(t *testing.T) { ctx := context.Background() w := &dockerVolumeWorkspace{volume: volumeID} From 4df03cfe5726f0dd3a1aa338a765913443b39eb0 Mon Sep 17 00:00:00 2001 From: Carter Brainerd Date: Wed, 9 Sep 2026 09:15:42 -0400 Subject: [PATCH 2/3] fix/batches: validate workspace paths and Docker mounts --- internal/batches/docker/mount.go | 24 ++++++++ internal/batches/docker/mount_test.go | 56 +++++++++++++++++++ internal/batches/executor/run_steps.go | 16 ++---- internal/batches/executor/run_steps_test.go | 5 -- internal/batches/workspace/bind_workspace.go | 26 ++++++++- .../batches/workspace/bind_workspace_test.go | 35 ++++++++++++ .../batches/workspace/volume_workspace.go | 22 +++++++- .../workspace/volume_workspace_test.go | 31 ++++++++++ 8 files changed, 193 insertions(+), 22 deletions(-) create mode 100644 internal/batches/docker/mount.go create mode 100644 internal/batches/docker/mount_test.go diff --git a/internal/batches/docker/mount.go b/internal/batches/docker/mount.go new file mode 100644 index 0000000000..95919a6497 --- /dev/null +++ b/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 +} diff --git a/internal/batches/docker/mount_test.go b/internal/batches/docker/mount_test.go new file mode 100644 index 0000000000..e4cd3eebde --- /dev/null +++ b/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) + }) + } +} diff --git a/internal/batches/executor/run_steps.go b/internal/batches/executor/run_steps.go index 6b6a61788e..82630288ef 100644 --- a/internal/batches/executor/run_steps.go +++ b/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" @@ -353,7 +354,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") } @@ -372,7 +373,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") } @@ -385,7 +386,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") } @@ -569,15 +570,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) { diff --git a/internal/batches/executor/run_steps_test.go b/internal/batches/executor/run_steps_test.go index 6b072d55c9..5d156904aa 100644 --- a/internal/batches/executor/run_steps_test.go +++ b/internal/batches/executor/run_steps_test.go @@ -56,11 +56,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{ diff --git a/internal/batches/workspace/bind_workspace.go b/internal/batches/workspace/bind_workspace.go index b925c57080..176277f920 100644 --- a/internal/batches/workspace/bind_workspace.go +++ b/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) { diff --git a/internal/batches/workspace/bind_workspace_test.go b/internal/batches/workspace/bind_workspace_test.go index 1048276c69..f4af2718b8 100644 --- a/internal/batches/workspace/bind_workspace_test.go +++ b/internal/batches/workspace/bind_workspace_test.go @@ -143,6 +143,41 @@ 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) + } + 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 := info.Mode().Perm(); got != 0700 { + t.Fatalf("outside directory permissions changed: got %o, want 700", got) + } +} + func TestPrepareGitRepoRemovesUntrustedGitMetadata(t *testing.T) { dir := t.TempDir() dotGit := filepath.Join(dir, ".git") diff --git a/internal/batches/workspace/volume_workspace.go b/internal/batches/workspace/volume_workspace.go index 96ea5ce5b4..011ffb436c 100644 --- a/internal/batches/workspace/volume_workspace.go +++ b/internal/batches/workspace/volume_workspace.go @@ -111,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 @@ -156,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, @@ -194,12 +198,19 @@ func (wc *dockerVolumeWorkspaceCreator) copyFilesIntoVolumes(ctx context.Context 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=" + mountTarget + ",ro", + "--mount", mount, }...) copyArgs = append(copyArgs, mountTarget, "/work/"+name) @@ -332,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") diff --git a/internal/batches/workspace/volume_workspace_test.go b/internal/batches/workspace/volume_workspace_test.go index 848ceec3e7..cc879614ac 100644 --- a/internal/batches/workspace/volume_workspace_test.go +++ b/internal/batches/workspace/volume_workspace_test.go @@ -416,6 +416,37 @@ func TestCopyFilesIntoVolumesDoesNotInterpolateNames(t *testing.T) { } } +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", + }, + } + + 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 TestUnzipRepoIntoVolumeRejectsMountSourceInjection(t *testing.T) { + expect.Commands(t) + wc := &dockerVolumeWorkspaceCreator{} + w := &dockerVolumeWorkspace{volume: volumeID} + if err := wc.unzipRepoIntoVolume(context.Background(), w, "/tmp/archive,source=/etc"); err == nil { + t.Fatal("expected unsafe mount source to be rejected") + } +} + func TestVolumeWorkspace_Close(t *testing.T) { ctx := context.Background() w := &dockerVolumeWorkspace{volume: volumeID} From 345ef5b12fbc09c040d01b2ab3dea4ade0c93ca0 Mon Sep 17 00:00:00 2001 From: Carter Brainerd Date: Wed, 9 Sep 2026 11:17:46 -0400 Subject: [PATCH 3/3] fix windows test --- internal/batches/workspace/bind_workspace_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/internal/batches/workspace/bind_workspace_test.go b/internal/batches/workspace/bind_workspace_test.go index f4af2718b8..f1bfe121bc 100644 --- a/internal/batches/workspace/bind_workspace_test.go +++ b/internal/batches/workspace/bind_workspace_test.go @@ -153,6 +153,10 @@ func TestCopyToWorkspaceRejectsPathTraversal(t *testing.T) { 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) @@ -160,7 +164,7 @@ func TestCopyToWorkspaceRejectsPathTraversal(t *testing.T) { creator := &dockerBindWorkspaceCreator{} workspace := &dockerBindWorkspace{dir: workspaceDir} - err := creator.copyToWorkspace(context.Background(), workspace, map[string]string{ + err = creator.copyToWorkspace(context.Background(), workspace, map[string]string{ "../victim/.gitignore": source, }) if err == nil || !strings.Contains(err.Error(), "outside the workspace") { @@ -173,8 +177,8 @@ func TestCopyToWorkspaceRejectsPathTraversal(t *testing.T) { if err != nil { t.Fatal(err) } - if got := info.Mode().Perm(); got != 0700 { - t.Fatalf("outside directory permissions changed: got %o, want 700", got) + if got, want := info.Mode().Perm(), before.Mode().Perm(); got != want { + t.Fatalf("outside directory permissions changed: got %o, want %o", got, want) } }