fix: restrict PluginContext for user-loaded plugins - #34053
ErnestHysa wants to merge 2 commits into
Conversation
Before: skin name from config was concatenated directly into path without validation, allowing ../ traversal to read arbitrary files After: skin name is validated against allowlist [a-zA-Z0-9_-] before path construction; symlinks are resolved via realpath and verified to stay within skins directory Impact: prevents arbitrary file read via crafted skin names in config
- Before: all plugins (bundled and user) received full PluginContext with LLM facade, credential pool access, and all tools - After: user-sourced plugins receive a restricted PluginContext that excludes credential pool access and limits LLM facade to read-only metadata - Impact: reduces privilege escalation surface if a user installs a malicious plugin; bundled plugins retain full context
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the security-focused contribution. The skin-path issue is worth preserving, but the PluginContext change does not create the claimed isolation boundary.
Problems
hermes_cli/plugins.py:1533only chooses a facade after arbitrary plugin code has already been imported. On current main,_load_plugin()imports the user directory module athermes_cli/plugins.py:1762before callingregister()at:1775; an enabled plugin therefore retains ordinary process capabilities regardless ofctx. This also removes documentedctx.llmandctx.dispatch_toolcapabilities from user plugins without providing sandboxing.hermes_cli/skin_engine.py:762uses a string-prefix containment check. A symlink resolving to a sibling such asskins_evil/...can passstartswith()while escaping the skins directory.- The diff has no tests for either security path.
Suggested changes
- Split and test the skin fix, using path-aware containment such as
Path.is_relative_to()after resolution. - Rework the plugin proposal around a real execution-isolation boundary, rather than treating a Python facade as one.
Automated hermes-sweeper review.
| logger.warning("Plugin '%s' has no register() function", manifest.name) | ||
| else: | ||
| ctx = PluginContext(manifest, self) | ||
| if manifest.source == "user": |
There was a problem hiding this comment.
This facade is selected only after _load_directory_module() has imported arbitrary user Python. Current main imports at hermes_cli/plugins.py:1762 before calling register() at :1775, so an enabled plugin can use ordinary process capabilities regardless of ctx. This cannot provide the claimed security boundary and breaks documented user-plugin APIs without actual execution isolation.
| skins_path = _skins_dir() | ||
| user_file = skins_path / f"{name}.yaml" | ||
| resolved_key_path = user_file.resolve() | ||
| if not str(resolved_key_path).startswith(str(skins_path.resolve())): |
There was a problem hiding this comment.
startswith() is not a path-containment check: a skin symlink resolving to a sibling directory such as skins_evil/... passes this prefix test while escaping skins_path. Use resolved-path containment (is_relative_to() or relative_to() with ValueError) and add a symlink regression test.
Summary
Restricts the
PluginContexthanded to user-installed plugins (~/.hermes/plugins/) by introducing aRestrictedPluginContextthat excludes credential pool access, the full LLM facade, and arbitrary tool dispatch.Before
All plugins — whether bundled with the repository or installed by the user to
~/.hermes/plugins/— received a fullPluginContextwith:ctx.llm— LLM facade with the user's API credentials and auth tokensctx.register_tool(override=True)— ability to replace any built-in toolctx.inject_message()— inject arbitrary content into active conversationctx.dispatch_tool()— call any registered tool directlyA user who installs a malicious plugin to their own
~/.hermes/plugins/directory could exfiltrate API keys, manipulate conversation history, or replace built-in tools with malicious versions.After
A new
RestrictedPluginContextclass is introduced for user-sourced plugins (manifest.source == "user"). Bundled plugins (source="bundled") continue to receive the fullPluginContext.RestrictedPluginContextprovides:register_tool()— tool registration (for legitimate plugin functionality)inject_message()— message injectionregister_cli_command()/register_command()— command registrationRestrictedPluginContextomits:llmproperty — returnsNone; no LLM facade accessdispatch_tool()— no arbitrary tool dispatchregister_context_engine,register_image_gen_provider,register_tts_provider, etc.)Impact
~/.hermes/plugins/had full access to all API keys, could make LLM calls on the user's behalf, and could replace any built-in tool