diff --git a/backend/secuscan/executor.py b/backend/secuscan/executor.py index a8523a245..062d6a1e3 100644 --- a/backend/secuscan/executor.py +++ b/backend/secuscan/executor.py @@ -5,8 +5,10 @@ import asyncio from asyncio import subprocess import os +import shutil import signal import base64 +import tempfile import uuid import json import time @@ -18,6 +20,39 @@ _CANCEL_GRACE_SECONDS = 5 +# Reserved command-template token. A plugin that needs a writable working +# directory puts this literal in its ``command_template`` instead of a +# hardcoded path (e.g. amass's ``-dir``). At execution time it is replaced with +# a fresh, private (0700) per-task directory created via ``tempfile.mkdtemp`` +# and removed once the scan finishes. This closes the predictable-world-writable +# ``/tmp/`` symlink-hijack / cross-user-collision class of bug: the path is +# unguessable per run and never shared or left behind. The token deliberately +# contains no ``{...}`` so it passes plugin validation and template +# interpolation through untouched as an opaque literal. +SCRATCH_DIR_PLACEHOLDER = "%SECUSCAN_SCRATCH_DIR%" + + +def _substitute_scratch_dir( + command: List[str], plugin_id: str +) -> Tuple[List[str], Optional[str]]: + """Resolve ``SCRATCH_DIR_PLACEHOLDER`` tokens to a fresh private directory. + + Returns ``(command, scratch_dir)``. When the command contains no + placeholder, ``command`` is returned unchanged and ``scratch_dir`` is + ``None``. Otherwise a new ``tempfile.mkdtemp`` directory is created (mode + 0700, unpredictable name) and every placeholder token is replaced with its + path; the caller is responsible for removing that directory afterwards. + """ + if SCRATCH_DIR_PLACEHOLDER not in command: + return command, None + + scratch_dir = tempfile.mkdtemp(prefix=f"secuscan-{plugin_id}-") + resolved = [ + scratch_dir if token == SCRATCH_DIR_PLACEHOLDER else token + for token in command + ] + return resolved, scratch_dir + from .auth import DEFAULT_OWNER_ID from .redaction import redact from .cache import get_cache @@ -562,44 +597,59 @@ async def _execute_standard_scanner( if not command: raise ValueError("Failed to build command") - from .validation import validate_command_network_egress - cmd_valid, cmd_err = validate_command_network_egress( - command, safe_mode, plugin_id, task_id - ) - if not cmd_valid: - raise ValueError(f"Command network egress validation failed: {cmd_err}") - - # Apply Docker Sandboxing if enabled - if settings.docker_enabled: - await self._ensure_docker_network() - docker_image = plugin.docker_image or "alpine:latest" - docker_cmd = [ - "docker", - "run", - "--rm", - "--name", - f"secuscan_task_{task_id}", - "--memory", - f"{settings.sandbox_memory_mb}m", - "--cpus", - str(settings.sandbox_cpu_quota), - "--cap-drop", "NET_RAW", - "--network", settings.docker_network, - docker_image, - ] - command = docker_cmd + command - - logger.info(f"Executing task {task_id}: {' '.join(command)}") - await self._broadcast(task_id, "status", TaskStatus.RUNNING.value) - await self._broadcast_phase(task_id, ScanPhase.RUNNING_COMMAND.value) - - # Execute command - start_time = time.time() - output, exit_code = await self._execute_command( - command, - task_id, - timeout=self._resolve_execution_timeout(inputs), - ) + # Replace any reserved scratch-dir placeholder with a fresh private + # per-task directory, and guarantee its removal once execution ends. + command, scratch_dir = _substitute_scratch_dir(command, plugin_id) + try: + from .validation import validate_command_network_egress + cmd_valid, cmd_err = validate_command_network_egress( + command, safe_mode, plugin_id, task_id + ) + if not cmd_valid: + raise ValueError(f"Command network egress validation failed: {cmd_err}") + + # Apply Docker Sandboxing if enabled + if settings.docker_enabled: + await self._ensure_docker_network() + docker_image = plugin.docker_image or "alpine:latest" + docker_cmd = [ + "docker", + "run", + "--rm", + "--name", + f"secuscan_task_{task_id}", + "--memory", + f"{settings.sandbox_memory_mb}m", + "--cpus", + str(settings.sandbox_cpu_quota), + "--cap-drop", "NET_RAW", + "--network", settings.docker_network, + ] + # A plugin that resolved SCRATCH_DIR_PLACEHOLDER got a path on + # the *host*; the container has its own filesystem, so bind-mount + # that private dir into the container at the same path or the + # tool cannot write there. The host dir is 0700-private and is + # removed in the finally block below; mount it read-write so the + # tool can populate it. + if scratch_dir: + docker_cmd += ["-v", f"{scratch_dir}:{scratch_dir}:rw"] + docker_cmd.append(docker_image) + command = docker_cmd + command + + logger.info(f"Executing task {task_id}: {' '.join(command)}") + await self._broadcast(task_id, "status", TaskStatus.RUNNING.value) + await self._broadcast_phase(task_id, ScanPhase.RUNNING_COMMAND.value) + + # Execute command + start_time = time.time() + output, exit_code = await self._execute_command( + command, + task_id, + timeout=self._resolve_execution_timeout(inputs), + ) + finally: + if scratch_dir: + shutil.rmtree(scratch_dir, ignore_errors=True) duration = time.time() - start_time # Save raw output diff --git a/plugins/amass/metadata.json b/plugins/amass/metadata.json index b892dfec1..975392253 100644 --- a/plugins/amass/metadata.json +++ b/plugins/amass/metadata.json @@ -21,7 +21,7 @@ "-d", "{target}", "-dir", - "/tmp/amass", + "%SECUSCAN_SCRATCH_DIR%", "-silent" ], "fields": [ @@ -59,5 +59,5 @@ "capabilities": [ "network" ], - "checksum": "e8dbca61aa046afe6ca08cf3188599f94866ea11ab1d6ded3394c6345d797e10" + "checksum": "78c4159360edbefa1afaaa3c6193f6540b71233fcd387333939f1769384d1d58" } diff --git a/testing/backend/test_amass_plugin.py b/testing/backend/test_amass_plugin.py index 0a7702253..cfb7eef15 100644 --- a/testing/backend/test_amass_plugin.py +++ b/testing/backend/test_amass_plugin.py @@ -114,7 +114,10 @@ def test_amass_command_renders_with_target(setup_test_environment): assert "-d" in command assert "secuscan.in" in command assert "-dir" in command - assert "/tmp/amass" in command + # The old hardcoded world-writable /tmp/amass path (issue #1803) must be gone, + # replaced by the reserved scratch-dir placeholder the executor resolves per task. + assert "/tmp/amass" not in command + assert "%SECUSCAN_SCRATCH_DIR%" in command assert "-silent" in command @@ -131,7 +134,7 @@ def test_amass_command_full_token_sequence(setup_test_environment): "-d", "secuscan.in", "-dir", - "/tmp/amass", + "%SECUSCAN_SCRATCH_DIR%", "-silent", ], f"Command template drift detected. Got: {command}" @@ -148,7 +151,7 @@ def test_amass_drops_target_token_when_absent(setup_test_environment): assert rendered is not None assert not any("{" in token for token in rendered), "Unresolved placeholder leaked" - assert rendered == ["amass", "enum", "-d", "-dir", "/tmp/amass", "-silent"] + assert rendered == ["amass", "enum", "-d", "-dir", "%SECUSCAN_SCRATCH_DIR%", "-silent"] populated = manager.build_command("amass", {"target": "secuscan.in"}) assert "secuscan.in" in populated diff --git a/testing/backend/unit/test_executor_scratch_dir.py b/testing/backend/unit/test_executor_scratch_dir.py new file mode 100644 index 000000000..626f4392d --- /dev/null +++ b/testing/backend/unit/test_executor_scratch_dir.py @@ -0,0 +1,215 @@ +""" +Unit tests for the reserved scratch-directory mechanism in executor.py. + +Issue #1803: the amass plugin hardcoded a world-writable, predictable +``/tmp/amass`` path (symlink-hijack / cross-user-collision / never-cleaned). +Plugins now use the ``SCRATCH_DIR_PLACEHOLDER`` token, which the executor +replaces at run time with a fresh private ``tempfile.mkdtemp`` directory and +removes when the scan ends. These tests cover ``_substitute_scratch_dir``. +""" + +import os +import shutil +import stat +import uuid +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from backend.secuscan.database import get_db, init_db +from backend.secuscan.config import settings +from backend.secuscan.executor import ( + SCRATCH_DIR_PLACEHOLDER, + TaskExecutor, + _substitute_scratch_dir, +) +from backend.secuscan.models import TaskStatus + + +def test_no_placeholder_returns_command_unchanged(): + """Commands without the placeholder are returned verbatim with no dir.""" + command = ["nmap", "-sV", "127.0.0.1"] + resolved, scratch_dir = _substitute_scratch_dir(command, "nmap") + assert resolved == command + assert scratch_dir is None + + +def test_placeholder_replaced_with_real_existing_dir(): + """The placeholder token is replaced by an actually-created directory.""" + command = ["amass", "enum", "-d", "example.com", "-dir", SCRATCH_DIR_PLACEHOLDER] + resolved, scratch_dir = _substitute_scratch_dir(command, "amass") + try: + assert scratch_dir is not None + assert os.path.isdir(scratch_dir) + assert SCRATCH_DIR_PLACEHOLDER not in resolved + # The '-dir' value is now the real scratch path. + assert resolved[resolved.index("-dir") + 1] == scratch_dir + finally: + if scratch_dir: + shutil.rmtree(scratch_dir, ignore_errors=True) + + +def test_scratch_dir_name_is_unpredictable(): + """Two runs get distinct, unguessable directories (no shared /tmp/amass).""" + cmd = ["amass", "-dir", SCRATCH_DIR_PLACEHOLDER] + r1, d1 = _substitute_scratch_dir(cmd, "amass") + r2, d2 = _substitute_scratch_dir(cmd, "amass") + try: + assert d1 != d2 + assert d1 not in ("/tmp/amass", "/tmp/amass/") + assert "secuscan-amass-" in os.path.basename(d1) + finally: + shutil.rmtree(d1, ignore_errors=True) + shutil.rmtree(d2, ignore_errors=True) + + +def test_multiple_placeholders_all_replaced_with_same_dir(): + """Every placeholder occurrence resolves to the one created directory.""" + command = [SCRATCH_DIR_PLACEHOLDER, "--out", SCRATCH_DIR_PLACEHOLDER] + resolved, scratch_dir = _substitute_scratch_dir(command, "tool") + try: + assert resolved[0] == scratch_dir + assert resolved[2] == scratch_dir + assert SCRATCH_DIR_PLACEHOLDER not in resolved + finally: + if scratch_dir: + shutil.rmtree(scratch_dir, ignore_errors=True) + + +@pytest.mark.skipif(os.name != "posix", reason="POSIX file-mode semantics") +def test_scratch_dir_is_private(): + """mkdtemp creates the directory 0700 so other local users cannot read it.""" + command = ["amass", "-dir", SCRATCH_DIR_PLACEHOLDER] + _resolved, scratch_dir = _substitute_scratch_dir(command, "amass") + try: + mode = stat.S_IMODE(os.stat(scratch_dir).st_mode) + assert mode == 0o700 + finally: + if scratch_dir: + shutil.rmtree(scratch_dir, ignore_errors=True) + + +# --------------------------------------------------------------------------- +# Docker command path (PR #2020 review): the host scratch dir must be +# bind-mounted into the sandbox container, else the tool cannot write there. +# --------------------------------------------------------------------------- + + +async def _run_standard_scanner_with_docker(command, docker_image="caffix/amass:latest"): + """Drive ``_execute_standard_scanner`` with Docker enabled. + + Returns ``(final_command, scratch_dir, existed_during_run)`` where + ``final_command`` is the argv actually handed to ``_execute_command``. + """ + await init_db(settings.database_path) + db = await get_db() + + task_id = str(uuid.uuid4()) + owner_id = str(uuid.uuid4()) + await db.execute( + """ + INSERT INTO tasks (id, owner_id, plugin_id, tool_name, target, + inputs_json, status, consent_granted, safe_mode) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?) + """, + (task_id, owner_id, "amass", "amass", "example.com", "{}", + TaskStatus.QUEUED.value, 1, 0), + ) + + executor = TaskExecutor() + + mock_plugin = MagicMock() + mock_plugin.id = "amass" + mock_plugin.name = "amass" + mock_plugin.docker_image = docker_image + + recorded = {} + + # Capture the real scratch dir the executor creates by wrapping the helper, + # so recovery is robust to path formats (e.g. Windows drive-letter colons). + real_substitute = _substitute_scratch_dir + + def wrapped_substitute(cmd, plugin_id): + resolved, sd = real_substitute(cmd, plugin_id) + recorded["scratch_dir"] = sd + return resolved, sd + + def fake_execute_command(cmd, task, timeout=None): + recorded["command"] = list(cmd) + sd = recorded.get("scratch_dir") + recorded["existed_during_run"] = bool(sd) and os.path.isdir(sd) + return ("mock output\n", 0) + + try: + with patch("backend.secuscan.executor.get_plugin_manager") as mock_pm, \ + patch("backend.secuscan.executor._substitute_scratch_dir", wrapped_substitute), \ + patch.object(settings, "docker_enabled", True), \ + patch.object(executor, "_ensure_docker_network", new=AsyncMock()), \ + patch("backend.secuscan.validation.validate_command_network_egress", + return_value=(True, None)), \ + patch.object(executor, "_execute_command", + side_effect=fake_execute_command), \ + patch.object(executor, "_classify_command_result", + return_value=(TaskStatus.COMPLETED.value, None)), \ + patch.object(executor, "_upsert_findings_and_report", new=AsyncMock()): + mock_pm.return_value.build_command.return_value = command + + await executor._execute_standard_scanner( + db=db, + task_id=task_id, + owner_id=owner_id, + plugin=mock_plugin, + plugin_id="amass", + target="example.com", + inputs={}, + safe_mode=0, + ) + finally: + await db.disconnect() + + return recorded["command"], recorded.get("scratch_dir"), recorded["existed_during_run"] + + +@pytest.mark.asyncio +async def test_docker_mode_bind_mounts_scratch_dir(): + """In Docker mode the private scratch dir is mounted into the container.""" + command = ["amass", "enum", "-d", "example.com", "-dir", SCRATCH_DIR_PLACEHOLDER] + final_command, scratch_dir, existed = await _run_standard_scanner_with_docker(command) + + # A docker run wrapper with an explicit bind mount was built. + assert final_command[:2] == ["docker", "run"] + assert "-v" in final_command + assert f"{scratch_dir}:{scratch_dir}:rw" in final_command + + # The placeholder is fully resolved and the vulnerable path is gone. + assert SCRATCH_DIR_PLACEHOLDER not in final_command + assert scratch_dir not in ("/tmp/amass", "/tmp/amass/") + assert "secuscan-amass-" in os.path.basename(scratch_dir) + + # The inner tool references the *same* path that was mounted. + assert final_command[final_command.index("-dir") + 1] == scratch_dir + + # The mount points at a directory that really existed during the run. + assert existed is True + + +@pytest.mark.asyncio +async def test_docker_mode_cleans_up_scratch_dir(): + """The scratch dir is removed after the Docker run finishes.""" + command = ["amass", "enum", "-d", "example.com", "-dir", SCRATCH_DIR_PLACEHOLDER] + _final_command, scratch_dir, existed = await _run_standard_scanner_with_docker(command) + + assert existed is True + # finally-block cleanup removed the host directory once execution ended. + assert not os.path.exists(scratch_dir) + + +@pytest.mark.asyncio +async def test_docker_mode_without_placeholder_adds_no_mount(): + """A plugin that uses no scratch token gets no bind mount.""" + command = ["nmap", "-sV", "127.0.0.1"] + final_command, _scratch, _existed = await _run_standard_scanner_with_docker( + command, docker_image="instrumentisto/nmap:latest" + ) + assert final_command[:2] == ["docker", "run"] + assert "-v" not in final_command