From 0f10fb43fd810e35ca085328a52a3c3154136619 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 29 Sep 2026 01:33:39 +0900 Subject: [PATCH] fix(sandbox): pass shell=False explicitly in bubblewrap capability probe Minimal successor to closed #2129, whose branch carried an unrelated 123-file deletion. Only the explicit shell=False argument, its mock assertions, and the sentinel learning are kept. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hc6PJfasfUdUFzngfdMpWJ --- .jules/sentinel.md | 4 ++++ scripts/ci/sandboxed_web_e2e.py | 1 + tests/test_sandboxed_web_e2e.py | 6 ++++++ 3 files changed, 11 insertions(+) diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 2da382f934..541038934c 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -51,3 +51,7 @@ **Vulnerability:** Denial of Service / Availability **Learning:** Strix security scanners crashed when the backend LLM returned an 'HTTP Error 502: Bad Gateway' response. This was because 'bad gateway' string match and generic 'APIError' were missing from the `is_llm_api_connection_error` function in the Strix retry gate. **Prevention:** Always include `bad gateway` and `APIError` in string match conditions when handling HTTP API Connection exceptions for LLM backends to ensure proper fail-closed and retry handling. +## 2026-09-12 - Prevent Command Injection via Explicit shell=False in Subprocess +**Vulnerability:** Command Injection hardening (implicit shell default; defense in depth) +**Learning:** Functions executing system commands, like `_probe_isolation_capability` using `subprocess.run`, implicitly default to `shell=False`. However, not explicitly declaring it allows security linters (like Bandit) to report false positives, and obscures the security posture against command injection if untrusted inputs were to reach the execution arguments. +**Prevention:** Always explicitly define `shell=False` in `subprocess.run()` and `subprocess.Popen()` calls, even when it is the default behavior. Ensure corresponding unit tests explicitly verify this configuration by asserting `kwargs.get("shell") is False` in mock implementations. diff --git a/scripts/ci/sandboxed_web_e2e.py b/scripts/ci/sandboxed_web_e2e.py index b0376c0822..9bca4a1522 100644 --- a/scripts/ci/sandboxed_web_e2e.py +++ b/scripts/ci/sandboxed_web_e2e.py @@ -240,6 +240,7 @@ def _probe_isolation_capability(backend: str) -> None: text=True, timeout=10, check=False, + shell=False, ) except (OSError, subprocess.TimeoutExpired) as exc: raise RuntimeError(f"bubblewrap capability probe could not run: {exc}") from exc diff --git a/tests/test_sandboxed_web_e2e.py b/tests/test_sandboxed_web_e2e.py index 1b1cdf3722..68e773c7db 100644 --- a/tests/test_sandboxed_web_e2e.py +++ b/tests/test_sandboxed_web_e2e.py @@ -1396,12 +1396,15 @@ def test_probe_isolation_capability_exercises_the_same_operations_as_real_comman def _fake_run(command, **kwargs): captured["command"] = command + captured["kwargs"] = kwargs return subprocess.CompletedProcess(command, 0, stdout="", stderr="") monkeypatch.setattr(sandboxed_web_e2e.subprocess, "run", _fake_run) sandboxed_web_e2e._probe_isolation_capability("/usr/bin/bwrap") command = captured["command"] + kwargs = captured["kwargs"] + assert kwargs.get("shell") is False assert "--new-session" in command assert command.count("--tmpfs") == 2 assert "/tmp" in command @@ -1434,12 +1437,15 @@ def test_probe_isolation_capability_ignores_path_shadowed_shell(monkeypatch, tmp def _fake_run(command, **kwargs): captured["command"] = command + captured["kwargs"] = kwargs return subprocess.CompletedProcess(command, 0, stdout="", stderr="") monkeypatch.setattr(sandboxed_web_e2e.subprocess, "run", _fake_run) sandboxed_web_e2e._probe_isolation_capability("/usr/bin/bwrap") command = captured["command"] + kwargs = captured["kwargs"] + assert kwargs.get("shell") is False probe_executable = command[-3] assert probe_executable in sandboxed_web_e2e.PROBE_SHELL_PATHS assert probe_executable != str(shadow_sh)