safe_eval fix (#57188)

* just dont pass locals - also fix globals - added tests * fixed tests (cherry picked from commit b9b0b23)
ansible · Jun 21, 2019 · 04e9427 · 04e9427
1 parent 5e0c623
commit 04e9427
Show file tree

Hide file tree

Showing 10 changed files with 73 additions and 12 deletions.
diff --git a/changelogs/fragments/fix_safe_eval.yml b/changelogs/fragments/fix_safe_eval.yml
@@ -0,0 +1,2 @@
+bugfixes:
+    - Handle improper variable substitution that was happening in safe_eval, it was always meant to just do 'type enforcement' and have Jinja2 deal with all variable interpolation. Also see CVE-2019-10156
diff --git a/lib/ansible/template/__init__.py b/lib/ansible/template/__init__.py
@@ -545,7 +545,7 @@ def template(self, variable, convert_bare=False, preserve_trailing_newlines=True
                                 # if this looks like a dictionary or list, convert it to such using the safe_eval method
                                 if (result.startswith("{") and not result.startswith(self.environment.variable_start_string)) or \
                                         result.startswith("[") or result in ("True", "False"):
-                                    eval_results = safe_eval(result, locals=self._available_variables, include_exceptions=True)
+                                    eval_results = safe_eval(result, include_exceptions=True)
                                     if eval_results[1] is None:
                                         result = eval_results[0]
                                         if unsafe:

diff --git a/lib/ansible/template/safe_eval.py b/lib/ansible/template/safe_eval.py
@@ -42,10 +42,14 @@ def safe_eval(expr, locals=None, include_exceptions=False):
 
     # define certain JSON types
     # eg. JSON booleans are unknown to python eval()
-    JSON_TYPES = {
+    OUR_GLOBALS = {
+        '__builtins__': {},  # avoid global builtins as per eval docs
         'false': False,
         'null': None,
         'true': True,
+        # also add back some builtins we do need
+        'True': True,
+        'False': False,
     }
 
     # this is the whitelist of AST nodes we are going to
@@ -138,7 +142,7 @@ def generic_visit(self, node, inside_call=False):
         # Note: passing our own globals and locals here constrains what
         # callables (and other identifiers) are recognized.  this is in
         # addition to the filtering of builtins done in CleansingNodeVisitor
-        result = eval(compiled, JSON_TYPES, dict(locals))
+        result = eval(compiled, OUR_GLOBALS, dict(locals))
 
         if include_exceptions:
             return (result, None)

diff --git a/test/integration/targets/docker_image/tasks/tests/old-options.yml b/test/integration/targets/docker_image/tasks/tests/old-options.yml
@@ -5,7 +5,7 @@
 
 - name: Registering image name
   set_fact:
-    inames: "{{ inames }} + [iname]"
+    inames: "{{ inames + [iname]}}"
 
 ####################################################################
 ## build ###########################################################

diff --git a/test/integration/targets/meraki_static_route/tasks/main.yml b/test/integration/targets/meraki_static_route/tasks/main.yml
@@ -35,7 +35,7 @@
     register: create_route
 
   - set_fact:
-      route_ids: "{{ route_ids }} + [ '{{ create_route.data.id }}' ]"
+      route_ids: "{{ route_ids + [create_route.data.id] }}"
 
   - name: Create second static_route
     meraki_static_route:
@@ -50,7 +50,7 @@
     register: second_create
 
   - set_fact:
-      route_ids: "{{ route_ids }} + [ '{{ second_create.data.id }}' ]"
+      route_ids: "{{ route_ids + [second_create.data.id] }}"
 
   - assert:
       that:
@@ -166,4 +166,4 @@
         state: absent
         org_name: '{{test_org_name}}'
         net_name: IntTestNetwork
-      delegate_to: localhost
+      delegate_to: localhost
diff --git a/test/integration/targets/netapp_eseries_host/tasks/run.yml b/test/integration/targets/netapp_eseries_host/tasks/run.yml
@@ -204,7 +204,7 @@
   set_fact:
     port_info: []
 - set_fact:
-    port_info: "{{ port_info }} + [{{ item[0] |combine(item[1]) }}]"
+    port_info: "{{ port_info + [item[0] |combine(item[1])] }}"
   loop: "{{ tmp }}"
 
 # Compile list of expected host port information for verifying changes
@@ -225,7 +225,7 @@
   set_fact:
     expected_port_info: []
 - set_fact:
-    expected_port_info: "{{ expected_port_info }} + [{{ item[0] |combine(item[1]) }}]"
+    expected_port_info: "{{ expected_port_info + [ item[0] |combine(item[1]) ] }}"
   loop: "{{ tmp }}"
 
 # Verify that each host object has the expected protocol type and address/port

diff --git a/test/integration/targets/postgresql/tasks/main.yml b/test/integration/targets/postgresql/tasks/main.yml
@@ -235,7 +235,7 @@
     - 'yes'
 
 - set_fact:
-    encryption_values: '{{ encryption_values }} + ["no"]'
+    encryption_values: '{{ encryption_values + ["no"]}}'
   when: postgres_version_resp.stdout is version('10', '<=')
 
 - include: test_password.yml

diff --git a/test/integration/targets/template/corner_cases.yml b/test/integration/targets/template/corner_cases.yml
@@ -0,0 +1,51 @@
+- name: test tempating corner cases
+  hosts: localhost
+  gather_facts: false
+  vars:
+    empty_list: []
+    dont: I SHOULD NOT BE TEMPLATED
+    other: I WORK
+  tasks:
+    - name: 'ensure we are not interpolating data from outside of j2 delmiters'
+      assert:
+        that:
+            - '"I SHOULD NOT BE TEMPLATED" not in adjacent'
+            - globals1 == "[[], globals()]"
+            - globals2 == "[[], globals]"
+      vars:
+        adjacent: "{{ empty_list }} + [dont]"
+        globals1: "[{{ empty_list }}, globals()]"
+        globals2: "[{{ empty_list }}, globals]"
+
+    - name: 'ensure we can add lists'
+      assert:
+        that:
+            - (empty_list + [other]) == [other]
+            - (empty_list + [other, other]) == [other, other]
+            - (dont_exist|default([]) + [other]) == [other]
+            - ([other] + [empty_list, other]) == [other, [], other]
+
+    - name: 'ensure comments go away and we still dont interpolate in string'
+      assert:
+        that:
+            - 'comm1 == " + [dont]"'
+            - 'comm2 == " #} + [dont]"'
+      vars:
+        comm1: '{# {{nothing}} {# #} + [dont]'
+        comm2: "{# {{nothing}} {# #} #} + [dont]"
+
+    - name: test additions with facts, set them up
+      set_fact:
+        inames: []
+        iname: "{{ prefix ~ '-options' }}"
+        iname_1: "{{ prefix ~ '-options-1' }}"
+      vars:
+        prefix: 'bo'
+
+    - name: add the facts
+      set_fact:
+        inames: '{{ inames + [iname, iname_1] }}'
+
+    - assert:
+        that:
+            - inames == ['bo-options', 'bo-options-1']
diff --git a/test/integration/targets/template/runme.sh b/test/integration/targets/template/runme.sh
@@ -12,3 +12,7 @@ ansible-playbook ansible_managed.yml -c  ansible_managed.cfg -i ../../inventory
 
 # Test for #42585
 ANSIBLE_ROLES_PATH=../ ansible-playbook custom_template.yml -i ../../inventory -e @../../integration_config.yml -v "$@"
+
+
+# Test for several corner cases #57188
+ansible-playbook corner_cases.yml -v "$@"
diff --git a/test/legacy/ovs.yaml b/test/legacy/ovs.yaml
@@ -22,7 +22,7 @@
         when: "limit_to in ['*', 'openvswitch_db']"
       rescue:
         - set_fact:
-            failed_modules: "{{ failed_modules }} + [ 'openvswitch_db' ]"
+            failed_modules: "{{ failed_modules + [ 'openvswitch_db' ]}}"
             test_failed: true
 
 
@@ -33,4 +33,4 @@
     - name: Has any previous test failed?
       fail:
         msg: "One or more tests failed, check log for details"
-      when: test_failed
+      when: test_failed