Stop ps from exposing process argv#269
Conversation
|
|
@codex review this PR Please use the following severity format for all findings:
Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc. |
|
Iteration 1 self-review result: 0 P0 findings, 0 P1 findings, 0 P2 findings, 0 P3 findings. Summary: reviewed the ps argv leak mitigation across Linux, macOS, and Windows. The implementation now uses process comm/executable names only, removes argv readers, updates documentation, and includes regression coverage for secret-bearing cmdline data. |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the PR diff for the ps argv leak mitigation.
Overall assessment: safe to merge.
| # | Priority | File | Finding |
|---|---|---|---|
| — | — | — | No findings |
Coverage Summary
| Code path | Test | Status |
|---|---|---|
| Linux ps ignores /proc//cmdline argv and reports stat comm | builtins/ps/ps_procpath_linux_test.go | Covered |
| Linux ps handles missing cmdline using unbracketed comm | builtins/ps/ps_procpath_linux_test.go | Covered |
| macOS kinfo conversion uses P_comm only | builtins/internal/procinfo/procinfo_darwin_test.go | Covered |
| Windows process entry conversion uses executable name only | builtins/internal/procinfo/procinfo_windows_test.go | Covered |
| Docs and feature metadata describe the argv-safe behavior | README.md, SHELL_FEATURES.md, builtins/features.go | Covered by review |
P0/P1/P2 findings: 0
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review this PR Please use the following severity format for all findings:
Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc. |
|
Iteration 1 self-review result: 0 P0 findings, 0 P1 findings, 0 P2 findings, 0 P3 findings. Summary: reviewed the ps argv leak mitigation across Linux, macOS, and Windows. The implementation uses process comm/executable names only, removes argv readers, updates docs/feature metadata, and includes regression coverage for secret-bearing cmdline data. |
AlexandreYang
left a comment
AlexandreYang
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed the current PR diff for the ps argv leak mitigation.
Overall assessment: safe to merge.
| # | Priority | File | Finding |
|---|---|---|---|
| — | — | — | No findings |
Coverage Summary
| Code path | Test | Status |
|---|---|---|
| Linux ps ignores /proc//cmdline argv and reports stat comm | builtins/ps/ps_procpath_linux_test.go | Covered |
| Linux ps handles missing cmdline using unbracketed comm | builtins/ps/ps_procpath_linux_test.go | Covered |
| macOS kinfo conversion uses P_comm only | builtins/internal/procinfo/procinfo_darwin_test.go | Covered |
| Windows process entry conversion uses executable name only | builtins/internal/procinfo/procinfo_windows_test.go | Covered |
| Docs and feature metadata describe argv-safe behavior | README.md, SHELL_FEATURES.md, builtins/features.go | Covered by review |
P0/P1/P2 findings: 0
Review-Fix Loop Summary
Iteration log
Final state
Remaining issues
|
- Stops ps from reading /proc/<pid>/cmdline on Linux and kern.procargs2 argv on macOS. - Makes CMD report only the process comm/executable name across Linux, macOS, and Windows. - Deletes argv-reading helpers, updates docs/feature metadata, and cleans stale analysis allowlist entries. Allowed ps could expose other processes' CLI arguments, including tokens commonly passed as flags. The secure default is to avoid argv entirely. Observed excerpt from running the actual command with a controlled process in the same session: ```bash ./rshell --allow-all-commands -c "ps" ``` Before this change, `CMD` included full argv: ```text PID TTY TIME CMD 78381 ? 00:00:00 /Users/alexandre.yang/.pyenv/versions/3.10.13/bin/python3 -c import time; time.sleep(30) --token=RSHELL_DEMO_SECRET_BEFORE 78382 ? 00:00:00 ./rshell --allow-all-commands -c ps ``` After this change, `CMD` contains only the process comm/executable name: ```text PID TTY TIME CMD 78528 ? 00:00:00 python3.10 78529 ? 00:00:00 rshell ``` - make fmt - go test ./builtins/ps ./builtins/internal/procinfo ./builtins/tests/ps - go test ./tests/ -run 'TestShellScenarios/cmd/ps' -timeout 120s - TMPDIR=/Users/alexandre.yang/worktrees/rshell/ps-vuln/.tmp RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s - GOOS=linux GOARCH=amd64 go test -c ./builtins/ps -o /tmp/rshell-ps-linux.test - GOOS=windows GOARCH=amd64 go test -c ./builtins/internal/procinfo -o /tmp/rshell-procinfo-windows.test.exe - go test ./... - [x] Tests added/updated - [x] Documentation updated
### What does this PR do? [backport] rshell v0.0.14-1 to fix ps Rshell release link: https://github.com/DataDog/rshell/releases/tag/v0.0.14-1 ### Motivation Backport needed for this fix: DataDog/rshell#269 ### Describe how you validated your changes ### Additional Notes Co-authored-by: axel.vonengel <[email protected]>
2 participants
What does this PR do?
Motivation
Allowed ps could expose other processes' CLI arguments, including tokens commonly passed as flags. The secure default is to avoid argv entirely.
Before / After
Observed excerpt from running the actual command with a controlled process in the same session:
./rshell --allow-all-commands -c "ps"Before this change,
CMDincluded full argv:After this change,
CMDcontains only the process comm/executable name:Testing
Checklist