Skip to content

Commit 1c8a91c

Browse files
kovtcharov-amdOvtcharov
andauthored
fix(security): remove pre-existing bandit HIGH findings and enable the HIGH gate (#2350)
Bandit ran warning-only, so 18 pre-existing HIGH-severity findings sat unblocked in core paths (CLI, subprocess helpers, MCP stdio transport, Lemonade launch). Before: a new `shell=True` injection site or weak-hash regression would pass lint and ship. After: all 18 are fixed (16 by removing the risk, 2 kept as justified suppressions) and any **new** HIGH finding now fails `util/lint.py --bandit`. ### How each finding was handled **Fixed by removing the risk (16):** `shell=True` string commands converted to `shell=False` args-lists. - `util.py` / `cli.py` port-kill helpers: run `netstat`/`lsof` bare and filter output in Python instead of piping through the shell; `taskkill`/`kill` take args-lists. Port inputs are `int()`-validated first. - `lemonade_client.py`: `os.system(taskkill …)` → `subprocess.run([...], shell=False)` (B605). - `logger.py`: `chcp` (a cmd.exe builtin) → `cmd /c chcp`. - `ui/build.py`: `npm` calls (`.cmd` shims) → `cmd /c npm …` on Windows, bare on other platforms. - `cli.py` mailto opener: `start "" <url>` → `os.startfile()` (ShellExecute, no shell parsing) — this one mattered because the URL is user-built. - `context7_cache.py`: MD5 is a cache key, fixed with `usedforsecurity=False` (B324). **Kept as justified suppressions (2)** — inline `# nosec B602` + a matching entry in `.security-suppressions.json`: - `agents/tools/shell_tools.py` — sandboxed shell executor; every command (and pipeline segment) is whitelist-validated before running, and `shell=True` is Windows-only so cmd.exe can resolve built-ins/pipes the tool exists to run. - `mcp/client/transports/stdio.py` — legacy `from_command()` accepts a full shell command **string** (documented contract), needs shell parsing; command is trusted SDK config, and modern `from_config()` already runs `shell=False`. ### The gate `util/check_security_gates.py` adds `new_bandit_highs(results, baseline)`, keyed by `(normalized path, test_id)` — never line number, so cosmetic line shifts don't trip it. Wired into `util/lint.py`'s `check_bandit()`; `.bandit-baseline.json` ships as an empty allowlist so **any** HIGH fails. ### Test plan - [x] `python -m bandit -r src/gaia -ll -f json` → 0 HIGH findings - [x] `python util/lint.py --bandit` → PASS - [x] Negative check: adding a `subprocess.run(f"echo {x}", shell=True)` probe makes the gate exit 1; passes again once removed - [x] `pytest tests/unit/test_check_security_gates.py` → 18 passed - [x] `pytest tests/unit/test_webui_build.py tests/unit/mcp/client/ tests/unit/test_shell_guardrails.py` → pass - [x] Positive-path functional test: started a real listener, confirmed the converted `netstat` detection + `taskkill` args-list finds and kills it and frees the port - [x] `black`/`isort` clean on all changed files --------- Co-authored-by: Ovtcharov <kovtchar@amd.com>
1 parent 4b5c16b commit 1c8a91c

14 files changed

Lines changed: 493 additions & 52 deletions

.bandit-baseline.json

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{
2+
"_comment": "Allowlist of accepted bandit HIGH-severity findings, keyed by (path, test_id) — line numbers are intentionally NOT part of the key. Empty means ANY HIGH finding fails the gate. All 18 pre-existing HIGH findings were fixed or converted to justified inline `# nosec` (see .security-suppressions.json); keep this empty unless a genuinely unavoidable HIGH is reviewed and accepted.",
3+
"findings": []
4+
}

.security-suppressions.json

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
{
2+
"_comment": "Justified security suppressions. Every inline `# nosec S<n>` / `# nosec B<n>` marker in src/gaia MUST have a matching entry here with a real justification. Do not blanket-suppress to quiet the scanner.",
3+
"suppressions": [
4+
{
5+
"path": "src/gaia/agents/tools/shell_tools.py",
6+
"rule": "B602",
7+
"justification": "Sandboxed shell tool. Every command (and each pipeline segment) is validated against a whitelist via _validate_command before execution; shell=True is enabled ONLY on Windows so cmd.exe can resolve built-ins (dir/cd/type) and pipes that Git-for-Windows tools rely on. Converting to args-list would break piped/whitelisted commands the tool exists to run."
8+
},
9+
{
10+
"path": "src/gaia/mcp/client/transports/stdio.py",
11+
"rule": "B602",
12+
"justification": "Legacy from_command() API accepts a full shell command STRING (documented contract, e.g. commands using pipes/env-expansion) and needs shell parsing. Command is caller-supplied SDK config, not external/untrusted input; modern from_config() passes an args list and runs shell=False."
13+
}
14+
]
15+
}

src/gaia/agents/tools/shell_tools.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -644,7 +644,7 @@ def run_shell_command(
644644
timeout=timeout,
645645
check=False,
646646
env=os.environ.copy(),
647-
shell=use_shell,
647+
shell=use_shell, # nosec B602 - Windows-only; command whitelist-validated above, shell needed for cmd.exe built-ins/pipes
648648
)
649649
duration = time.monotonic() - start_time
650650

src/gaia/cli.py

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3653,9 +3653,11 @@ def main():
36533653
system = platform.system()
36543654
try:
36553655
if system == "Windows":
3656-
subprocess.run(
3657-
["start", "", mailto_url], shell=True, check=True
3658-
)
3656+
# os.startfile uses ShellExecute (no shell parsing),
3657+
# safe for the user-built mailto URL (which contains
3658+
# '&'-separated query params cmd would mis-parse).
3659+
# Windows-only attr; guarded by the platform check.
3660+
os.startfile(mailto_url) # pylint: disable=no-member
36593661
elif system == "Darwin": # macOS
36603662
subprocess.run(["open", mailto_url], check=True)
36613663
else: # Linux/Unix
@@ -4776,11 +4778,14 @@ def main():
47764778

47774779
def kill_process_by_port(port):
47784780
"""Find and kill a process running on a specific port."""
4781+
try:
4782+
port = int(port)
4783+
except (ValueError, TypeError):
4784+
return {"success": False, "message": f"Invalid port number: {port!r}"}
47794785
try:
47804786
if sys.platform.startswith("win"):
4781-
# Windows implementation
4782-
cmd = f"netstat -ano | findstr :{port}"
4783-
output = subprocess.check_output(cmd, shell=True).decode()
4787+
# Windows implementation (filter netstat output in Python, no shell pipe)
4788+
output = subprocess.check_output(["netstat", "-ano"]).decode()
47844789
if output:
47854790
# Split output into lines and process each line
47864791
for line in output.strip().split("\n"):
@@ -4792,7 +4797,9 @@ def kill_process_by_port(port):
47924797
pid = int(parts[-1])
47934798
if pid > 0: # Ensure we don't try to kill PID 0
47944799
subprocess.run(
4795-
f"taskkill /PID {pid} /F", shell=True, check=True
4800+
["taskkill", "/PID", str(pid), "/F"],
4801+
shell=False,
4802+
check=True,
47964803
)
47974804
return {
47984805
"success": True,
@@ -4808,16 +4815,19 @@ def kill_process_by_port(port):
48084815
# Linux/Unix implementation
48094816
try:
48104817
# Use lsof to find process using the port
4811-
cmd = f"lsof -ti:{port}"
4812-
output = subprocess.check_output(cmd, shell=True).decode().strip()
4818+
output = (
4819+
subprocess.check_output(["lsof", f"-ti:{port}"]).decode().strip()
4820+
)
48134821
if output:
48144822
pids = output.split("\n")
48154823
killed_pids = []
48164824
for pid_str in pids:
48174825
try:
48184826
pid = int(pid_str.strip())
48194827
if pid > 0:
4820-
subprocess.run(f"kill -9 {pid}", shell=True, check=True)
4828+
subprocess.run(
4829+
["kill", "-9", str(pid)], shell=False, check=True
4830+
)
48214831
killed_pids.append(str(pid))
48224832
except (ValueError, subprocess.CalledProcessError):
48234833
continue
@@ -4834,8 +4844,8 @@ def kill_process_by_port(port):
48344844
# If lsof is not available, try netstat + ps approach
48354845
try:
48364846
# Use netstat to find the port, then extract PID
4837-
cmd = f"netstat -tulpn | grep :{port}"
4838-
output = subprocess.check_output(cmd, shell=True).decode()
4847+
# (filter output in Python, no shell pipe)
4848+
output = subprocess.check_output(["netstat", "-tulpn"]).decode()
48394849
if output:
48404850
for line in output.strip().split("\n"):
48414851
if f":{port}" in line:
@@ -4847,8 +4857,8 @@ def kill_process_by_port(port):
48474857
pid = int(part.split("/")[0])
48484858
if pid > 0:
48494859
subprocess.run(
4850-
f"kill -9 {pid}",
4851-
shell=True,
4860+
["kill", "-9", str(pid)],
4861+
shell=False,
48524862
check=True,
48534863
)
48544864
return {

src/gaia/llm/lemonade_client.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1109,7 +1109,17 @@ def terminate_server(self):
11091109
# For subprocess.Popen
11101110
if sys.platform.startswith("win") and self.server_process.pid:
11111111
# On Windows, use taskkill to ensure process tree is terminated
1112-
os.system(f"taskkill /F /PID {self.server_process.pid} /T")
1112+
subprocess.run(
1113+
[
1114+
"taskkill",
1115+
"/F",
1116+
"/PID",
1117+
str(self.server_process.pid),
1118+
"/T",
1119+
],
1120+
shell=False,
1121+
check=False,
1122+
)
11131123
elif self.server_process.pid:
11141124
# On Linux/Unix, kill the process group to terminate child processes
11151125
try:

src/gaia/logger.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,12 @@ def configure_console_encoding():
2323

2424
# Also try to set the console code page to UTF-8
2525
try:
26+
# chcp is a cmd.exe builtin; invoke via cmd /c so we avoid shell=True
2627
subprocess.run(
27-
["chcp", "65001"], capture_output=True, shell=True, check=False
28+
["cmd", "/c", "chcp", "65001"],
29+
capture_output=True,
30+
shell=False,
31+
check=False,
2832
)
2933
except (subprocess.SubprocessError, OSError, FileNotFoundError):
3034
pass # Ignore if chcp command fails

src/gaia/mcp/client/transports/stdio.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,9 +80,13 @@ def connect(self) -> bool:
8080
merged_env = os.environ.copy()
8181
merged_env.update(self.env)
8282

83+
# Legacy from_command() path passes a full shell command string
84+
# (documented contract) and needs shell parsing; modern from_config()
85+
# passes an args list and runs shell=False. Command is caller-supplied
86+
# SDK config, not external/untrusted input.
8387
self._process = subprocess.Popen(
8488
cmd,
85-
shell=use_shell,
89+
shell=use_shell, # nosec B602 - legacy shell-string API, trusted SDK config
8690
stdin=subprocess.PIPE,
8791
stdout=subprocess.PIPE,
8892
stderr=subprocess.PIPE,

src/gaia/mcp/context7_cache.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ def _doc_cache_file(self, library: str, query: str) -> Path:
114114
Path to cache file
115115
"""
116116
key = f"{library}:{query}"
117-
hash_key = hashlib.md5(key.encode()).hexdigest()[:12]
117+
hash_key = hashlib.md5(key.encode(), usedforsecurity=False).hexdigest()[:12]
118118
safe_lib = library.replace("/", "_").replace(".", "_")
119119
return self.docs_dir / f"{safe_lib}_{hash_key}.json"
120120

src/gaia/ui/build.py

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -79,20 +79,24 @@ def ensure_webui_built(log_fn=print, warn_fn=None, _webui_dir=None):
7979
warn_fn("Warning: npm not found. Cannot auto-rebuild Agent UI frontend.")
8080
return False
8181

82-
# On Windows, npm is a .cmd batch file requiring shell execution
83-
_shell = sys.platform == "win32"
82+
# On Windows npm is a .cmd batch file, which CreateProcess can't launch
83+
# directly; invoke via `cmd /c` (args are static) so we avoid shell=True.
84+
def _npm(*args):
85+
if sys.platform == "win32":
86+
return ["cmd", "/c", "npm", *args]
87+
return ["npm", *args]
8488

8589
# Step 1 — npm install (only if node_modules/ missing)
8690
if not (webui_dir / "node_modules").is_dir():
8791
log_fn("Installing Agent UI frontend dependencies...")
8892
try:
8993
subprocess.run(
90-
["npm", "install"],
94+
_npm("install"),
9195
cwd=str(webui_dir),
9296
check=True,
9397
capture_output=True,
9498
text=True,
95-
shell=_shell,
99+
shell=False,
96100
)
97101
except subprocess.CalledProcessError as e:
98102
warn_fn(f"Warning: npm install failed: {e.stderr}")
@@ -106,10 +110,10 @@ def ensure_webui_built(log_fn=print, warn_fn=None, _webui_dir=None):
106110
log_fn("Building Agent UI frontend...")
107111
try:
108112
subprocess.run(
109-
["npm", "run", "build"],
113+
_npm("run", "build"),
110114
cwd=str(webui_dir),
111115
check=True,
112-
shell=_shell,
116+
shell=False,
113117
)
114118
log_fn("Agent UI frontend built successfully.")
115119
return True

src/gaia/util.py

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,10 @@ def kill_process_on_port(port):
1515
raise ValueError(f"Invalid port number: {port!r}")
1616
try:
1717
if sys.platform.startswith("win"):
18-
# Windows: use netstat + taskkill
18+
# Windows: use netstat + taskkill (filter output in Python, no shell pipe)
1919
result = subprocess.run(
20-
f"netstat -ano | findstr :{port}",
21-
shell=True,
20+
["netstat", "-ano"],
21+
shell=False,
2222
capture_output=True,
2323
text=True,
2424
check=False,
@@ -39,7 +39,7 @@ def kill_process_on_port(port):
3939
print(f"Found process with PID {pid} on port {port}")
4040
try:
4141
subprocess.run(
42-
f"taskkill /F /PID {pid}", shell=True, check=False
42+
["taskkill", "/F", "/PID", pid], shell=False, check=False
4343
)
4444
print(f"Killed process with PID {pid}")
4545
except Exception as e:
@@ -50,8 +50,8 @@ def kill_process_on_port(port):
5050
else:
5151
# Unix/macOS: use lsof + kill
5252
result = subprocess.run(
53-
f"lsof -ti :{port}",
54-
shell=True,
53+
["lsof", "-ti", f":{port}"],
54+
shell=False,
5555
capture_output=True,
5656
text=True,
5757
check=False,
@@ -64,7 +64,9 @@ def kill_process_on_port(port):
6464
if pid:
6565
print(f"Found process with PID {pid} on port {port}")
6666
try:
67-
subprocess.run(f"kill -9 {pid}", shell=True, check=False)
67+
subprocess.run(
68+
["kill", "-9", pid], shell=False, check=False
69+
)
6870
print(f"Killed process with PID {pid}")
6971
except Exception as e:
7072
print(f"Error killing PID {pid}: {e}")

0 commit comments

Comments
 (0)