Fix shell injection via shell=True in subprocess calls#23894
Fix shell injection via shell=True in subprocess calls#23894carljm merged 4 commits intoastral-sh:mainfrom
shell=True in subprocess calls#23894Conversation
|
I think if we're going to do this, we should enable the corresponding Ruff rule: subprocess-popen-with-shell-equals-true (S602), but these don't strike me as very problematic uses. |
Enable Ruff's S602 rule (`subprocess-popen-with-shell-equals-true` from flake8-bandit) in pyproject.toml to enforce no-shell subprocess calls going forward, and fix the existing violations it would catch. pyproject.toml: - Add `S602` to the lint `select` list so any future `shell=True` usage in the scripts is caught automatically by the linter. scripts/setup_primer_project.py: - `project.install_cmd` (a freeform shell string from the mypy-primer project registry) was interpolated into a shell command and executed with `shell=True`. A compromised or malicious entry in the upstream registry could inject arbitrary shell commands. - `project.deps` were joined into a single shell string and also run with `shell=True`. - Fix: use `shlex.split()` to tokenise both strings into argument lists and pass them directly to subprocess without invoking a shell. scripts/ty_benchmark/src/benchmark/snapshot.py: - `command.prepare` was executed with `shell=True`, making it a latent injection point should the field ever be populated from external input. - Fix: tokenise with `shlex.split()` and drop `shell=True`; import `shlex` which was not previously imported in this module.
aac1288 to
06d2297
Compare
|
Mypy_primer is a highly trusted project that has very few maintainers -- and the maintainer team it does have includes several maintainers of ty. It feels pretty unlikely that an attacker would attempt to attack Ruff via mypy_primer. I do see the theoretical security risk here, but we already effectively audit each new commit to mypy_primer whenever we manually bump the pinned SHA here that's used from our GitHub workflows: Line 25 in 3d7eabe scripts/mypy_primer.sh.
Not sure if there's a good way to pin the mypy_primer dependencies of these scripts and ensure that that pin is kept in sync with |
Yeah that's true, the pinned SHA could be a stronger defense than just sanitizing subprocess arguments. I’m happy to pivot the PR to this approach since it kills the supply-chain risk at the source. I'd personally keep the shell=False changes as cheap defense-in-depth tho. |
That could work. A simple pre-commit hook would be the option I'd prefer there (as long as it's fast), as we'd quickly spot the problem locally when running checks, as well as in CI.
Fair enough. Does |
As suggested in review, pin the mypy-primer dependency in setup_primer_project.py to the same SHA already used in mypy_primer.sh, and add a pre-commit hook to keep them in sync. scripts/setup_primer_project.py: - Add `rev = "850d65d9..."` to the uv.sources block so the script uses the same audited mypy-primer commit as the CI workflow, closing the supply-chain vector at the source rather than just hardening the subprocess invocation. scripts/check_mypy_primer_sha.py: - New helper script that greps the SHA from both mypy_primer.sh and setup_primer_project.py and fails if they differ. Runs in ~55ms (pure Python, no imports beyond stdlib). .pre-commit-config.yaml: - Add `check-mypy-primer-sha` local hook that runs the above script only when either of the two relevant files is changed, so it is near-zero cost in the common case.
I've updated the PR to use the SHA pin approach. setup_primer_project.py now matches the rev = "850d65d9..." in the uv.sources block from mypy_primer.sh. I also added a small scripts/check_mypy_primer_sha.py script as a local pre-commit hook—it only fires if one of those two files is touched. One quick sanity check on the shlex bit: Since shlex.split() defaults to POSIX rules, it'll probably trip over Windows-style quoting. I noticed ty_benchmark already handles this with an mslex fallback so maybe I should add the same thing in snapshot.py |
|
Move the None-check into extract() so its return type is str rather than str | None, allowing ty to verify the final print without a not-subscriptable diagnostic.
Mirror the existing benchmark shlex/mslex import pattern in snapshot.py so command.prepare is tokenized with Windows-correct rules on win32, and remove the redundant unconditional shlex import from benchmark/__init__.py.
I followed up on the Windows question, now for snapshot.py on windows we are using mslex as shlex, just to keep shell false while preserving Windows-correct tokenization. |
* main: (94 commits) Fix shell injection via `shell=True` in subprocess calls (#23894) [ty] Refactor `relation.rs` to store state on a struct rather than passing around 7 arguments every time we recurse (#23837) Don't return code actions for non-Python documents (#23905) [ty] Make the default database truly statically infallible (#23929) [ty] Add `Download` button to ty playground which creates a zip export (#23478) [ty] Respect `kw_only` overwrites in dataclasses (#23930) [ty] Clarify in diagnostics that `from __future__ import annotations` only stringifies type annotations (#23928) [ty] Add a `Copy Markdown` button to playground (#23002) [ty] Fix folding range classification of lines starting with `#` (#23831) [ty] Fix folding ranges for notebooks (#23830) [ty] fix too-many-cycle panics when inferring literal type loop variables (#23875) Add `RegularCallableTypeOf` and `into_regular_callable` in `ty_extensions` (#23909) [ty] treat properties as full structural types (#23925) [ty] Avoid duplicated work during multi-inference (#23923) [ty]: make `possibly-missing-attribute` ignored by default [ty]: split out `possibly-missing-submodule` from `possibly-missing-attribute` Update astral-sh/setup-uv action to v7.5.0 (#23922) [ty] Show truthiness in ConstraintSet display and simplify falsy error message (#23913) Bump 0.15.6 (#23919) [ty] Narrow type context during collection literal inference (#23844) ...
Summary
Enable Ruff's own
S602rule (subprocess-popen-with-shell-equals-truefrom flake8-bandit) inpyproject.tomlto enforce no-shell subprocess calls going forward, and fix the two existing violations it catches.pyproject.toml- addsS602to the lintselectlist so any futureshell=Trueusage in the scripts is caught automatically by the linter.scripts/setup_primer_project.py-project.install_cmdandproject.depscome from the mypy-primer project registry (an externally-fetched third-party config). Both were joined into shell strings and executed withshell=True, making them a supply-chain injection vector. Fixed by tokenising withshlex.split()and droppingshell=True.scripts/ty_benchmark/src/benchmark/snapshot.py-command.preparewas passed tosubprocess.run(..., shell=True). While no current caller sets this field, it is a latent injection point. Fixed by tokenising withshlex.split()and droppingshell=True; adds the missingimport shlex.Test Plan
Both scripts are developer-only utilities. The changes are semantically equivalent for well-formed inputs -
shlex.split()produces the same argument list the shell would have constructed, while refusing to pass metacharacters through to a shell process.Verified no other
shell=Trueuses remain inscripts/orpython/that would be newly flagged by S602.