From e0faaadbdec38cb40f6e2586e0d7e0d27dfc7ce0 Mon Sep 17 00:00:00 2001 From: Carter Brainerd Date: Tue, 8 Sep 2026 10:24:59 -0400 Subject: [PATCH] fix/batches: prevent Git commondir config bypass --- .../batches/workspace/bind_workspace_test.go | 26 ++++++++++++++++++- internal/batches/workspace/git.go | 11 +++++++- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/internal/batches/workspace/bind_workspace_test.go b/internal/batches/workspace/bind_workspace_test.go index 1048276c69..b8c32e929c 100644 --- a/internal/batches/workspace/bind_workspace_test.go +++ b/internal/batches/workspace/bind_workspace_test.go @@ -195,7 +195,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) @@ -210,6 +211,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) } @@ -231,6 +252,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) { diff --git a/internal/batches/workspace/git.go b/internal/batches/workspace/git.go index 43c1c8ba70..c028759f72 100644 --- a/internal/batches/workspace/git.go +++ b/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 }