Add SCS110 and SCS111

- SCS110: Use of `os.popen()` should be avoided, as it internally uses
          `subprocess.Popen` with `shell=True`
- SCS111: Use of `shlex.quote()` should be avoided on non-POSIX
          platforms (such as Windows)

Author: Takishima

CHANGELOG.md

@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+-   Added SCS110 to avoid using `os.popen()` as it internally uses `subprocess.Popen` with `shell=True`
+-   Added SCS111 to avoid using `shlex.quote()` on non-POSIX platforms.
+
 ## [1.1.0] - 2021-07-02
 
 ### Added

README.md

@@ -23,6 +23,8 @@ flake8 plugin that enforces some secure coding standards.
 | SCS107 | Use of debugging code shoud not be present in production code (e.g. `import pdb`)                            |
 | SCS108 | `assert` statements should not be present in production code                                                 |
 | SCS109 | Use of builtin `open` for writing is discouraged in favor of `os.open` to allow for setting file permissions |
+| SCS110 | Avoid using `os.popen()` as it internally uses `subprocess.Popen` with `shell=True`                          |
+| SCS111 | Use of `shlex.quote()` should be avoided on non-POSIX platforms                                              |
 
 ## Pre-commit hook
 

flake8_secure_coding_standard.py

@@ -16,13 +16,14 @@
 """Main file for the flake8_secure_coding_standard plugin."""
 
 import ast
+import platform
 import sys
 from typing import Any, Dict, Generator, List, Tuple, Type
 
 if sys.version_info < (3, 8):  # pragma: no cover (<PY38)
     import importlib_metadata  # pylint: disable=E0401
 
-    ast_Constant = ast.NameConstant
+    ast_Constant = ast.NameConstant  # pylint: disable=invalid-name
 else:  # pragma: no cover (PY38+)
     ast_Constant = ast.Constant
     import importlib.metadata as importlib_metadata
@@ -47,6 +48,24 @@
     'SCS109 Use of builtin `open` for writing is discouraged in favor of `os.open` '
     + 'to allow for setting file permissions'
 )  # noqa: E501
+SCS110 = (
+    'Use of `os.popen()` should be avoided, as it internally uses `subprocess.Popen` with `shell=True`'  # noqa: E501
+)
+SCS111 = 'Use of `shlex.quote()` should be avoided on non-POSIX platforms (such as Windows)'  # noqa: E501
+
+
+# ==============================================================================
+# Helper functions
+
+
+def _is_posix():
+    """Return True if the current system is POSIX-compatible."""
+    # NB: we could simply use `os.name` instead of `platform.system()`. However, that solution would be difficult to
+    #     test using `mock` as a few modules (like `pytest`) actually use it internally...
+    return platform.system() in ('Linux', 'Darwin')
+
+
+# ==============================================================================
 
 
 def _is_os_system_call(node: ast.Call) -> bool:
@@ -58,6 +77,15 @@ def _is_os_system_call(node: ast.Call) -> bool:
     )
 
 
+def _is_os_popen_call(node: ast.Call) -> bool:
+    return (
+        isinstance(node.func, ast.Attribute)
+        and isinstance(node.func.value, ast.Name)
+        and node.func.value.id == 'os'
+        and node.func.attr == 'popen'
+    )
+
+
 def _is_os_path_call(node: ast.Call) -> bool:
     return (
         isinstance(node.func, ast.Attribute)  # pylint: disable=R0916
@@ -193,6 +221,15 @@ def _is_jsonpickle_encode_call(node: ast.Call) -> bool:
     return False
 
 
+def _is_shlex_quote_call(node: ast.Call) -> bool:
+    return not _is_posix() and (
+        isinstance(node.func, ast.Attribute)
+        and isinstance(node.func.value, ast.Name)
+        and node.func.value.id == 'shlex'
+        and node.func.attr == 'quote'
+    )
+
+
 class Visitor(ast.NodeVisitor):
     """AST visitor class for the plugin."""
 
@@ -215,12 +252,17 @@ def visit_Call(self, node: ast.Call) -> None:
             self.errors.append((node.lineno, node.col_offset, SCS102))
         elif _is_os_path_call(node):
             self.errors.append((node.lineno, node.col_offset, SCS100))
+        elif _is_os_popen_call(node):
+            self.errors.append((node.lineno, node.col_offset, SCS110))
         elif _is_subprocess_shell_true_call(node):
             self.errors.append((node.lineno, node.col_offset, SCS103))
         elif _is_builtin_open_for_writing(node):
             self.errors.append((node.lineno, node.col_offset, SCS109))
         elif isinstance(node.func, ast.Name) and (node.func.id in ('eval', 'exec')):
             self.errors.append((node.lineno, node.col_offset, SCS101))
+        elif _is_shlex_quote_call(node):
+            self.errors.append((node.lineno, node.col_offset, SCS111))
+
         self.generic_visit(node)
 
     def visit_Import(self, node: ast.Import) -> None:
@@ -254,6 +296,15 @@ def visit_ImportFrom(self, node: ast.ImportFrom) -> None:
                 # Cover:
                 # * from os import system
                 self.errors.append((node.lineno, node.col_offset, SCS102))
+            elif node.module == 'os' and alias.name == 'popen':
+                # Cover:
+                # * from os import popen
+                self.errors.append((node.lineno, node.col_offset, SCS110))
+            elif not _is_posix() and node.module == 'shlex' and alias.name == 'quote':
+                # Cover:
+                # * from shlex import quote
+                # * from shlex import quote as quoted
+                self.errors.append((node.lineno, node.col_offset, SCS111))
 
         self.generic_visit(node)
 

tests/shell_exec_test.py

@@ -55,6 +55,8 @@ def test_shell_exec_ok(s):
         ('os.system("ls -l")', flake8_scs.SCS102),
         ('from os import system', flake8_scs.SCS102),
         ('from os import system as os_system', flake8_scs.SCS102),
+        ('from os import popen', flake8_scs.SCS110),
+        ('from os import popen as os_popen', flake8_scs.SCS110),
         ('subprocess.Popen(["cat", "/etc/passwd"], b, e, i, o, e, pre, c, True)', flake8_scs.SCS103),
         ('subprocess.Popen(["cat", "/etc/passwd"], b, e, i, o, e, pre, c, True, cwd)', flake8_scs.SCS103),
         ('subprocess.run(["cat", "/etc/passwd"], shell=True)', flake8_scs.SCS103),
@@ -65,6 +67,12 @@ def test_shell_exec_ok(s):
         ('sp.check_call(["cat", "/etc/passwd"], shell=True)', flake8_scs.SCS103),
         ('subprocess.check_output(["cat", "/etc/passwd"], shell=True)', flake8_scs.SCS103),
         ('sp.check_output(["cat", "/etc/passwd"], shell=True)', flake8_scs.SCS103),
+        ('os.popen("cat")', flake8_scs.SCS110),
+        ('os.popen("cat", "r")', flake8_scs.SCS110),
+        ('os.popen("cat", "r", 1)', flake8_scs.SCS110),
+        ('os.popen("cat", buffering=1)', flake8_scs.SCS110),
+        ('os.popen("cat", mode="w")', flake8_scs.SCS110),
+        ('os.popen("cat", mode="w", buffering=1)', flake8_scs.SCS110),
     ),
 )
 def test_shell_exec_not_ok(s, msg_id):

tests/shlex_quote_test.py

@@ -0,0 +1,55 @@
+# -*- coding: utf-8 -*-
+# Copyright 2021 Damien Nguyen
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import ast
+
+import pytest
+
+import flake8_secure_coding_standard as flake8_scs
+
+
+def results(s):
+    return {'{}:{}: {}'.format(*r) for r in flake8_scs.Plugin(ast.parse(s)).run()}
+
+
+@pytest.mark.parametrize('s', ('', 'int(0)'))
+def test_shlex_quote_always_success(s):
+    assert results(s) == set()
+
+
+@pytest.mark.parametrize(
+    'platform, expected_success',
+    (
+        ('Linux', True),
+        ('Darwin', True),
+        ('Java', False),
+        ('Windows', False),
+    ),
+)
+@pytest.mark.parametrize(
+    's',
+    (
+        'from shlex import quote',
+        'from shlex import quote as quoted',
+        'shlex.quote("ls -l")',
+        'shlex.quote(command_str)',
+    ),
+)
+def test_shlex_quote(mocker, platform, expected_success, s):
+    mocker.patch('platform.system', lambda: platform)
+    if expected_success:
+        assert results(s) == set()
+    else:
+        assert results(s) == {'1:0: ' + flake8_scs.SCS111}