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 4dfeb14e63..11a9f2b774 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" @@ -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) { diff --git a/internal/batches/executor/run_steps_test.go b/internal/batches/executor/run_steps_test.go index ccdab14c9d..684273ed45 100644 --- a/internal/batches/executor/run_steps_test.go +++ b/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{ 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 b8c32e929c..71d5291ea7 100644 --- a/internal/batches/workspace/bind_workspace_test.go +++ b/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") diff --git a/internal/batches/workspace/volume_workspace.go b/internal/batches/workspace/volume_workspace.go index b161090b77..011ffb436c 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" @@ -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") diff --git a/internal/batches/workspace/volume_workspace_test.go b/internal/batches/workspace/volume_workspace_test.go index 9f6706741c..cc879614ac 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,65 @@ 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 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}