fix(security): sandbox tool isolation bypass — distinguish None from explicit empty/non-overlapping enabled_tools - #47494
Conversation
Distinguish None (legacy fallback) from explicit empty/non-overlapping enabled_tools list. The old `if not sandbox_tools` widened access when the intersection was empty, letting sessions restricted to ["execute_code"] silently regain terminal, file ops, and web access. Co-authored-by: WAXLYY <158675678+WAXLYY@users.noreply.github.com>
|
Related: #6614 (original fix by @WAXLYY). This PR is the rebased, more-complete version — it adds a |
egilewski
left a comment
There was a problem hiding this comment.
Recommendation: approve
I reviewed this in security mode against current GitHub main 36ae958473b8530ffb1a395c4944b8cdbcae82fe, PR base 36ae958473b8530ffb1a395c4944b8cdbcae82fe, and PR head eb896e41072a47a79a66361cada00a090c66b59a.
Validation:
git merge-tree --write-tree refs/remotes/upstream/main refs/remotes/upstream/pr/47494: passed, producing treede145588792f96b6046556e7753a11d32c0e173c.- Current-main execute-code-only probe with
enabled_tools=["execute_code"]: reproduced the sandbox widening;from hermes_tools import terminalsucceeded and printedterminal regained. - PR-head execute-code-only probe with
enabled_tools=["execute_code"]: passed;from hermes_tools import terminalfailed withImportError, so an execute-code-only session no longer regains terminal access. - PR head
pytest -p no:cacheprovider tests/tools/test_code_execution.py -k 'SandboxToolResolution or none_enabled_tools_uses_all or empty_enabled_tools_blocks_all_sandbox_tools or nonoverlapping_tools_do_not_fallback or execute_code_only_session_cannot_regain_terminal' -q:8 passed. - PR head
pytest -p no:cacheprovider tests/tools/test_code_execution.py -q:73 passed. - CodeRabbit clean-pass review completed. Its only finding was a Python 3.8/3.9 type-syntax compatibility concern for
list[str] | None; I checkedpyproject.toml, and Hermes declaresrequires-python = ">=3.11,<3.14", so that is not a blocker.
I did not find a blocker in the reviewed scope. The PR preserves the legacy enabled_tools is None fallback while making explicit empty or non-overlapping session tool lists authoritative on both local and remote execute-code paths, and the tests cover the security boundary directly.
Signed: GPT-5.5-xhigh in Codex
|
The intention is not to sandbox it, so, this would be a regression. |
Summary
Closes the sandbox tool isolation bypass described in #6614.
Bug
tools/code_execution_tool.py— both_execute_remoteandexecute_codehad:if not sandbox_toolsfires whenenabled_toolsis falsy (None,[]) OR when the intersection is empty. A session restricted to["execute_code"]silently regains terminal, file ops, and web access inside the sandbox.Fix
Adds
_resolve_sandbox_tools()that distinguishesNone(legacy fallback) from an explicit list (authoritative, no widening). Replaces both call sites.Tests
4 unit tests for
_resolve_sandbox_tools+ 3 integration tests verifying the behavior change end-to-end. All 7 pass.Credit
Original fix by @WAXLYY. This is a rebase onto current
mainwith the reviewer's feedback incorporated.