fix/security: shell-escape attacker-controlled filenames in batch change templates

go.mod

@@ -19,7 +19,6 @@ require (
 	github.com/jedib0t/go-pretty/v6 v6.6.3
 	github.com/jig/teereadcloser v0.0.0-20181016160506-953720c48e05
 	github.com/json-iterator/go v1.1.12
-	github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51
 	github.com/mattn/go-isatty v0.0.20
 	github.com/neelance/parallel v0.0.0-20160708114440-4de9ce63d14c
 	github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8
@@ -75,6 +74,7 @@ require (
 	github.com/jackc/pgpassfile v1.0.0 // indirect
 	github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect
 	github.com/jackc/pgx/v5 v5.9.2 // indirect
+	github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 // indirect
 	github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 // indirect
 	github.com/planetscale/vtprotobuf v0.6.1-0.20240319094008-0393e58bdf10 // indirect
 	github.com/sasha-s/go-deadlock v0.3.5 // indirect

lib/batches/template/templating.go

@@ -11,6 +11,7 @@ import (
 	"github.com/gobwas/glob"
 	"github.com/grafana/regexp"
 
+	"github.com/kballard/go-shellquote"
 	"github.com/sourcegraph/sourcegraph/lib/batches/execution"
 	"github.com/sourcegraph/sourcegraph/lib/batches/git"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
@@ -19,10 +20,28 @@ import (
 const startDelim = "${{"
 const endDelim = "}}"
 
+// shellEscapeAll returns a copy of in where every element has been quoted
+// with shellquote.Join so that it is safe to splat into a /bin/sh command line.
+// Elements without shell metacharacters are returned unmodified, so callers
+// that pre-existed this escaping (e.g. `${{ join steps.modified_files " " }}`
+// against safe filenames) continue to see the same rendered output.
+//
+// 🚨 SECURITY: This is used to defang attacker-controlled filenames coming
+// out of `git diff` parsing before they reach the rendered step script. See
+// VULN-91 / HackerOne report 3767160.
+func shellEscapeAll(in ...string) []string {
+	out := make([]string, len(in))
+	for i, s := range in {
+		out[i] = shellquote.Join(s)
+	}
+	return out
+}
+
 var builtins = template.FuncMap{
-	"join":    strings.Join,
-	"split":   strings.Split,
-	"replace": strings.ReplaceAll,
+	"join":            strings.Join,
+	"shellquote_join": shellquote.Join,
+	"split":           strings.Split,
+	"replace":         strings.ReplaceAll,
 	"join_if": func(sep string, elems ...string) string {
 		var nonBlank []string
 		for _, e := range elems {
@@ -163,9 +182,23 @@ type Repository struct {
 	FileMatches []string
 }
 
+// SearchResultPaths returns the repository's matched paths in a form that is
+// safe to splat into a /bin/sh command line.
+//
+// 🚨 SECURITY: paths originate from a Sourcegraph search and ultimately from
+// `git`, so they may contain attacker-controlled shell metacharacters (see
+// VULN-91). Every element is run through shellquote.Join before it is exposed
+// to the step template. Elements without metacharacters are returned
+// unmodified, so existing usage like `${{ join repository.search_result_paths
+// " " }}` keeps producing the same output for benign filenames.
 func (r Repository) SearchResultPaths() (list fileMatchPathList) {
-	sort.Strings(r.FileMatches)
-	return r.FileMatches
+	paths := make([]string, len(r.FileMatches))
+	copy(paths, r.FileMatches)
+	sort.Strings(paths)
+	for i, p := range paths {
+		paths[i] = shellquote.Join(p)
+	}
+	return paths
 }
 
 type fileMatchPathList []string
@@ -208,10 +241,24 @@ func (stepCtx *StepContext) ToFuncMap() template.FuncMap {
 			return m
 		}
 
-		m["modified_files"] = res.ChangedFiles.Modified
-		m["added_files"] = res.ChangedFiles.Added
-		m["deleted_files"] = res.ChangedFiles.Deleted
-		m["renamed_files"] = res.ChangedFiles.Renamed
+		// 🚨 SECURITY: file lists are derived from `git diff` output and can
+		// contain attacker-controlled filenames with shell metacharacters.
+		// We shell-escape each element before exposing them to the step
+		// template to prevent command injection when the rendered template
+		// is executed by /bin/sh. The slice shape is preserved so
+		// `${{ join … " " }}` and `${{ range … }}` continue to work as
+		// before. See VULN-91.
+		//
+		// NOTE: stdout/stderr are intentionally NOT pre-escaped. They are
+		// commonly captured into `outputs` and reused as plain values (e.g.
+		// a filename written by `echo`), where pre-quoting would change
+		// semantics. Users that splat stdout/stderr into a shell command
+		// against untrusted data should pipe through the `shellquote_join`
+		// builtin: `${{ shellquote_join previous_step.stdout }}`.
+		m["modified_files"] = shellEscapeAll(res.ChangedFiles.Modified...)
+		m["added_files"] = shellEscapeAll(res.ChangedFiles.Added...)
+		m["deleted_files"] = shellEscapeAll(res.ChangedFiles.Deleted...)
+		m["renamed_files"] = shellEscapeAll(res.ChangedFiles.Renamed...)
 		m["stdout"] = res.Stdout
 		m["stderr"] = res.Stderr
 
@@ -295,11 +342,13 @@ func (tmplCtx *ChangesetTemplateContext) ToFuncMap() template.FuncMap {
 			return tmplCtx.Outputs
 		},
 		"steps": func() map[string]any {
+			// 🚨 SECURITY: shell-escape per element to defang attacker
+			// controlled filenames from `git diff`. See VULN-91.
 			return map[string]any{
-				"modified_files": tmplCtx.Steps.Changes.Modified,
-				"added_files":    tmplCtx.Steps.Changes.Added,
-				"deleted_files":  tmplCtx.Steps.Changes.Deleted,
-				"renamed_files":  tmplCtx.Steps.Changes.Renamed,
+				"modified_files": shellEscapeAll(tmplCtx.Steps.Changes.Modified...),
+				"added_files":    shellEscapeAll(tmplCtx.Steps.Changes.Added...),
+				"deleted_files":  shellEscapeAll(tmplCtx.Steps.Changes.Deleted...),
+				"renamed_files":  shellEscapeAll(tmplCtx.Steps.Changes.Renamed...),
 				"path":           tmplCtx.Steps.Path,
 			}
 		},

lib/batches/template/templating_security_test.go

@@ -0,0 +1,152 @@
+package template
+
+import (
+	"bytes"
+	"strings"
+	"testing"
+
+	"github.com/sourcegraph/sourcegraph/lib/batches/execution"
+	"github.com/sourcegraph/sourcegraph/lib/batches/git"
+)
+
+// TestVULN91_NoShellInjectionFromFilenames is a regression test for VULN-91
+// (HackerOne report 3767160). It verifies that attacker-controlled filenames
+// surfaced through the various filename-bearing template variables cannot
+// inject shell metacharacters into the rendered step script.
+//
+// The PoC in the report relies on filenames like
+//
+//	`id > /tmp/PWNED`.go
+//	foo.go; echo INJECTED; #.go
+//
+// being rendered verbatim through `${{ join repository.search_result_paths " " }}`
+// and friends, then executed under /bin/sh. After the fix, each filename must
+// be shell-quoted so the metacharacters lose their shell meaning.
+func TestVULN91_NoShellInjectionFromFilenames(t *testing.T) {
+	maliciousFilenames := []string{
+		"main.go",
+		"`id > /tmp/PWNED && cat /tmp/PWNED`.go",
+		"foo.go; echo INJECTED_$(whoami) > /tmp/PWNED2; #.go",
+		"with space.go",
+		"with'quote.go",
+		"with\nnewline.go",
+	}
+
+	// These template snippets all unconditionally splat filename-bearing
+	// variables into a shell command. None of them may contain raw shell
+	// metacharacters after rendering.
+	templates := map[string]string{
+		"step run via repository.search_result_paths": `gofmt -w ${{ join repository.search_result_paths " " }}`,
+		"step run via previous_step.modified_files":   `gofmt -w ${{ join previous_step.modified_files " " }}`,
+		"step run via previous_step.added_files":      `gofmt -w ${{ join previous_step.added_files " " }}`,
+		"step run via previous_step.deleted_files":    `rm -f ${{ join previous_step.deleted_files " " }}`,
+		"step run via previous_step.renamed_files":    `touch ${{ join previous_step.renamed_files " " }}`,
+		"step run via steps.modified_files":           `gofmt -w ${{ join steps.modified_files " " }}`,
+		"step run via direct repository fmt":          `gofmt -w ${{ repository.search_result_paths }}`,
+	}
+
+	stepCtx := &StepContext{
+		Repository: Repository{
+			Name:        "github.com/example/repo",
+			Branch:      "main",
+			FileMatches: maliciousFilenames,
+		},
+		PreviousStep: execution.AfterStepResult{
+			ChangedFiles: git.Changes{
+				Modified: maliciousFilenames,
+				Added:    maliciousFilenames,
+				Deleted:  maliciousFilenames,
+				Renamed:  maliciousFilenames,
+			},
+		},
+		Steps: StepsContext{
+			Changes: git.Changes{
+				Modified: maliciousFilenames,
+				Added:    maliciousFilenames,
+				Deleted:  maliciousFilenames,
+				Renamed:  maliciousFilenames,
+			},
+		},
+	}
+
+	// Tokens that, if they appear unescaped in the rendered output, would
+	// trigger shell command substitution, command chaining, or redirection.
+	dangerousSubstrings := []string{
+		"`id",
+		"$(",
+		"; echo",
+		"> /tmp/PWNED",
+		"PWNED2",
+	}
+
+	for name, tmpl := range templates {
+		t.Run(name, func(t *testing.T) {
+			var out bytes.Buffer
+			if err := RenderStepTemplate("vuln-91", tmpl, &out, stepCtx); err != nil {
+				t.Fatalf("RenderStepTemplate returned error: %v", err)
+			}
+			rendered := out.String()
+			for _, bad := range dangerousSubstrings {
+				if containsUnquoted(rendered, bad) {
+					t.Errorf("rendered output contains unescaped shell metasequence %q\nrendered: %s", bad, rendered)
+				}
+			}
+		})
+	}
+
+	// Also exercise the ChangesetTemplateContext path.
+	tmplCtx := &ChangesetTemplateContext{
+		Repository: Repository{
+			Name:        "github.com/example/repo",
+			Branch:      "main",
+			FileMatches: maliciousFilenames,
+		},
+		Steps: StepsContext{
+			Changes: git.Changes{
+				Modified: maliciousFilenames,
+				Added:    maliciousFilenames,
+				Deleted:  maliciousFilenames,
+				Renamed:  maliciousFilenames,
+			},
+		},
+	}
+	ctTemplates := map[string]string{
+		"changeset via repository.search_result_paths": `body: ${{ join repository.search_result_paths " " }}`,
+		"changeset via steps.modified_files":           `body: ${{ join steps.modified_files " " }}`,
+	}
+	for name, tmpl := range ctTemplates {
+		t.Run(name, func(t *testing.T) {
+			rendered, err := RenderChangesetTemplateField("vuln-91", tmpl, tmplCtx)
+			if err != nil {
+				t.Fatalf("RenderChangesetTemplateField returned error: %v", err)
+			}
+			for _, bad := range dangerousSubstrings {
+				if containsUnquoted(rendered, bad) {
+					t.Errorf("rendered output contains unescaped shell metasequence %q\nrendered: %s", bad, rendered)
+				}
+			}
+		})
+	}
+}
+
+// containsUnquoted reports whether needle occurs in haystack outside of a
+// single-quoted shell context. After shellquote.Join, dangerous bytes are
+// either backslash-escaped (e.g. \`) or wrapped in single quotes that
+// terminate the surrounding string literal, so any naive substring match
+// reliably finds an injection. We additionally allow the substring to appear
+// inside a single-quoted region (which shell does NOT interpret) to avoid
+// false positives on the literal text of the (now safely quoted) filename.
+func containsUnquoted(haystack, needle string) bool {
+	for i := 0; i+len(needle) <= len(haystack); i++ {
+		if haystack[i:i+len(needle)] != needle {
+			continue
+		}
+		// Count single quotes before this position; if odd, we're inside a
+		// single-quoted region and the bytes are harmless.
+		quotes := strings.Count(haystack[:i], "'")
+		if quotes%2 == 0 {
+			return true
+		}
+	}
+	return false
+}

lib/go.mod

@@ -56,6 +56,7 @@ require (
 	github.com/gorilla/css v1.0.1 // indirect
 	github.com/jackc/pgpassfile v1.0.0 // indirect
 	github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect
+	github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51
 	github.com/klauspost/compress v1.18.0 // indirect
 	github.com/kr/pretty v0.3.1 // indirect
 	github.com/kr/text v0.2.0 // indirect

lib/go.sum

@@ -93,6 +93,8 @@ github.com/jackc/pgx/v5 v5.9.2 h1:3ZhOzMWnR4yJ+RW1XImIPsD1aNSz4T4fyP7zlQb56hw=
 github.com/jackc/pgx/v5 v5.9.2/go.mod h1:mal1tBGAFfLHvZzaYh77YS/eC6IX9OWbRV1QIIM0Jn4=
 github.com/jackc/puddle/v2 v2.2.2 h1:PR8nw+E/1w0GLuRFSmiioY6UooMp6KJv0/61nB7icHo=
 github.com/jackc/puddle/v2 v2.2.2/go.mod h1:vriiEXHvEE654aYKXXjOvZM39qJ0q+azkZFrfEOc3H4=
+github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 h1:Z9n2FFNUXsshfwJMBgNA0RU6/i7WVaAegv3PtuIHPMs=
+github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51/go.mod h1:CzGEWj7cYgsdH8dAjBGEr58BoE7ScuLd+fwFZ44+/x8=
 github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8=
 github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck=
 github.com/klauspost/compress v1.18.0 h1:c/Cqfb0r+Yi+JtIEq73FWXVkRonBlf0CRNYc8Zttxdo=