Reword E8003 and extend it to cover a few more cases

Author: Takishima

CHANGELOG.md

@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+-   Reworded E8003 and extend it to include a few more cases:
+    + `subprocess.getoutput()`
+    + `subprocess.getstatusoutput()`
+    + `asyncio.create_subprocess_shell()`
+    + `loop.subprocess_shell()`
+
 ## [1.2.0] - 2021-07-19
 
 ### Added

README.md

@@ -16,7 +16,7 @@ pylint plugin that enforces some secure coding standards.
 | R8000 | Use `os.path.realpath()` instead of `os.path.abspath()` and `os.path.relpath()`                              |
 | E8001 | Avoid using `exec()` and `eval()`                                                                            |
 | E8002 | Avoid using `os.sytem()`                                                                                     |
-| E8003 | Avoid using `shell=True` when calling `subprocess` functions                                                 |
+| E8003 | Avoid using `shell=True` in subprocess functions or using functions that internally set this                 |
 | R8004 | Avoid using `tempfile.mktemp()`, prefer `tempfile.mkstemp()` instead                                         |
 | E8005 | Avoid using unsafe PyYAML loading functions                                                                  |
 | E8006 | Avoid using `jsonpickle.decode()`                                                                            |

pylint_secure_coding_standard.py

@@ -95,19 +95,34 @@ def _is_builtin_open_for_writing(node):
     return False
 
 
-def _is_subprocess_shell_true_call(node):
-    if (
-        isinstance(node.func, astroid.Attribute)
-        and isinstance(node.func.expr, astroid.Name)
-        and node.func.expr.name in ('subprocess', 'sp')
-    ):
-        if node.keywords:
-            for keyword in node.keywords:
-                if keyword.arg == 'shell' and isinstance(keyword.value, astroid.Const) and bool(keyword.value.value):
-                    return True
+def _is_shell_true_call(node):
+    if not (isinstance(node.func, astroid.Attribute) and isinstance(node.func.expr, astroid.Name)):
+        return False
+
+    # subprocess module
+    if node.func.expr.name in ('subprocess', 'sp'):
+        if node.func.attrname in ('call', 'check_call', 'check_output', 'Popen', 'run'):
+            if node.keywords:
+                for keyword in node.keywords:
+                    if (
+                        keyword.arg == 'shell'
+                        and isinstance(keyword.value, astroid.Const)
+                        and bool(keyword.value.value)
+                    ):
+                        return True
 
-        if len(node.args) > 8 and isinstance(node.args[8], astroid.Const) and bool(node.args[8].value):
+            if len(node.args) > 8 and isinstance(node.args[8], astroid.Const) and bool(node.args[8].value):
+                return True
+
+        if node.func.attrname in ('getoutput', 'getstatusoutput'):
             return True
+
+    # asyncio module
+    if (node.func.expr.name == 'asyncio' and node.func.attrname == 'create_subprocess_shell') or (
+        node.func.expr.name == 'loop' and node.func.attrname == 'subprocess_shell'
+    ):
+        return True
+
     return False
 
 
@@ -229,9 +244,14 @@ class SecureCodingStandardChecker(BaseChecker):
         ),
         'E8002': ('Avoid using `os.sytem()`', 'avoid-os-system', 'Use of `os.system()` should be avoided'),
         'E8003': (
-            'Avoid using `shell=True` when calling `subprocess` functions',
+            'Avoid using `shell=True` when calling `subprocess` functions and avoid functions that internally call it',
             'avoid-shell-true',
-            'Use of `shell=True` in subprocess functions should be avoided',
+            ' '.join(
+                [
+                    'Use of `shell=True` in subprocess functions or use of functions that internally set it should be'
+                    'should be avoided',
+                ]
+            ),
         ),
         'R8004': (
             'Avoid using `tempfile.mktemp()`, prefer `tempfile.mkstemp()` instead',
@@ -292,7 +312,7 @@ def visit_call(self, node):
             self.add_message('avoid-os-system', node=node)
         elif _is_os_path_call(node):
             self.add_message('replace-os-relpath-abspath', node=node)
-        elif _is_subprocess_shell_true_call(node):
+        elif _is_shell_true_call(node):
             self.add_message('avoid-shell-true', node=node)
         elif _is_os_popen_call(node):
             self.add_message('avoid-os-popen', node=node)
@@ -320,6 +340,11 @@ def visit_importfrom(self, node):
             self.add_message('replace-mktemp', node=node)
         elif node.modname in ('os.path', 'op') and [name for (name, _) in node.names if name in ('relpath', 'abspath')]:
             self.add_message('replace-os-relpath-abspath', node=node)
+        elif (
+            node.modname == 'subprocess'
+            and [name for (name, _) in node.names if name in ('getoutput', 'getstatusoutput')]
+        ) or ((node.modname == 'asyncio' and [name for (name, _) in node.names if name == 'create_subprocess_shell'])):
+            self.add_message('avoid-shell-true', node=node)
         elif node.modname == 'os' and [name for (name, _) in node.names if name == 'system']:
             self.add_message('avoid-os-system', node=node)
         elif node.modname == 'os' and [name for (name, _) in node.names if name == 'popen']:

tests/shell_exec_test.py

@@ -53,6 +53,12 @@ def test_shell_true_ok(self):
             ('from os import system as os_system', 'avoid-os-system'),
             ('from os import popen', 'avoid-os-popen'),
             ('from os import popen as os_popen', 'avoid-os-popen'),
+            ('from subprocess import getoutput', 'avoid-shell-true'),
+            ('from subprocess import getoutput as sp_getoutput', 'avoid-shell-true'),
+            ('from subprocess import getstatusoutput', 'avoid-shell-true'),
+            ('from subprocess import getstatusoutput as sp_getstatusoutput', 'avoid-shell-true'),
+            ('from asyncio import create_subprocess_shell', 'avoid-shell-true'),
+            ('from asyncio import create_subprocess_shell as create_sp_shell', 'avoid-shell-true'),
         ),
     )
     def test_shell_true_importfrom(self, s, msg_id):
@@ -74,6 +80,17 @@ def test_shell_true_importfrom(self, s, msg_id):
             ('sp.check_call(["cat", "/etc/passwd"], shell=True)', 'avoid-shell-true'),
             ('subprocess.check_output(["cat", "/etc/passwd"], shell=True)', 'avoid-shell-true'),
             ('sp.check_output(["cat", "/etc/passwd"], shell=True)', 'avoid-shell-true'),
+            ('subprocess.getoutput("ls /bin/ls")', 'avoid-shell-true'),
+            ('sp.getoutput("ls /bin/ls")', 'avoid-shell-true'),
+            ('subprocess.getstatusoutput("ls /bin/ls")', 'avoid-shell-true'),
+            ('sp.getstatusoutput("ls /bin/ls")', 'avoid-shell-true'),
+            ('asyncio.create_subprocess_shell("ls /bin/ls")', 'avoid-shell-true'),
+            ('asyncio.create_subprocess_shell(cmd)', 'avoid-shell-true'),
+            ('asyncio.create_subprocess_shell("ls /bin/ls", stdin=PIPE, stdout=PIPE)', 'avoid-shell-true'),
+            ('asyncio.create_subprocess_shell(cmd, stdin=PIPE, stdout=PIPE)', 'avoid-shell-true'),
+            ('loop.subprocess_shell(asyncio.SubprocessProtocol, "ls /bin/ls")', 'avoid-shell-true'),
+            ('loop.subprocess_shell(asyncio.SubprocessProtocol, cmd)', 'avoid-shell-true'),
+            ('loop.subprocess_shell(asyncio.SubprocessProtocol, cmd, **kwds)', 'avoid-shell-true'),
             ('os.popen("cat")', 'avoid-os-popen'),
             ('os.popen("cat", "r")', 'avoid-os-popen'),
             ('os.popen("cat", "r", 1)', 'avoid-os-popen'),