Skip to content

fix(security): enforce execute_code sandbox tool isolation - #6614

Closed
WAXLYY wants to merge 1 commit into
NousResearch:mainfrom
WAXLYY:fix/execute-code-sandbox-isolation
Closed

WAXLYY wants to merge 1 commit into
NousResearch:mainfrom
WAXLYY:fix/execute-code-sandbox-isolation

Conversation

@WAXLYY

@WAXLYY WAXLYY commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix a sandbox tool-isolation bypass in execute_code.

Previously, when a session passed an explicit enabled_tools list that had no overlap with sandbox tools, Hermes silently fell back to granting all sandbox tools. In practice, this allowed a session with only execute_code enabled to regain capabilities such as terminal, file operations, and web access from inside the sandbox.

This change enforces the boundary consistently by introducing _resolve_sandbox_tools() and using it in both sandbox call paths.

What changed

  • added _resolve_sandbox_tools() in tools/code_execution_tool.py:77
  • switched both local and remote sandbox call sites to use it:
    • tools/code_execution_tool.py:715
    • tools/code_execution_tool.py:923
  • preserved backward-compatible fallback only when enabled_tools is None
  • prevented privilege expansion when enabled_tools is an explicit empty list or an explicit list with no overlap with sandbox tools
  • added regression coverage:
    • platform-independent boundary tests in tests/tools/test_code_execution.py:120
    • exploit-oriented regression test in tests/tools/test_code_execution.py:733

Why

This was a security boundary issue.

Before this fix, explicit tool restrictions could be bypassed inside the execute_code sandbox because non-overlapping tool lists were treated the same as “no restriction provided,” causing Hermes to re-enable all sandbox tools. That behavior violated caller intent and expanded privileges unexpectedly.

The new resolution logic distinguishes between:

  • enabled_tools is None → keep legacy fallback behavior
  • explicit empty list / explicit non-overlapping list → do not widen permissions

Security impact

High severity.

A session intended to expose only execute_code could previously regain access to:

  • terminal
  • read_file
  • write_file
  • web access

This PR closes that isolation bypass and makes the sandbox honor explicit tool constraints.

How to test

Ran:

python -m pytest tests\tools\test_code_execution.py::TestSandboxToolResolution -q -n0

4 passed

@trevorgordon981

Copy link
Copy Markdown
Contributor

This is a critical hardening measure. Without strict tool isolation within the execute_code sandbox, there's a risk of privilege escalation or unintended side-channel access to host resources.\n\nHave you considered adding explicit integration tests that attempt to access restricted tools (like terminal or memory) from within a spawned execute_code process? This would provide concrete validation that the isolation boundaries are holding.\n\nAlso, worth double-checking if this change impacts any existing workflows that rely on legacy behaviors—though in a security context, breaking changes are often necessary and expected.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P0 Critical — data loss, security, crash loop tool/code-exec execute_code sandbox labels Apr 29, 2026
@egilewski

Copy link
Copy Markdown
Contributor

Recommendation: needs rework/rebase before merge. The security invariant is valid, but the PR patch is stale against current upstream/main.

Checked current upstream/main as published by GitHub at 3c73d1852e372d1fe03dc5931d2f95be059caa67 and PR head 832d595c819e146636ff7d786e71cdbb86ebc73b.

Evidence:

  • Current main still reproduces the bypass: a focused execute_code(..., enabled_tools=["execute_code"]) probe can import terminal from hermes_tools and returns status=success.
  • PR head blocks the same probe with ImportError: cannot import name 'terminal' from 'hermes_tools'.
  • PR head focused regressions pass: python -m pytest -o addopts='' tests/tools/test_code_execution.py::TestSandboxToolResolution tests/tools/test_code_execution.py::TestExecuteCodeEdgeCases::test_execute_code_only_session_cannot_regain_terminal tests/tools/test_code_execution.py::TestExecuteCodeEdgeCases::test_empty_enabled_tools_blocks_all_sandbox_tools tests/tools/test_code_execution.py::TestExecuteCodeEdgeCases::test_nonoverlapping_tools_do_not_fallback -q -> 7 passed, 7 warnings.
  • Current-main legacy fallback tests still pass: test_empty_enabled_tools_uses_all and test_nonoverlapping_tools_fallback -> 2 passed, confirming the current behavior still widens access.
  • Integration check fails: git apply --check pr6614.patch against current main fails in both tools/code_execution_tool.py and tests/tools/test_code_execution.py, so this branch is not mergeable as-is.
  • CodeRabbit was run on the reconstructed original-base PR diff (coderabbit review --plain --base HEAD~1 --type committed) and reported no findings. coderabbit doctor still reports the known local storage failure for /home/mac/.coderabbit, but service/auth/network checks passed and the review completed.

Suggested next step: rebase/adapt the same _resolve_sandbox_tools() semantics to current tools/code_execution_tool.py line positions, preserving the enabled_tools is None legacy fallback while treating any explicit list as authoritative.

Signed: GPT-5.5-xhigh in Codex

@Nullstate-Studio

Copy link
Copy Markdown

Rebased this fix onto current main. See #47494 for the ready-to-merge version.

Changes from the original patch:

  • Rebased onto current main (original patch was stale)
  • list[str] | None union syntax (reviewer feedback)
  • Integration test assertions use self.assertTrue with clear messages instead of brittle string concatenation
  • Added test_execute_code_only_session_cannot_regain_terminal for the real policy case

All 7 tests pass. Credit to @WAXLYY for the original fix.

@teknium1

Copy link
Copy Markdown
Collaborator

The intention is not to sandbox it, so, this would be a regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P0 Critical — data loss, security, crash loop tool/code-exec execute_code sandbox type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants