diff --git a/changelogs/fragments/unsafe_hostvars_fix.yml b/changelogs/fragments/unsafe_hostvars_fix.yml new file mode 100644 index 00000000000..cf7ca888e23 --- /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 d70e1368858..6713300f5e1 100644 --- a/lib/ansible/template/__init__.py +++ b/lib/ansible/template/__init__.py @@ -48,7 +48,7 @@ from ansible.module_utils.six import string_types 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 @@ -312,35 +312,6 @@ def _wrap_native_text(func): return functools.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 612ed50113f..860235eb4f7 100644 --- a/lib/ansible/template/native_helpers.py +++ b/lib/ansible/template/native_helpers.py @@ -5,13 +5,19 @@ from __future__ import annotations 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 = { @@ -28,6 +34,40 @@ class Json2Python(ast.NodeTransformer): 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. @@ -43,17 +83,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'): @@ -68,6 +119,9 @@ def ansible_eval_concat(nodes): except (TypeError, ValueError, SyntaxError, MemoryError): pass + if unsafe: + out = wrap_var(out) + return out @@ -78,7 +132,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): @@ -95,6 +161,8 @@ def ansible_native_concat(nodes): if not head: return None + unsafe = False + if len(head) == 1: out = head[0] @@ -115,10 +183,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( @@ -128,10 +207,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 6f8491dcca6..d0e2b983074 100644 --- a/lib/ansible/vars/hostvars.py +++ b/lib/ansible/vars/hostvars.py @@ -92,10 +92,12 @@ class HostVars(Mapping): return self._find_host(host_name) is not None def __iter__(self): - yield from self._inventory.hosts + # 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 00000000000..00eb16d995d --- /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 b37467a2719..f9050b99447 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 "$@"