From aabcc270842a9cbd5ff2258b6ee4b47a14a9b915 Mon Sep 17 00:00:00 2001 From: Jordan Borean Date: Thu, 21 Nov 2024 05:56:25 +1000 Subject: [PATCH] Fix CVE-2024-11079 hostvars unsafe context (#84339) Fix to preserve an unsafe variable when accessing through an intermediary variable from hostvars. (cherry picked from commit 2936b80dbbc7efb889934aeec80f6142c10266ce) --- changelogs/fragments/unsafe_hostvars_fix.yml | 2 + lib/ansible/template/__init__.py | 31 +---- lib/ansible/template/native_helpers.py | 122 +++++++++++++++++- lib/ansible/vars/hostvars.py | 7 +- .../targets/template/cve-2024-11079.yml | 30 +++++ test/integration/targets/template/runme.sh | 4 + 6 files changed, 159 insertions(+), 37 deletions(-) create mode 100644 changelogs/fragments/unsafe_hostvars_fix.yml create mode 100644 test/integration/targets/template/cve-2024-11079.yml diff --git a/changelogs/fragments/unsafe_hostvars_fix.yml b/changelogs/fragments/unsafe_hostvars_fix.yml new file mode 100644 index 00000000000000..cf7ca888e23473 --- /dev/null +++ b/changelogs/fragments/unsafe_hostvars_fix.yml @@ -0,0 +1,2 @@ +security_fixes: + - Templating will not prefer AnsibleUnsafe when a variable is referenced via hostvars - CVE-2024-11079 diff --git a/lib/ansible/template/__init__.py b/lib/ansible/template/__init__.py index 22fce42e03467c..fedda75ac044f2 100644 --- a/lib/ansible/template/__init__.py +++ b/lib/ansible/template/__init__.py @@ -49,7 +49,7 @@ from ansible.module_utils.common.text.converters import to_native, to_text, to_bytes from ansible.module_utils.common.collections import is_sequence from ansible.plugins.loader import filter_loader, lookup_loader, test_loader -from ansible.template.native_helpers import ansible_native_concat, ansible_eval_concat, ansible_concat +from ansible.template.native_helpers import AnsibleUndefined, ansible_native_concat, ansible_eval_concat, ansible_concat from ansible.template.template import AnsibleJ2Template from ansible.template.vars import AnsibleJ2Vars from ansible.utils.display import Display @@ -329,35 +329,6 @@ def wrapper(*args, **kwargs): return _update_wrapper(wrapper, func) -class AnsibleUndefined(StrictUndefined): - ''' - A custom Undefined class, which returns further Undefined objects on access, - rather than throwing an exception. - ''' - def __getattr__(self, name): - if name == '__UNSAFE__': - # AnsibleUndefined should never be assumed to be unsafe - # This prevents ``hasattr(val, '__UNSAFE__')`` from evaluating to ``True`` - raise AttributeError(name) - # Return original Undefined object to preserve the first failure context - return self - - def __getitem__(self, key): - # Return original Undefined object to preserve the first failure context - return self - - def __repr__(self): - return 'AnsibleUndefined(hint={0!r}, obj={1!r}, name={2!r})'.format( - self._undefined_hint, - self._undefined_obj, - self._undefined_name - ) - - def __contains__(self, item): - # Return original Undefined object to preserve the first failure context - return self - - class AnsibleContext(Context): ''' A custom context, which intercepts resolve_or_missing() calls and sets a flag diff --git a/lib/ansible/template/native_helpers.py b/lib/ansible/template/native_helpers.py index abe75c03326bd9..b20ae28cc3a80b 100644 --- a/lib/ansible/template/native_helpers.py +++ b/lib/ansible/template/native_helpers.py @@ -7,13 +7,19 @@ import ast +from collections.abc import Mapping from itertools import islice, chain from types import GeneratorType +from ansible.module_utils.common.collections import is_sequence from ansible.module_utils.common.text.converters import to_text from ansible.module_utils.six import string_types from ansible.parsing.yaml.objects import AnsibleVaultEncryptedUnicode from ansible.utils.native_jinja import NativeJinjaText +from ansible.utils.unsafe_proxy import wrap_var +import ansible.module_utils.compat.typing as t + +from jinja2.runtime import StrictUndefined _JSON_MAP = { @@ -30,6 +36,40 @@ def visit_Name(self, node): return ast.Constant(value=_JSON_MAP[node.id]) +def _is_unsafe(value: t.Any) -> bool: + """ + Our helper function, which will also recursively check dict and + list entries due to the fact that they may be repr'd and contain + a key or value which contains jinja2 syntax and would otherwise + lose the AnsibleUnsafe value. + """ + to_check = [value] + seen = set() + + while True: + if not to_check: + break + + val = to_check.pop(0) + val_id = id(val) + + if val_id in seen: + continue + seen.add(val_id) + + if isinstance(val, AnsibleUndefined): + continue + if isinstance(val, Mapping): + to_check.extend(val.keys()) + to_check.extend(val.values()) + elif is_sequence(val): + to_check.extend(val) + elif getattr(val, '__UNSAFE__', False): + return True + + return False + + def ansible_eval_concat(nodes): """Return a string of concatenated compiled nodes. Throw an undefined error if any of the nodes is undefined. @@ -45,17 +85,28 @@ def ansible_eval_concat(nodes): if not head: return '' + unsafe = False + if len(head) == 1: out = head[0] if isinstance(out, NativeJinjaText): return out + unsafe = _is_unsafe(out) out = to_text(out) else: if isinstance(nodes, GeneratorType): nodes = chain(head, nodes) - out = ''.join([to_text(v) for v in nodes]) + + out_values = [] + for v in nodes: + if not unsafe and _is_unsafe(v): + unsafe = True + + out_values.append(to_text(v)) + + out = ''.join(out_values) # if this looks like a dictionary, list or bool, convert it to such if out.startswith(('{', '[')) or out in ('True', 'False'): @@ -70,6 +121,9 @@ def ansible_eval_concat(nodes): except (TypeError, ValueError, SyntaxError, MemoryError): pass + if unsafe: + out = wrap_var(out) + return out @@ -80,7 +134,19 @@ def ansible_concat(nodes): Used in Templar.template() when jinja2_native=False and convert_data=False. """ - return ''.join([to_text(v) for v in nodes]) + unsafe = False + values = [] + for v in nodes: + if not unsafe and _is_unsafe(v): + unsafe = True + + values.append(to_text(v)) + + out = ''.join(values) + if unsafe: + out = wrap_var(out) + + return out def ansible_native_concat(nodes): @@ -97,6 +163,8 @@ def ansible_native_concat(nodes): if not head: return None + unsafe = False + if len(head) == 1: out = head[0] @@ -117,10 +185,21 @@ def ansible_native_concat(nodes): # short-circuit literal_eval for anything other than strings if not isinstance(out, string_types): return out + + unsafe = _is_unsafe(out) + else: if isinstance(nodes, GeneratorType): nodes = chain(head, nodes) - out = ''.join([to_text(v) for v in nodes]) + + out_values = [] + for v in nodes: + if not unsafe and _is_unsafe(v): + unsafe = True + + out_values.append(to_text(v)) + + out = ''.join(out_values) try: evaled = ast.literal_eval( @@ -130,10 +209,45 @@ def ansible_native_concat(nodes): ast.parse(out, mode='eval') ) except (TypeError, ValueError, SyntaxError, MemoryError): + if unsafe: + out = wrap_var(out) + return out if isinstance(evaled, string_types): quote = out[0] - return f'{quote}{evaled}{quote}' + evaled = f'{quote}{evaled}{quote}' + + if unsafe: + evaled = wrap_var(evaled) return evaled + + +class AnsibleUndefined(StrictUndefined): + """ + A custom Undefined class, which returns further Undefined objects on access, + rather than throwing an exception. + """ + def __getattr__(self, name): + if name == '__UNSAFE__': + # AnsibleUndefined should never be assumed to be unsafe + # This prevents ``hasattr(val, '__UNSAFE__')`` from evaluating to ``True`` + raise AttributeError(name) + # Return original Undefined object to preserve the first failure context + return self + + def __getitem__(self, key): + # Return original Undefined object to preserve the first failure context + return self + + def __repr__(self): + return 'AnsibleUndefined(hint={0!r}, obj={1!r}, name={2!r})'.format( + self._undefined_hint, + self._undefined_obj, + self._undefined_name + ) + + def __contains__(self, item): + # Return original Undefined object to preserve the first failure context + return self diff --git a/lib/ansible/vars/hostvars.py b/lib/ansible/vars/hostvars.py index 0cd620334fe182..9ab0795e8007a5 100644 --- a/lib/ansible/vars/hostvars.py +++ b/lib/ansible/vars/hostvars.py @@ -94,11 +94,12 @@ def __contains__(self, host_name): return self._find_host(host_name) is not None def __iter__(self): - for host in self._inventory.hosts: - yield host + # include implicit localhost only if it has variables set + yield from self._inventory.hosts | {'localhost': self._inventory.localhost} if self._inventory.localhost else {} def __len__(self): - return len(self._inventory.hosts) + # include implicit localhost only if it has variables set + return len(self._inventory.hosts) + (1 if self._inventory.localhost else 0) def __repr__(self): out = {} diff --git a/test/integration/targets/template/cve-2024-11079.yml b/test/integration/targets/template/cve-2024-11079.yml new file mode 100644 index 00000000000000..00eb16d995dc58 --- /dev/null +++ b/test/integration/targets/template/cve-2024-11079.yml @@ -0,0 +1,30 @@ +- name: test CVE-2024-11079 loop variables preserve unsafe hostvars + hosts: localhost + gather_facts: false + tasks: + - set_fact: + foo: + safe: + prop: '{{ "{{" }} unsafe_var {{ "}}" }}' + unsafe: + prop: !unsafe '{{ unsafe_var }}' + + - name: safe var through hostvars loop is templated + assert: + that: + - item.prop == expected + loop: + - "{{ hostvars['localhost']['foo']['safe'] }}" + vars: + unsafe_var: bar + expected: bar + + - name: unsafe var through hostvars loop is not templated + assert: + that: + - item.prop == expected + loop: + - "{{ hostvars['localhost']['foo']['unsafe'] }}" + vars: + unsafe_var: bar + expected: !unsafe '{{ unsafe_var }}' diff --git a/test/integration/targets/template/runme.sh b/test/integration/targets/template/runme.sh index b37467a2719634..f9050b994478dd 100755 --- a/test/integration/targets/template/runme.sh +++ b/test/integration/targets/template/runme.sh @@ -41,6 +41,10 @@ ansible-playbook 72262.yml -v "$@" # ensure unsafe is preserved, even with extra newlines ansible-playbook unsafe.yml -v "$@" +# CVE 2024-11079 +ANSIBLE_JINJA2_NATIVE=true ansible-playbook cve-2024-11079.yml -v "$@" +ANSIBLE_JINJA2_NATIVE=false ansible-playbook cve-2024-11079.yml -v "$@" + # ensure Jinja2 overrides from a template are used ansible-playbook template_overrides.yml -v "$@"