Repository navigation
fix: stop scenario metadata from reaching shells and host commands - #155
adamsrnmsu wants to merge 1 commit into
Conversation
|
@adamsrnmsu Please separate your changes into 3 PRs. You opened 3 PRs, with the same changes in all 3, lol. One commit per PR my guy. |
98ef201 to
78726f9
Compare
|
@GhostofGoes Split as requested. #153, #154 and #155 are now each one commit on top of |
|
Glad someone is doing a security pass to clean up the pile of years of exec sins. Please ensure you test this PR. |
78726f9 to
7398d5b
Compare
Several apps and SCORCH components splice scenario metadata into shell strings, minimega and ovs-vsctl commands, or run scenario-supplied scripts directly on the phenix host. - utils.run_command() executes an argument vector instead of a shell string; a str is shlex-split. collector, pcap, mm, tcpdump, trim_pcap() and pcap_capinfos() pass argument lists, and tshark output is redirected from Python rather than through 'bash -c ... >'. - Values that end up in ovs-vsctl or minimega commands are validated as single tokens: the mirror app's bridge, VLAN alias and HIL interfaces (Go), the mgmt_tap bridge, erspan bridges, interfaces, IPs, session key and excluded VLANs, tcpdump interfaces, mm capture filenames and filters, and the experiment and compute names in mm_compute_cmd(). - Validators for the art and cc components and the pipe 'via' program run on the phenix host, not in a VM, so they now need an explicit opt-in: PHENIX_SCORCH_HOST_VALIDATORS=1, read once in common/settings.py. Without it validators are skipped with a warning and a via fails the component. This is a breaking default. - The kafka PID file leaves world-writable /tmp for the experiment's SCORCH files directory (still independent of loop and count, so configure keeps detecting a running listener), and cleanup only kills the PID if it is still a kafka listener. - The ssh component takes an optional known_hosts file and rejects unknown host keys when one is given; without it, it warns before trusting the key. Documented in the affected READMEs, AGENTS.md and the changelog; Go and Python tests cover the new validation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016KAfcDUSerQCCWwBM9BxAo
7398d5b to
4618094
Compare
| ### Security | ||
| - **Common**: `run_command()` no longer uses a shell; `mm_compute_cmd()` rejects multi-token names. | ||
| - **Mirror, mgmt_tap, SCORCH**: Metadata is validated before reaching `ovs-vsctl`, minimega or `tshark`; commands run as argument vectors, not `bash -c`. | ||
| - **SCORCH**: kafka's PID file leaves `/tmp`; ssh takes optional `known_hosts`. |
There was a problem hiding this comment.
"Leaves /tmp"... where did it go? And did it take the kids with it?
| If no validator is provided then it is assumed the test succeeded if the atomic | ||
| executor exits cleanly. | ||
|
|
||
| > [!IMPORTANT] |
There was a problem hiding this comment.
I think this should be enabled by default to avoid breaking existing environments, with the ability to lock things down more in environments where the security is needed (and there's less trust)
|
|
||
| tempfile = f"/tmp/{uuid.uuid4()!s}.sh" | ||
| with open(tempfile, "w") as tf: | ||
| tempfile = Path(f"/tmp/{uuid.uuid4()!s}.sh") |
There was a problem hiding this comment.
Isn't there a module in python standard library for secure temporary file creation?
Description
Keep scenario metadata out of shells and host-side commands. Several components splice metadata into shell strings or into
ovs-vsctland minimega commands, and some run scenario-supplied scripts directly on the phenix host.utils.run_command()runs an argument vector; a string isshlex-split. collector, pcap, mm, tcpdump,trim_pcap()andpcap_capinfos()pass lists, and tshark output is redirected from Python instead of throughbash -c '... > file'. The remaining string callers (tshark-Y "...", mergecap, editcap,phenix version) use no shell features and split the same way.ovs-vsctlor minimega: the mirror app's bridge, VLAN alias and HIL interfaces (Go), the mgmt_tap bridge, erspan bridges, interfaces, IPs, session key and excluded VLANs, tcpdump interfaces, mm capture filenames and filters, and the experiment and compute names inmm_compute_cmd().artandccvalidators and thepipeviaprogram run on the phenix host, not in a VM, so they now requirePHENIX_SCORCH_HOST_VALIDATORS=1, read once incommon/settings.py. Without it, validators are skipped with a warning and aviafails the component. Documented in the art, cc and pipe READMEs and in the AGENTS.md environment-variable table./tmp, andcleanuponly kills the PID if/proc/<pid>/cmdlinestill shows a kafka listener.known_hostsmetadata field. When set, unknown host keys are rejected; when absent, the component keeps trusting the key but logs a warning.Related Issues/PRs
One of four independent PRs: #152, #153, #154. Each is a single commit on
mainand they can merge in any order. #154 also adds a### Securitysection toCHANGELOG.md, so whichever of the two merges second needs a small rebase. No other files conflict.Type of Change
fix)Checklist
Testing
make checkandmake testinsrc/pythonon Python 3.12: 708 passed on this branch alone, and 743 with all four PRs merged together. New tests incommon/tests/test_command_helpers.pycovermm_compute_cmdtoken validation and confirm thatrun_command("echo 'a; touch /tmp/x' *")reachesechowithout shell expansion.golangci-lint run(v2.11.3, the pinned version) reports 0 issues, andgo test -race ./...passes. The newutil_test.gocoversvalidateOVSTokenand the metadata extraction paths.Additional Notes
This series started as a single hardening patch. Where it departs from that patch:
base_dir, which is per loop and count.configureskips when the PID file already exists, so a per-loop path would let every loop start another listener. It now lives in the experiment'sfiles_dir/scorch/: still loop-independent, but no longer in/tmp.util.gofailed the repo's golangci-lint (golines,noinlineerr). It is reformatted, with the error checks moved out of theifstatements.An open question for reviewers: with the env var unset,
artandccskip a configured validator with only a warning, so a result that would have failed validation now reads as passed. Failing the component instead, aspipedoes forvia, would be stricter and closer to the "don't silently recover" rule in AGENTS.md. This PR keeps the warning; happy to switch.🤖 Generated with Claude Code
https://claude.ai/code/session_016KAfcDUSerQCCWwBM9BxAo