Merge branch 'main' into carterbrainerd-vuln-143-src-batch-volume-mode-a-server-supplied-workspacepath-is

Author: cbrnrd

internal/batches/executor/run_steps.go

@@ -317,8 +317,9 @@ func executeSingleStep(
 	}
 	defer cleanup()
 
-	// Resolve step.Env given the current environment.
-	stepEnv, err := step.Env.Resolve(opts.GlobalEnv)
+	// Resolve step.Env given the current environment. Executor control values
+	// must never be selectable by an author-controlled step.
+	stepEnv, err := step.Env.Resolve(withoutReservedExecutorEnv(opts.GlobalEnv))
 	if err != nil {
 		err = errors.Wrap(err, "resolving step environment")
 		opts.UI.StepPreparingFailed(stepIdx+1, err)
@@ -465,6 +466,17 @@ func executeSingleStep(
 	return stdout, stderr, nil
 }
 
+func withoutReservedExecutorEnv(env []string) []string {
+	filtered := make([]string, 0, len(env))
+	for _, variable := range env {
+		name, _, found := strings.Cut(variable, "=")
+		if !found || !strings.HasPrefix(name, "SRC_EXECUTOR_") {
+			filtered = append(filtered, variable)
+		}
+	}
+	return filtered
+}
+
 func setOutputs(stepOutputs batcheslib.Outputs, global map[string]any, stepCtx *template.StepContext) error {
 	for name, output := range stepOutputs {
 		var value bytes.Buffer

internal/batches/executor/run_steps_test.go

@@ -2,6 +2,7 @@ package executor
 
 import (
 	"context"
+	"encoding/json"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -10,9 +11,40 @@ import (
 	"github.com/stretchr/testify/require"
 
 	batcheslib "github.com/sourcegraph/sourcegraph/lib/batches"
+	batchenv "github.com/sourcegraph/sourcegraph/lib/batches/env"
 	"github.com/sourcegraph/sourcegraph/lib/batches/template"
 )
 
+func TestWithoutReservedExecutorEnv(t *testing.T) {
+	env := []string{
+		"ALLOWED=value",
+		"SRC_EXECUTOR_JOB_TOKEN=secret",
+		"SRC_EXECUTOR_FUTURE_SECRET=secret",
+		"VALUE=contains-SRC_EXECUTOR_JOB_TOKEN",
+		"MALFORMED",
+	}
+
+	require.Equal(t, []string{
+		"ALLOWED=value",
+		"VALUE=contains-SRC_EXECUTOR_JOB_TOKEN",
+		"MALFORMED",
+	}, withoutReservedExecutorEnv(env))
+
+	var stepEnv batchenv.Environment
+	require.NoError(t, json.Unmarshal([]byte(`[
+		"ALLOWED",
+		"SRC_EXECUTOR_JOB_TOKEN",
+		"SRC_EXECUTOR_FUTURE_SECRET"
+	]`), &stepEnv))
+	resolved, err := stepEnv.Resolve(withoutReservedExecutorEnv(env[:4]))
+	require.NoError(t, err)
+	require.Equal(t, map[string]string{
+		"ALLOWED":                    "value",
+		"SRC_EXECUTOR_JOB_TOKEN":     "",
+		"SRC_EXECUTOR_FUTURE_SECRET": "",
+	}, resolved)
+}
+
 func TestParseContainerTempPath(t *testing.T) {
 	for _, valid := range []string{"/tmp/tmp.abc-123_456", "/tmp/tmp.abc-123_456\n"} {
 		t.Run("valid_"+valid, func(t *testing.T) {

internal/batches/workspace/bind_workspace_test.go

@@ -234,7 +234,8 @@ func TestDockerBindWorkspace_DiffRestoresTrustedGitConfig(t *testing.T) {
 	}
 
 	dir := *workspace.WorkDir()
-	configPath := filepath.Join(dir, ".git", "config")
+	dotGit := filepath.Join(dir, ".git")
+	configPath := filepath.Join(dotGit, "config")
 	trustedConfig, err := os.ReadFile(configPath)
 	if err != nil {
 		t.Fatal(err)
@@ -249,6 +250,26 @@ func TestDockerBindWorkspace_DiffRestoresTrustedGitConfig(t *testing.T) {
 	if err := config.Close(); err != nil {
 		t.Fatal(err)
 	}
+
+	// A commondir file redirects Git to the config in another directory. That
+	// config must not survive metadata restoration and reach host-side Git.
+	attackerCommon := filepath.Join(dir, "attacker-common")
+	if err := os.CopyFS(attackerCommon, os.DirFS(dotGit)); err != nil {
+		t.Fatal(err)
+	}
+	attackerConfig, err := os.OpenFile(filepath.Join(attackerCommon, "config"), os.O_APPEND|os.O_WRONLY, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := attackerConfig.WriteString("[filter \"attack\"]\n\tclean = command-that-must-not-run\n\trequired = true\n"); err != nil {
+		t.Fatal(err)
+	}
+	if err := attackerConfig.Close(); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(dotGit, "commondir"), []byte("../attacker-common\n"), 0644); err != nil {
+		t.Fatal(err)
+	}
 	if err := os.WriteFile(filepath.Join(dir, ".gitattributes"), []byte("*.txt filter=attack\n"), 0644); err != nil {
 		t.Fatal(err)
 	}
@@ -270,6 +291,9 @@ func TestDockerBindWorkspace_DiffRestoresTrustedGitConfig(t *testing.T) {
 	if !cmp.Equal(restoredConfig, trustedConfig) {
 		t.Fatalf("Git config was not restored:\n%s", cmp.Diff(string(trustedConfig), string(restoredConfig)))
 	}
+	if _, err := os.Stat(filepath.Join(dotGit, "commondir")); !os.IsNotExist(err) {
+		t.Fatalf("untrusted commondir was not removed: %v", err)
+	}
 }
 
 func TestUnzipRejectsGitMetadata(t *testing.T) {

internal/batches/workspace/git.go

@@ -13,6 +13,7 @@ import (
 
 type gitMetadataSnapshot struct {
 	dotGit         *gitControlFile
+	commonDir      *gitControlFile
 	config         *gitControlFile
 	configWorktree *gitControlFile
 }
@@ -34,7 +35,10 @@ func snapshotGitMetadata(dir string) (*gitMetadataSnapshot, error) {
 	case info.Mode().IsRegular():
 		snapshot.dotGit, err = snapshotGitControlFile(dotGit)
 	case info.IsDir():
-		snapshot.config, err = snapshotGitControlFile(filepath.Join(dotGit, "config"))
+		snapshot.commonDir, err = snapshotOptionalGitControlFile(filepath.Join(dotGit, "commondir"))
+		if err == nil {
+			snapshot.config, err = snapshotGitControlFile(filepath.Join(dotGit, "config"))
+		}
 		if err == nil {
 			snapshot.configWorktree, err = snapshotOptionalGitControlFile(filepath.Join(dotGit, "config.worktree"))
 		}
@@ -87,6 +91,11 @@ func (s *gitMetadataSnapshot) restore(dir string) error {
 	if !info.IsDir() || info.Mode()&os.ModeSymlink != 0 {
 		return fmt.Errorf("%s is no longer a directory", dotGit)
 	}
+	// commondir changes which repository config Git reads. Restore it before
+	// any host-side Git command can follow an attacker-controlled redirect.
+	if err := restoreGitControlFile(filepath.Join(dotGit, "commondir"), s.commonDir); err != nil {
+		return err
+	}
 	if err := restoreGitControlFile(filepath.Join(dotGit, "config"), s.config); err != nil {
 		return err
 	}

internal/servegit/gitservice.go

@@ -105,13 +105,18 @@ func (s *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "invalid path specified: "+err.Error(), http.StatusBadRequest)
 		return
 	}
-	if _, err = s.RootFS.Stat(relDir); os.IsNotExist(err) {
+	info, err := s.RootFS.Stat(relDir)
+	if os.IsNotExist(err) {
 		http.Error(w, "repository not found", http.StatusNotFound)
 		return
 	} else if err != nil {
 		http.Error(w, "failed to stat repo: "+err.Error(), http.StatusInternalServerError)
 		return
 	}
+	if !info.IsDir() {
+		http.Error(w, "repository not found", http.StatusNotFound)
+		return
+	}
 
 	body := r.Body
 	defer body.Close()

internal/servegit/serve_test.go

@@ -69,6 +69,52 @@ func TestReposHandler(t *testing.T) {
 	}
 }
 
+func TestHandlerRejectsGitdirFile(t *testing.T) {
+	root := t.TempDir()
+	servedRoot := filepath.Join(root, "served")
+	publicRepo := filepath.Join(servedRoot, "public")
+	privateRepo := filepath.Join(root, "private")
+	for _, repo := range []string{publicRepo, privateRepo} {
+		if err := os.MkdirAll(repo, 0755); err != nil {
+			t.Fatal(err)
+		}
+	}
+	gitInit(t, publicRepo)
+	gitInit(t, privateRepo)
+
+	pointer := filepath.Join(publicRepo, "gitdir-pointer")
+	if err := os.WriteFile(pointer, []byte("gitdir: ../../private/.git\n"), 0644); err != nil {
+		t.Fatal(err)
+	}
+	runCmd(t, publicRepo, "git", "add", "gitdir-pointer")
+	runCmd(t, publicRepo, "git", "commit", "-m", "add gitdir pointer")
+	runCmd(t, privateRepo, "git", "commit", "--allow-empty", "-m", "private commit")
+	// Git accepts this ordinary tracked file as a repository and follows its
+	// gitdir pointer outside servedRoot.
+	runCmd(t, publicRepo, "git", "upload-pack", "--strict", "--advertise-refs", pointer)
+
+	rootFS, err := os.OpenRoot(servedRoot)
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { rootFS.Close() })
+
+	h := (&Serve{
+		Info:   testLogger(t),
+		Debug:  discardLogger,
+		Addr:   testAddress,
+		Root:   servedRoot,
+		RootFS: rootFS,
+	}).handler()
+	req := httptest.NewRequest(http.MethodGet, "/repos/public/gitdir-pointer/info/refs?service=git-upload-pack", nil)
+	rec := httptest.NewRecorder()
+	h.ServeHTTP(rec, req)
+
+	if rec.Code != http.StatusNotFound {
+		t.Fatalf("gitdir file status = %d, want %d; body: %s", rec.Code, http.StatusNotFound, rec.Body.String())
+	}
+}
+
 func testReposHandler(t *testing.T, h http.Handler, repos []Repo) {
 	ts := httptest.NewServer(h)
 	t.Cleanup(ts.Close)