fix(security): gate deregister() with the same plugin opt-in policy as register() - #55840
fix(security): gate deregister() with the same plugin opt-in policy as register()#55840srojk34 wants to merge 2 commits into
Conversation
alt-glitch
added
type/security
|
suggesting changes The patch blocks the unowned built-in deregister bypass, but it also blocks a plugin from cleaning up a tool it owns when the handler is defined in one plugin submodule and cleanup runs from the plugin root module. I reproduced that on PR head with a run-root probe: register Security evidence:
Signed: GPT-5.5-xhigh in Codex |
…to package A plugin could bypass the register(override=True) authorization chain by calling registry.deregister(name) to clear the existing entry, then calling plain registry.register() over the now-empty slot — register() only runs its override check when an existing entry is present, so removing the entry first skips the check entirely (NousResearch#55599 sibling gap). Ownership check binds to the plugin package root (hermes_plugins.{name}), not the exact leaf module string: hermes_plugins.pkg (root cleanup code) is allowed to deregister a tool whose handler was defined in hermes_plugins.pkg.handlers — they share the same package. Exact module equality would have blocked same-plugin self-cleanup (egilewski review). MCP toolsets (mcp-*) are exempt — dynamic tool refresh legitimately nukes and repaves its own tools on every server refresh.
f09273f to
ddc062f
Compare
|
suggesting changes The patch fixes the unowned deregister bypass and the same-plugin root-vs-submodule cleanup case, but it still misses the plugin package boundary for operator opt-in: A focused PR-head probe registered I verified current main is vulnerable to the original deregister-to-register bypass, PR head blocks unowned deregister without opt-in, same-plugin submodule cleanup is covered for plugin-owned tools, and Signed: GPT-5.5-xhigh in Codex |
…ookup
_plugin_override_policy is keyed by the plugin package root
(e.g. hermes_plugins.allowed), but the lookup used caller_mod
(the exact leaf module string). A call from hermes_plugins.allowed.cleanup
would evaluate _plugin_override_policy.get("hermes_plugins.allowed.cleanup")
→ False and raise PermissionError even when the plugin registered opt-in
under its package root.
Switch the policy lookup to caller_root (.join of the first two segments)
so submodule callers inherit the package-level allow_tool_override grant.
Adds a focused regression test for the opted-in submodule case.
|
fully addressed The new head fixes the remaining package-boundary issue from my last pass. I rechecked the original unowned deregister/register bypass against current main, the rejected unowned removal path, same-plugin submodule cleanup, operator opt-in from a plugin submodule, MCP dynamic deregistration coverage, focused registry tests, merge-tree compatibility with current main, and CodeRabbit's PR-scoped registry finding; I do not see a remaining high-confidence security blocker in this PR. Signed: GPT-5.5-xhigh in Codex |
|
Thanks @srojk34 — this is a real, well-scoped fix and it's landing. Your branch was ~253 commits behind Verified before landing: 41/41 One follow-up worth noting (tracked separately, not a knock on this PR): the module-name/frame identity both gates use is forgeable by actively-hostile plugin code — so this is an operator-consent + auditability guard against silent built-in shadowing, at the project's established plugin-trust level, rather than a hostile-plugin sandbox. Upgrading both Closing this in favor of #56261. Credit is preserved in the git history there. Thanks again! |
4 participants
Summary
ToolRegistry.deregister()had zero authorization: any plugin could silently remove a built-in tool's entry and then re-register over the empty slot with a malicious handler, completely bypassing the three-commitregister(override=True)opt-in chain (179eb8c → 3101222 → 12f5624 / fix(security): operator opt-in for plugin tool_override (sink-enforced) + enable-time consent #55599).register()only runs the override-policy check when anexistingentry is present — clearing the entry first causes the check to be skipped entirely, with noPermissionErrorand no audit log.register()hardening series left unaddressed.Fix
Before removing a non-MCP entry,
deregister()now inspects the calling module via_caller_module()(frame-based, matching_plugin_owner_of()'s bind-to-definition-site philosophy). If the caller is in thehermes_plugins.*namespace, does not own the entry's handler, and lacks operator opt-in, aPermissionErroris raised and the entry is left intact.MCP toolsets (
mcp-*) are exempt — dynamic tool refresh legitimately nukes-and-repaves its own tools and has no plugin-override concept.Test plan
test_plugin_cannot_deregister_unowned_tool_without_opt_in— core blocked casetest_plugin_with_opt_in_can_deregister_unowned_tool— operator opt-in allows ittest_plugin_can_deregister_its_own_tool— self-cleanup always allowedtest_mcp_toolset_always_deregisterable— MCP refresh exempttest_core_code_deregister_always_allowed— internal Hermes code never gatedtest_full_bypass_blocked— the full deregister→register chain is now rejected end-to-endpytest tests/tools/test_registry.py→ 39 passed, 0 failed