Adding a test for partial paths in exec functions

When using functions like subprocess.Popen etc to launch an
external executable, the full path should be given. This prevents
an attacker from manipulting the search path or placing a bogus
executable that will be launched instead of the intended one.

Change-Id: I4a11f988bc3e954331ab0f0902ea849c6ec31888

bandit/config/bandit.yaml

@@ -40,6 +40,7 @@ profiles:
             - any_other_function_with_shell_equals_true
             - start_process_with_a_shell
             - start_process_with_no_shell
+            - start_process_with_partial_path
         exclude:
 
     SqlInjection:

bandit/plugins/injection_shell.py

@@ -14,6 +14,8 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import ast
+
 import bandit
 from bandit.core.test_properties import checks
 from bandit.core.test_properties import takes_config
@@ -82,3 +84,27 @@ def start_process_with_no_shell(context, config):
             confidence=bandit.MEDIUM,
             text="Starting a process without a shell."
         )
+
+
+@takes_config('shell_injection')
+@checks('Call')
+def start_process_with_partial_path(context, config):
+    if config and len(context.call_args):
+        if(context.call_function_name_qual in config['subprocess'] or
+           context.call_function_name_qual in config['shell'] or
+           context.call_function_name_qual in config['no_shell']):
+
+            delims = ['/', '\\', '.']
+            node = context.node.args[0]
+            # some calls take an arg list, check the first part
+            if isinstance(node, ast.List):
+                node = node.elts[0]
+
+            # make sure the param is a string literal and not a var name
+            if(isinstance(node, ast.Str) and node.s[0] not in delims):
+                return bandit.Issue(
+                    severity=bandit.LOW,
+                    confidence=bandit.HIGH,
+                    text=("Starting a process with a partial executable path"
+                          " %s" % context.call_args_string)
+                )

docs/partial_paths.md

@@ -0,0 +1,41 @@
+Avoid spawning subprocess with partial paths
+=====================
+When launching a subprocess from within Python, care should be taken over
+executable paths. The search path, normally the 'PATH' environment variable,
+will be used to discover a executable binary if a fully qualified path is not
+given. This can allow an attacker to manipulate the search path, or place a
+similarly named executable at an early point, such that it will be executed in
+preference to the expected executable.
+
+Paths should be given either fully qualified from the filesystem root, or
+relative to the running processes working directory. If it is desirable to use
+unqualified executable names for the perpose of location independent deployments
+then consider using paths relative to the deployment directory or deducing the
+paths using mechanisms such as `os.cwd()`
+
+### Correct
+Fully qualified paths, or relative paths:
+```python
+
+os.Popen('/bin/ls -l', shell=False)
+os.Popen(['/bin/ls', '-l'], shell=False)
+os.Popen(['../ls', '-l'], shell=False)
+
+```
+
+### Incorrect
+Unqualified executable names:
+```python
+
+os.Popen('ls -l', shell=False)
+os.Popen(['ls', '-l'], shell=False)
+
+```
+
+## Consequences
+The following consequences may arise from the use of unqualified paths
+
+* Unintended execution of malicious binaries
+
+## References
+* https://cwe.mitre.org/data/definitions/426.html

examples/imports-aliases.py

@@ -6,7 +6,7 @@ import hashlib as hhhh
 from pickle import loads as lp
 import pickle as p
 
-pop('gcc --version', shell=True)
+pop('/bin/gcc --version', shell=True)
 
 h.md5('1')
 hh.md5('2')

examples/os-startfile.py

@@ -1,6 +1,5 @@
 import os
 
-os.startfile('foo.docx')
-os.startfile('bad.exe')
-os.startfile('text.txt')
-
+os.startfile('/bin/foo.docx')
+os.startfile('/bin/bad.exe')
+os.startfile('/bin/text.txt')

examples/os_system.py

@@ -1,3 +1,3 @@
 import os
 
-os.system('echo hi')
+os.system('/bin/echo hi')

examples/partial_path_process.py

@@ -0,0 +1,10 @@
+from subprocess import Popen as pop
+
+pop('gcc --version', shell=False)
+pop('/bin/gcc --version', shell=False)
+pop(var, shell=False)
+
+pop(['ls', '-l'], shell=False)
+pop(['/bin/ls', '-l'], shell=False)
+
+pop('../ls -l', shell=False)

examples/popen_wrappers.py

@@ -2,14 +2,14 @@ import commands
 import popen2
 
 
-print(commands.getstatusoutput('echo / | xargs ls'))
-print(commands.getoutput('echo / | xargs ls'))
+print(commands.getstatusoutput('/bin/echo / | xargs ls'))
+print(commands.getoutput('/bin/echo / | xargs ls'))
 
 # This one is safe.
-print(commands.getstatus('echo / | xargs ls'))
+print(commands.getstatus('/bin/echo / | xargs ls'))
 
-print(popen2.popen2('echo / | xargs ls')[0].read())
-print(popen2.popen3('echo / | xargs ls')[0].read())
-print(popen2.popen4('echo / | xargs ls')[0].read())
-print(popen2.Popen3('echo / | xargs ls').fromchild.read())
-print(popen2.Popen4('echo / | xargs ls').fromchild.read())
+print(popen2.popen2('/bin/echo / | xargs ls')[0].read())
+print(popen2.popen3('/bin/echo / | xargs ls')[0].read())
+print(popen2.popen4('/bin/echo / | xargs ls')[0].read())
+print(popen2.Popen3('/bin/echo / | xargs ls').fromchild.read())
+print(popen2.Popen4('/bin/echo / | xargs ls').fromchild.read())

examples/skip.py

@@ -1,7 +1,7 @@
-subprocess.call(["ls", "-l"])
-subprocess.call(["ls", "-l"]) #noqa
-subprocess.call(["ls", "-l"]) # noqa
-subprocess.call(["ls", "-l"]) # nosec
-subprocess.call(["ls", "-l"])
-subprocess.call(["ls", "-l"]) #nosec
-subprocess.call(["ls", "-l"])
+subprocess.call(["/bin/ls", "-l"])
+subprocess.call(["/bin/ls", "-l"]) #noqa
+subprocess.call(["/bin/ls", "-l"]) # noqa
+subprocess.call(["/bin/ls", "-l"]) # nosec
+subprocess.call(["/bin/ls", "-l"])
+subprocess.call(["/bin/ls", "-l"]) #nosec
+subprocess.call(["/bin/ls", "-l"])

examples/subprocess_shell.py

@@ -5,20 +5,20 @@ from subprocess import Popen as pop
 def Popen(*args, **kwargs):
     print('hi')
 
-pop('gcc --version', shell=True)
-Popen('gcc --version', shell=True)
+pop('/bin/gcc --version', shell=True)
+Popen('/bin/gcc --version', shell=True)
 
-subprocess.Popen('gcc --version', shell=True)
-subprocess.Popen(['gcc', '--version'], shell=False)
-subprocess.Popen(['gcc', '--version'])
+subprocess.Popen('/bin/gcc --version', shell=True)
+subprocess.Popen(['/bin/gcc', '--version'], shell=False)
+subprocess.Popen(['/bin/gcc', '--version'])
 
-subprocess.call(["ls",
+subprocess.call(["/bin/ls",
                  "-l"
                  ])
-subprocess.call('ls -l', shell=True)
+subprocess.call('/bin/ls -l', shell=True)
 
-subprocess.check_call(['ls', '-l'], shell=False)
-subprocess.check_call('ls -l', shell=True)
+subprocess.check_call(['/bin/ls', '-l'], shell=False)
+subprocess.check_call('/bin/ls -l', shell=True)
 
-subprocess.check_output(['ls', '-l'])
-subprocess.check_output('ls -l', shell=True)
+subprocess.check_output(['/bin/ls', '-l'])
+subprocess.check_output('/bin/ls -l', shell=True)

examples/utils-shell.py

@@ -1,10 +1,8 @@
 import utils
 import utils as u
 
-u.execute('gcc --version', shell=True)
-utils.execute('gcc --version', shell=True)
-u.execute_with_timeout('gcc --version', shell=True)
-utils.execute_with_timeout('gcc --version', shell=True)
-utils.execute_with_timeout(['gcc', '--version'], shell=False)
-
-
+u.execute('/bin/gcc --version', shell=True)
+utils.execute('/bin/gcc --version', shell=True)
+u.execute_with_timeout('/bin/gcc --version', shell=True)
+utils.execute_with_timeout('/bin/gcc --version', shell=True)
+utils.execute_with_timeout(['/bin/gcc', '--version'], shell=False)

examples/wildcard-injection.py

@@ -2,15 +2,15 @@ import os as o
 import subprocess as subp
 
 # Vulnerable to wildcard injection
-o.system("tar xvzf *")
-o.system('chown *')
-o.popen2('chmod *')
-subp.Popen('chown *', shell=True)
+o.system("/bin/tar xvzf *")
+o.system('/bin/chown *')
+o.popen2('/bin/chmod *')
+subp.Popen('/bin/chown *', shell=True)
 
 # Not vulnerable to wildcard injection
-subp.Popen('rsync *')
-subp.Popen("chmod *")
-subp.Popen(['chown', '*'])
-subp.Popen(["chmod", sys.argv[1], "*"],
+subp.Popen('/bin/rsync *')
+subp.Popen("/bin/chmod *")
+subp.Popen(['/bin/chown', '*'])
+subp.Popen(["/bin/chmod", sys.argv[1], "*"],
                  stdin=subprocess.PIPE, stdout=subprocess.PIPE)
 o.spawnvp(os.P_WAIT, 'tar', ['tar', 'xvzf', '*'])

tests/test_functional.py

@@ -372,6 +372,13 @@ class FunctionalTests(unittest.TestCase):
                   'CONFIDENCE': {'HIGH': 2}}
         self.check_example('paramiko_injection.py', expect)
 
+    def test_partial_path(self):
+        '''Test process spawning with partial file paths.'''
+        expect = {'SEVERITY': {'LOW': 9},
+                  'CONFIDENCE': {'HIGH': 9}}
+
+        self.check_example('partial_path_process.py', expect)
+
     def test_multiline_code(self):
         '''Test issues in multiline statements return code as expected.'''
         self.run_example('multiline-str.py')