mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-29 06:31:32 +00:00
fix(plugins): block plugin-api path traversal + project RCE (#29156)
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.
This commit is contained in:
xxxigm
Teknium
parent
da636e982b
commit
8bf99227f0
1 changed files with 88 additions and 4 deletions
|
|
@ -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/<name>/``.
|
||||
|
||||
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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue