From 8254c266f962d5febe46396d5083bb9c1da74840 Mon Sep 17 00:00:00 2001

From: Brian Coca <brian.coca+git@gmail.com>

Date: Tue, 4 Jun 2019 08:43:15 -0400

Subject: [PATCH 1/2] just dont pass locals

 - also fix globals

 - added tests

---

 changelogs/fragments/fix_safe_eval.yml        |  2 +

 lib/ansible/template/__init__.py              |  2 +-

 lib/ansible/template/safe_eval.py             |  8 ++-

 .../targets/template/corner_cases.yml         | 51 +++++++++++++++++++

 test/integration/targets/template/runme.sh    |  4 ++

 5 files changed, 64 insertions(+), 3 deletions(-)

 create mode 100644 changelogs/fragments/fix_safe_eval.yml

 create mode 100644 test/integration/targets/template/corner_cases.yml

diff --git a/changelogs/fragments/fix_safe_eval.yml b/changelogs/fragments/fix_safe_eval.yml

new file mode 100644

index 000000000000..19220b34ffb1

--- /dev/null

+++ 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

index f88b7165db1f..ec4bf6771379 100644

--- a/lib/ansible/template/__init__.py

+++ b/lib/ansible/template/__init__.py

@@ -543,7 +543,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

index 9c70be4a897d..4f5b856180af 100644

--- 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/template/corner_cases.yml b/test/integration/targets/template/corner_cases.yml

new file mode 100644

index 000000000000..48782f79590c

--- /dev/null

+++ 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

index 2b58bc92dd3e..c4f50b1c7e28 100755

--- 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 "$@"

From fbda0028750a17a032d83dad9d1fb284f9ea68a4 Mon Sep 17 00:00:00 2001

From: Brian Coca <brian.coca+git@gmail.com>

Date: Thu, 6 Jun 2019 13:26:14 -0400

Subject: [PATCH 2/2] fixed tests

---

 .../targets/docker_image/tasks/tests/old-options.yml        | 2 +-

 test/integration/targets/meraki_static_route/tasks/main.yml | 6 +++---

 test/integration/targets/netapp_eseries_host/tasks/run.yml  | 4 ++--

 test/integration/targets/postgresql/tasks/main.yml          | 2 +-

 test/legacy/ovs.yaml                                        | 4 ++--

 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/test/integration/targets/docker_image/tasks/tests/old-options.yml b/test/integration/targets/docker_image/tasks/tests/old-options.yml

index 5571cf96fabc..5824a56d1fec 100644

--- 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

index 322e36f855e7..10ba31eab975 100644

--- 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:

@@ -167,4 +167,4 @@

         state: absent

         org_name: '{{test_org_name}}'

         net_name: IntTestNetwork

-      delegate_to: localhost

\ No newline at end of file

+      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

index fd0a8d5fa209..70519b4b9423 100644

--- 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

index d395b2820f13..5d3a21b61e71 100644

--- 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/legacy/ovs.yaml b/test/legacy/ovs.yaml

index 4eff414f1de8..35d3acc0fd2f 100644

--- 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

\ No newline at end of file

+      when: test_failed