fix(sandbox): block .. escapes when shell destinations do not exist - #66
Draft
cursor[bot] wants to merge 1 commit into
Draft
fix(sandbox): block .. escapes when shell destinations do not exist#66cursor[bot] wants to merge 1 commit into
cursor[bot] wants to merge 1 commit into
Conversation
Lexical prefix matching treated workspace/../../tmp/newfile as inside the workspace whenever realpath failed. sandbox_exec does not pivot_root, so that was a real filesystem escape. Walk to the first existing ancestor instead, matching tools/file.c. Co-authored-by: esadrianno <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug and impact
With sandbox
workspace_onlyenabled (the documented default), a shell command whose destination does not exist yet was allowed if the string started with the workspace path. That lets the agent copy the SQLite store (sessions, memories, cron) out of the workspace, or write files elsewhere the process can write:sandbox_execonlyunshares namespaces andchdirs; it does notpivot_root. The allowlist is the filesystem gate.Root cause
allowlist_path_is_under_workspaceusedrealpathwhen the path existed, then fell back to a lexical prefix check.realpathfails for a missing file, soworkspace/../../tmp/newfilematched the workspace prefix and was allowed. Existing destinations still went throughrealpathand were denied.The file tool already walks to the first existing ancestor. The shell allowlist did not.
Fix
When
realpathfails, walk parents withdirnameuntil an existing directory resolves, then apply the workspace prefix check to that resolved ancestor. New files inside the workspace stay allowed.Validation
tests/test_allowlist.cagainst the patched allowlist (this environment has no libcurl, somake test_allowlistcannot link the default-lcurl).test_allowlist: all tests passed, including a new case that previously failed:workspace/../../tmp/sc_al_stolen_<pid>is blocked, whileworkspace/brand_new.txtis still allowed.