openstack
/
bandit
Code Issues Proposed changes

Distinguish between formatted and simple commands 

Originally shell=True did not check what kind of command is being run.
This patch extends the logic to treat static strings differently than
computed ones (but still, not ignore them).

New checks are split into 3 categories: computed string, static string
with special shell chars, and simple static string. Those have
respectively HIGH, MEDIUM and LOW severity.

New results:

> Issue: subprocess call with shell=True seems safe, but may be changed
> in the future, consider rewriting without shell
   Severity: Low   Confidence: High
   Location: examples/subprocess_shell.py:24
24subprocess.check_output('/bin/ls -l', shell=True)

>> Issue: call with shell=True contains special shell characters,
>> consider moving extra logic into Python code
   Severity: Medium   Confidence: High
   Location: examples/subprocess_shell.py:26
26subprocess.Popen('/bin/ls *', shell=True)

>> Issue: subprocess call with shell=True identified, security issue.
   Severity: High   Confidence: High
   Location: examples/subprocess_shell.py:27
27subprocess.Popen('/bin/ls %s' % ('something',), shell=True)
28subprocess.Popen('/bin/ls {}'.format('something'), shell=True)

co-authored-by: Tim Kelsey <tim.kelsey@hpe.com>
Change-Id: Ib5dde50cd12d2648cd3d67d449b9578e9f2943db
Closes-bug: 1509061
This commit is contained in:
Stanisław Pitucha 2015-10-23 11:30:08 +11:00 committed by Timothy Kelsey
parent 3acbc1db06
commit 70d01d3bc7
4 changed files with 84 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

51
bandit/plugins/injection_shell.py
View File

@ -15,23 +15,62 @@
# under the License.
import ast
import re
import bandit
from bandit.core.test_properties import checks
from bandit.core.test_properties import takes_config
def _has_special_characters(command):
# check if it contains any of the characters that may cause globing,
# multiple commands, subshell, or variable resolution
# glob: [ { * ?
# variable: $
# subshell: ` $
return bool(re.search(r'[{|\[;$\*\?`]', command))
@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'):
return bandit.Issue(
severity=bandit.HIGH,
confidence=bandit.HIGH,
text="subprocess call with shell=True identified, security "
"issue."
)
if len(context.call_args) > 0:
command = context.call_args[0]
no_formatting = isinstance(command, str)
if no_formatting:
no_special_chars = not _has_special_characters(command)
else:
no_special_chars = False
if no_formatting and no_special_chars:
return bandit.Issue(
severity=bandit.LOW,
confidence=bandit.HIGH,
text="subprocess call with shell=True seems safe, but "
"may be changed in the future, consider "
"rewriting without shell"
)
elif no_formatting:
return bandit.Issue(
severity=bandit.MEDIUM,
confidence=bandit.HIGH,
text="call with shell=True contains special shell "
"characters, consider moving extra logic into "
"Python code"
)
else:
return bandit.Issue(
severity=bandit.HIGH,
confidence=bandit.HIGH,
text="subprocess call with shell=True identified, "
"security issue."
)
else:
# no arguments? no issue
pass
@takes_config('shell_injection')

36
docs/source/tests/subprocess_popen_with_shell_equals_true.rst
View File

@ -13,8 +13,16 @@ spawning and warn appropriately. Specifically, this test looks for the spawning
of a subprocess using a command shell. This type of subprocess invocation is
dangerous as it is vulnerable to various shell injection attacks. Great care
should be taken to sanitize all input in order to mitigate this risk. Calls of
this type are identified by a parameter of "shell=True" being given in addition
to the command to run, Bandit will report a HIGH severity warning.
this type are identified by a parameter of "shell=True" being given.
Additionally, this plugin scans the command string given and adjusts its
reported severity based on how it is presented. If the command string is a
simple static string containing no special shell characters, then the resulting
issue has low severity. If the string is static, but contains shell formatting
characters or wildcards, then the reported issue is medium. Finally, if the
string is computed using Python's string manipulation or formatting operations,
then the reported issue has high severity. These severity levels reflect the
likelihood that the code is vulnerable to injection.
See also:
@ -53,12 +61,28 @@ Sample Output
-------------
.. code-block:: none
>> Issue: subprocess call with shell=True seems safe, but may be changed in
the future, consider rewriting without shell
Severity: Low Confidence: High
Location: ./examples/subprocess_shell.py:21
20 subprocess.check_call(['/bin/ls', '-l'], shell=False)
21 subprocess.check_call('/bin/ls -l', shell=True)
22
>> Issue: call with shell=True contains special shell characters, consider
moving extra logic into Python code
Severity: Medium Confidence: High
Location: ./examples/subprocess_shell.py:26
25
26 subprocess.Popen('/bin/ls *', shell=True)
27 subprocess.Popen('/bin/ls %s' % ('something',), shell=True)
>> Issue: subprocess call with shell=True identified, security issue.
Severity: High Confidence: High
Location: ./examples/subprocess_shell.py:24
23 subprocess.check_output(['/bin/ls', '-l'])
24 subprocess.check_output('/bin/ls -l', shell=True)
25
Location: ./examples/subprocess_shell.py:27
26 subprocess.Popen('/bin/ls *', shell=True)
27 subprocess.Popen('/bin/ls %s' % ('something',), shell=True)
28 subprocess.Popen('/bin/ls {}'.format('something'), shell=True)
References
----------

4
examples/subprocess_shell.py
View File

@ -22,3 +22,7 @@ subprocess.check_call('/bin/ls -l', shell=True)
subprocess.check_output(['/bin/ls', '-l'])
subprocess.check_output('/bin/ls -l', shell=True)
subprocess.Popen('/bin/ls *', shell=True)
subprocess.Popen('/bin/ls %s' % ('something',), shell=True)
subprocess.Popen('/bin/ls {}'.format('something'), shell=True)

10
tests/functional/test_functional.py
View File

@ -168,7 +168,7 @@ class FunctionalTests(testtools.TestCase):
def test_imports_aliases(self):
'''Test the `import X as Y` syntax.'''
expect = {
'SEVERITY': {'LOW': 3, 'MEDIUM': 5, 'HIGH': 1},
'SEVERITY': {'LOW': 4, 'MEDIUM': 5, 'HIGH': 0},
'CONFIDENCE': {'HIGH': 9}
}
self.check_example('imports-aliases.py', expect)
@ -297,8 +297,8 @@ class FunctionalTests(testtools.TestCase):
def test_subprocess_shell(self):
'''Test for `subprocess.Popen` with `shell=True`.'''
expect = {
'SEVERITY': {'HIGH': 5, 'MEDIUM': 1, 'LOW': 7},
'CONFIDENCE': {'HIGH': 13}
'SEVERITY': {'HIGH': 2, 'MEDIUM': 2, 'LOW': 12},
'CONFIDENCE': {'HIGH': 16}
}
self.check_example('subprocess_shell.py', expect)
@ -310,7 +310,7 @@ class FunctionalTests(testtools.TestCase):
def test_utils_shell(self):
'''Test for `utils.execute*` with `shell=True`.'''
expect = {
'SEVERITY': {'HIGH': 4, 'LOW': 1},
'SEVERITY': {'LOW': 5},
'CONFIDENCE': {'HIGH': 5}
}
self.check_example('utils-shell.py', expect)
@ -318,7 +318,7 @@ class FunctionalTests(testtools.TestCase):
def test_wildcard_injection(self):
'''Test for wildcard injection in shell commands.'''
expect = {
'SEVERITY': {'HIGH': 5, 'MEDIUM':3, 'LOW': 6},
'SEVERITY': {'HIGH': 4, 'MEDIUM':4, 'LOW': 6},
'CONFIDENCE': {'MEDIUM': 8, 'HIGH': 6}
}
self.check_example('wildcard-injection.py', expect)