Skip to content

Commit ed0828d

Browse files
cbrnrdcbrnrd
authored
fix/batches: prevent executor environment leakage to steps (#1374)
## Problem Batch steps can request environment variables by name. The executor passed its full process environment into this resolver, so a step could request reserved `SRC_EXECUTOR_*` values. These values include executor control credentials that must not be available inside an ordinary batch step. This fixes [VULN-141](https://linear.app/sourcegraph/issue/VULN-141/i-can-use-a-reserved-executor-bearer-from-a-normal-v2-batch-step-to). ## Solution Filter the reserved `SRC_EXECUTOR_*` namespace from the outer environment before resolving step variables. The filter is applied at the step execution boundary, so it covers every caller and also protects future variables in the reserved namespace. Normal environment variables and explicit step values continue to work. ## Verification Evidence - Added a regression test showing that normal outer variables still resolve. - Added coverage showing that current and future `SRC_EXECUTOR_*` outer variables resolve to empty values. - `go test ./internal/batches/...` - `go test ./cmd/src`
1 parent 4d07a31 commit ed0828d

2 files changed

Lines changed: 46 additions & 2 deletions

File tree

‎internal/batches/executor/run_steps.go‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -316,8 +316,9 @@ func executeSingleStep(
316316
}
317317
defer cleanup()
318318

319-
// Resolve step.Env given the current environment.
320-
stepEnv, err := step.Env.Resolve(opts.GlobalEnv)
319+
// Resolve step.Env given the current environment. Executor control values
320+
// must never be selectable by an author-controlled step.
321+
stepEnv, err := step.Env.Resolve(withoutReservedExecutorEnv(opts.GlobalEnv))
321322
if err != nil {
322323
err = errors.Wrap(err, "resolving step environment")
323324
opts.UI.StepPreparingFailed(stepIdx+1, err)
@@ -464,6 +465,17 @@ func executeSingleStep(
464465
return stdout, stderr, nil
465466
}
466467

468+
func withoutReservedExecutorEnv(env []string) []string {
469+
filtered := make([]string, 0, len(env))
470+
for _, variable := range env {
471+
name, _, found := strings.Cut(variable, "=")
472+
if !found || !strings.HasPrefix(name, "SRC_EXECUTOR_") {
473+
filtered = append(filtered, variable)
474+
}
475+
}
476+
return filtered
477+
}
478+
467479
func setOutputs(stepOutputs batcheslib.Outputs, global map[string]any, stepCtx *template.StepContext) error {
468480
for name, output := range stepOutputs {
469481
var value bytes.Buffer

‎internal/batches/executor/run_steps_test.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package executor
22

33
import (
44
"context"
5+
"encoding/json"
56
"os"
67
"path/filepath"
78
"runtime"
@@ -10,9 +11,40 @@ import (
1011
"github.com/stretchr/testify/require"
1112

1213
batcheslib "github.com/sourcegraph/sourcegraph/lib/batches"
14+
batchenv "github.com/sourcegraph/sourcegraph/lib/batches/env"
1315
"github.com/sourcegraph/sourcegraph/lib/batches/template"
1416
)
1517

18+
func TestWithoutReservedExecutorEnv(t *testing.T) {
19+
env := []string{
20+
"ALLOWED=value",
21+
"SRC_EXECUTOR_JOB_TOKEN=secret",
22+
"SRC_EXECUTOR_FUTURE_SECRET=secret",
23+
"VALUE=contains-SRC_EXECUTOR_JOB_TOKEN",
24+
"MALFORMED",
25+
}
26+
27+
require.Equal(t, []string{
28+
"ALLOWED=value",
29+
"VALUE=contains-SRC_EXECUTOR_JOB_TOKEN",
30+
"MALFORMED",
31+
}, withoutReservedExecutorEnv(env))
32+
33+
var stepEnv batchenv.Environment
34+
require.NoError(t, json.Unmarshal([]byte(`[
35+
"ALLOWED",
36+
"SRC_EXECUTOR_JOB_TOKEN",
37+
"SRC_EXECUTOR_FUTURE_SECRET"
38+
]`), &stepEnv))
39+
resolved, err := stepEnv.Resolve(withoutReservedExecutorEnv(env[:4]))
40+
require.NoError(t, err)
41+
require.Equal(t, map[string]string{
42+
"ALLOWED": "value",
43+
"SRC_EXECUTOR_JOB_TOKEN": "",
44+
"SRC_EXECUTOR_FUTURE_SECRET": "",
45+
}, resolved)
46+
}
47+
1648
func TestParseContainerTempPath(t *testing.T) {
1749
for _, valid := range []string{"/tmp/tmp.abc-123_456", "/tmp/tmp.abc-123_456\n"} {
1850
t.Run("valid_"+valid, func(t *testing.T) {

0 commit comments

Comments
 (0)