Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions internal/batches/executor/run_steps.go
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,16 @@ func createFilesToMount(tempDir string, step batcheslib.Step, stepContext *templ
// can mount them into the container.
filesToMount := make(map[string]*os.File, len(files))
for name, content := range files {
// Defense-in-depth: the mount target name is concatenated directly
// into a Docker `--mount type=bind,...,target=<name>` argument. A comma
// in the name would let an attacker break out of the mount spec and
// inject arbitrary source=/target= pairs (e.g. /var/run/docker.sock).
// Parse-time validation rejects this, but the executor receives
// pre-parsed steps via JSON and never re-runs that validation, so we
// re-check here at the sink.
if strings.Contains(name, ",") {
return nil, cleanup, errors.Newf("files target path %q contains invalid characters", name)
}
Comment on lines +543 to +552

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so this would of failed which is what we want, but now we fail earlier? And that is better from both a UX perspective + extra defense incase the other check for some reason doesn't run?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct!

fp, err := os.CreateTemp(tempDir, "")
if err != nil {
return nil, cleanup, errors.Wrap(err, "creating temporary file")
Expand Down
16 changes: 16 additions & 0 deletions internal/batches/executor/run_steps_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,25 @@ import (

"github.com/stretchr/testify/require"

batcheslib "github.com/sourcegraph/sourcegraph/lib/batches"
"github.com/sourcegraph/sourcegraph/lib/batches/template"
)

func TestCreateFilesToMount_RejectsCommaInTargetPath(t *testing.T) {
step := batcheslib.Step{
Files: map[string]string{
"/tmp/x,source=/var/run/docker.sock,target=/var/run/docker.sock": "IGNORED",
},
}

_, cleanup, err := createFilesToMount(t.TempDir(), step, &template.StepContext{})
if cleanup != nil {
cleanup()
}
require.Error(t, err)
require.Contains(t, err.Error(), "contains invalid characters")
}

func TestRenderStepContainer(t *testing.T) {
t.Run("static image", func(t *testing.T) {
got, err := renderStepContainer("alpine:3", &template.StepContext{})
Expand Down
Loading