diff --git a/awx/main/conf.py b/awx/main/conf.py index 63714ab4bb..0ae74a6dfc 100644 --- a/awx/main/conf.py +++ b/awx/main/conf.py @@ -132,6 +132,15 @@ register( required=False, ) +register( + 'ALLOW_JINJA_IN_JOB_TEMPLATE_EXTRA_VARS', + field_class=fields.BooleanField, + label=_('Allow Jinja template execution in Job Template extra vars'), + help_text=_('Ansible allows variable substitution and templating via the Jinja2 templating language for a variety of arguments (such as --extra-vars); enabling this flag allows arbitrary Jinja templates to be used on extra vars defined in Job Templates.'), # noqa + category=_('Jobs'), + category_slug='jobs', +) + register( 'AWX_PROOT_ENABLED', field_class=fields.BooleanField, diff --git a/awx/main/models/credential.py b/awx/main/models/credential.py index e4bbfff675..fbdcbcafe8 100644 --- a/awx/main/models/credential.py +++ b/awx/main/models/credential.py @@ -2,7 +2,6 @@ # All Rights Reserved. from collections import OrderedDict import functools -import json import logging import operator import os @@ -25,6 +24,7 @@ from awx.main.fields import (ImplicitRoleField, CredentialInputField, CredentialTypeInputField, CredentialTypeInjectorField) from awx.main.utils import decrypt_field +from awx.main.utils.safe_yaml import safe_dump from awx.main.validators import validate_ssh_private_key from awx.main.models.base import * # noqa from awx.main.models.mixins import ResourceMixin @@ -589,7 +589,7 @@ class CredentialType(CommonModelNameNotUnique): def build_extra_vars_file(vars, private_dir): handle, path = tempfile.mkstemp(dir = private_dir) f = os.fdopen(handle, 'w') - f.write(json.dumps(vars)) + f.write(safe_dump(vars)) f.close() os.chmod(path, stat.S_IRUSR) return path diff --git a/awx/main/tasks.py b/awx/main/tasks.py index 1aeae770d2..b9af133218 100644 --- a/awx/main/tasks.py +++ b/awx/main/tasks.py @@ -58,6 +58,7 @@ from awx.main.utils import (get_ansible_version, get_ssh_version, decrypt_field, wrap_args_with_proot, get_system_task_capacity, OutputEventFilter, parse_yaml_or_json, ignore_inventory_computed_fields, ignore_inventory_group_removal, get_type_for_model, extract_ansible_vars) +from awx.main.utils.safe_yaml import safe_dump from awx.main.utils.reload import restart_local_services, stop_local_services from awx.main.utils.handlers import configure_external_logger from awx.main.consumers import emit_channel_notification @@ -625,7 +626,7 @@ class BaseTask(LogErrorsTask): def build_extra_vars_file(self, vars, **kwargs): handle, path = tempfile.mkstemp(dir=kwargs.get('private_data_dir', None)) f = os.fdopen(handle, 'w') - f.write(json.dumps(vars)) + f.write(safe_dump(vars, kwargs.get('safe_dict', {}) or None)) f.close() os.chmod(path, stat.S_IRUSR) return path @@ -1213,7 +1214,20 @@ class RunJob(BaseTask): extra_vars.update(json.loads(job.display_extra_vars())) else: extra_vars.update(json.loads(job.decrypted_extra_vars())) - extra_vars_path = self.build_extra_vars_file(vars=extra_vars, **kwargs) + + # By default, all extra vars disallow Jinja2 template usage for + # security reasons; top level key-values defined in JT.extra_vars, however, + # are whitelisted as "safe" (because they can only be set by users with + # higher levels of privilege - those that have the ability create and + # edit Job Templates) + safe_dict = {} + if job.job_template and settings.ALLOW_JINJA_IN_JOB_TEMPLATE_EXTRA_VARS is True: + safe_dict = job.job_template.extra_vars_dict + extra_vars_path = self.build_extra_vars_file( + vars=extra_vars, + safe_dict=safe_dict, + **kwargs + ) args.extend(['-e', '@%s' % (extra_vars_path)]) # Add path to playbook (relative to project.local_path). diff --git a/awx/main/tests/unit/models/test_survey_models.py b/awx/main/tests/unit/models/test_survey_models.py index 284f69296c..a235984ed8 100644 --- a/awx/main/tests/unit/models/test_survey_models.py +++ b/awx/main/tests/unit/models/test_survey_models.py @@ -1,6 +1,7 @@ import tempfile import pytest import json +import yaml from awx.main.utils.encryption import encrypt_value from awx.main.tasks import RunJob @@ -9,6 +10,7 @@ from awx.main.models import ( JobTemplate, WorkflowJobTemplate ) +from awx.main.utils.safe_yaml import SafeLoader ENCRYPTED_SECRET = encrypt_value('secret') @@ -72,7 +74,7 @@ def test_job_safe_args_redacted_passwords(job): safe_args = run_job.build_safe_args(job, **kwargs) ev_index = safe_args.index('-e') + 1 extra_var_file = open(safe_args[ev_index][1:], 'r') - extra_vars = json.load(extra_var_file) + extra_vars = yaml.load(extra_var_file, SafeLoader) extra_var_file.close() assert extra_vars['secret_key'] == '$encrypted$' @@ -83,7 +85,7 @@ def test_job_args_unredacted_passwords(job, tmpdir_factory): args = run_job.build_args(job, **kwargs) ev_index = args.index('-e') + 1 extra_var_file = open(args[ev_index][1:], 'r') - extra_vars = json.load(extra_var_file) + extra_vars = yaml.load(extra_var_file, SafeLoader) extra_var_file.close() assert extra_vars['secret_key'] == 'my_password' diff --git a/awx/main/tests/unit/test_tasks.py b/awx/main/tests/unit/test_tasks.py index f807e02165..ffbd90a625 100644 --- a/awx/main/tests/unit/test_tasks.py +++ b/awx/main/tests/unit/test_tasks.py @@ -23,6 +23,7 @@ from awx.main.models import ( InventorySource, InventoryUpdate, Job, + JobTemplate, Notification, Project, ProjectUpdate, @@ -32,7 +33,7 @@ from awx.main.models import ( from awx.main import tasks from awx.main.utils import encrypt_field, encrypt_value - +from awx.main.utils.safe_yaml import SafeLoader @contextmanager @@ -177,7 +178,7 @@ def parse_extra_vars(args): for chunk in args: if chunk.startswith('@/tmp/'): with open(chunk.strip('@'), 'r') as f: - extra_vars.update(json.load(f)) + extra_vars.update(yaml.load(f, SafeLoader)) return extra_vars @@ -243,7 +244,8 @@ class TestJobExecution: cancel_flag=False, project=Project(), playbook='helloworld.yml', - verbosity=3 + verbosity=3, + job_template=JobTemplate(extra_vars='') ) # mock the job.extra_credentials M2M relation so we can avoid DB access @@ -263,6 +265,131 @@ class TestJobExecution: return self.instance.pk +class TestExtraVarSanitation(TestJobExecution): + # By default, extra vars are marked as `!unsafe` in the generated yaml + # _unless_ they've been specified on the JobTemplate's extra_vars (which + # are deemed trustable, because they can only be added by users w/ enough + # privilege to add/modify a Job Template) + + UNSAFE = '{{ lookup(''pipe'',''ls -la'') }}' + + def test_vars_unsafe_by_default(self): + self.instance.created_by = User(pk=123, username='angry-spud') + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + + # ensure that strings are marked as unsafe + for unsafe in ['awx_job_template_name', 'tower_job_template_name', + 'awx_user_name', 'tower_job_launch_type', + 'awx_project_revision', + 'tower_project_revision', 'tower_user_name', + 'awx_job_launch_type']: + assert hasattr(extra_vars[unsafe], '__UNSAFE__') + + # ensure that non-strings are marked as safe + for safe in ['awx_job_template_id', 'awx_job_id', 'awx_user_id', + 'tower_user_id', 'tower_job_template_id', + 'tower_job_id']: + assert not hasattr(extra_vars[safe], '__UNSAFE__') + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) + + def test_launchtime_vars_unsafe(self): + self.instance.extra_vars = json.dumps({'msg': self.UNSAFE}) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['msg'] == self.UNSAFE + assert hasattr(extra_vars['msg'], '__UNSAFE__') + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) + + def test_nested_launchtime_vars_unsafe(self): + self.instance.extra_vars = json.dumps({'msg': {'a': [self.UNSAFE]}}) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['msg'] == {'a': [self.UNSAFE]} + assert hasattr(extra_vars['msg']['a'][0], '__UNSAFE__') + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) + + def test_whitelisted_jt_extra_vars(self): + self.instance.job_template.extra_vars = self.instance.extra_vars = json.dumps({'msg': self.UNSAFE}) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['msg'] == self.UNSAFE + assert not hasattr(extra_vars['msg'], '__UNSAFE__') + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) + + def test_nested_whitelisted_vars(self): + self.instance.extra_vars = json.dumps({'msg': {'a': {'b': [self.UNSAFE]}}}) + self.instance.job_template.extra_vars = self.instance.extra_vars + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['msg'] == {'a': {'b': [self.UNSAFE]}} + assert not hasattr(extra_vars['msg']['a']['b'][0], '__UNSAFE__') + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) + + def test_sensitive_values_dont_leak(self): + # JT defines `msg=SENSITIVE`, the job *should not* be able to do + # `other_var=SENSITIVE` + self.instance.job_template.extra_vars = json.dumps({'msg': self.UNSAFE}) + self.instance.extra_vars = json.dumps({ + 'msg': 'other-value', + 'other_var': self.UNSAFE + }) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + + assert extra_vars['msg'] == 'other-value' + assert hasattr(extra_vars['msg'], '__UNSAFE__') + + assert extra_vars['other_var'] == self.UNSAFE + assert hasattr(extra_vars['other_var'], '__UNSAFE__') + + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) + + def test_overwritten_jt_extra_vars(self): + self.instance.job_template.extra_vars = json.dumps({'msg': 'SAFE'}) + self.instance.extra_vars = json.dumps({'msg': self.UNSAFE}) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['msg'] == self.UNSAFE + assert hasattr(extra_vars['msg'], '__UNSAFE__') + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) + + class TestGenericRun(TestJobExecution): def test_cancel_flag(self): @@ -1009,6 +1136,7 @@ class TestJobCredentials(TestJobExecution): args, cwd, env, stdout = args extra_vars = parse_extra_vars(args) assert extra_vars["api_token"] == "ABC123" + assert hasattr(extra_vars["api_token"], '__UNSAFE__') return ['successful', 0] self.run_pexpect.side_effect = run_pexpect_side_effect diff --git a/awx/main/tests/unit/utils/test_safe_yaml.py b/awx/main/tests/unit/utils/test_safe_yaml.py new file mode 100644 index 0000000000..1c06ff5ca1 --- /dev/null +++ b/awx/main/tests/unit/utils/test_safe_yaml.py @@ -0,0 +1,59 @@ +from copy import deepcopy +from awx.main.utils.safe_yaml import safe_dump + + +def test_empty(): + assert safe_dump({}) == '' + + +def test_kv_int(): + assert safe_dump({'a': 1}) == "!unsafe 'a': 1\n" + + +def test_kv_float(): + assert safe_dump({'a': 1.5}) == "!unsafe 'a': 1.5\n" + + +def test_kv_unsafe(): + assert safe_dump({'a': 'b'}) == "!unsafe 'a': !unsafe 'b'\n" + + +def test_kv_unsafe_in_list(): + assert safe_dump({'a': ['b']}) == "!unsafe 'a':\n- !unsafe 'b'\n" + + +def test_kv_unsafe_in_mixed_list(): + assert safe_dump({'a': [1, 'b']}) == "!unsafe 'a':\n- 1\n- !unsafe 'b'\n" + + +def test_kv_unsafe_deep_nesting(): + yaml = safe_dump({'a': [1, [{'b': {'c': [{'d': 'e'}]}}]]}) + for x in ('a', 'b', 'c', 'd', 'e'): + assert "!unsafe '{}'".format(x) in yaml + + +def test_kv_unsafe_multiple(): + assert safe_dump({'a': 'b', 'c': 'd'}) == '\n'.join([ + "!unsafe 'a': !unsafe 'b'", + "!unsafe 'c': !unsafe 'd'", + "" + ]) + + +def test_safe_marking(): + assert safe_dump({'a': 'b'}, safe_dict={'a': 'b'}) == "a: b\n" + + +def test_safe_marking_mixed(): + assert safe_dump({'a': 'b', 'c': 'd'}, safe_dict={'a': 'b'}) == '\n'.join([ + "a: b", + "!unsafe 'c': !unsafe 'd'", + "" + ]) + + +def test_safe_marking_deep_nesting(): + deep = {'a': [1, [{'b': {'c': [{'d': 'e'}]}}]]} + yaml = safe_dump(deep, deepcopy(deep)) + for x in ('a', 'b', 'c', 'd', 'e'): + assert "!unsafe '{}'".format(x) not in yaml diff --git a/awx/main/utils/safe_yaml.py b/awx/main/utils/safe_yaml.py new file mode 100644 index 0000000000..6b8bdfd2b4 --- /dev/null +++ b/awx/main/utils/safe_yaml.py @@ -0,0 +1,66 @@ +import six +import yaml + + +__all__ = ['safe_dump', 'SafeLoader'] + + +class SafeStringDumper(yaml.SafeDumper): + + def represent_data(self, value): + if isinstance(value, six.string_types): + return self.represent_scalar('!unsafe', value) + return super(SafeStringDumper, self).represent_data(value) + + +class SafeLoader(yaml.Loader): + + def construct_yaml_unsafe(self, node): + class UnsafeText(six.text_type): + __UNSAFE__ = True + node = UnsafeText(self.construct_scalar(node)) + return node + + +SafeLoader.add_constructor( + u'!unsafe', + SafeLoader.construct_yaml_unsafe +) + + +def safe_dump(x, safe_dict=None): + """ + Used to serialize an extra_vars dict to YAML + + By default, extra vars are marked as `!unsafe` in the generated yaml + _unless_ they've been deemed "trusted" (meaning, they likely were set/added + by a user with a high level of privilege). + + This function allows you to pass in a trusted `safe_dict` to whitelist + certain extra vars so that they are _not_ marked as `!unsafe` in the + resulting YAML. Anything _not_ in this dict will automatically be + `!unsafe`. + + safe_dump({'a': 'b', 'c': 'd'}) -> + !unsafe 'a': !unsafe 'b' + !unsafe 'c': !unsafe 'd' + + safe_dump({'a': 'b', 'c': 'd'}, safe_dict={'a': 'b'}) + a: b + !unsafe 'c': !unsafe 'd' + """ + yamls = [] + safe_dict = safe_dict or {} + # Compare the top level keys so that we can find values that have + # equality matches (and consider those branches safe) + for k, v in x.items(): + dumper = yaml.SafeDumper + if safe_dict.get(k) != v: + dumper = SafeStringDumper + yamls.append(yaml.dump_all( + [{k: v}], + None, + Dumper=dumper, + default_flow_style=False, + )) + return ''.join(yamls) diff --git a/awx/settings/defaults.py b/awx/settings/defaults.py index 89d1727188..66e17a0174 100644 --- a/awx/settings/defaults.py +++ b/awx/settings/defaults.py @@ -585,6 +585,9 @@ CAPTURE_JOB_EVENT_HOSTS = False # Rebuild Host Smart Inventory memberships. AWX_REBUILD_SMART_MEMBERSHIP = False +# By default, allow arbitrary Jinja templating in extra_vars defined on a Job Template +ALLOW_JINJA_IN_JOB_TEMPLATE_EXTRA_VARS = True + # Enable bubblewrap support for running jobs (playbook runs only). # Note: This setting may be overridden by database settings. AWX_PROOT_ENABLED = True