From 8bf99227f0b107a58a03d48a520d596b609e5f54 Mon Sep 17 00:00:00 2001 From: xxxigm Date: Wed, 20 May 2026 20:00:03 +0700 Subject: [PATCH] fix(plugins): block plugin-api path traversal + project RCE (#29156) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GHSA-5qr3-c538-wm9j — half two of the bypass chain. ``_mount_plugin_api_routes`` imports each dashboard plugin's manifest ``api`` field as a Python module via ``importlib.util.spec_from_file_location`` — arbitrary code execution by design. Two primitives in the surrounding code turned that "by design" RCE into a usable attack: 1. Absolute paths in the manifest swallow the plugin directory. ``Path('safe/dashboard') / '/tmp/evil.py'`` resolves to ``/tmp/evil.py``, so a single manifest line ``{"api": "/tmp/payload.py"}`` was enough to redirect the importer at any Python file on disk. 2. ``..`` traversal in the manifest climbs out of the dashboard directory. ``Path('plugins/safe/dashboard') / '../../../tmp/evil.py'`` lands in ``/tmp/evil.py`` after ``resolve()`` — the static-asset handler (``serve_plugin_asset``) already defends against this via ``is_relative_to``; the api-mount path didn't. Fix at three layers so a regression in any one can't re-open the advisory: * New ``_safe_plugin_api_relpath`` validator runs at *discovery* time and stores only sanitised relative paths on the plugin entry's ``_api_file`` field. Absolute paths, ``..`` traversal, empty / non-string values, and paths that ``resolve()`` outside the plugin's ``dashboard/`` directory are rejected with a warning naming the plugin. ``has_api`` follows the sanitised value so the dashboard frontend doesn't render a fake "Backend API" badge for plugins whose api was scrubbed. * ``_mount_plugin_api_routes`` re-validates the resolved path against the live filesystem just before the import — defence in depth in case ``_dir`` is tampered with post-cache or a future caller bypasses the discovery-time validator. * Project plugins (``source == "project"``) are refused outright for backend import. ``./.hermes/plugins/`` ships with the CWD, so any threat model that includes "user opens a malicious repo" treats it as attacker-controlled; project plugins can still extend the UI via static JS/CSS but their Python ``api`` is no longer auto-imported. Combined with the truthy env-gate fix from the previous commit, the original advisory chain now fails at two distinct choke points. --- hermes_cli/web_server.py | 92 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 88 insertions(+), 4 deletions(-) diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 33a4f27fcfa..d5d319dda09 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -4047,6 +4047,43 @@ async def set_dashboard_theme(body: ThemeSetBody): # Dashboard plugin system # --------------------------------------------------------------------------- +def _safe_plugin_api_relpath(api_field: Any, *, dashboard_dir: Path) -> Optional[str]: + """Validate the manifest's ``api`` field for the plugin loader. + + The web server later imports this file as a Python module via + ``importlib.util.spec_from_file_location`` (arbitrary code + execution by design — that's how plugins extend the backend). + Pre-#29156 the field was used as-is, which meant: + + * An absolute path swallowed the plugin's dashboard directory + entirely — ``Path('safe/dashboard') / '/tmp/evil.py'`` resolves + to ``/tmp/evil.py``, so any attacker-controlled manifest could + point the import at any Python file on disk (GHSA-5qr3-c538-wm9j). + * A ``../..`` traversal could climb out of the plugin into + neighbouring directories on the search path. + + Return the original string when the resolved path stays under + ``dashboard_dir``; return ``None`` (with a warning logged at the + call site) otherwise so the plugin still loads its static JS/CSS + but its backend ``api`` is rejected. + """ + if not isinstance(api_field, str) or not api_field.strip(): + return None + candidate = Path(api_field) + if candidate.is_absolute(): + return None + try: + resolved = (dashboard_dir / candidate).resolve() + base = dashboard_dir.resolve() + except (OSError, RuntimeError): + return None + try: + resolved.relative_to(base) + except ValueError: + return None + return api_field + + def _discover_dashboard_plugins() -> list: """Scan plugins/*/dashboard/manifest.json for dashboard extensions. @@ -4113,6 +4150,23 @@ def _discover_dashboard_plugins() -> list: slots: List[str] = [] if isinstance(slots_src, list): slots = [s for s in slots_src if isinstance(s, str) and s] + # Validate ``api`` at discovery time so the value cached + # on the plugin entry is already safe to feed into the + # importer. An attacker-controlled manifest can name + # any absolute path or ``..`` traversal here — the + # web server then imports that file as a Python module + # (RCE, GHSA-5qr3-c538-wm9j). + raw_api = data.get("api") + dashboard_dir = child / "dashboard" + safe_api = _safe_plugin_api_relpath(raw_api, dashboard_dir=dashboard_dir) + if raw_api and safe_api is None: + _log.warning( + "Plugin %s: refusing unsafe api path %r (must be a " + "relative file inside the plugin's dashboard/ " + "directory); backend routes from this plugin will " + "not be mounted", + name, raw_api, + ) plugins.append({ "name": name, "label": data.get("label", name), @@ -4123,10 +4177,10 @@ def _discover_dashboard_plugins() -> list: "slots": slots, "entry": data.get("entry", "dist/index.js"), "css": data.get("css"), - "has_api": bool(data.get("api")), + "has_api": bool(safe_api), "source": source, - "_dir": str(child / "dashboard"), - "_api_file": data.get("api"), + "_dir": str(dashboard_dir), + "_api_file": safe_api, }) except Exception as exc: _log.warning("Bad dashboard plugin manifest %s: %s", manifest_file, exc) @@ -4481,12 +4535,42 @@ def _mount_plugin_api_routes(): Each plugin's ``api`` field points to a Python file that must expose a ``router`` (FastAPI APIRouter). Routes are mounted under ``/api/plugins//``. + + Backend import is restricted to ``bundled`` and ``user`` sources. + Project plugins (``./.hermes/plugins/``) ship with the CWD and are + therefore attacker-controlled in any threat model where the user + opens a malicious repo; they can extend the dashboard UI via + static JS/CSS but their Python ``api`` file is never auto-imported + by the web server. See GHSA-5qr3-c538-wm9j (#29156). """ for plugin in _get_dashboard_plugins(): api_file_name = plugin.get("_api_file") if not api_file_name: continue - api_path = Path(plugin["_dir"]) / api_file_name + if plugin.get("source") == "project": + _log.warning( + "Plugin %s: ignoring backend api=%s (project plugins may " + "not auto-import Python code; move the plugin to " + "~/.hermes/plugins/ if you trust it)", + plugin["name"], api_file_name, + ) + continue + dashboard_dir = Path(plugin["_dir"]) + api_path = dashboard_dir / api_file_name + try: + resolved_api = api_path.resolve() + resolved_base = dashboard_dir.resolve() + resolved_api.relative_to(resolved_base) + except (OSError, RuntimeError, ValueError): + # Discovery already filters this, but re-check here in case + # ``_dir`` was tampered with after caching or a future caller + # bypasses the validator. Defence in depth keeps the import + # primitive contained even if the upstream check regresses. + _log.warning( + "Plugin %s: refusing to import api file outside its " + "dashboard directory (%s)", plugin["name"], api_path, + ) + continue if not api_path.exists(): _log.warning("Plugin %s declares api=%s but file not found", plugin["name"], api_file_name) continue