🛡️ Sentinel: [CRITICAL] Fix DoS vulnerability in subprocess calls

.jules/sentinel.md

@@ -41,3 +41,8 @@
 **Vulnerability:** Argument Injection via relative paths starting with a hyphen in command-line utilities (CWE-88).
 **Learning:** Even when `ffmpeg` inputs are protected by `-i`, command-line utilities (like `ffprobe` and `ffmpeg` filters) can interpret user input (like a file path) starting with a hyphen (e.g., `-version.wav`) as options if passed as a relative path.
 **Prevention:** File paths must be converted to absolute paths using `.resolve()` before they are passed to `subprocess.run`. This prefixes the path with a root, drive, or UNC prefix rather than a leading hyphen, thereby averting the possibility of argument injection.
+
+## 2025-01-20 - Unbounded Subprocess Calls Cause Denial of Service (DoS)
+**Vulnerability:** External binaries (`ffmpeg`, `ffprobe`, `brctl`) were executed using `subprocess.run` without an explicit `timeout`.
+**Learning:** FastAPI/Python will wait indefinitely for these subprocesses. If an attacker supplies a malicious file that causes `ffmpeg` or `ffprobe` to hang, or if the external tools simply freeze, the server threads become blocked permanently, exhausting server resources and causing a Denial of Service.
+**Prevention:** Always implement an explicit `timeout` parameter and handle `subprocess.TimeoutExpired` exceptions when invoking `subprocess.run`. Tailor the timeout to the expected duration of the binary (e.g., 60s for probes, 3600s for conversions) and ensure resource limits are enforced. Use `# nosec B603` to silence false positive static analysis warnings on secure arrays of arguments.

media_shrinker.py

@@ -465,7 +465,11 @@ def probe_media(
         "-i",
         str(Path(source_path).resolve()),
     ]
-    completed = subprocess.run(command, check=False, capture_output=True, text=True)
+    try:
+        completed = subprocess.run(command, check=False, capture_output=True, text=True, timeout=60)  # nosec B603
+    except subprocess.TimeoutExpired as exc:  # pragma: no cover
+        raise MediaShrinkerError(f"ffprobe timed out for {source_path} after 60s") from exc
+
     if completed.returncode != 0:
         raise MediaShrinkerError(
             f"ffprobe failed for {source_path}: {completed.stderr.strip()}"
@@ -518,17 +522,22 @@ def detect_silence_intervals(
 ) -> list[SilenceInterval]:
     """Run ffmpeg silencedetect and return paired silence intervals."""
 
-    completed = subprocess.run(
-        build_silencedetect_command(
-            source_path,
-            ffmpeg_path=ffmpeg_path,
-            silence_noise=silence_noise,
-            silence_min_duration_seconds=silence_min_duration_seconds,
-        ),
-        check=False,
-        capture_output=True,
-        text=True,
-    )
+    try:
+        completed = subprocess.run(
+            build_silencedetect_command(
+                source_path,
+                ffmpeg_path=ffmpeg_path,
+                silence_noise=silence_noise,
+                silence_min_duration_seconds=silence_min_duration_seconds,
+            ),
+            check=False,
+            capture_output=True,
+            text=True,
+            timeout=3600,
+        )  # nosec B603
+    except subprocess.TimeoutExpired as exc:  # pragma: no cover
+        raise MediaShrinkerError(f"silencedetect timed out for {source_path} after 3600s") from exc
+
     if completed.returncode != 0:
         raise MediaShrinkerError(
             f"silencedetect failed for {source_path}: {completed.stderr.strip()}"
@@ -643,12 +652,17 @@ def download_from_icloud(source_path: Path, *, brctl_path: str = "brctl") -> Non
         raise MediaShrinkerError(
             f"iCloud download requested but '{brctl_path}' was not found"
         )
-    completed = subprocess.run(
-        build_icloud_download_command(source_path, brctl_path=brctl_path),
-        check=False,
-        capture_output=True,
-        text=True,
-    )
+    try:
+        completed = subprocess.run(
+            build_icloud_download_command(source_path, brctl_path=brctl_path),
+            check=False,
+            capture_output=True,
+            text=True,
+            timeout=3600,
+        )  # nosec B603
+    except subprocess.TimeoutExpired as exc:  # pragma: no cover
+        raise MediaShrinkerError(f"iCloud download timed out for {source_path} after 3600s") from exc
+
     if completed.returncode != 0:
         raise MediaShrinkerError(
             f"iCloud download failed for {source_path}: {completed.stderr.strip()}"
@@ -1586,10 +1600,12 @@ def _execute_plan(
         )
         try:
             completed = subprocess.run(
-                command, check=False, capture_output=True, text=True
-            )
+                command, check=False, capture_output=True, text=True, timeout=3600
+            )  # nosec B603
         except FileNotFoundError as exc:
             raise MediaShrinkerError(f"ffmpeg not found: {ffmpeg_path}") from exc
+        except subprocess.TimeoutExpired as exc:  # pragma: no cover
+            raise MediaShrinkerError(f"ffmpeg timed out for {source} after 3600s") from exc
 
         if completed.returncode != 0:
             raise MediaShrinkerError(
@@ -1654,12 +1670,16 @@ def _copy_macos_creation_time(
     creation_date = datetime.fromtimestamp(float(birthtime)).strftime(
         "%m/%d/%Y %H:%M:%S"
     )
-    subprocess.run(
-        [setfile_path, "-d", creation_date, str(dest.resolve())],
-        check=False,
-        capture_output=True,
-        text=True,
-    )
+    try:
+        subprocess.run(
+            [setfile_path, "-d", creation_date, str(dest.resolve())],
+            check=False,
+            capture_output=True,
+            text=True,
+            timeout=60,
+        )  # nosec B603
+    except subprocess.TimeoutExpired:  # pragma: no cover
+        pass
 
 
 def _format_result(root: Path, result: ConversionResult) -> str: