From ac8401b709cef085c81b01da4989ff4f7c7264cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Sun, 19 Jul 2026 16:12:56 +0100 Subject: [PATCH] fix: harden submodule init against malicious .gitmodules (#1802) * fix: harden submodule init against malicious .gitmodules initSubmodules ran a bare `git submodule update --init --recursive` in a worktree whose HEAD is attacker-controlled during a fork-PR review task, with no protocol restriction, no hooks disable, and no cmd.Env. Attacker `.gitmodules` could pick a command-executing transport (`ext::sh -c`, CVE-2018-17456), a local `file://` path, or a plain-http URL (cloud metadata SSRF). Harden the single sink so all call sites benefit: - protocol.allow=never default-denies every transport; only https/ssh are re-enabled (blocks file://, git://, plain http:// SSRF). - protocol.ext.allow=never hard-pins the RCE transport off; on the command line it outranks any ambient protocol.ext.allow=always. - core.hooksPath=/dev/null neutralizes attacker-planted hooks. - GIT_TERMINAL_PROMPT=0 stops attacker URLs from hanging on a prompt. Residual: attacker-chosen https/ssh URLs are still fetched (transports real submodules require); fully closing that needs gating submodule init off for untrusted worktrees, tracked separately. * fix: address submodule hardening review feedback - Reuse newNonInteractiveGitCmd so submodule init inherits the full non-interactive env set (GIT_SSH_COMMAND=ssh -oBatchMode=yes, SSH/GCM askpass suppression, WaitDelay). Prevents an attacker ssh:// submodule URL from hanging the review runner on host-key/passphrase prompts until the context deadline (GIT_TERMINAL_PROMPT=0 alone only covers git's HTTP prompt, not SSH). - Correct the file:// comment: command-line -c outranks repo/global git config, so ambient protocol.file.allow=always does NOT re-enable file submodules here. This is intentional for untrusted trees; a trusted opt-in must be an explicit per-repo -c, not implicit host config. - Add a file:// URL variant to the transport-block test alongside the bare-path form (CVE-2022-39253 classification differs by git version). --------- Co-authored-by: Kandev Agent --- apps/backend/internal/worktree/submodule.go | 60 +++++- .../worktree/submodule_hardening_test.go | 184 ++++++++++++++++++ 2 files changed, 242 insertions(+), 2 deletions(-) create mode 100644 apps/backend/internal/worktree/submodule_hardening_test.go diff --git a/apps/backend/internal/worktree/submodule.go b/apps/backend/internal/worktree/submodule.go index 3dae88b85c..d80237227e 100644 --- a/apps/backend/internal/worktree/submodule.go +++ b/apps/backend/internal/worktree/submodule.go @@ -2,12 +2,69 @@ package worktree import ( "context" + "os" "os/exec" "strings" "go.uber.org/zap" ) +// hardenedSubmoduleConfig are the `-c` git config overrides applied when +// initializing submodules. Submodule init parses .gitmodules from the +// worktree's HEAD, which for a fork-PR review task is fully +// attacker-controlled. A bare `git submodule update --init --recursive` +// would let that .gitmodules pick the transport git uses to fetch each +// submodule — including command-executing transports (`ext::sh -c …`, +// CVE-2018-17456) and local `file://` paths. These overrides lock the +// transport down before any attacker content is read: +// +// - protocol.allow=never default-deny every transport… +// - protocol.https.allow=always …then re-enable only https… +// - protocol.ssh.allow=always …and ssh (the transports real +// submodules use). +// - protocol.ext.allow=never hard-pin the command-executing +// transport off. Passed on the command +// line it outranks any ambient/global +// `protocol.ext.allow=always`, so it can +// never be re-enabled out from under us. +// +// file:// and git:// fall through to protocol.allow=never and are denied. +// Because these are command-line `-c` overrides, they outrank repo/global +// config files — so a host with `protocol.file.allow=always` in .git/config +// or ~/.gitconfig will NOT re-enable file submodules here. That is +// intentional: the worktree content is untrusted, so we do not want ambient +// config to widen the transport surface. If a Kandev deployment ever needs +// local-path submodules for trusted repos, that must be a deliberate +// per-repo opt-in that appends `-c protocol.file.allow=always`, not an +// implicit reliance on host git config. +var hardenedSubmoduleConfig = []string{ + "protocol.allow=never", + "protocol.https.allow=always", + "protocol.ssh.allow=always", + "protocol.ext.allow=never", + // Neutralize attacker-planted hooks (CVE-2018-11235 / CVE-2024-32002 + // class): point core.hooksPath at an empty location so no hook in the + // submodule's .git/modules tree can execute during checkout. + "core.hooksPath=" + os.DevNull, +} + +// newSubmoduleUpdateCmd builds the hardened `git submodule update --init +// --recursive` command for dir. It reuses newNonInteractiveGitCmd so the +// submodule fetch inherits the full non-interactive env set (GIT_SSH_COMMAND +// with BatchMode, SSH/GCM askpass suppression, GIT_TERMINAL_PROMPT=0) and +// WaitDelay — critical because an attacker `ssh://…` submodule URL would +// otherwise hang the review runner on host-key/passphrase prompts until the +// context deadline. Kept separate from initSubmodules so tests can assert the +// hardening flags/env are present at the sink. +func (m *Manager) newSubmoduleUpdateCmd(ctx context.Context, dir string) *exec.Cmd { + args := make([]string, 0, len(hardenedSubmoduleConfig)*2+4) + for _, c := range hardenedSubmoduleConfig { + args = append(args, "-c", c) + } + args = append(args, "submodule", "update", "--init", "--recursive") + return m.newNonInteractiveGitCmd(ctx, dir, args...) +} + // getSubmodulePaths returns the paths of all submodules registered in HEAD. // It reads from the git object store (git ls-tree), so it works in --no-checkout worktrees. // Returns nil (not an error) if there are no submodules. @@ -39,8 +96,7 @@ func getSubmodulePaths(ctx context.Context, dir string) ([]string, error) { // Failures are non-fatal: submodule URLs may be unreachable (private repos, // missing credentials), but the worktree is still usable for non-submodule files. func (m *Manager) initSubmodules(ctx context.Context, dir string) { - cmd := exec.CommandContext(ctx, "git", "submodule", "update", "--init", "--recursive") - cmd.Dir = dir + cmd := m.newSubmoduleUpdateCmd(ctx, dir) output, err := runGitCmdCombinedOutput(ctx, cmd) if err != nil { m.logger.Warn("git submodule update --init failed (non-fatal)", diff --git a/apps/backend/internal/worktree/submodule_hardening_test.go b/apps/backend/internal/worktree/submodule_hardening_test.go new file mode 100644 index 0000000000..4a5a332b6d --- /dev/null +++ b/apps/backend/internal/worktree/submodule_hardening_test.go @@ -0,0 +1,184 @@ +package worktree + +import ( + "context" + "fmt" + "net" + "net/http" + "os" + "path/filepath" + "strings" + "sync/atomic" + "testing" + "time" +) + +// buildMaliciousRepo writes an attacker-controlled .gitmodules + gitlink, +// commits it, then creates a checked-out worktree pointing at that commit — +// exactly mirroring production's `git worktree add ` +// during a fork-PR review task. The returned worktree path is what +// production passes to initSubmodules, so tests can drive the real sink. +func buildMaliciousRepo(t *testing.T, submoduleURL, submodulePath string) (worktreePath string) { + t.Helper() + + repo := t.TempDir() + runGit(t, repo, "init", "-b", "main") + runGit(t, repo, "config", "user.email", "attacker@evil.example") + runGit(t, repo, "config", "user.name", "Attacker") + runGit(t, repo, "config", "commit.gpgsign", "false") + + gitmodules := fmt.Sprintf("[submodule \"evil\"]\n\tpath = %s\n\turl = %s\n", submodulePath, submoduleURL) + if err := os.WriteFile(filepath.Join(repo, ".gitmodules"), []byte(gitmodules), 0644); err != nil { + t.Fatalf("write .gitmodules: %v", err) + } + runGit(t, repo, "add", ".gitmodules") + + // Register a gitlink (mode 160000) at the submodule path so + // `git submodule update --init` treats it as a submodule to fetch. The + // commit hash need not exist remotely; the clone (the sink) is attempted + // before the checkout of the gitlink commit. + fakeCommit := "0000000000000000000000000000000000000001" + runGit(t, repo, "update-index", "--add", "--cacheinfo", + fmt.Sprintf("160000,%s,%s", fakeCommit, submodulePath)) + runGit(t, repo, "commit", "-m", "totally normal PR, please review") + + wt := filepath.Join(t.TempDir(), "review-wt") + runGit(t, repo, "worktree", "add", wt, "-b", "pr-head", "main") + return wt +} + +// TestInitSubmodules_BlocksExtRCE asserts the ext:: command-executing +// transport can no longer execute, even when an ambient/global git config +// tries to re-enable it (the hardened command-line -c outranks it). +func TestInitSubmodules_BlocksExtRCE(t *testing.T) { + marker := filepath.Join(t.TempDir(), "pwned") + payload := "ext::sh -c touch% " + marker // CVE-2018-17456 arg encoding + + wt := buildMaliciousRepo(t, payload, "evil") + + // Simulate a host that would otherwise allow ext:: (older git, or an + // operator/global protocol.ext.allow=always). Pre-fix this would RCE. + t.Setenv("GIT_CONFIG_COUNT", "1") + t.Setenv("GIT_CONFIG_KEY_0", "protocol.ext.allow") + t.Setenv("GIT_CONFIG_VALUE_0", "always") + + m := &Manager{logger: newTestLogger()} + m.initSubmodules(context.Background(), wt) + + if _, err := os.Stat(marker); err == nil { + t.Fatalf("SECURITY REGRESSION: ext:: submodule executed attacker command (marker created)") + } +} + +// TestInitSubmodules_BlocksFileTransport asserts local-path submodules are +// denied by default (no explicit per-repo opt-in), closing the local-path / +// path-traversal surface for untrusted trees. Exercises both the bare-path +// form and the explicit file:// URL form — the latter is the shape an +// attacker's .gitmodules is most likely to use, and on some git versions the +// two take different transport-classification code paths (CVE-2022-39253). +func TestInitSubmodules_BlocksFileTransport(t *testing.T) { + // A real local repo to point at; the point is that the transport is + // refused, not that the target is missing. + target := t.TempDir() + runGit(t, target, "init", "-b", "main") + runGit(t, target, "config", "user.email", "a@b.c") + runGit(t, target, "config", "user.name", "a") + runGit(t, target, "config", "commit.gpgsign", "false") + if err := os.WriteFile(filepath.Join(target, "x.txt"), []byte("x\n"), 0644); err != nil { + t.Fatal(err) + } + runGit(t, target, "add", ".") + runGit(t, target, "commit", "-m", "init") + + for _, url := range []string{target, "file://" + target} { + wt := buildMaliciousRepo(t, url, "evil") + m := &Manager{logger: newTestLogger()} + m.initSubmodules(context.Background(), wt) + + if _, err := os.Stat(filepath.Join(wt, "evil", "x.txt")); err == nil { + t.Fatalf("SECURITY REGRESSION: local submodule %q initialized despite protocol.allow=never", url) + } + } +} + +// TestInitSubmodules_BlocksPlainHTTPFetch asserts that a plain http:// (not +// https) submodule URL — the classic cloud-metadata SSRF vector, e.g. +// http://169.254.169.254/… — is refused. Only https and ssh are allowlisted, +// so protocol.allow=never denies http. The pre-fix code fetched it (see the +// PoC report); this test locks the fix in. +// +// The remaining residual is an attacker-chosen *https* (or ssh) URL, which is +// the transport real submodules use and cannot be blanket-denied without also +// breaking legitimate submodules. Closing that for untrusted (fork-PR) +// worktrees requires gating submodule init off entirely — see the report. +func TestInitSubmodules_BlocksPlainHTTPFetch(t *testing.T) { + var hit int32 + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + srv := &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.StoreInt32(&hit, 1) + w.WriteHeader(http.StatusNotFound) + })} + defer func() { _ = srv.Close() }() + go func() { _ = srv.Serve(ln) }() + + url := fmt.Sprintf("http://%s/evil.git", ln.Addr().String()) + wt := buildMaliciousRepo(t, url, "evil") + + m := &Manager{logger: newTestLogger()} + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + m.initSubmodules(ctx, wt) + + time.Sleep(200 * time.Millisecond) + if atomic.LoadInt32(&hit) == 1 { + t.Fatalf("SECURITY REGRESSION: plain http:// submodule URL was fetched (SSRF); expected protocol.allow=never to block it") + } +} + +// TestNewSubmoduleUpdateCmd_HasHardeningFlags asserts the sink command +// carries every hardening flag and env, at the sink so all call sites +// benefit. This is the structural guard against silent regressions. +func TestNewSubmoduleUpdateCmd_HasHardeningFlags(t *testing.T) { + dir := filepath.Join(t.TempDir(), "wt") + m := &Manager{logger: newTestLogger()} + cmd := m.newSubmoduleUpdateCmd(context.Background(), dir) + joined := strings.Join(cmd.Args, " ") + + wantFlags := []string{ + "-c protocol.allow=never", + "-c protocol.https.allow=always", + "-c protocol.ssh.allow=always", + "-c protocol.ext.allow=never", + "-c core.hooksPath=" + os.DevNull, + "submodule update --init --recursive", + } + for _, want := range wantFlags { + if !strings.Contains(joined, want) { + t.Errorf("submodule command missing %q; got: %s", want, joined) + } + } + + // The full non-interactive env set must be present so an attacker ssh:// + // (or http) submodule URL cannot hang the runner on a prompt. + wantEnv := []string{ + "GIT_TERMINAL_PROMPT=0", + "GIT_SSH_COMMAND=ssh -oBatchMode=yes", + "SSH_ASKPASS=/bin/false", + "GCM_INTERACTIVE=Never", + } + haveEnv := make(map[string]bool, len(cmd.Env)) + for _, e := range cmd.Env { + haveEnv[e] = true + } + for _, want := range wantEnv { + if !haveEnv[want] { + t.Errorf("submodule command env missing %q; got: %v", want, cmd.Env) + } + } + if cmd.Dir != dir { + t.Errorf("cmd.Dir = %q, want %q", cmd.Dir, dir) + } +}