diff --git a/integration_tests/test_subprocess_shell_false.py b/integration_tests/test_subprocess_shell_false.py new file mode 100644 index 00000000..dd520269 --- /dev/null +++ b/integration_tests/test_subprocess_shell_false.py @@ -0,0 +1,18 @@ +from core_codemods.subprocess_shell_false import SubprocessShellFalse +from integration_tests.base_test import ( + BaseIntegrationTest, + original_and_expected_from_code_path, +) + + +class TestSubprocessShellFalse(BaseIntegrationTest): + codemod = SubprocessShellFalse + code_path = "tests/samples/subprocess_run.py" + original_code, expected_new_code = original_and_expected_from_code_path( + code_path, [(1, "subprocess.run(\"echo 'hi'\", shell=False)\n")] + ) + expected_diff = "--- \n+++ \n@@ -1,2 +1,2 @@\n import subprocess\n-subprocess.run(\"echo 'hi'\", shell=True)\n+subprocess.run(\"echo 'hi'\", shell=False)\n" + expected_line_change = "2" + change_description = SubprocessShellFalse.CHANGE_DESCRIPTION + # expected because output code points to fake file + allowed_exceptions = (FileNotFoundError,) diff --git a/src/codemodder/scripts/generate_docs.py b/src/codemodder/scripts/generate_docs.py index e2e77fb6..ff8f0d11 100644 --- a/src/codemodder/scripts/generate_docs.py +++ b/src/codemodder/scripts/generate_docs.py @@ -178,6 +178,10 @@ class DocMetadata: importance="Low", guidance_explained="Since literals and new objects have their own identities, comparisons against them using `is` operators are most likely a bug and thus we deem the change safe.", ), + "subprocess-shell-false": DocMetadata( + importance="High", + guidance_explained="In most cases setting `shell=False` is correct and leads to much safer code. However there are valid use cases for `shell=True` when using shell functionality like pipes or wildcard is required. In such cases it is important to run only trusted, validated commands.", + ), } diff --git a/src/core_codemods/__init__.py b/src/core_codemods/__init__.py index a4e1cdba..cceb8428 100644 --- a/src/core_codemods/__init__.py +++ b/src/core_codemods/__init__.py @@ -38,6 +38,7 @@ from .sql_parameterization import SQLQueryParameterization from .exception_without_raise import ExceptionWithoutRaise from .literal_or_new_object_identity import LiteralOrNewObjectIdentity +from .subprocess_shell_false import SubprocessShellFalse registry = CodemodCollection( origin="pixee", @@ -75,6 +76,7 @@ WithThreadingLock, SQLQueryParameterization, SecureFlaskSessionConfig, + SubprocessShellFalse, FileResourceLeak, DjangoReceiverOnTop, NumpyNanEquality, diff --git a/src/core_codemods/docs/pixee_python_subprocess-shell-false.md b/src/core_codemods/docs/pixee_python_subprocess-shell-false.md new file mode 100644 index 00000000..5794657c --- /dev/null +++ b/src/core_codemods/docs/pixee_python_subprocess-shell-false.md @@ -0,0 +1,11 @@ +This codemod sets the `shell` keyword argument to `False` in `subprocess` module function calls that have set it to `True`. + +Setting `shell=True` will execute the provided command through the system shell which can lead to shell injection vulnerabilities. In the worst case this can give an attacker the ability to run arbitrary commands on your system. In most cases using `shell=False` is sufficient and leads to much safer code. + +The changes from this codemod look like this: + +```diff + import subprocess +- subprocess.run("echo 'hi'", shell=True) ++ subprocess.run("echo 'hi'", shell=False) +``` diff --git a/src/core_codemods/subprocess_shell_false.py b/src/core_codemods/subprocess_shell_false.py new file mode 100644 index 00000000..4286d5ab --- /dev/null +++ b/src/core_codemods/subprocess_shell_false.py @@ -0,0 +1,52 @@ +import libcst as cst +from libcst import matchers +from codemodder.codemods.api import BaseCodemod, ReviewGuidance +from codemodder.codemods.utils_mixin import NameResolutionMixin +from codemodder.codemods.api.helpers import NewArg + + +class SubprocessShellFalse(BaseCodemod, NameResolutionMixin): + NAME = "subprocess-shell-false" + SUMMARY = "Use `shell=False` in `subprocess` Function Calls" + REVIEW_GUIDANCE = ReviewGuidance.MERGE_AFTER_CURSORY_REVIEW + DESCRIPTION = "Set `shell` keyword argument to `False`" + REFERENCES = [ + { + "url": "https://docs.python.org/3/library/subprocess.html#security-considerations", + "description": "", + }, + { + "url": "https://en.wikipedia.org/wiki/Code_injection#Shell_injection", + "description": "", + }, + { + "url": "https://stackoverflow.com/a/3172488", + "description": "", + }, + ] + SUBPROCESS_FUNCS = [ + f"subprocess.{func}" + for func in {"run", "call", "check_output", "check_call", "Popen"} + ] + + def leave_Call(self, original_node: cst.Call, updated_node: cst.Call): + if not self.filter_by_path_includes_or_excludes( + self.node_position(original_node) + ): + return original_node + + if self.find_base_name(original_node.func) in self.SUBPROCESS_FUNCS: + for arg in original_node.args: + if matchers.matches( + arg, + matchers.Arg( + keyword=matchers.Name("shell"), value=matchers.Name("True") + ), + ): + self.report_change(original_node) + new_args = self.replace_args( + original_node, + [NewArg(name="shell", value="False", add_if_missing=False)], + ) + return self.update_arg_target(updated_node, new_args) + return original_node diff --git a/tests/codemods/test_subprocess_shell_false.py b/tests/codemods/test_subprocess_shell_false.py new file mode 100644 index 00000000..b19c9513 --- /dev/null +++ b/tests/codemods/test_subprocess_shell_false.py @@ -0,0 +1,58 @@ +import pytest +from core_codemods.subprocess_shell_false import SubprocessShellFalse +from tests.codemods.base_codemod_test import BaseCodemodTest + +each_func = pytest.mark.parametrize( + "func", ["check_output", "check_call", "run", "call", "Popen"] +) + + +class TestSubprocessShellFalse(BaseCodemodTest): + codemod = SubprocessShellFalse + + def test_name(self): + assert self.codemod.name() == "subprocess-shell-false" + + @each_func + def test_import(self, tmpdir, func): + input_code = f""" + import subprocess + subprocess.{func}(args, shell=True) + """ + expexted_output = f""" + import subprocess + subprocess.{func}(args, shell=False) + """ + self.run_and_assert(tmpdir, input_code, expexted_output) + assert len(self.file_context.codemod_changes) == 1 + + @each_func + def test_from_import(self, tmpdir, func): + input_code = f""" + from subprocess import {func} + {func}(args, shell=True) + """ + expexted_output = f""" + from subprocess import {func} + {func}(args, shell=False) + """ + self.run_and_assert(tmpdir, input_code, expexted_output) + assert len(self.file_context.codemod_changes) == 1 + + @each_func + def test_no_shell(self, tmpdir, func): + input_code = f""" + import subprocess + subprocess.{func}(args, timeout=1) + """ + self.run_and_assert(tmpdir, input_code, input_code) + assert len(self.file_context.codemod_changes) == 0 + + @each_func + def test_shell_False(self, tmpdir, func): + input_code = f""" + import subprocess + subprocess.{func}(args, shell=False) + """ + self.run_and_assert(tmpdir, input_code, input_code) + assert len(self.file_context.codemod_changes) == 0 diff --git a/tests/samples/subprocess_run.py b/tests/samples/subprocess_run.py new file mode 100644 index 00000000..70bf26f1 --- /dev/null +++ b/tests/samples/subprocess_run.py @@ -0,0 +1,2 @@ +import subprocess +subprocess.run("echo 'hi'", shell=True)