diff --git a/.dockerignore b/.dockerignore index 5b45455e..4a16e4ff 100644 --- a/.dockerignore +++ b/.dockerignore @@ -3,8 +3,6 @@ __pycache__ *.py[cod] *.log -.env -.env.* .venv venv node_modules @@ -23,3 +21,41 @@ install.bat OmniParser_CraftOS debug_images workspace + +# --------------------------------------------------------------------------- +# Secrets and per-install state. `COPY . .` runs against a developer's working +# tree, and the release workflow pushes the resulting image to a public +# registry — anything listed here would otherwise be baked into a layer and +# published. Layers are permanent: deleting the file in a later layer does not +# remove it. Keep this list in step with .gitignore. +# --------------------------------------------------------------------------- +.env +.env.* +.credentials +.credentials/ +config.json +app/config/settings.json +app/config/mcp_config.json +app/config/skills_config.json +app/config/scheduler_config.json +app/data/.usage +app/data/.file_index +agent_file_system +agent_bundle +.craftbot +**/.craftbot +**/logs +**/agent_logs.txt +**/memory.txt +**/USER.md +**/CONVERSATION_HISTORY.md +**/EVENT.md +**/EVENT_UNPROCESSED.md +**/onboarding_config.json +**/gui/config +**/.whatsapp_web_sessions +.playwright-mcp +.claude +.vscode +.idea +runtime diff --git a/Dockerfile b/Dockerfile index 8078bb42..4e96292b 100644 --- a/Dockerfile +++ b/Dockerfile @@ -24,16 +24,16 @@ RUN apt-get update \ x11-apps \ fonts-dejavu \ curl \ - gnupg \ - lsb-release \ && rm -rf /var/lib/apt/lists/* -# Install Docker CLI (needed for docker exec against sibling desktop container) -RUN curl -fsSL https://download.docker.com/linux/debian/gpg | gpg --dearmor -o /usr/share/keyrings/docker-archive-keyring.gpg \ - && echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/docker-archive-keyring.gpg] https://download.docker.com/linux/debian $(lsb_release -cs) stable" > /etc/apt/sources.list.d/docker.list \ - && apt-get update \ - && apt-get install -y --no-install-recommends docker-ce-cli \ - && rm -rf /var/lib/apt/lists/* +# The Docker CLI used to be installed here, paired with a bind mount of +# /var/run/docker.sock in docker-compose.yml. Nothing in the codebase ever +# called it: a repo-wide search for `docker exec`, `docker run`, `docker.sock`, +# `DOCKER_HOST` and the docker SDK finds no usage. The socket is the host's +# control plane, so mounting it into a container that executes model-chosen +# shell commands hands that container root on the host. Both are removed; if a +# future feature needs to drive Docker, give it a scoped proxy rather than the +# raw socket. WORKDIR /app @@ -44,4 +44,16 @@ RUN pip install --no-cache-dir --upgrade pip \ COPY . . +# Run as an unprivileged user. The agent executes commands the model chooses, +# so a container escape or a bad command should not land as root. The UID is +# fixed (not auto-assigned) so a host bind mount can be chowned to match: +# sudo chown -R 10001:10001 ./workspace +# The directories the compose file bind-mounts are pre-created here so they +# exist with the right owner when no host directory is supplied. +RUN useradd --create-home --uid 10001 --shell /usr/sbin/nologin craftbot \ + && mkdir -p /app/workspace /app/logs \ + && chown -R craftbot:craftbot /app + +USER craftbot + CMD ["python", "-m", "app.main"] diff --git a/agent_core/core/impl/mcp/server.py b/agent_core/core/impl/mcp/server.py index b28004b5..ba4d564b 100644 --- a/agent_core/core/impl/mcp/server.py +++ b/agent_core/core/impl/mcp/server.py @@ -58,6 +58,39 @@ def get_default_stdio_cwd() -> Optional[str]: return _default_stdio_cwd +# Windows runs a .cmd/.bat through cmd.exe however it is spawned, and cmd.exe +# re-parses the whole command line — so an argument carrying one of these +# escapes into a second command even when the arguments are passed as a list. +# No quoting closes it (this is the "BatBadBut" class, CVE-2024-24576 in other +# runtimes), so the argument is refused rather than pretended to be escaped. +# `^` and `%` are deliberately absent: `^` only escapes the next character and +# cannot start a command, and rejecting `%` would break percent-encoded URLs, +# which are ordinary MCP arguments. The worst `%` can do is expand an +# environment variable into a value the same config already controls. +_CMD_METACHARACTERS = '"&|<>\r\n' + + +def _reject_unsafe_batch_args(command: str, args: List[str]) -> None: + """Refuse arguments cmd.exe would reinterpret when the target is a batch + wrapper. No-op off Windows and for real executables, where the argument + list reaches the process untouched.""" + if sys.platform != "win32": + return + if os.path.splitext(command)[1].lower() not in (".cmd", ".bat"): + return + for arg in args: + found = sorted({c for c in str(arg) if c in _CMD_METACHARACTERS}) + if found: + shown = "".join(c if c not in "\r\n" else repr(c).strip("'") for c in found) + raise ValueError( + f"MCP server argument {arg!r} contains {shown!r}, which cmd.exe " + f"would interpret while running the batch wrapper " + f"{os.path.basename(command)} — it could run a second command. " + f"Rewrite the argument, or point the server at the real " + f"executable instead of the .cmd shim." + ) + + @dataclass class MCPTool: """Represents an MCP tool discovered from a server.""" @@ -177,41 +210,25 @@ async def connect(self) -> bool: f" (cwd={cwd or os.getcwd()})" ) - # Start the subprocess + # Start the subprocess. One exec path for both platforms: the + # arguments reach the OS as a list, so an argument can never be + # read back as part of the command line. _resolve_command has + # already turned `command` into a concrete path (npx.cmd, uvx.exe, + # ...), so there is nothing left for a shell to look up. + _reject_unsafe_batch_args(command, self.args) try: - if sys.platform == "win32": - # On Windows, use shell=True to properly resolve commands like npx - # This allows Windows to find npx.cmd in PATH - full_command = f'"{command}" ' + " ".join( - f'"{arg}"' for arg in self.args - ) - logger.debug( - f"[StdioTransport] Windows shell command: {full_command}" - ) - self._process = await asyncio.create_subprocess_shell( - full_command, - stdin=asyncio.subprocess.PIPE, - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, - env=full_env, - cwd=cwd, - limit=10 - * 1024 - * 1024, # 10MB limit for large MCP responses (e.g., screenshots) - ) - else: - self._process = await asyncio.create_subprocess_exec( - command, - *self.args, - stdin=asyncio.subprocess.PIPE, - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, - env=full_env, - cwd=cwd, - limit=10 - * 1024 - * 1024, # 10MB limit for large MCP responses (e.g., screenshots) - ) + self._process = await asyncio.create_subprocess_exec( + command, + *self.args, + stdin=asyncio.subprocess.PIPE, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + env=full_env, + cwd=cwd, + limit=10 + * 1024 + * 1024, # 10MB limit for large MCP responses (e.g., screenshots) + ) except FileNotFoundError as e: logger.error( f"[StdioTransport] Command not found: '{command}'. Make sure it is installed and in PATH. Error: {e}" diff --git a/app/agent_app/manager.py b/app/agent_app/manager.py index 0ac6a068..90156fa3 100644 --- a/app/agent_app/manager.py +++ b/app/agent_app/manager.py @@ -12,6 +12,7 @@ import asyncio import errno +import hmac import json import os import re @@ -33,7 +34,6 @@ from app import node_runtime from app.process_ledger import ( ROLE_AGENT_APP, - ROLE_TUNNEL, get_ledger, kill_tree, listening_pids, @@ -4172,8 +4172,18 @@ def validate_bridge_token(self, token: str) -> Optional[str]: Returns: project_id if token is valid, None otherwise. """ + if not token: + return None + # Constant-time: this compares a caller-supplied header against a live + # secret, and a plain == leaks how many leading characters matched. + # Compared as bytes because compare_digest's str form rejects + # non-ASCII, and this value arrives from an HTTP header. + presented = token.encode("utf-8", "surrogateescape") for project_id, project in self.projects.items(): - if project.bridge_token and project.bridge_token == token: + if not project.bridge_token: + continue + expected = project.bridge_token.encode("utf-8", "surrogateescape") + if hmac.compare_digest(presented, expected): return project_id return None diff --git a/docker-compose.yml b/docker-compose.yml index a40cf43b..ae7a1e2d 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -37,10 +37,22 @@ services: environment: - OMNIPARSER_BASE_URL=http://omniparser-server:7861 - USE_OMNIPARSER=${USE_OMNIPARSER:-True} + # The image runs as UID 10001 (see Dockerfile). Bind-mounted host + # directories keep their host ownership, so chown them to match once: + # sudo chown -R 10001:10001 ./workspace volumes: - - /var/run/docker.sock:/var/run/docker.sock + # /var/run/docker.sock was mounted here and is deliberately gone. It is + # the host's Docker control plane: a container holding it can start a + # privileged container and take the host. Nothing in the codebase used + # it (no `docker exec`, no docker SDK, no DOCKER_HOST), and this + # container runs commands the model chooses, so it was pure blast + # radius. Do not add it back without a scoped proxy in front. - ./workspace:/app/workspace - ./config.json:/app/config.json + security_opt: + # A process inside cannot gain privileges it did not start with + # (blocks setuid escalation paths). + - no-new-privileges:true depends_on: omniparser: condition: service_healthy diff --git a/tests/test_bridge_token_validation.py b/tests/test_bridge_token_validation.py new file mode 100644 index 00000000..3d0acf9e --- /dev/null +++ b/tests/test_bridge_token_validation.py @@ -0,0 +1,77 @@ +"""validate_bridge_token: the integration bridge's only authentication. + +An Agent App backend presents this token to POST /api/integrations/proxy, +which then injects the user's real OAuth credentials into an outbound call. +The token therefore guards every connected integration, and it is compared +against a value an attacker supplies in an HTTP header -- so the comparison +has to be constant-time, and it has to survive whatever bytes arrive. +""" + +import types + +import pytest + +from app.agent_app.manager import AgentAppManager + +TOKEN = "kR3n-9bQwTf1xZa7LmPd0sVhYcE2gJu4" + + +@pytest.fixture +def manager(): + mgr = AgentAppManager.__new__(AgentAppManager) # no filesystem, no ports + mgr.projects = { + "alpha": types.SimpleNamespace(bridge_token=TOKEN), + "beta": types.SimpleNamespace(bridge_token="another-token-entirely"), + "gamma": types.SimpleNamespace(bridge_token=""), # never launched + } + return mgr + + +def test_the_right_token_resolves_to_its_project(manager): + assert manager.validate_bridge_token(TOKEN) == "alpha" + assert manager.validate_bridge_token("another-token-entirely") == "beta" + + +@pytest.mark.parametrize( + "presented", + [ + "", + None, + "wrong", + TOKEN[:-1], # prefix + TOKEN + "x", # extension + TOKEN[:-1] + "X", # last character differs + TOKEN.upper(), # case + " " + TOKEN, # leading space + TOKEN + " ", # trailing space + ], +) +def test_anything_else_is_refused(manager, presented): + assert manager.validate_bridge_token(presented) is None + + +def test_a_project_with_no_token_is_never_matched(manager): + """An empty bridge_token means 'not launched'. Presenting '' must not + match it -- that would hand out a project id for free.""" + assert manager.validate_bridge_token("") is None + + +@pytest.mark.parametrize( + "presented", + [ + "tökén-with-non-ascii", + "\udcff\udcfe", # lone surrogates, as a mangled header can carry + "x" * 10000, # absurd length + "null\x00byte", + ], +) +def test_hostile_input_is_refused_without_raising(manager, presented): + """hmac.compare_digest rejects non-ASCII str outright, so the comparison + works on bytes. A caller controls this value; it must not be able to turn + a failed auth into a 500.""" + assert manager.validate_bridge_token(presented) is None + + +def test_no_project_at_all(manager): + manager.projects = {} + assert manager.validate_bridge_token(TOKEN) is None diff --git a/tests/test_mcp_stdio_spawn.py b/tests/test_mcp_stdio_spawn.py new file mode 100644 index 00000000..6582bfa3 --- /dev/null +++ b/tests/test_mcp_stdio_spawn.py @@ -0,0 +1,121 @@ +"""Argument handling when launching a stdio MCP server. + +An MCP server's command and args come from configuration, and configuration +is editable in the UI, importable from a profile bundle and writable by the +agent itself. So the args are not trusted input, and the spawn must not let +one of them turn into a second command. + +The old code built a shell string -- f'"{command}" ' + " ".join(f'"{a}"') -- +and handed it to create_subprocess_shell, so a single embedded quote closed +the quoting and everything after it ran. Two things replace it: the argument +list goes to the OS as a list, and the one case a list cannot make safe +(a Windows .cmd/.bat wrapper, which cmd.exe re-parses no matter how it is +spawned) is refused up front. +""" + +import sys + +import pytest + +from agent_core.core.impl.mcp.server import _reject_unsafe_batch_args + +WINDOWS_ONLY = pytest.mark.skipif( + sys.platform != "win32", reason="cmd.exe argument re-parsing is Windows-only" +) + +# Each of these ran a second command through the old shell-string build. +INJECTIONS = [ + 'x" & echo pwned & rem ', + "y & echo pwned", + "z | echo pwned", + "w > pwned.txt", + "a < pwned.txt", + 'q" && echo pwned', + "line1\nline2", + "line1\r\nline2", +] + +# Ordinary MCP arguments. None may be refused. +LEGITIMATE = [ + "-y", + "@modelcontextprotocol/server-filesystem", + "https://example.com/a%20b", + "%APPDATA%", + "C:\\Users\\someone\\My Documents", + "--port=3000", + "a-file_name.v2.json", + "", +] + + +@WINDOWS_ONLY +@pytest.mark.parametrize("arg", INJECTIONS) +def test_batch_wrapper_refuses_an_injecting_argument(arg): + with pytest.raises(ValueError) as excinfo: + _reject_unsafe_batch_args("C:\\tools\\npx.cmd", [arg]) + # The message has to tell the user what to do, not just that it failed. + assert "npx.cmd" in str(excinfo.value) + + +@WINDOWS_ONLY +@pytest.mark.parametrize("arg", LEGITIMATE) +def test_batch_wrapper_accepts_ordinary_arguments(arg): + _reject_unsafe_batch_args("C:\\tools\\npx.cmd", [arg]) + + +@WINDOWS_ONLY +def test_a_url_with_a_query_string_is_refused_on_a_batch_wrapper(): + """This one deserves stating plainly, because it looks like a false + positive and is not: cmd.exe splits on '&' while running the wrapper, so + `https://h/p?a=1&b=2` does not survive the trip whatever we do. Refusing + it with an explanation beats delivering a silently truncated URL. Against + a real executable the same argument is fine -- see the test below.""" + with pytest.raises(ValueError): + _reject_unsafe_batch_args("C:\\tools\\npx.cmd", ["https://h/p?a=1&b=2"]) + + _reject_unsafe_batch_args("C:\\tools\\node.exe", ["https://h/p?a=1&b=2"]) + + +@WINDOWS_ONLY +def test_percent_is_allowed_so_encoded_urls_still_work(): + """'%' can expand an environment variable, but only into a value the same + config already controls -- and refusing it would reject every + percent-encoded URL, which is an ordinary argument.""" + _reject_unsafe_batch_args("C:\\tools\\npx.cmd", ["https://example.com/a%20b"]) + _reject_unsafe_batch_args("C:\\tools\\npx.cmd", ["%APPDATA%"]) + + +@WINDOWS_ONLY +def test_caret_is_allowed_because_it_cannot_start_a_command(): + _reject_unsafe_batch_args("C:\\tools\\npx.cmd", ["a^b"]) + + +@WINDOWS_ONLY +@pytest.mark.parametrize("arg", INJECTIONS) +@pytest.mark.parametrize("exe", ["C:\\tools\\node.exe", "C:\\tools\\server"]) +def test_real_executables_accept_anything(exe, arg): + """A list argument reaches a real executable untouched, so there is + nothing to refuse -- narrowing the check to batch wrappers keeps it from + rejecting arguments that were never dangerous.""" + _reject_unsafe_batch_args(exe, [arg]) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX has no batch wrappers") +@pytest.mark.parametrize("arg", INJECTIONS) +def test_posix_never_refuses(arg): + _reject_unsafe_batch_args("/usr/bin/npx", [arg]) + _reject_unsafe_batch_args("/usr/local/bin/some.cmd", [arg]) + + +@WINDOWS_ONLY +def test_extension_match_is_case_insensitive(): + with pytest.raises(ValueError): + _reject_unsafe_batch_args("C:\\tools\\NPX.CMD", ["a & b"]) + with pytest.raises(ValueError): + _reject_unsafe_batch_args("C:\\tools\\run.BAT", ["a | b"]) + + +@WINDOWS_ONLY +def test_every_argument_is_checked_not_just_the_first(): + with pytest.raises(ValueError): + _reject_unsafe_batch_args("C:\\tools\\npx.cmd", ["-y", "ok", "bad & echo x"])