Semgrep subprocess shell False codemod (#706)

* refactor codemod into core codemod

* semgrep subprocess shell false codemod

Author: clavedeluna

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,)

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(

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,
     ],
 )

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",
+)

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),
+)

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),
+        )