security: fix command injection vulnerabilities (CVSS 9.8)
Replace shell=True with list-based subprocess execution to prevent command injection via malicious user input. Changes: - tools/transcription_tools.py: Use shlex.split() + shell=False - tools/environments/docker.py: List-based commands with container ID validation Fixes CVE-level vulnerability where malicious file paths or container IDs could inject arbitrary commands. CVSS: 9.8 (Critical) Refs: V-001 in SECURITY_AUDIT_REPORT.md
This commit is contained in:
@@ -509,22 +509,48 @@ class DockerEnvironment(BaseEnvironment):
|
||||
"""Stop and remove the container. Bind-mount dirs persist if persistent=True."""
|
||||
if self._container_id:
|
||||
try:
|
||||
# SECURITY FIX: Use list-based commands instead of shell=True
|
||||
# to prevent command injection via malicious container IDs
|
||||
# Stop in background so cleanup doesn't block
|
||||
stop_cmd = (
|
||||
f"(timeout 60 {self._docker_exe} stop {self._container_id} || "
|
||||
f"{self._docker_exe} rm -f {self._container_id}) >/dev/null 2>&1 &"
|
||||
container_id = self._container_id
|
||||
# Validate container ID format to prevent injection
|
||||
if not re.match(r'^[a-f0-9]{12,64}$', container_id):
|
||||
logger.warning("Invalid container ID format: %s", container_id)
|
||||
return
|
||||
|
||||
# Use subprocess with list args instead of shell=True
|
||||
subprocess.Popen(
|
||||
["timeout", "60", self._docker_exe, "stop", container_id],
|
||||
stdout=subprocess.DEVNULL,
|
||||
stderr=subprocess.DEVNULL,
|
||||
)
|
||||
subprocess.Popen(stop_cmd, shell=True)
|
||||
except Exception as e:
|
||||
logger.warning("Failed to stop container %s: %s", self._container_id, e)
|
||||
|
||||
if not self._persistent:
|
||||
# Also schedule removal (stop only leaves it as stopped)
|
||||
try:
|
||||
subprocess.Popen(
|
||||
f"sleep 3 && {self._docker_exe} rm -f {self._container_id} >/dev/null 2>&1 &",
|
||||
shell=True,
|
||||
# Use a delayed removal via threading instead of shell
|
||||
def delayed_remove(docker_exe, container_id, delay=3):
|
||||
import time
|
||||
time.sleep(delay)
|
||||
try:
|
||||
subprocess.run(
|
||||
[docker_exe, "rm", "-f", container_id],
|
||||
stdout=subprocess.DEVNULL,
|
||||
stderr=subprocess.DEVNULL,
|
||||
check=False,
|
||||
)
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
import threading
|
||||
remove_thread = threading.Thread(
|
||||
target=delayed_remove,
|
||||
args=(self._docker_exe, self._container_id, 3),
|
||||
daemon=True,
|
||||
)
|
||||
remove_thread.start()
|
||||
except Exception:
|
||||
pass
|
||||
self._container_id = None
|
||||
|
||||
@@ -343,13 +343,17 @@ def _transcribe_local_command(file_path: str, model_name: str) -> Dict[str, Any]
|
||||
if prep_error:
|
||||
return {"success": False, "transcript": "", "error": prep_error}
|
||||
|
||||
# SECURITY FIX: Use list-based command execution instead of shell=True
|
||||
# to prevent command injection via malicious file paths or parameters
|
||||
command = command_template.format(
|
||||
input_path=shlex.quote(prepared_input),
|
||||
output_dir=shlex.quote(output_dir),
|
||||
language=shlex.quote(language),
|
||||
model=shlex.quote(normalized_model),
|
||||
input_path=prepared_input, # shlex.quote not needed with list execution
|
||||
output_dir=output_dir,
|
||||
language=language,
|
||||
model=normalized_model,
|
||||
)
|
||||
subprocess.run(command, shell=True, check=True, capture_output=True, text=True)
|
||||
# Parse the command string into a list safely
|
||||
command_parts = shlex.split(command)
|
||||
subprocess.run(command_parts, shell=False, check=True, capture_output=True, text=True)
|
||||
|
||||
txt_files = sorted(Path(output_dir).glob("*.txt"))
|
||||
if not txt_files:
|
||||
|
||||
Reference in New Issue
Block a user