fix(server): block additional git code-execution vectors in DockerSandboxAdapter - #916
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSecurity hardening of Git configuration injection in Docker sandbox environments. Expanded environment variable blocklists for Git-related code-execution vectors and reworked the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
application-server
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| Map<String, String> env = captor.getValue().environment(); | ||
|
|
||
| // No volume mounts → COUNT equals security config count exactly | ||
| int count = Integer.parseInt(env.get("GIT_CONFIG_COUNT")); |
There was a problem hiding this comment.
Pull request overview
Hardens the Docker-based sandbox execution environment against additional Git configuration and environment-variable code execution vectors by enforcing a broader set of safe Git settings and ensuring they’re applied consistently even when repositories are cloned at runtime.
Changes:
- Introduces a centralized
GIT_SECURITY_CONFIGSlist and injects it viaGIT_CONFIG_*env vars (highest precedence) to neutralize multiple.git/configexecution vectors. - Fixes a gating issue so Git security configs are injected regardless of whether
volumeMountsare present (while still addingsafe.directoryonly for mounts). - Extends environment variable blocklists to include Git execution-related vars and the
GIT_CONFIG_prefix; addsGIT_TERMINAL_PROMPT=0andGIT_ATTR_NOSYSTEM=1.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/application-server/src/main/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxAdapter.java | Always injects hardened Git config env overrides; blocks additional Git env-based execution vectors; adds non-interactive/hardening Git env vars. |
| server/application-server/src/test/java/de/tum/in/www1/hephaestus/agent/sandbox/docker/DockerSandboxAdapterTest.java | Adds unit tests validating Git security config injection behavior and new blocklist/prefix-block behavior. |
| @Test | ||
| @DisplayName("should overwrite security env vars even if caller bypasses blocklist") | ||
| void shouldOverwriteSecurityEnvVarsViaOrdering() { | ||
| setupHappyPath(); | ||
|
|
||
| // GIT_TERMINAL_PROMPT and GIT_ATTR_NOSYSTEM are in BLOCKED_ENV_VARS, so a caller | ||
| // can't inject them. This test verifies the defense-in-depth: even if they somehow | ||
| // leaked through, the security injection at the end of buildEnvironment() wins. | ||
| // We test this by verifying the final values are always the security-hardened ones. | ||
| sandboxAdapter.execute(createSpec()); |
There was a problem hiding this comment.
This test’s name/description claims to verify that security env vars “overwrite … even if caller bypasses blocklist”, but the test never injects conflicting caller-provided values (and the relevant keys are blocked anyway). As written it duplicates coverage from the other git-security tests and doesn’t actually validate the stated defense-in-depth behavior. Consider either (a) renaming/rewording the test to match what it asserts, or (b) reworking it to demonstrate an actual overwrite/collision scenario (e.g., for a non-blocked key that is intentionally overridden later in buildEnvironment).
…dboxAdapter Closes #904 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
576ea04 to
6ebc423
Compare
📚 Documentation Preview
|
|
🎉 This PR is included in version 0.51.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
Closes #904.
Found during PE audit of PR #892:
DockerSandboxAdapterblockscore.hooksPathandcore.fsmonitorviaGIT_CONFIG_COUNT, but several additional code-execution vectors were unblocked — a malicious.git/configin a cloned repo could execute arbitrary commands when the agent runsgit diff,git commit, or uses SSH/credential prompts.This PR hardens the git security surface across three independent defense layers, and fixes a bug where git security configs were only injected when volume mounts were present (missing protection for repos cloned at runtime).
Changes
1. Extracted
GIT_SECURITY_CONFIGSconstant (11 git config keys)Declarative, auditable list of git config key-value pairs injected via
GIT_CONFIG_COUNT/GIT_CONFIG_KEY_*/GIT_CONFIG_VALUE_*env vars (highest precedence in git — overrides.git/config):core.hooksPath/nonexistentcore.fsmonitorfalsecore.sshCommand""core.askPass""core.editor""git commit,git rebase -icore.pagercatgit diff,git log)core.gitProxy""git://protocolsequence.editor""credential.helper""diff.external""git diffprotocol.ext.allowneverext::transport protocol (CVE-2022-25912)2. Fixed condition gate bug (security gap)
Previously, git security configs were only injected when
spec.volumeMounts()was non-empty. If the agent rangit cloneat runtime (no pre-mounted repos), none of the git security configs were active. Now:safe.directoryentries: only when volume mounts are present (path-specific)3. Blocked git-related env vars from callers
Added to
BLOCKED_ENV_VARS(exact match, case-insensitive):GIT_SSH,GIT_SSH_COMMAND,GIT_ASKPASS,GIT_EDITOR,GIT_EXEC_PATH,GIT_TEMPLATE_DIR— direct command executionGIT_EXTERNAL_DIFF,GIT_PROXY_COMMAND,GIT_SEQUENCE_EDITOR,GIT_PAGER— override config equivalents independentlyGIT_TERMINAL_PROMPT,GIT_ATTR_NOSYSTEM— prevent callers from overriding hardening varsAdded
GIT_CONFIG_toBLOCKED_ENV_PREFIXESto prevent callers from injectingGIT_CONFIG_COUNT/GIT_CONFIG_KEY_*/GIT_CONFIG_VALUE_*that could override our security settings.4. Additional hardening env vars
GIT_TERMINAL_PROMPT=0— prevents git from prompting interactively (would hang the agent)GIT_ATTR_NOSYSTEM=1— prevents system-wide gitattributes from being loadedKnown Limitations (tracked follow-ups)
filter.*.clean/filter.*.smudge/filter.*.process— wildcard-keyed configs that cannot be enumerated or blocked viaGIT_CONFIG_COUNT. A malicious repo can ship.gitattributes+.git/configwithfilter.evil.clean=/exploit. Requires.git/configsanitization post-clone.merge.*.driver/diff.*.textconv— same wildcard limitation; lower priority since they require explicit merge/diff tool invocation.How to Test
Unit tests (75 tests in DockerSandboxAdapterTest):
Architecture tests (9 tests):
All unit tests (2048 tests):
Summary by CodeRabbit