From 70e2a67034ad91563c70a74be54dbeeac49dfd45 Mon Sep 17 00:00:00 2001 From: "54411234+Ailcope@users.noreply.github.com" <54411234+Ailcope@users.noreply.github.com> Date: Fri, 24 Jul 2026 21:32:57 +0200 Subject: [PATCH 1/3] fix(security): close findings from a full security audit A full read of src/ (auth, dashboard, proxmox, ssh, bmc, deploy) turned up one hole that mattered and a set of smaller ones. The important one: the chat's human-approval gate covered ssh_run / proxmox_run but not proxmox_write_file, so an instruction injected through a log line, a config file or a web-search result could write ~/.ssh/authorized_keys on a guest with no modal -- exec through the front door was gated, exec through the side door was not. - chat: _NEEDS_CONFIRMATION now covers guest-file writes and the destructive tools (vm_bulk_action, vm_stop/restart/migrate, snapshot rollback/delete, backup_restore, bmc_power_off/reset). New _CONFIRM_WHEN_ARG_PRESENT gates proxmox_vm_config only when `updates` is set; dry_run=True calls and exec_id-only polling still skip the modal. - bmc: BMCDevice.verify_tls was declared but never parsed from the YAML, so `verify_tls: true` was silently ignored and every Redfish call ran with certificate verification off -- while sending the BMC admin password in Basic Auth on each request. Parse it, surface it in validate-config, document it in beaconmcp.yaml.example. - bmc: stop passing the BMC password as `ipmitool -P `; argv is world-readable through /proc//cmdline. Use `-E` + IPMI_PASSWORD. - dashboard: chmod 0600 dashboard.db and its WAL sidecars. clients.json and tokens.db were locked down already, but this file stores mcp_bearer in plaintext and was left at the process umask. - deploy: chmod 600 the .env written by install.sh and by `beaconmcp init`; add UMask=0077 / NoNewPrivileges / PrivateTmp to the systemd unit. - auth: is_trusted_redirect_uri matched loopback callbacks by string prefix, so http://localhost:1@attacker.example/cb passed. Resolve the parsed hostname instead. /oauth/authorize caught this downstream, the DCR register endpoint did not. - proxmox: every endpoint is an f-string with caller-supplied path segments (snapname, storage, archive). Reject anything not shaped like a plain segment before the request leaves the process. - audit: redact session_id, code_verifier, totp_secret, session_key, access_token and private_key. - dashboard: /app/overview and /app/usage passed _render's arguments in the wrong order and 500'd on every request. Tests: 341 passed. --- README.md | 2 +- beaconmcp.yaml.example | 12 ++ deploy/beaconmcp.service | 8 + deploy/install.sh | 5 + docs/dashboard.md | 18 ++- src/beaconmcp/audit.py | 8 +- src/beaconmcp/auth.py | 60 ++++++-- src/beaconmcp/bmc/ipmi.py | 19 ++- src/beaconmcp/config.py | 6 + src/beaconmcp/dashboard/app.py | 20 +-- src/beaconmcp/dashboard/chat.py | 65 ++++++-- src/beaconmcp/dashboard/db.py | 29 ++++ src/beaconmcp/proxmox/client.py | 43 +++++- src/beaconmcp/wizard.py | 8 + tests/test_bmc_ipmi.py | 20 ++- tests/test_security_hardening.py | 249 +++++++++++++++++++++++++++++++ 16 files changed, 525 insertions(+), 47 deletions(-) create mode 100644 tests/test_security_hardening.py diff --git a/README.md b/README.md index e619049..28e489b 100644 --- a/README.md +++ b/README.md @@ -264,7 +264,7 @@ BeaconMCP exposes tools that cause irreversible changes: `ssh_run`, `proxmox_run - **Disable auto-approve** on every external MCP client (Assistant Desktop, Gemini CLI, ChatGPT MCP). Keep per-call approval enabled; refuse "always allow this tool". - **Read the `command` argument** before approving any `ssh_run` or `proxmox_run` call. Ask: if this ran against the wrong VM or host, could I recover? -- **The integrated chat** at `/app/chat` already forces human confirmation for every `ssh_run` / `proxmox_run` call that carries a `command` (polling-only calls with just `exec_id` are read-only and skip the modal). Read the arguments shown on the confirmation card even when you click through fast. No answer within 5 minutes counts as refusal. +- **The integrated chat** at `/app/chat` forces human confirmation for every code-execution tool (`ssh_run`, `proxmox_run`, `proxmox_write_file`, the transfer tools) and every destructive one (`vm_bulk_action`, `proxmox_vm_stop`, snapshot rollback/delete, backup restore, `bmc_power_off`, `bmc_power_reset`). Polling-only calls (`exec_id` alone), `dry_run=True` calls, and the read shape of `proxmox_vm_config` skip the modal. See [docs/dashboard.md](docs/dashboard.md#mandatory-confirmation-for-dangerous-tools) for the full list. Read the arguments on the confirmation card even when you click through fast. No answer within 5 minutes counts as refusal. - **Prefer read-only tools** (`*_list_*`, `*_status`, `*_get_*`, `get_logs`, `health_status`) for exploration — they cannot break anything and are never gated by confirmation. - **Do not share a `/app/tokens` bearer** with a client you do not fully control. A leaked token grants arbitrary shell access on your Proxmox nodes for the token's full lifetime (named tokens last `server.named_token_ttl`, 30 days by default), so revoke it from `/app/tokens` the moment it leaks. diff --git a/beaconmcp.yaml.example b/beaconmcp.yaml.example index 5880220..db590ed 100644 --- a/beaconmcp.yaml.example +++ b/beaconmcp.yaml.example @@ -208,6 +208,18 @@ bmc: password: ${RACK2_IPMI_PASSWORD} # no jump_host = direct connection + - id: rack3-idrac + type: idrac # idrac | supermicro | redfish -> Redfish over HTTPS + host: 192.168.10.22 + user: root + password: ${RACK3_IDRAC_PASSWORD} + # Verify the BMC's TLS certificate on Redfish calls. Defaults to + # false because BMCs ship self-signed certs out of the factory; the + # admin password is sent on every request, so turn this on (after + # installing a cert the host trusts) for any BMC that is not on a + # fully trusted management VLAN. + verify_tls: false + features: dashboard: enabled: true diff --git a/deploy/beaconmcp.service b/deploy/beaconmcp.service index 49a8267..0b64ff0 100644 --- a/deploy/beaconmcp.service +++ b/deploy/beaconmcp.service @@ -11,5 +11,13 @@ ExecStart=/opt/beaconmcp/.venv/bin/python -m beaconmcp serve Restart=on-failure RestartSec=5 +# Files the server creates at runtime (clients.json, tokens.db, dashboard.db, +# audit.log) all hold secrets. Default umask 0022 would make them +# world-readable before the explicit chmod calls land; 0077 closes that +# window and covers anything not chmod'd individually. +UMask=0077 +NoNewPrivileges=true +PrivateTmp=true + [Install] WantedBy=multi-user.target diff --git a/deploy/install.sh b/deploy/install.sh index ae1965f..78fe123 100755 --- a/deploy/install.sh +++ b/deploy/install.sh @@ -49,6 +49,11 @@ if [ ! -f "$INSTALL_DIR/.env" ]; then else echo "[*] Existing .env preserved." fi +# The .env holds every secret the server has: Proxmox API tokens, BMC admin +# passwords, SSH passwords, the dashboard session key. `cp` and the shell +# create it with the default umask (0644), which leaves it readable by every +# local user. Make it owner-only, on fresh and pre-existing installs alike. +chmod 600 "$INSTALL_DIR/.env" # 5.b beaconmcp.yaml config file if [ ! -f "$INSTALL_DIR/beaconmcp.yaml" ]; then diff --git a/docs/dashboard.md b/docs/dashboard.md index d17f35e..67c4f53 100644 --- a/docs/dashboard.md +++ b/docs/dashboard.md @@ -68,16 +68,28 @@ Constraints: - **Thinking effort** — dropdown with `minimal` / `low` / `medium` / `high`. `gemini-2.5-pro` cannot disable thinking, so `minimal` is clamped to the 128-token floor automatically. - **Markdown rendering** — the client parses headings (`#`–`######`), ordered and unordered lists, blockquotes, horizontal rules, code fences (with `lang-*` class), inline code, bold/italic/strikethrough, and HTTP(S) links. -## Mandatory confirmation for shell-capable tools +## Mandatory confirmation for dangerous tools -`ssh_run` and `proxmox_run` **never** run a command without manual approval from the chat UI. A call carrying only `exec_id=` is treated as read-only polling and skips the modal; any call that carries a `command` triggers the gate. When Gemini fires a gated call: +The model's input is untrusted: a log line, a config file, or a web-search result can carry an injected instruction. So two classes of tool **never** run without manual approval from the chat UI. + +**Arbitrary code execution** — `ssh_run`, `proxmox_run`, and the primitives that become code execution in one hop (`proxmox_write_file`, `proxmox_upload_file`, `proxmox_download_file`, `proxmox_delete_transfer`). Writing `~/.ssh/authorized_keys` or a file under `/etc/cron.d` is exactly as good as a shell, which is why the file tools sit alongside the exec tools. + +**Destructive or irreversible** — `vm_bulk_action`, `proxmox_vm_stop`, `proxmox_vm_restart`, `proxmox_vm_migrate`, `proxmox_snapshot_rollback`, `proxmox_snapshot_delete`, `proxmox_backup_restore`, `bmc_power_off`, `bmc_power_reset`. + +Three call shapes are let through without a modal, because they don't change anything: + +- `ssh_run` / `proxmox_run` carrying only `exec_id=` — that's read-only polling of an already-approved session. +- Any gated tool called with `dry_run=True` — it only reports what it *would* do. +- `proxmox_vm_config` without `updates` — the read shape of a read-or-write tool. + +When Gemini fires a gated call: 1. The tool card switches to an "approval required" state (orange badge, auto-expanded so arguments are visible). 2. Two buttons: **Approve** / **Reject**. 3. The Gemini turn blocks server-side until the decision is made (5-minute timeout). 4. On rejection, Gemini receives a `FunctionResponse {"error": "user_rejected"}` and can revise its reply. -The allow-list is hard-coded in `src/beaconmcp/dashboard/chat.py` (`_NEEDS_CONFIRMATION` + `_tool_call_requires_confirmation`). Only the integrated chat enforces this gate; external MCP clients (Assistant Desktop, Gemini CLI, ChatGPT MCP) must enable their own per-call approval mode (see the **Security** section of the root README). +The allow-list is hard-coded in `src/beaconmcp/dashboard/chat.py` (`_NEEDS_CONFIRMATION`, `_CONFIRM_WHEN_ARG_PRESENT`, `_tool_call_requires_confirmation`). Only the integrated chat enforces this gate; external MCP clients (Assistant Desktop, Gemini CLI, ChatGPT MCP) must enable their own per-call approval mode (see the **Security** section of the root README). ## Stored data diff --git a/src/beaconmcp/audit.py b/src/beaconmcp/audit.py index bd43b52..d15b2b5 100644 --- a/src/beaconmcp/audit.py +++ b/src/beaconmcp/audit.py @@ -32,8 +32,12 @@ # Argument keys whose values are always masked before emission. _REDACT_KEYS = frozenset({ - "password", "secret", "token", "token_secret", "client_secret", - "api_key", "authorization", "totp", "bearer", + "password", "passwd", "secret", "token", "token_secret", "client_secret", + "client_secret_hash", "api_key", "apikey", "authorization", "totp", + "totp_secret", "bearer", "access_token", "refresh_token", + # OAuth / session material: a code_verifier or a session_id is as good + # as a credential to whoever reads the log. + "code_verifier", "session_id", "session_key", "private_key", }) diff --git a/src/beaconmcp/auth.py b/src/beaconmcp/auth.py index 00865c2..8b12814 100644 --- a/src/beaconmcp/auth.py +++ b/src/beaconmcp/auth.py @@ -31,6 +31,7 @@ from enum import Enum from pathlib import Path from typing import Any +from urllib.parse import urlparse import pyotp @@ -103,20 +104,35 @@ def current_client_id() -> str | None: CLIENTS_FILE = Path("/opt/beaconmcp/clients.json") -# Redirect URI prefixes that are not CORS origins and therefore cannot be -# modeled via ``server.allowed_origins``. -TRUSTED_NON_ORIGIN_REDIRECT_PREFIXES: tuple[str, ...] = ( - # OS-level custom URI schemes used by desktop clients. +# OS-level custom URI schemes used by desktop clients. Not CORS origins, so +# they cannot be modeled via ``server.allowed_origins``. Prefix-matched: the +# scheme itself is the authority (only the registering app can claim it). +TRUSTED_CUSTOM_SCHEME_PREFIXES: tuple[str, ...] = ( "vscode://", "vscode-insiders://", "cursor://", - # Local loopback for CLI / terminal clients (RFC 8252 style). - "http://localhost:", - "http://localhost/", - "http://127.0.0.1:", - "http://127.0.0.1/", - "http://[::1]:", - "http://[::1]/", +) + +# Loopback callback hosts for CLI / terminal clients (RFC 8252 style). +# Matched against the *parsed* hostname, never by string prefix: a prefix +# test on "http://localhost:" also accepts +# ``http://localhost:1@attacker.example/cb``, whose real host is +# attacker.example -- a straight authorization-code exfiltration path. +TRUSTED_LOOPBACK_HOSTS: frozenset[str] = frozenset( + {"localhost", "127.0.0.1", "::1"} +) + +# Kept for backwards compatibility with anything importing the old name. +TRUSTED_NON_ORIGIN_REDIRECT_PREFIXES: tuple[str, ...] = ( + TRUSTED_CUSTOM_SCHEME_PREFIXES + + ( + "http://localhost:", + "http://localhost/", + "http://127.0.0.1:", + "http://127.0.0.1/", + "http://[::1]:", + "http://[::1]/", + ) ) @@ -128,17 +144,31 @@ def is_trusted_redirect_uri( Web origins are sourced from ``allowed_origins`` (typically ``config.server.allowed_origins``), so redirect trust follows the same - operator-controlled allowlist as CORS. Non-origin redirect forms (custom - URI schemes and loopback callbacks) are covered by - :data:`TRUSTED_NON_ORIGIN_REDIRECT_PREFIXES`. + operator-controlled allowlist as CORS. Non-origin redirect forms are + covered by :data:`TRUSTED_CUSTOM_SCHEME_PREFIXES` (desktop schemes) and + :data:`TRUSTED_LOOPBACK_HOSTS` (RFC 8252 loopback callbacks). """ if not isinstance(redirect_uri, str): return False redirect_uri = redirect_uri.strip() if not redirect_uri: return False - if any(redirect_uri.startswith(p) for p in TRUSTED_NON_ORIGIN_REDIRECT_PREFIXES): + if any(redirect_uri.startswith(p) for p in TRUSTED_CUSTOM_SCHEME_PREFIXES): return True + # Loopback: resolve the real host instead of trusting the string shape, + # so userinfo tricks (``http://localhost:1@evil.example/cb``) don't slip + # through as "starts with http://localhost:". + try: + parsed = urlparse(redirect_uri) + except ValueError: + return False + if parsed.scheme == "http": + hostname = (parsed.hostname or "").strip().lower() + if hostname in TRUSTED_LOOPBACK_HOSTS: + return True + # Any other plaintext-HTTP callback is untrusted, full stop -- + # never fall through to the origin allowlist for it. + return False if allowed_origins: for origin in allowed_origins: if not isinstance(origin, str) or not origin: diff --git a/src/beaconmcp/bmc/ipmi.py b/src/beaconmcp/bmc/ipmi.py index 0dd1dc9..f8095e9 100644 --- a/src/beaconmcp/bmc/ipmi.py +++ b/src/beaconmcp/bmc/ipmi.py @@ -9,6 +9,7 @@ from __future__ import annotations import asyncio +import os from typing import Any from ..config import BMCDevice, Config @@ -25,15 +26,30 @@ def __init__(self, device: BMCDevice, config: Config) -> None: self._config = config def _argv(self, *extra: str) -> list[str]: + """Build the ipmitool argv. + + The BMC password is deliberately NOT passed as ``-P ``: + argv is world-readable through ``/proc//cmdline`` (``ps aux``), + so every local user on the host could read the BMC admin password + for as long as the call ran. ``-E`` makes ipmitool pick it up from + the ``IPMI_PASSWORD`` environment variable instead, which is only + readable by the process owner and root. See :meth:`_env`. + """ return [ "ipmitool", "-H", self._device.host, "-U", self._device.user, - "-P", self._device.password, + "-E", "-I", "lanplus", *extra, ] + def _env(self) -> dict[str, str]: + """Child environment carrying the BMC password out of band.""" + env = dict(os.environ) + env["IPMI_PASSWORD"] = self._device.password or "" + return env + async def _run(self, *extra: str) -> dict[str, Any]: argv = self._argv(*extra) try: @@ -41,6 +57,7 @@ async def _run(self, *extra: str) -> dict[str, Any]: *argv, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, + env=self._env(), ) stdout_b, stderr_b = await proc.communicate() except FileNotFoundError: diff --git a/src/beaconmcp/config.py b/src/beaconmcp/config.py index d6a12c4..14f1272 100644 --- a/src/beaconmcp/config.py +++ b/src/beaconmcp/config.py @@ -358,6 +358,11 @@ def _build(cls, raw: dict) -> Config: user=_required(d, "user", f"bmc.devices[{device_id}]"), password=_required(d, "password", f"bmc.devices[{device_id}]"), jump_host=d.get("jump_host"), + # Was declared on the dataclass but never read from the + # YAML, so `verify_tls: true` silently did nothing and + # every Redfish call went out with certificate + # verification disabled. Parse it for real. + verify_tls=_bool(d.get("verify_tls", False)), ) ) @@ -708,6 +713,7 @@ def mask(value: str) -> str: "user": d.user, "password": mask(d.password), "jump_host": d.jump_host, + "verify_tls": d.verify_tls, } for d in self.bmc_devices ], diff --git a/src/beaconmcp/dashboard/app.py b/src/beaconmcp/dashboard/app.py index a6fe530..97ffec2 100644 --- a/src/beaconmcp/dashboard/app.py +++ b/src/beaconmcp/dashboard/app.py @@ -485,12 +485,12 @@ async def overview_get(request: Request) -> Response: if not _bearer_live(deps, session): return RedirectResponse("/app/refresh?next=/app/overview", status_code=302) return _render( - request, "overview.html", - { - "client_name": deps.client_store.get_name(session.client_id) - or "Unknown", - }, + request, + client_name=( + deps.client_store.get_name(session.client_id) # type: ignore[attr-defined] + or "Unknown" + ), ) async def usage_get(request: Request) -> Response: @@ -500,12 +500,12 @@ async def usage_get(request: Request) -> Response: if not _bearer_live(deps, session): return RedirectResponse("/app/refresh?next=/app/usage", status_code=302) return _render( - request, "usage_cost.html", - { - "client_name": deps.client_store.get_name(session.client_id) - or "Unknown", - }, + request, + client_name=( + deps.client_store.get_name(session.client_id) # type: ignore[attr-defined] + or "Unknown" + ), ) async def tokens_get(request: Request) -> Response: diff --git a/src/beaconmcp/dashboard/chat.py b/src/beaconmcp/dashboard/chat.py index b03e19f..3e1db51 100644 --- a/src/beaconmcp/dashboard/chat.py +++ b/src/beaconmcp/dashboard/chat.py @@ -141,27 +141,57 @@ class UsageAccumulated: # Tool names that MUST go through a human approval step before we run -# them from a Gemini turn. Anything that can fire arbitrary shell on a -# host or VM (SSH directly, QEMU Guest Agent exec via proxmox_run) belongs -# here -- otherwise a single compromised/confused turn could rm -rf a -# production box. Legacy ``*_exec_command*`` names are kept for -# defense-in-depth in case an older MCP server is still wired up. -# Keep this list tight; every entry adds a modal click to the UX. +# them from a Gemini turn. Two classes belong here: +# +# 1. Anything that can fire arbitrary code on a host or VM -- SSH +# directly, QEMU Guest Agent exec via proxmox_run, and any primitive +# that lets an attacker *become* code execution (writing +# ~/.ssh/authorized_keys or /etc/cron.d via proxmox_write_file / +# proxmox_upload_file is exactly as good as a shell). +# 2. Anything destructive or irreversible at the guest/hardware level +# (mass power actions, snapshot rollback/delete, backup restore, +# BMC power cuts). +# +# The gate exists because the model's input is untrusted: a log line, a +# file, or a web-search result can carry an injected instruction. Without +# the modal, one confused turn rm -rf's a production box. +# +# Legacy ``*_exec_command*`` names are kept for defense-in-depth in case +# an older MCP server is still wired up. _NEEDS_CONFIRMATION: frozenset[str] = frozenset({ - # Current (unified) tools. + # --- arbitrary code execution ------------------------------------- "ssh_run", "proxmox_run", - # Large-file transfers: write into / read from a guest filesystem. + # Guest filesystem writes are code execution in one hop. + "proxmox_write_file", "proxmox_upload_file", "proxmox_download_file", "proxmox_delete_transfer", - # Legacy names (pre-unified tools) -- kept defensively. + # --- destructive / irreversible ----------------------------------- + "vm_bulk_action", + "proxmox_vm_stop", + "proxmox_vm_restart", + "proxmox_vm_migrate", + "proxmox_snapshot_rollback", + "proxmox_snapshot_delete", + "proxmox_backup_restore", + "bmc_power_off", + "bmc_power_reset", + # --- legacy names (pre-unified tools) ----------------------------- "ssh_exec_command", "ssh_exec_command_async", "proxmox_exec_command", "proxmox_exec_command_async", }) +# Tools that are read-only in one call shape and mutating in another. +# ``proxmox_vm_config`` returns the config without ``updates`` and rewrites +# it with them, so gating the whole tool would put a modal in front of +# every read. Map: tool name -> arg whose presence makes the call mutate. +_CONFIRM_WHEN_ARG_PRESENT: dict[str, str] = { + "proxmox_vm_config": "updates", +} + def _tool_call_requires_confirmation(name: str, args: Any) -> bool: """Return True when a tool call needs human approval before running. @@ -172,13 +202,22 @@ def _tool_call_requires_confirmation(name: str, args: Any) -> bool: is set and ``command`` is not -- is read-only and must not trigger a confirmation modal. We keep the allow-list name-based for everything else, then peel off the poll case here. + + ``dry_run=True`` calls are also let through: those tools return a + "would do X" string without touching the cluster. """ + mutating_arg = _CONFIRM_WHEN_ARG_PRESENT.get(name) + if mutating_arg is not None: + return bool(isinstance(args, dict) and args.get(mutating_arg)) if name not in _NEEDS_CONFIRMATION: return False - if name in {"ssh_run", "proxmox_run"} and isinstance(args, dict): - exec_id = args.get("exec_id") - command = args.get("command") - if exec_id and not command: + if isinstance(args, dict): + if name in {"ssh_run", "proxmox_run"}: + exec_id = args.get("exec_id") + command = args.get("command") + if exec_id and not command: + return False + if args.get("dry_run") is True: return False return True diff --git a/src/beaconmcp/dashboard/db.py b/src/beaconmcp/dashboard/db.py index 2278778..7607758 100644 --- a/src/beaconmcp/dashboard/db.py +++ b/src/beaconmcp/dashboard/db.py @@ -166,6 +166,30 @@ def __init__(self, path: Path | None = None) -> None: # Run migrations once, on the init thread. with self._connect() as conn: _migrate(conn) + self._restrict_permissions() + + def _restrict_permissions(self) -> None: + """Make the database owner-only (0600), like clients.json / tokens.db. + + This file holds live dashboard session rows: the AES-encrypted + client_secret *and* the MCP bearer token in plaintext. sqlite3 + creates it with the process umask (0644 on a default Debian box), + so any local user could read a bearer straight out of it and drive + the MCP API. WAL mode also produces ``-wal`` / ``-shm`` sidecars + that hold recently-written pages; they get the same treatment. + Best-effort: a chmod failure (e.g. a network filesystem) must not + stop the server from booting. + """ + for candidate in ( + self._path, + self._path.with_name(self._path.name + "-wal"), + self._path.with_name(self._path.name + "-shm"), + ): + try: + if candidate.exists(): + os.chmod(candidate, 0o600) + except OSError: + pass def _connect(self) -> sqlite3.Connection: conn = sqlite3.connect( @@ -177,6 +201,11 @@ def _connect(self) -> sqlite3.Connection: conn.execute("PRAGMA journal_mode = WAL") conn.execute("PRAGMA synchronous = NORMAL") conn.execute("PRAGMA foreign_keys = ON") + # Re-assert 0600 here too: enabling WAL (re-)creates the -wal/-shm + # sidecars with the process umask on every fresh connection, so a + # one-shot chmod in __init__ would miss the ones a later worker + # thread creates. + self._restrict_permissions() return conn def conn(self) -> sqlite3.Connection: diff --git a/src/beaconmcp/proxmox/client.py b/src/beaconmcp/proxmox/client.py index 2af32ac..da51c9a 100644 --- a/src/beaconmcp/proxmox/client.py +++ b/src/beaconmcp/proxmox/client.py @@ -1,6 +1,7 @@ from __future__ import annotations import logging +import re import threading import time from typing import Any @@ -12,6 +13,40 @@ _logger = logging.getLogger("beaconmcp.proxmox") +# Every Proxmox endpoint this server builds is an f-string with caller-supplied +# values spliced in as path segments (`.../snapshot/{snapname}/rollback`, +# `.../storage/{storage}/content`, ...). A value containing `/` or `..` would +# silently re-target the request at a different API endpoint than the tool +# advertises. Constrain each segment to the character set Proxmox itself +# allows for node names, storage ids, snapshot names and guest types. +# +# Deliberately excludes `/`, `..`, `.` and a leading `_` -- the latter because +# `api_call` walks the path with getattr() and proxmoxer's __getattr__ refuses +# (or, worse, resolves) dunder/private attribute names. +_SAFE_PATH_SEGMENT = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._@+-]*$") + + +class UnsafePathSegmentError(ValueError): + """Raised when a caller-supplied value would escape its path segment.""" + + +def _split_api_path(path: str) -> list[str]: + """Split an API path into segments, rejecting anything traversal-shaped. + + Raises :class:`UnsafePathSegmentError` with the offending segment so the + caller can surface an actionable message instead of issuing a request + against an endpoint nobody asked for. + """ + parts = [p for p in path.strip("/").split("/")] + for part in parts: + if not _SAFE_PATH_SEGMENT.match(part): + raise UnsafePathSegmentError( + f"illegal Proxmox API path segment {part!r} in {path!r}: " + "segments must match [A-Za-z0-9][A-Za-z0-9._@+-]* " + "(no slashes, no '..')" + ) + return parts + # Transient-error retry: Proxmox API over the wire frequently hiccups on # momentary network blips (TCP reset during cluster sync, TLS renegotiation # behind a reverse proxy, etc). One quick retry with a short backoff covers @@ -70,12 +105,18 @@ def api_call(self, node_name: str, method: str, path: str, **kwargs: Any) -> Any network errors get one quick retry; sustained unreachability returns the descriptive error message. """ + try: + parts = _split_api_path(path) + except UnsafePathSegmentError as e: + _logger.warning("rejected Proxmox API call on %s: %s", node_name, e) + return {"error": str(e)} + last_exc: Exception | None = None for attempt in range(_RETRY_ATTEMPTS): try: conn = self._get_connection(node_name) obj = conn - for part in path.strip("/").split("/"): + for part in parts: obj = getattr(obj, part) return getattr(obj, method)(**kwargs) except NodeNotFoundError: diff --git a/src/beaconmcp/wizard.py b/src/beaconmcp/wizard.py index 2a1015b..dbbc985 100644 --- a/src/beaconmcp/wizard.py +++ b/src/beaconmcp/wizard.py @@ -1565,6 +1565,14 @@ def _merge_env_placeholders(env_path: Path, names: list[str]) -> None: f.write("\n# Added by `beaconmcp init` — fill these in.\n") for name in to_add: f.write(f"{name}=\n") + # This file is where every BMC password, SSH password and Proxmox API + # token ends up. `open("a")` creates it with the process umask (0644 on + # a stock Debian box), so lock it down explicitly -- same treatment as + # clients.json / tokens.db. Best-effort: never block a save on chmod. + try: + os.chmod(env_path, 0o600) + except OSError: + pass # --------------------------------------------------------------------------- diff --git a/tests/test_bmc_ipmi.py b/tests/test_bmc_ipmi.py index 200c74b..8050f25 100644 --- a/tests/test_bmc_ipmi.py +++ b/tests/test_bmc_ipmi.py @@ -71,12 +71,30 @@ async def test_power_on_calls_ipmitool_with_correct_argv() -> None: assert argv[0] == "ipmitool" assert "-H" in argv and "10.0.0.11" in argv assert "-U" in argv and "admin" in argv - assert "-P" in argv and "pw" in argv assert argv[-3:] == ("chassis", "power", "on") assert result["action"] == "power_on" assert result["result"] == "success" +@pytest.mark.asyncio +async def test_password_never_appears_in_argv() -> None: + """The BMC password must travel via IPMI_PASSWORD, not `-P` on argv. + + argv is readable by every local user through /proc//cmdline. + """ + backend = _backend() + mock = AsyncMock(return_value=_FakeProc(b"Chassis Power is on\n")) + + with patch("asyncio.create_subprocess_exec", mock): + await backend.power_status() + + argv = mock.await_args.args + assert "-P" not in argv + assert "pw" not in argv + assert "-E" in argv + assert mock.await_args.kwargs["env"]["IPMI_PASSWORD"] == "pw" + + @pytest.mark.asyncio async def test_power_status_parses_on_off() -> None: backend = _backend() diff --git a/tests/test_security_hardening.py b/tests/test_security_hardening.py new file mode 100644 index 0000000..26403d5 --- /dev/null +++ b/tests/test_security_hardening.py @@ -0,0 +1,249 @@ +"""Regression tests for the security-audit hardening pass. + +Each test pins one specific weakness that was found and fixed, so a future +refactor that reopens it fails here rather than in production. +""" + +from __future__ import annotations + +import os +import stat +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent / "src")) + +from beaconmcp import audit +from beaconmcp.auth import is_trusted_redirect_uri +from beaconmcp.config import Config, ConfigError +from beaconmcp.dashboard.chat import _tool_call_requires_confirmation +from beaconmcp.dashboard.db import Database +from beaconmcp.proxmox.client import ProxmoxClient + + +# --- redirect_uri: loopback userinfo evasion -------------------------------- + + +@pytest.mark.parametrize( + "uri", + [ + # Real host is attacker.example; the string merely *starts with* + # a trusted loopback prefix. + "http://localhost:1@attacker.example/cb", + "http://127.0.0.1:8080@evil.example/callback", + "http://localhost@evil.example/cb", + "http://[::1]:80@evil.example/cb", + # Plain-HTTP host that is simply not loopback. + "http://evil.example/cb", + # An allowed HTTPS origin must not be reachable over plaintext http. + "http://assistant.ai/cb", + ], +) +def test_loopback_prefix_evasion_rejected(uri: str) -> None: + assert not is_trusted_redirect_uri(uri, ["https://assistant.ai"]), uri + + +@pytest.mark.parametrize( + "uri", + [ + "http://localhost:54321/callback", + "http://localhost/callback", + "http://127.0.0.1:3000/oauth/cb", + "http://[::1]:8080/cb", + "vscode://ms-vscode.remote/callback", + ], +) +def test_genuine_loopback_and_scheme_callbacks_still_accepted(uri: str) -> None: + assert is_trusted_redirect_uri(uri), uri + + +# --- Proxmox API path-segment injection ------------------------------------- + + +class _StubConfig: + pve_nodes: list = [] + verify_ssl = False + + def get_node(self, name): # pragma: no cover - never reached + raise AssertionError("connection must not be attempted") + + +@pytest.mark.parametrize( + "path", + [ + # A snapname of "../../../access/users" re-targets the request. + "nodes/pve1/qemu/100/snapshot/../../../access/users/rollback", + "nodes/pve1/storage/../../access/domains/content", + "nodes/pve1/qemu/100/snapshot//rollback", + "nodes/pve1/qemu/100/snapshot/_store/rollback", + ], +) +def test_traversal_path_segments_rejected_before_any_request(path: str) -> None: + client = ProxmoxClient(_StubConfig()) # type: ignore[arg-type] + result = client.get("pve1", path) + assert isinstance(result, dict) + assert "illegal Proxmox API path segment" in result["error"] + + +def test_legitimate_paths_are_not_rejected() -> None: + """The guard must not fire on the shapes real tools build.""" + from beaconmcp.proxmox.client import _split_api_path + + for path in ( + "nodes", + "version", + "nodes/pve-1/qemu/100/status/current", + "nodes/pve1/lxc/200/snapshot/pre-upgrade_2024/rollback", + "nodes/pve1/storage/local-lvm/content", + "nodes/pve1/storage/pbs.backup/content", + "nodes/pve1/qemu/100/agent/exec-status", + "nodes/pve1/vzdump", + ): + assert _split_api_path(path) + + +# --- chat: dangerous-tool confirmation gate --------------------------------- + + +@pytest.mark.parametrize( + "name,args", + [ + # Writing a guest file is code execution in one hop + # (~/.ssh/authorized_keys, /etc/cron.d/...). + ("proxmox_write_file", {"node": "pve1", "vmid": 100, "path": "/x", "content": "y"}), + ("proxmox_upload_file", {"source": "a", "dest": "/b"}), + ("proxmox_run", {"node": "pve1", "vmid": 100, "command": "id"}), + ("ssh_run", {"host": "pve1", "command": "id"}), + # Destructive / irreversible. + ("vm_bulk_action", {"vmids": [1, 2], "action": "stop"}), + ("proxmox_vm_stop", {"node": "pve1", "vmid": 100}), + ("proxmox_snapshot_rollback", {"node": "pve1", "vmid": 100, "snapname": "s"}), + ("proxmox_snapshot_delete", {"node": "pve1", "vmid": 100, "snapname": "s"}), + ("proxmox_backup_restore", {"node": "pve1", "vmid": 100, "archive": "a"}), + ("bmc_power_off", {"device_id": "rack1"}), + ("bmc_power_reset", {"device_id": "rack1"}), + # Read-or-write tool, in its writing shape. + ("proxmox_vm_config", {"node": "pve1", "vmid": 100, "updates": {"memory": 1}}), + ], +) +def test_dangerous_tools_require_confirmation(name: str, args: dict) -> None: + assert _tool_call_requires_confirmation(name, args), name + + +@pytest.mark.parametrize( + "name,args", + [ + # Pure reads. + ("proxmox_list_vms", {}), + ("proxmox_read_file", {"node": "pve1", "vmid": 100, "path": "/etc/hosts"}), + ("cluster_overview", {}), + ("bmc_power_status", {"device_id": "rack1"}), + # Read shape of the read-or-write tool. + ("proxmox_vm_config", {"node": "pve1", "vmid": 100}), + ("proxmox_vm_config", {"node": "pve1", "vmid": 100, "updates": None}), + # Polling an already-approved exec session is read-only. + ("proxmox_run", {"exec_id": "abc123"}), + ("ssh_run", {"exec_id": "abc123"}), + # dry_run tools only describe what they would do. + ("proxmox_snapshot_delete", {"node": "pve1", "vmid": 1, "snapname": "s", "dry_run": True}), + ], +) +def test_read_only_calls_do_not_require_confirmation(name: str, args: dict) -> None: + assert not _tool_call_requires_confirmation(name, args), name + + +# --- BMC verify_tls actually reaches the backend ---------------------------- + + +def _bmc_yaml(tmp_path: Path, verify_tls: str) -> Path: + path = tmp_path / "beaconmcp.yaml" + path.write_text( + "version: 1\n" + "bmc:\n" + " devices:\n" + " - id: rack1\n" + " type: redfish\n" + " host: 10.0.0.5\n" + " user: root\n" + " password: pw\n" + f" verify_tls: {verify_tls}\n" + ) + return path + + +def test_verify_tls_is_parsed_from_yaml(tmp_path: Path) -> None: + cfg = Config.load(config_path=_bmc_yaml(tmp_path, "true")) + assert cfg.bmc_devices[0].verify_tls is True + + cfg = Config.load(config_path=_bmc_yaml(tmp_path, "false")) + assert cfg.bmc_devices[0].verify_tls is False + + +def test_verify_tls_reaches_the_redfish_backend(tmp_path: Path) -> None: + from beaconmcp.bmc import build_registry + + cfg = Config.load(config_path=_bmc_yaml(tmp_path, "true")) + backend = build_registry(cfg)["rack1"] + assert backend._verify is True # type: ignore[attr-defined] + + +def test_verify_tls_defaults_to_false_when_absent(tmp_path: Path) -> None: + path = tmp_path / "beaconmcp.yaml" + path.write_text( + "version: 1\n" + "bmc:\n" + " devices:\n" + " - id: rack1\n" + " type: redfish\n" + " host: 10.0.0.5\n" + " user: root\n" + " password: pw\n" + ) + cfg = Config.load(config_path=path) + assert cfg.bmc_devices[0].verify_tls is False + + +# --- dashboard.db must not be world-readable -------------------------------- + + +def test_dashboard_db_is_owner_only(tmp_path: Path) -> None: + db_file = tmp_path / "dashboard.db" + Database(db_file) + mode = stat.S_IMODE(os.stat(db_file).st_mode) + assert mode & 0o077 == 0, f"dashboard.db is group/world accessible: {mode:o}" + for sidecar in ( + db_file.with_name(db_file.name + "-wal"), + db_file.with_name(db_file.name + "-shm"), + ): + if sidecar.exists(): + side_mode = stat.S_IMODE(os.stat(sidecar).st_mode) + assert side_mode & 0o077 == 0, f"{sidecar.name}: {side_mode:o}" + + +# --- audit redaction -------------------------------------------------------- + + +def test_audit_redacts_session_and_oauth_material() -> None: + redacted = audit._redact( + { + "session_id": "s3cr3t-session", + "code_verifier": "verifier", + "totp_secret": "JBSWY3DPEHPK3PXP", + "nested": {"access_token": "tok", "client_id": "beaconmcp_1"}, + "client_id": "beaconmcp_1", + } + ) + assert redacted["session_id"] == "***" + assert redacted["code_verifier"] == "***" + assert redacted["totp_secret"] == "***" + assert redacted["nested"]["access_token"] == "***" + # Non-secret identifiers stay readable -- the log is useless otherwise. + assert redacted["client_id"] == "beaconmcp_1" + assert redacted["nested"]["client_id"] == "beaconmcp_1" + + +def test_config_error_is_importable() -> None: + """Guard against the ConfigError import above going stale.""" + assert issubclass(ConfigError, Exception) From dd0cfd62392b9dda0469f5e6aa35119799a6da17 Mon Sep 17 00:00:00 2001 From: Showdown76py Date: Wed, 29 Jul 2026 04:48:13 +0200 Subject: [PATCH 2/3] fix(security): don't let a stray dry_run argument bypass the confirm gate The new dry_run escape in _tool_call_requires_confirmation was applied to every gated tool, but only the three snapshot tools declare the parameter. FastMCP validates arguments with a plain pydantic model, so extra='ignore' silently drops an undeclared dry_run server-side: ssh_run with command="rm -rf /" and dry_run=True skipped the modal and then ran for real. That is a bypass of the pre-existing ssh_run / proxmox_run gate, and it is reachable through exactly the injected instruction the gate exists to stop. Restrict the escape to _DRY_RUN_AWARE. Two tests pin it: the bypass shape on six tools that lack the parameter, and a check that every allow-listed tool really declares dry_run so the list cannot drift. Also skip the dashboard.db permission test on Windows, where os.chmod only toggles the read-only bit. --- src/beaconmcp/dashboard/chat.py | 26 ++++++++++++-- tests/test_security_hardening.py | 61 ++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 3 deletions(-) diff --git a/src/beaconmcp/dashboard/chat.py b/src/beaconmcp/dashboard/chat.py index 3e1db51..d57618f 100644 --- a/src/beaconmcp/dashboard/chat.py +++ b/src/beaconmcp/dashboard/chat.py @@ -192,6 +192,25 @@ class UsageAccumulated: "proxmox_vm_config": "updates", } +# Tools that actually implement a ``dry_run`` parameter and return a +# "would do X" string without touching the cluster. Only these may skip +# the modal on ``dry_run=True``. +# +# This list must stay exhaustive, and honouring dry_run cannot be assumed +# from the argument alone: FastMCP validates tool arguments with a plain +# pydantic model, so pydantic's default ``extra='ignore'`` silently drops +# any argument the tool does not declare. A trusting +# ``args.get("dry_run") is True`` check would therefore let +# ``ssh_run(command="rm -rf /", dry_run=True)`` past the modal and then +# run it for real, with the stray argument discarded server-side -- +# turning the gate into a one-word bypass for exactly the injected +# instruction it exists to stop. +_DRY_RUN_AWARE: frozenset[str] = frozenset({ + "proxmox_snapshot_create", + "proxmox_snapshot_rollback", + "proxmox_snapshot_delete", +}) + def _tool_call_requires_confirmation(name: str, args: Any) -> bool: """Return True when a tool call needs human approval before running. @@ -203,8 +222,9 @@ def _tool_call_requires_confirmation(name: str, args: Any) -> bool: confirmation modal. We keep the allow-list name-based for everything else, then peel off the poll case here. - ``dry_run=True`` calls are also let through: those tools return a - "would do X" string without touching the cluster. + ``dry_run=True`` is also let through, but only for the tools in + :data:`_DRY_RUN_AWARE` that declare the parameter -- see the note + there on why the argument alone is not evidence the tool honours it. """ mutating_arg = _CONFIRM_WHEN_ARG_PRESENT.get(name) if mutating_arg is not None: @@ -217,7 +237,7 @@ def _tool_call_requires_confirmation(name: str, args: Any) -> bool: command = args.get("command") if exec_id and not command: return False - if args.get("dry_run") is True: + if name in _DRY_RUN_AWARE and args.get("dry_run") is True: return False return True diff --git a/tests/test_security_hardening.py b/tests/test_security_hardening.py index 26403d5..cd40a4b 100644 --- a/tests/test_security_hardening.py +++ b/tests/test_security_hardening.py @@ -154,6 +154,63 @@ def test_read_only_calls_do_not_require_confirmation(name: str, args: dict) -> N assert not _tool_call_requires_confirmation(name, args), name +@pytest.mark.parametrize( + "name,args", + [ + ("ssh_run", {"host": "pve1", "command": "rm -rf /", "dry_run": True}), + ("proxmox_run", {"node": "pve1", "vmid": 100, "command": "id", "dry_run": True}), + ("proxmox_write_file", { + "node": "pve1", "vmid": 100, + "path": "/root/.ssh/authorized_keys", "content": "ssh-rsa ...", + "dry_run": True, + }), + ("vm_bulk_action", {"vmids": [1, 2], "action": "stop", "dry_run": True}), + ("bmc_power_off", {"device_id": "rack1", "dry_run": True}), + ("proxmox_backup_restore", { + "node": "pve1", "vmid": 100, "archive": "a", "dry_run": True, + }), + ], +) +def test_dry_run_does_not_bypass_gate_on_tools_lacking_it(name: str, args: dict) -> None: + """A stray ``dry_run`` must not talk a tool past the modal. + + None of these tools declare ``dry_run``, and FastMCP validates + arguments with a plain pydantic model, so the extra key is dropped + server-side and the call runs for real. Trusting the argument would + make the gate bypassable by adding one word to an injected + instruction. + """ + assert _tool_call_requires_confirmation(name, args), name + + +def test_dry_run_aware_tools_actually_declare_dry_run() -> None: + """Every entry in the allow-list must really implement ``dry_run``. + + Guards the list against drifting as tools are renamed or reworked: + an entry whose tool has lost the parameter would silently become a + bypass again. + """ + import inspect + + from beaconmcp.dashboard.chat import _DRY_RUN_AWARE + from beaconmcp.proxmox.vms import register_vm_tools + + registered: dict[str, object] = {} + + class _Recorder: + def tool(self, *_a: object, **_kw: object): + def deco(fn): + registered[fn.__name__] = fn + return fn + return deco + + register_vm_tools(_Recorder(), object()) # type: ignore[arg-type] + + assert _DRY_RUN_AWARE <= registered.keys() + for name in _DRY_RUN_AWARE: + assert "dry_run" in inspect.signature(registered[name]).parameters, name + + # --- BMC verify_tls actually reaches the backend ---------------------------- @@ -208,6 +265,10 @@ def test_verify_tls_defaults_to_false_when_absent(tmp_path: Path) -> None: # --- dashboard.db must not be world-readable -------------------------------- +@pytest.mark.skipif( + sys.platform == "win32", + reason="Windows os.chmod only toggles the read-only bit; POSIX modes are meaningless there", +) def test_dashboard_db_is_owner_only(tmp_path: Path) -> None: db_file = tmp_path / "dashboard.db" Database(db_file) From 6e56320d46a493eaee7fdf1aacd94d913cbd5e4f Mon Sep 17 00:00:00 2001 From: Showdown76py Date: Wed, 29 Jul 2026 04:53:02 +0200 Subject: [PATCH 3/3] ci: give the config job the RACK3_IDRAC_PASSWORD stub The example config gained a third BMC device to document verify_tls, and the config job enumerates the template's ${VAR} references explicitly, so validate-config rejected the unset one and the job went red. --- .github/workflows/ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bea9799..4e605ba 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -65,6 +65,7 @@ jobs: PVE2_TOKEN_SECRET: ci-dummy RACK1_ILO_PASSWORD: ci-dummy RACK2_IPMI_PASSWORD: ci-dummy + RACK3_IDRAC_PASSWORD: ci-dummy VPS2_PW: ci-dummy run: beaconmcp validate-config --config beaconmcp.yaml.example