From 5fb1a59aed1a8b1ca5d55e1cb200f2229abc4c8c Mon Sep 17 00:00:00 2001 From: Carter Brainerd Date: Tue, 8 Sep 2026 11:33:40 -0400 Subject: [PATCH] fix/batches: prevent command injection from repository paths --- .../batches/workspace/volume_workspace.go | 94 +++++++++++++++---- .../workspace/volume_workspace_test.go | 58 ++++++++++-- 2 files changed, 124 insertions(+), 28 deletions(-) diff --git a/internal/batches/workspace/volume_workspace.go b/internal/batches/workspace/volume_workspace.go index b161090b77..ef1642e1c1 100644 --- a/internal/batches/workspace/volume_workspace.go +++ b/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!), diff --git a/internal/batches/workspace/volume_workspace_test.go b/internal/batches/workspace/volume_workspace_test.go index 9f6706741c..d3216b9f1d 100644 --- a/internal/batches/workspace/volume_workspace_test.go +++ b/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}