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

[cbrnrd]

Note: this issue was brought to our attention via a HackerOne report.

Problem

src-cli renders batch spec run: fields through Go's text/template, which performs no output escaping, and the join builtin is a plain strings.Join. The rendered string is written to a script file and executed under /bin/sh inside the batch-change container.

Several template variables expose raw filenames parsed from git diff output and from Sourcegraph search results:

• repository.search_result_paths
• steps.{modified,added,deleted,renamed}_files
• previous_step.{modified,added,deleted,renamed}_files
• step.{modified,added,deleted,renamed}_files

Because git permits filenames containing shell metacharacters (`, $, ;, |, &, >, <, newlines, …), an attacker who controls filenames in a target repository can inject arbitrary shell commands into the rendered script. The reproducer in the report uses the canonical gofmt -w ${{ join repository.search_result_paths " " }} example from the Batch Changes docs to exfiltrate SRC_ACCESS_TOKEN and tamper with the workspace.

This is a supply-chain attack vector: planting a maliciously named file in a public repo is enough to trigger code execution in any organization that runs a batch change matching it. The container has the workspace mounted read-write plus access to SRC_ACCESS_TOKEN and any env: secrets.

Solution

Shell-escape every filename-bearing value at the template FuncMap boundary using github.com/kballard/go-shellquote, so the rendered text is always safe to splat into /bin/sh regardless of what the underlying spec author wrote.

Changes in lib/batches/template/templating.go:

• Added a shellEscapeAll([]string) []string helper that runs shellquote.Join over each element. Elements without metacharacters pass through unmodified, so existing specs targeting benign filenames keep producing identical output.

• Applied it to the four *_files fields in both StepContext.ToFuncMap (covers step, previous_step, steps) and ChangesetTemplateContext.ToFuncMap (covers changeset templates).

• Pre-escaped each element of Repository.SearchResultPaths() so ${{ repository.search_result_paths }} and ${{ join repository.search_result_paths " " }} are both safe.

• Kept the slice shape ([]string) rather than collapsing into a pre-joined string, so ${{ join … " " }} and ${{ range … }} keep working unchanged for spec authors.

• Left stdout / stderr raw. They are routinely captured into outputs and reused as plain values (e.g. a filename written by echo); pre-quoting silently changes those semantics. Spec authors who splat stdio against untrusted data should opt in with the new shellquote_join template builtin:

  run: do-something ${{ shellquote_join previous_step.stdout }}

  This trade-off is documented inline next to the assignment.

The fix is purely at the template layer; the git diff parser in lib/batches/git/changes.go is intentionally left alone so unusual-but-legitimate filenames aren't silently dropped.

Verification Evidence

Regression test

Added lib/batches/template/templating_security_test.go (TestVULN91_NoShellInjectionFromFilenames). It feeds the report's exact PoC filenames

`id > /tmp/PWNED && cat /tmp/PWNED`.go
foo.go; echo INJECTED_$(whoami) > /tmp/PWNED2; #.go
with space.go
with'quote.go
with<newline>.go

through every previously vulnerable variable in both StepContext and ChangesetTemplateContext, and asserts that none of `id, $(, ; echo, > /tmp/PWNED, or PWNED2 appear outside a single-quoted shell region in the rendered output.

Regression test catches the unpatched code

Reverting just the shellEscapeAll(res.ChangedFiles.Modified) call to its original res.ChangedFiles.Modified and re-running:

--- FAIL: TestVULN91_NoShellInjectionFromFilenames/step_run_via_previous_step.modified_files
    rendered output contains unescaped shell metasequence "`id"
    rendered: gofmt -w main.go `id > /tmp/PWNED && cat /tmp/PWNED`.go foo.go; echo INJECTED_$(whoami) > /tmp/PWNED2; #.go …
    rendered output contains unescaped shell metasequence "$("
    rendered output contains unescaped shell metasequence "; echo"
    rendered output contains unescaped shell metasequence "> /tmp/PWNED"
    rendered output contains unescaped shell metasequence "PWNED2"

i.e. the test loudly reproduces the exact injection from the HackerOne report on unpatched code, and passes on the patched code.

Full test suite

$ go test ./...                      # src-cli module
ok      github.com/sourcegraph/src-cli/cmd/src
ok      github.com/sourcegraph/src-cli/internal/batches/docker
ok      github.com/sourcegraph/src-cli/internal/batches/executor
ok      github.com/sourcegraph/src-cli/internal/batches/service
…        (all packages pass)

$ go test ./...                      # lib/ module
ok      github.com/sourcegraph/sourcegraph/lib/batches/template
…        (all packages pass)

Pre-existing executor tests that exercise ${{ join previous_step.modified_files " " }} and ${{ previous_step.modified_files }} continue to render the same output for benign filenames (shellquote.Join("modified.txt") → modified.txt), so no batch spec authored against the documented examples sees a behavior change.

Test Plan

• New unit test TestVULN91_NoShellInjectionFromFilenames covers every affected variable on both StepContext and ChangesetTemplateContext.
• Verified the test fails against the pre-fix code path with the report's exact PoC strings.
• Full go test ./... passes for both modules.

[keegancsmith]

lib is vendored in from our monorepo, so you need to make changes rather in the monorepo and then vendor it back in here.

I'm also not sure we should be escaping these values. Our templates are not just used for constructing commands in batch changes. I'll let @burmudar comment further, I think he had looked into this stuff not so long ago?? (Or did we both look into it? :) )

Generally batch changes are run on repos you have write access to though... which makes this not really an issue in practice. However, I do see us needing to provide some way to do shell escaping since that means previous uses of this would naively break i fyou r filename had a space in it

[keegancsmith]

no need to sort

[cbrnrd]

Why not? the previous version of the function sorted the paths.

[burmudar]

lib is vendored in from our monorepo, so you need to make changes rather in the monorepo and then vendor it back in here.

I'm also not sure we should be escaping these values. Our templates are not just used for constructing commands in batch changes. I'll let @burmudar comment further, I think he had looked into this stuff not so long ago?? (Or did we both look into it? :) )

Generally batch changes are run on repos you have write access to though... which makes this not really an issue in practice. However, I do see us needing to provide some way to do shell escaping since that means previous uses of this would naively break i fyou r filename had a space in it

Yeah we did at some point, it's all a bit muddled now but I took a jog down memory lane. This templating is used to render into step-condition or the run step so on some level it is good to escape this.

• I think we need to add shellquote_split too if we have join
• Document both so that customers can use them more deliberately.

[cbrnrd]

Generally batch changes are run on repos you have write access to though... which makes this not really an issue in practice.

I disagree. Even if you have access to the repository, that doesn't necessarily make it a trusted source you want to be running code from. Since the attack vector here is a filename, just by including a malicious repo in your spec can trigger command execution locally.