security: an allowlisted reader must not auto-run outside the granted roots - #310
Open
Saidheerajgollu wants to merge 1 commit into
Open
security: an allowlisted reader must not auto-run outside the granted roots#310Saidheerajgollu wants to merge 1 commit into
Saidheerajgollu wants to merge 1 commit into
Conversation
… roots
`read_file` refuses to leave the workspace - `target.relative_to(root)`, else "path
escapes the workspace" - and it refuses traversal too. `run_shell` applies no such
check, so with `cat` on the allowlist the same bytes come back with no approval prompt:
read_file(path="/Users/me/.ssh/id_rsa") -> {"error": "path escapes the workspace"}
cat /Users/me/.ssh/id_rsa -> auto-runs, contents returned
Being on the allowlist says the PROGRAM is safe to run unattended. It says nothing
about where that program is pointed, and today nothing else does either: `evaluate`
path-scopes WRITE_LOCAL by inspecting `arguments["path"]`, which the shell does not
have. The engine already carries the right helper for this - `_under_root`, resolving
against every granted root - but nothing has ever called it. This wires it up.
An allowlisted command is checked for argument tokens that name a location, and
auto-run is withheld if any of them resolves outside every granted root. Withheld, not
denied: the command still runs once the user approves it, so the cost of a false
positive is one prompt.
Only tokens that genuinely look like paths are considered, so patterns and subcommands
(`'*.py'`, `status`, `install`) are ignored and ordinary use is untouched: relative
paths resolve against the workspace and stay inside it unless they climb out with `..`.
`_under_root` resolves symlinks, so a link planted in the workspace cannot launder a
target outside it, and `--file=/etc/passwd` is read off the right of the `=` rather
than skipped as a flag.
Closes the shell hole in the same class as andrewyng#189 and andrewyng#293 (tools reaching outside the
session roots), for the tool with the widest reach.
21 cases: reads outside the roots, the symlink and `--opt=value` shapes, reads inside
the workspace and inside a second granted root, and patterns/subcommands that must not
be mistaken for paths.
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.
Fixes #309.
Summary
read_filerefuses to read outside the granted roots.run_shelldoes not, so an allowlistedcatreturns exactly what the file tool declines to hand over, with no approval prompt:Being on the allowlist says the program is safe to run unattended. It says nothing about where it is pointed, and nothing else covers that either:
evaluatepath-scopesWRITE_LOCALby inspectingarguments["path"], which the shell has no equivalent of.This withholds auto-run when an allowlisted command names a path outside every granted root. Withheld, not denied - the command still runs once the user approves it, so a false positive costs one prompt.
Approach
The engine already has the right helper.
_under_rootresolves a path against every granted root, not just the workspace:rg '_under_root\b'returns one hit - the definition. It has never been called; only_under_writable_rootis wired up, for writes. This wires the read side up rather than introducing a parallel notion of confinement.Same boundary as
read_file, and the same one #289 is applying to the browser tools andemail_toolsalready applies to outgoing attachments.Keeping the false-positive rate at zero
The check only looks at argument tokens that genuinely name a location, which is what keeps ordinary use intact:
'*.py',status,install,HEAD~1are ignored.._under_rootresolves symlinks, so a link planted in the workspace cannot launder a target outside it--file=/etc/passwdis read off the right of the=, rather than skipped as a flagEscapes covered:
/etc/passwd,~/.ssh/id_rsa,../../../etc/passwd,head -n 5 /etc/hosts,wc -l /etc/passwd,grep -r AKIA /Users,ls /,pytest --collect-only /tmp/evil_test.py, an absolute path to a directory outside the roots, the in-workspace symlink, andgrep --file=/etc/passwd.Unaffected:
ls -la,pwd,git status -s,git log --oneline -5,git diff --stat,grep -n TODO README.md,grep -rn TODO src/,find . -name '*.py',find . -type f -maxdepth 2,cat README.md,cat ./src/app.py,wc -l src/app.py,pytest -q, an absolute path inside the workspace, and reads inside a second granted root.Scope
Auto-run only. Nothing here changes what the user can approve, what
automode does, or what happens once a command is insession_allow_commands. The added condition sits on one branch ofevaluateand falls through to the existing approval path, so no other decision route changes.I kept the decision reason generic rather than returning a specific "outside the session directories" string, to keep the diff to one condition. Happy to surface the offending path in the reason if you'd rather the approval card explain itself - it is a two-line change.
Relationship to #307 / #308
Independent, and deliberately on a separate branch off
mainso they can merge in either order. #308 is about which program may run unattended; this is about where it may be pointed. Different axis, no overlap in the diff.They do compose: #308 notes that
docs/config.example.tomlrecommendscat,grep,findandls, and that a trusted workspace can append toallowed_commands. This change is what makes that allowlist safe to hold readers at all.It also closes the one case I explicitly carved out of #307 as out of scope -
pytest /tmp/evil_test.py- because that escape is a path, and this is the axis that can express it.Test plan
tests/test_permissions_risk.py, alongside the existing allowlist tests: reads outside the roots (parametrized over 9 shapes), the symlink case, reads inside the workspace and inside a second granted root (parametrized over 10), and absolute paths inside a rootpytest tests/test_permissions_risk.py- 51 passed, no existing test modifiedtests/test_fake_slack.pyfailures andtest_bedrock_provider.pyare pre-existing onmain(missingmessaging/bedrockextras, Telegram Connector Issue #257 and setup_dev_env.sh misses the bedrock extra — fresh dev env fails 7 tests out of the box #284) and unrelated.