False positive for jinja template with autoescape and select_autoescape

[kinow]

Describe the bug

If a developer does not import from jinja2 import select_autoescape, but instead imports import jinja2 and then use it as autoescape=jinja2.autoescape, it causes bandit to report it as an issue B701:jinja2_autoescape_false.

While working on Cylc started working on an issue to fix Codacy issues, and noticed that this issue was still reported regardless of the jinja2.autoescape. Here's the link to the Codacy reported issue (no idea how long Codacy keeps it though).

It looks like this was an issue in the past (https://bugs.launchpad.net/bandit/+bug/1684249 later migrated to GitHub as #271). But the problem is that the current fix only looks at the AST's function ID. Here's the current code:

bandit/bandit/plugins/jinja2_templates.py

Lines 109 to 112 in 3371a6d

	# Check if select_autoescape function is used.
	elif isinstance(value, ast.Call) and getattr(
	value.func, 'id', None) == 'select_autoescape':
	return

But when you use jinja2.select_autoescape, the select_autoescape part is now the argument of the call function, not the ID.

To Reproduce
Steps to reproduce the behavior:

1. Create an example file as follows with some name like example1.py:

# file: example1.py
import jinja2

template_env = jinja2.Environment(
    loader=jinja2.FileSystemLoader("/abc/templates"),
    autoescape=jinja2.select_autoescape(
        enabled_extensions=('html', 'xml'))
)

2. Run bandit against the file
3. Note the error reported in the terminal as follows:

$ bandit /home/kinow/Development/python/workspace/cylc/lib/cylc/reviewpy
[main]	INFO	profile include tests: None
[main]	INFO	profile exclude tests: None
[main]	INFO	cli include tests: None
[main]	INFO	cli exclude tests: None
[main]	INFO	running on Python 3.7.2
Run started:2019-02-10 11:18:29.313192

Test results:
	No issues identified.

Code scanned:
	Total lines of code: 0
	Total lines skipped (#nosec): 0

Run metrics:
	Total issues (by severity):
		Undefined: 0
		Low: 0
		Medium: 0
		High: 0
	Total issues (by confidence):
		Undefined: 0
		Low: 0
		Medium: 0
		High: 0
Files skipped (1):
	/home/kinow/Development/python/workspace/cylc/lib/cylc/reviewpy (No such file or directory)
(venv) kinow@ranma:/tmp$ bandit /home/kinow/Development/python/workspace/cylc/lib/cylc/review.py
[main]	INFO	profile include tests: None
[main]	INFO	profile exclude tests: None
[main]	INFO	cli include tests: None
[main]	INFO	cli exclude tests: None
[main]	INFO	running on Python 3.7.2
Run started:2019-02-10 11:18:31.650883

Test results:
>> Issue: [B701:jinja2_autoescape_false] Using jinja2 templates with autoescape=False is dangerous and can lead to XSS. Ensure autoescape=True or use the select_autoescape function to mitigate XSS vulnerabilities.
   Severity: High   Confidence: Medium
   Location: /home/kinow/Development/python/workspace/cylc/lib/cylc/review.py:83
   More Info: https://bandit.readthedocs.io/en/latest/plugins/b701_jinja2_autoescape_false.html
82	        # Autoescape markup to prevent code injection from user inputs.
83	        template_env = jinja2.Environment(
84	            loader=jinja2.FileSystemLoader(
85	                get_util_home("lib", "cylc", "cylc-review", "template")),
86	            autoescape=jinja2.select_autoescape(
87	                enabled_extensions=('html', 'xml'), default_for_string=True),
88	        )

--------------------------------------------------

Code scanned:
	Total lines of code: 799
	Total lines skipped (#nosec): 0

Run metrics:
	Total issues (by severity):
		Undefined: 0.0
		Low: 0.0
		Medium: 0.0
		High: 1.0
	Total issues (by confidence):
		Undefined: 0.0
		Low: 0.0
		Medium: 1.0
		High: 0.0
Files skipped (0):

4. Then alter the code to use from jinja2.select_autoescape and modify the autoescape=select_autoescape part too. After that bandit stops reporting the issue. Both code examples are valid, and the same functionality / security.

Expected behavior
No issues reported.

Bandit version

bandit 1.5.1
  python version = 3.7.2 (default, Dec 29 2018, 06:19:36) [GCC 7.3.0]

Additional context
To confirm what is going on, it is also possible to play with ast, astor, and produce some examples.

>>> import ast
>>> import astor
>>> from pprint import pprint
>>> 
>>> 
>>> # works with current version
... s1 = """
... from jinja2 import select_autoescape, Environment, FileSystemLoader
... 
... template_env = Environment(
...     loader=FileSystemLoader("/abc/templates"),
...     autoescape=select_autoescape(
...         enabled_extensions=('html', 'xml'))
... )
... """
>>> 
>>> # broken in current version, but same as previous code
... s2 = """
... import jinja2
... 
... template_env = jinja2.Environment(
...     loader=jinja2.FileSystemLoader("/abc/templates"),
...     autoescape=jinja2.select_autoescape(
...         enabled_extensions=('html', 'xml'))
... )
... """
>>> 
>>> tree1 = ast.parse(s1)
>>> tree2 = ast.parse(s2)
>>> 

Here the first tree (tree1) is what works now.

>>> pprint(astor.dump_tree(tree1))
('Module(\n'
 '    body=[\n'
 "        ImportFrom(module='jinja2',\n"
 "            names=[alias(name='select_autoescape', asname=None),\n"
 "                alias(name='Environment', asname=None),\n"
 "                alias(name='FileSystemLoader', asname=None)],\n"
 '            level=0),\n'
 "        Assign(targets=[Name(id='template_env')],\n"
 "            value=Call(func=Name(id='Environment'),\n"
 '                args=[],\n'
 '                keywords=[\n'
 "                    keyword(arg='loader',\n"
 "                        value=Call(func=Name(id='FileSystemLoader'), "
 "args=[Str(s='/abc/templates')], keywords=[])),\n"
 "                    keyword(arg='autoescape',\n"
 "                        value=Call(func=Name(id='select_autoescape'),\n"
 '                            args=[],\n'
 '                            keywords=[\n'
 "                                keyword(arg='enabled_extensions', "
 "value=Tuple(elts=[Str(s='html'), Str(s='xml')]))]))]))])")

Note the value=Call(func=Name(id='select_autoescape'),\n", where the function ID is present.

Now the tree2, which has jinja2.select_autoescape.

>>> pprint(astor.dump_tree(tree2))
('Module(\n'
 "    body=[Import(names=[alias(name='jinja2', asname=None)]),\n"
 "        Assign(targets=[Name(id='template_env')],\n"
 "            value=Call(func=Attribute(value=Name(id='jinja2'), "
 "attr='Environment'),\n"
 '                args=[],\n'
 '                keywords=[\n'
 "                    keyword(arg='loader',\n"
 "                        value=Call(func=Attribute(value=Name(id='jinja2'), "
 "attr='FileSystemLoader'),\n"
 "                            args=[Str(s='/abc/templates')],\n"
 '                            keywords=[])),\n'
 "                    keyword(arg='autoescape',\n"
 "                        value=Call(func=Attribute(value=Name(id='jinja2'), "
 "attr='select_autoescape'),\n"
 '                            args=[],\n'
 '                            keywords=[\n'
 "                                keyword(arg='enabled_extensions', "
 "value=Tuple(elts=[Str(s='html'), Str(s='xml')]))]))]))])")

Note that here what we have is actually value=Call(func=Attribute(value=Name(id='jinja2'), "
"attr='select_autoescape'),\n". Where we have the ID being the module imported or alias, and the attribute being now where we can find select_autoescape.

Finally, we could discuss adding further validation for the module alias. Checking the context of the parsed tree we could find how the user aliased the module jinja2. But I am not sure if that's necessary.

Here's what I mean.

>>> s3 = """
... import jinja2 as not_what_you_expected
... 
... template_env = jinja2.Environment(
...     loader=jinja2.FileSystemLoader("/abc/templates"),
...     autoescape=not_what_you_expected.select_autoescape(
...         enabled_extensions=('html', 'xml'))
... )
... """
>>> 
>>> tree3 = ast.parse(s3)
>>> 
>>> pprint(astor.dump_tree(tree3))
('Module(\n'
 "    body=[Import(names=[alias(name='jinja2', "
 "asname='not_what_you_expected')]),\n"
 "        Assign(targets=[Name(id='template_env')],\n"
 "            value=Call(func=Attribute(value=Name(id='jinja2'), "
 "attr='Environment'),\n"
 '                args=[],\n'
 '                keywords=[\n'
 "                    keyword(arg='loader',\n"
 "                        value=Call(func=Attribute(value=Name(id='jinja2'), "
 "attr='FileSystemLoader'),\n"
 "                            args=[Str(s='/abc/templates')],\n"
 '                            keywords=[])),\n'
 "                    keyword(arg='autoescape',\n"
 '                        value=Call(\n'
 '                            '
 "func=Attribute(value=Name(id='not_what_you_expected'), "
 "attr='select_autoescape'),\n"
 '                            args=[],\n'
 '                            keywords=[\n'
 "                                keyword(arg='enabled_extensions', "
 "value=Tuple(elts=[Str(s='html'), Str(s='xml')]))]))]))])")

Here we actually have ** "func=Attribute(value=Name(id='not_what_you_expected'), "
"attr='select_autoescape'),\n"**. Where not_what_you_expected is the alias of the jinja2 module as it was imported.

Hope that makes sense.

Thank you for bandit 👍
Bruno

[ericwb]

Fixed with #454