From 8254c266f962d5febe46396d5083bb9c1da74840 Mon Sep 17 00:00:00 2001 From: Brian Coca 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 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