diff --git a/integration_tests/test_subprocess_shell_false.py b/integration_tests/test_subprocess_shell_false.py index 31e56d59..1d854f3b 100644 --- a/integration_tests/test_subprocess_shell_false.py +++ b/integration_tests/test_subprocess_shell_false.py @@ -1,5 +1,8 @@ from codemodder.codemods.test import BaseIntegrationTest -from core_codemods.subprocess_shell_false import SubprocessShellFalse +from core_codemods.subprocess_shell_false import ( + SubprocessShellFalse, + SubprocessShellFalseTransformer, +) class TestSubprocessShellFalse(BaseIntegrationTest): @@ -12,6 +15,6 @@ class TestSubprocessShellFalse(BaseIntegrationTest): expected_diff = "--- \n+++ \n@@ -1,2 +1,2 @@\n import subprocess\n-subprocess.run(['ls', '-l'], shell=True)\n+subprocess.run(['ls', '-l'], shell=False)\n" expected_line_change = "2" - change_description = SubprocessShellFalse.change_description + change_description = SubprocessShellFalseTransformer.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 4105f816..78da3641 100644 --- a/src/codemodder/scripts/generate_docs.py +++ b/src/codemodder/scripts/generate_docs.py @@ -324,6 +324,7 @@ class DocMetadata: "enable-jinja2-autoescape", "jwt-decode-verify", "use-defusedxml", + "subprocess-shell-false", ] SEMGREP_CODEMODS = { name: DocMetadata( diff --git a/src/core_codemods/__init__.py b/src/core_codemods/__init__.py index c72568dd..29531af3 100644 --- a/src/core_codemods/__init__.py +++ b/src/core_codemods/__init__.py @@ -56,6 +56,7 @@ from .secure_random import SecureRandom from .semgrep.semgrep_enable_jinja2_autoescape import SemgrepEnableJinja2Autoescape from .semgrep.semgrep_jwt_decode_verify import SemgrepJwtDecodeVerify +from .semgrep.semgrep_subprocess_shell_false import SemgrepSubprocessShellFalse from .semgrep.semgrep_use_defused_xml import SemgrepUseDefusedXml from .sonar.sonar_break_or_continue_out_of_loop import SonarBreakOrContinueOutOfLoop from .sonar.sonar_disable_graphql_introspection import SonarDisableGraphQLIntrospection @@ -202,5 +203,6 @@ SemgrepEnableJinja2Autoescape, SemgrepJwtDecodeVerify, SemgrepUseDefusedXml, + SemgrepSubprocessShellFalse, ], ) diff --git a/src/core_codemods/semgrep/semgrep_subprocess_shell_false.py b/src/core_codemods/semgrep/semgrep_subprocess_shell_false.py new file mode 100644 index 00000000..2f683f2c --- /dev/null +++ b/src/core_codemods/semgrep/semgrep_subprocess_shell_false.py @@ -0,0 +1,9 @@ +from core_codemods.semgrep.api import SemgrepCodemod +from core_codemods.subprocess_shell_false import SubprocessShellFalse + +SemgrepSubprocessShellFalse = SemgrepCodemod.from_core_codemod( + name="subprocess-shell-false", + other=SubprocessShellFalse, + rule_id="python.lang.security.audit.subprocess-shell-true.subprocess-shell-true", + rule_name="subprocess-shell-true", +) diff --git a/src/core_codemods/subprocess_shell_false.py b/src/core_codemods/subprocess_shell_false.py index f4fee917..79d30607 100644 --- a/src/core_codemods/subprocess_shell_false.py +++ b/src/core_codemods/subprocess_shell_false.py @@ -3,26 +3,22 @@ from libcst.metadata import ParentNodeProvider from codemodder.codemods.check_annotations import is_disabled_by_annotations -from codemodder.codemods.libcst_transformer import NewArg +from codemodder.codemods.libcst_transformer import ( + LibcstResultTransformer, + LibcstTransformerPipeline, + NewArg, +) from codemodder.codemods.utils_mixin import NameResolutionMixin -from core_codemods.api import Metadata, Reference, ReviewGuidance, SimpleCodemod +from core_codemods.api import ( + CoreCodemod, + Metadata, + Reference, + ReviewGuidance, + SimpleCodemod, +) -class SubprocessShellFalse(SimpleCodemod, NameResolutionMixin): - metadata = Metadata( - name="subprocess-shell-false", - summary="Use `shell=False` in `subprocess` Function Calls", - review_guidance=ReviewGuidance.MERGE_AFTER_CURSORY_REVIEW, - references=[ - Reference( - url="https://docs.python.org/3/library/subprocess.html#security-considerations" - ), - Reference( - url="https://en.wikipedia.org/wiki/Code_injection#Shell_injection" - ), - Reference(url="https://stackoverflow.com/a/3172488"), - ], - ) +class SubprocessShellFalseTransformer(LibcstResultTransformer, NameResolutionMixin): change_description = "Set `shell` keyword argument to `False`" SUBPROCESS_FUNCS = [ f"subprocess.{func}" @@ -68,3 +64,22 @@ def first_arg_is_not_string(self, original_node: cst.Call) -> bool: value=m.SimpleString() | m.ConcatenatedString() | m.FormattedString() ), ) + + +SubprocessShellFalse = CoreCodemod( + metadata=Metadata( + name="subprocess-shell-false", + summary="Use `shell=False` in `subprocess` Function Calls", + review_guidance=ReviewGuidance.MERGE_AFTER_CURSORY_REVIEW, + references=[ + Reference( + url="https://docs.python.org/3/library/subprocess.html#security-considerations" + ), + Reference( + url="https://en.wikipedia.org/wiki/Code_injection#Shell_injection" + ), + Reference(url="https://stackoverflow.com/a/3172488"), + ], + ), + transformer=LibcstTransformerPipeline(SubprocessShellFalseTransformer), +) diff --git a/tests/codemods/semgrep/test_semgrep_subprocess_shell_false.py b/tests/codemods/semgrep/test_semgrep_subprocess_shell_false.py new file mode 100644 index 00000000..313a10bf --- /dev/null +++ b/tests/codemods/semgrep/test_semgrep_subprocess_shell_false.py @@ -0,0 +1,66 @@ +import json + +from codemodder.codemods.test import BaseSASTCodemodTest +from core_codemods.semgrep.semgrep_subprocess_shell_false import ( + SemgrepSubprocessShellFalse, +) + + +class TestSemgrepSubprocessShellFalse(BaseSASTCodemodTest): + codemod = SemgrepSubprocessShellFalse + tool = "semgrep" + + def test_name(self): + assert self.codemod.name == "subprocess-shell-false" + + def test_import(self, tmpdir): + input_code = """\ + from subprocess import run + run(args, shell=True) + """ + expexted_output = """\ + from subprocess import run + run(args, shell=False) + """ + + results = { + "runs": [ + { + "results": [ + { + "fingerprints": {"matchBasedId/v1": "123"}, + "locations": [ + { + "physicalLocation": { + "artifactLocation": { + "uri": "code.py", + "uriBaseId": "%SRCROOT%", + }, + "region": { + "endColumn": 22, + "endLine": 2, + "snippet": { + "text": "run(args, shell=True)" + }, + "startColumn": 1, + "startLine": 2, + }, + } + } + ], + "message": { + "text": "Found 'subprocess' function 'run' with 'shell=True'. This is dangerous because this call will spawn the command using a shell process. Doing so propagates current shell settings and variables, which makes it much easier for a malicious actor to execute commands. Use 'shell=False' instead." + }, + "properties": {}, + "ruleId": "python.lang.security.audit.subprocess-shell-true.subprocess-shell-true", + } + ] + } + ] + } + self.run_and_assert( + tmpdir, + input_code, + expexted_output, + results=json.dumps(results), + )