os.system et al. all spawn a shell so we should use the same logic

Change-Id: Idee7d98884fd2dd1c8cf0138b82783cdbaad0a26
Closes-bug: 1513840

bandit/plugins/injection_shell.py

@@ -31,20 +31,30 @@ def _has_special_characters(command):
     return bool(re.search(r'[{|\[;$\*\?`]', command))
 
 
+def _evaluate_shell_call(context):
+    no_formatting = isinstance(context.node.args[0], ast.Str)
+    if no_formatting:
+        command = context.call_args[0]
+        no_special_chars = not _has_special_characters(command)
+    else:
+        no_special_chars = False
+
+    if no_formatting and no_special_chars:
+        return bandit.LOW
+    elif no_formatting:
+        return bandit.MEDIUM
+    else:
+        return bandit.HIGH
+
+
 @takes_config('shell_injection')
 @checks('Call')
 def subprocess_popen_with_shell_equals_true(context, config):
     if config and context.call_function_name_qual in config['subprocess']:
         if context.check_call_arg_value('shell', 'True'):
             if len(context.call_args) > 0:
-                no_formatting = isinstance(context.node.args[0], ast.Str)
-                if no_formatting:
-                    command = context.call_args[0]
-                    no_special_chars = not _has_special_characters(command)
-                else:
-                    no_special_chars = False
-
-                if no_formatting and no_special_chars:
+                sev = _evaluate_shell_call(context)
+                if sev == bandit.LOW:
                     return bandit.Issue(
                         severity=bandit.LOW,
                         confidence=bandit.HIGH,
@@ -52,7 +62,7 @@ def subprocess_popen_with_shell_equals_true(context, config):
                              "may be changed in the future, consider "
                              "rewriting without shell"
                     )
-                elif no_formatting:
+                elif sev == bandit.MEDIUM:
                     return bandit.Issue(
                         severity=bandit.MEDIUM,
                         confidence=bandit.HIGH,
@@ -67,9 +77,6 @@ def subprocess_popen_with_shell_equals_true(context, config):
                         text="subprocess call with shell=True identified, "
                              "security issue."
                     )
-            else:
-                # no arguments? no issue
-                pass
 
 
 @takes_config('shell_injection')
@@ -107,11 +114,31 @@ def any_other_function_with_shell_equals_true(context, config):
 @checks('Call')
 def start_process_with_a_shell(context, config):
     if config and context.call_function_name_qual in config['shell']:
-        return bandit.Issue(
-            severity=bandit.MEDIUM,
-            confidence=bandit.MEDIUM,
-            text="Starting a process with a shell: check for injection."
-        )
+        if len(context.call_args) > 0:
+            sev = _evaluate_shell_call(context)
+            if sev == bandit.LOW:
+                return bandit.Issue(
+                    severity=bandit.LOW,
+                    confidence=bandit.HIGH,
+                    text="Starting a process with a shell: "
+                         "Seems safe, but may be changed in the future, "
+                         "consider rewriting without shell"
+                )
+            elif sev == bandit.MEDIUM:
+                return bandit.Issue(
+                    severity=bandit.MEDIUM,
+                    confidence=bandit.HIGH,
+                    text="Starting a process with a shell and special shell "
+                         "characters, consider moving extra logic into "
+                         "Python code"
+                )
+            else:
+                return bandit.Issue(
+                    severity=bandit.HIGH,
+                    confidence=bandit.HIGH,
+                    text="Starting a process with a shell, possible injection"
+                         " detected, security issue."
+                )
 
 
 @takes_config('shell_injection')

examples/os-popen.py

@@ -11,3 +11,5 @@ os.popen2('/bin/uname -av')
 os.popen3('/bin/uname -av')
 os.popen4('/bin/uname -av')
 
+os.popen4('/bin/uname -av; rm -rf /')
+os.popen4(some_var)

tests/functional/test_functional.py

@@ -228,7 +228,8 @@ class FunctionalTests(testtools.TestCase):
 
     def test_os_popen(self):
         '''Test for `os.popen`.'''
-        expect = {'SEVERITY': {'MEDIUM': 7}, 'CONFIDENCE': {'MEDIUM': 7}}
+        expect = {'SEVERITY': {'LOW': 7, 'MEDIUM': 1, 'HIGH': 1},
+                  'CONFIDENCE': {'HIGH': 9}}
         self.check_example('os-popen.py', expect)
 
     def test_os_spawn(self):
@@ -243,7 +244,7 @@ class FunctionalTests(testtools.TestCase):
 
     def test_os_system(self):
         '''Test for `os.system`.'''
-        expect = {'SEVERITY': {'MEDIUM': 1}, 'CONFIDENCE': {'MEDIUM': 1}}
+        expect = {'SEVERITY': {'LOW': 1}, 'CONFIDENCE': {'HIGH': 1}}
         self.check_example('os_system.py', expect)
 
     def test_pickle(self):
@@ -256,7 +257,7 @@ class FunctionalTests(testtools.TestCase):
 
     def test_popen_wrappers(self):
         '''Test the `popen2` and `commands` modules.'''
-        expect = {'SEVERITY': {'MEDIUM': 7}, 'CONFIDENCE': {'MEDIUM': 7}}
+        expect = {'SEVERITY': {'MEDIUM': 7}, 'CONFIDENCE': {'HIGH': 7}}
         self.check_example('popen_wrappers.py', expect)
 
     def test_random_module(self):
@@ -319,7 +320,7 @@ class FunctionalTests(testtools.TestCase):
         '''Test for wildcard injection in shell commands.'''
         expect = {
             'SEVERITY': {'HIGH': 4, 'MEDIUM':4, 'LOW': 6},
-            'CONFIDENCE': {'MEDIUM': 8, 'HIGH': 6}
+            'CONFIDENCE': {'MEDIUM': 5, 'HIGH': 9}
         }
         self.check_example('wildcard-injection.py', expect)