Additional lock down of conditionals

lib/ansible/playbook/conditional.py

@@ -29,6 +29,7 @@ from ansible.compat.six import text_type
 from ansible.errors import AnsibleError, AnsibleUndefinedVariable
 from ansible.playbook.attribute import FieldAttribute
 from ansible.template import Templar
+from ansible.template.safe_eval import safe_eval
 from ansible.module_utils._text import to_native
 
 DEFINED_REGEX = re.compile(r'(hostvars\[.+\]|[\w_]+)\s+(not\s+is|is|is\s+not)\s+(defined|undefined)')
@@ -104,7 +105,9 @@ class Conditional:
                 if not self._check_conditional(conditional, templar, all_vars):
                     return False
         except Exception as e:
-            raise AnsibleError("The conditional check '%s' failed. The error was: %s" % (to_native(conditional), to_native(e)), obj=ds)
+            raise AnsibleError(
+                "The conditional check '%s' failed. The error was: %s" % (to_native(conditional), to_native(e)), obj=ds
+            )
 
         return True
 
@@ -143,35 +146,50 @@ class Conditional:
             # and we don't want future templating calls to do unsafe things
             disable_lookups |= hasattr(conditional, '__UNSAFE__')
 
-            # now we generated the "presented" string, which is a jinja2 if/else block
-            # used to evaluate the conditional. First, we do some low-level jinja2 parsing
-            # involving the AST format of the statement to ensure we don't do anything
-            # unsafe (using the disable_lookup flag above)
-            e = templar.environment.overlay()
-            e.filters.update(templar._get_filters())
-            e.tests.update(templar._get_tests())
-
-            presented = "{%% if %s %%} True {%% else %%} False {%% endif %%}" % conditional
-            res = e._parse(presented, None, None)
-            res = generate(res, e, None, None)
-            parsed = ast.parse(res, mode='exec')
-
+            # First, we do some low-level jinja2 parsing involving the AST format of the
+            # statement to ensure we don't do anything unsafe (using the disable_lookup flag above)
             class CleansingNodeVisitor(ast.NodeVisitor):
-                def generic_visit(self, node, inside_call=False):
+                def generic_visit(self, node, inside_call=False, inside_yield=False):
                     if isinstance(node, ast.Call):
                         inside_call = True
+                    elif isinstance(node, ast.Yield):
+                        inside_yield = True
                     elif isinstance(node, ast.Str):
-                        # calling things with a dunder is generally bad at this point...
-                        if inside_call and disable_lookups and node.s.startswith("__"):
-                            raise AnsibleError("Invalid access found in the presented conditional: '%s'" % conditional)
+                        if disable_lookups:
+                            if inside_call and node.s.startswith("__"):
+                                # calling things with a dunder is generally bad at this point...
+                                raise AnsibleError(
+                                    "Invalid access found in the conditional: '%s'" % conditional
+                                )
+                            elif inside_yield:
+                                # we're inside a yield, so recursively parse and traverse the AST
+                                # of the result to catch forbidden syntax from executing
+                                parsed = ast.parse(node.s, mode='exec')
+                                cnv = CleansingNodeVisitor()
+                                cnv.visit(parsed)
                     # iterate over all child nodes
                     for child_node in ast.iter_child_nodes(node):
-                        self.generic_visit(child_node, inside_call=inside_call)
+                        self.generic_visit(
+                            child_node,
+                            inside_call=inside_call,
+                            inside_yield=inside_yield
+                        )
+            try:
+                e = templar.environment.overlay()
+                e.filters.update(templar._get_filters())
+                e.tests.update(templar._get_tests())
+
+                res = e._parse(conditional, None, None)
+                res = generate(res, e, None, None)
+                parsed = ast.parse(res, mode='exec')
 
-            cnv = CleansingNodeVisitor()
-            cnv.visit(parsed)
+                cnv = CleansingNodeVisitor()
+                cnv.visit(parsed)
+            except Exception as e:
+                raise AnsibleError("Invalid conditional detected: %s" % to_native(e))
 
-            # and finally we templated the presented string and look at the resulting string
+            # and finally we generate and template the presented string and look at the resulting string
+            presented = "{%% if %s %%} True {%% else %%} False {%% endif %%}" % conditional
             val = templar.template(presented, disable_lookups=disable_lookups).strip()
             if val == "True":
                 return True
@@ -180,8 +198,8 @@ class Conditional:
             else:
                 raise AnsibleError("unable to evaluate conditional: %s" % original)
         except (AnsibleUndefinedVariable, UndefinedError) as e:
-            # the templating failed, meaning most likely a variable was undefined. If we happened to be
-            # looking for an undefined variable, return True, otherwise fail
+            # the templating failed, meaning most likely a variable was undefined. If we happened
+            # to be looking for an undefined variable, return True, otherwise fail
             try:
                 # first we extract the variable name from the error message
                 var_name = re.compile(r"'(hostvars\[.+\]|[\w_]+)' is undefined").search(str(e)).groups()[0]
@@ -206,5 +224,7 @@ class Conditional:
                 # trigger the AnsibleUndefinedVariable exception again below
                 raise
             except Exception as new_e:
-                raise AnsibleUndefinedVariable("error while evaluating conditional (%s): %s" % (original, e))
+                raise AnsibleUndefinedVariable(
+                    "error while evaluating conditional (%s): %s" % (original, e)
+                )