From bba7f45972f92693ac0837f476011f0649c79b4d Mon Sep 17 00:00:00 2001 From: Bill Nottingham Date: Fri, 2 Feb 2018 23:30:51 -0500 Subject: [PATCH 1/6] Pass extra vars via file rather than via commandline, including custom creds. The extra vars file created lives in the playbook private runtime directory, and will be reaped along with the rest of the directory. Adjust assorted unit tests as necessary. --- awx/main/models/credential.py | 14 +- awx/main/tasks.py | 17 ++- .../tests/unit/models/test_survey_models.py | 8 +- awx/main/tests/unit/test_tasks.py | 141 +++++++++++------- 4 files changed, 118 insertions(+), 62 deletions(-) diff --git a/awx/main/models/credential.py b/awx/main/models/credential.py index f33ed07ee4..e4bbfff675 100644 --- a/awx/main/models/credential.py +++ b/awx/main/models/credential.py @@ -586,11 +586,21 @@ class CredentialType(CommonModelNameNotUnique): extra_vars[var_name] = Template(tmpl).render(**namespace) safe_extra_vars[var_name] = Template(tmpl).render(**safe_namespace) + 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.close() + os.chmod(path, stat.S_IRUSR) + return path + if extra_vars: - args.extend(['-e', json.dumps(extra_vars)]) + path = build_extra_vars_file(extra_vars, private_data_dir) + args.extend(['-e', '@%s' % path]) if safe_extra_vars: - safe_args.extend(['-e', json.dumps(safe_extra_vars)]) + path = build_extra_vars_file(safe_extra_vars, private_data_dir) + safe_args.extend(['-e', '@%s' % path]) @CredentialType.default diff --git a/awx/main/tasks.py b/awx/main/tasks.py index 52d70f51d3..1aeae770d2 100644 --- a/awx/main/tasks.py +++ b/awx/main/tasks.py @@ -622,6 +622,14 @@ 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.close() + os.chmod(path, stat.S_IRUSR) + return path + def add_ansible_venv(self, env, add_awx_lib=True): env['VIRTUAL_ENV'] = settings.ANSIBLE_VENV_PATH env['PATH'] = os.path.join(settings.ANSIBLE_VENV_PATH, "bin") + ":" + env['PATH'] @@ -1205,7 +1213,8 @@ class RunJob(BaseTask): extra_vars.update(json.loads(job.display_extra_vars())) else: extra_vars.update(json.loads(job.decrypted_extra_vars())) - args.extend(['-e', json.dumps(extra_vars)]) + extra_vars_path = self.build_extra_vars_file(vars=extra_vars, **kwargs) + args.extend(['-e', '@%s' % (extra_vars_path)]) # Add path to playbook (relative to project.local_path). args.append(job.playbook) @@ -1460,7 +1469,8 @@ class RunProjectUpdate(BaseTask): 'scm_revision_output': self.revision_path, 'scm_revision': project_update.project.scm_revision, }) - args.extend(['-e', json.dumps(extra_vars)]) + extra_vars_path = self.build_extra_vars_file(vars=extra_vars, **kwargs) + args.extend(['-e', '@%s' % (extra_vars_path)]) args.append('project_update.yml') return args @@ -2220,7 +2230,8 @@ class RunAdHocCommand(BaseTask): "{} are prohibited from use in ad hoc commands." ).format(", ".join(removed_vars))) extra_vars.update(ad_hoc_command.extra_vars_dict) - args.extend(['-e', json.dumps(extra_vars)]) + extra_vars_path = self.build_extra_vars_file(vars=extra_vars, **kwargs) + args.extend(['-e', '@%s' % (extra_vars_path)]) args.extend(['-m', ad_hoc_command.module_name]) args.extend(['-a', ad_hoc_command.module_args]) diff --git a/awx/main/tests/unit/models/test_survey_models.py b/awx/main/tests/unit/models/test_survey_models.py index abd0b5fbba..284f69296c 100644 --- a/awx/main/tests/unit/models/test_survey_models.py +++ b/awx/main/tests/unit/models/test_survey_models.py @@ -71,7 +71,9 @@ def test_job_safe_args_redacted_passwords(job): run_job = RunJob() safe_args = run_job.build_safe_args(job, **kwargs) ev_index = safe_args.index('-e') + 1 - extra_vars = json.loads(safe_args[ev_index]) + extra_var_file = open(safe_args[ev_index][1:], 'r') + extra_vars = json.load(extra_var_file) + extra_var_file.close() assert extra_vars['secret_key'] == '$encrypted$' @@ -80,7 +82,9 @@ def test_job_args_unredacted_passwords(job, tmpdir_factory): run_job = RunJob() args = run_job.build_args(job, **kwargs) ev_index = args.index('-e') + 1 - extra_vars = json.loads(args[ev_index]) + extra_var_file = open(args[ev_index][1:], 'r') + extra_vars = json.load(extra_var_file) + 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 20a051ec30..f807e02165 100644 --- a/awx/main/tests/unit/test_tasks.py +++ b/awx/main/tests/unit/test_tasks.py @@ -172,6 +172,15 @@ def pytest_generate_tests(metafunc): ) +def parse_extra_vars(args): + extra_vars = {} + for chunk in args: + if chunk.startswith('@/tmp/'): + with open(chunk.strip('@'), 'r') as f: + extra_vars.update(json.load(f)) + return extra_vars + + class TestJobExecution: """ For job runs, test that `ansible-playbook` is invoked with the proper @@ -296,15 +305,18 @@ class TestGenericRun(TestJobExecution): def test_created_by_extra_vars(self): self.instance.created_by = User(pk=123, username='angry-spud') - self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, _ = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - assert '"tower_user_id": 123,' in ' '.join(args) - assert '"tower_user_name": "angry-spud"' in ' '.join(args) - assert '"awx_user_id": 123,' in ' '.join(args) - assert '"awx_user_name": "angry-spud"' in ' '.join(args) + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['tower_user_id'] == 123 + assert extra_vars['tower_user_name'] == "angry-spud" + assert extra_vars['awx_user_id'] == 123 + assert extra_vars['awx_user_name'] == "angry-spud" + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) def test_survey_extra_vars(self): self.instance.extra_vars = json.dumps({ @@ -313,12 +325,15 @@ class TestGenericRun(TestJobExecution): self.instance.survey_passwords = { 'super_secret': '$encrypted$' } - self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, _ = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - assert '"super_secret": "CLASSIFIED"' in ' '.join(args) + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['super_secret'] == "CLASSIFIED" + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) def test_awx_task_env(self): patch = mock.patch('awx.main.tasks.settings.AWX_TASK_ENV', {'FOO': 'BAR'}) @@ -385,16 +400,19 @@ class TestAdhocRun(TestJobExecution): def test_created_by_extra_vars(self): self.instance.created_by = User(pk=123, username='angry-spud') - self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, _ = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - assert '"tower_user_id": 123,' in ' '.join(args) - assert '"tower_user_name": "angry-spud"' in ' '.join(args) - assert '"awx_user_id": 123,' in ' '.join(args) - assert '"awx_user_name": "angry-spud"' in ' '.join(args) - assert '"awx_foo": "awx-bar' in ' '.join(args) + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars['tower_user_id'] == 123 + assert extra_vars['tower_user_name'] == "angry-spud" + assert extra_vars['awx_user_id'] == 123 + assert extra_vars['awx_user_name'] == "angry-spud" + assert extra_vars['awx_foo'] == "awx-bar" + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) class TestIsolatedExecution(TestJobExecution): @@ -986,14 +1004,16 @@ class TestJobCredentials(TestJobExecution): inputs = {'api_token': 'ABC123'} ) self.instance.extra_credentials.add(credential) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars["api_token"] == "ABC123" + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, _ = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - - assert '-e {"api_token": "ABC123"}' in ' '.join(args) - def test_custom_environment_injectors_with_boolean_extra_vars(self): some_cloud = CredentialType( kind='cloud', @@ -1018,12 +1038,15 @@ class TestJobCredentials(TestJobExecution): inputs={'turbo_button': True} ) self.instance.extra_credentials.add(credential) - self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, _ = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - assert '-e {"turbo_button": "True"}' in ' '.join(args) + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars["turbo_button"] == "True" + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) def test_custom_environment_injectors_with_complicated_boolean_template(self): some_cloud = CredentialType( @@ -1049,12 +1072,15 @@ class TestJobCredentials(TestJobExecution): inputs={'turbo_button': True} ) self.instance.extra_credentials.add(credential) - self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, _ = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - assert '-e {"turbo_button": "FAST!"}' in ' '.join(args) + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars["turbo_button"] == "FAST!" + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect + self.task.run(self.pk) def test_custom_environment_injectors_with_secret_extra_vars(self): """ @@ -1085,13 +1111,16 @@ class TestJobCredentials(TestJobExecution): ) credential.inputs['password'] = encrypt_field(credential, 'password') self.instance.extra_credentials.add(credential) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert extra_vars["password"] == "SUPER-SECRET-123" + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, _ = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - - assert '-e {"password": "SUPER-SECRET-123"}' in ' '.join(args) assert 'SUPER-SECRET-123' not in json.dumps(self.task.update_model.call_args_list) def test_custom_environment_injectors_with_file(self): @@ -1217,20 +1246,22 @@ class TestProjectUpdateCredentials(TestJobExecution): pk=1, credential_type=ssh, ) + + def run_pexpect_side_effect(*args, **kwargs): + args, cwd, env, stdout = args + extra_vars = parse_extra_vars(args) + assert ' '.join(args).startswith('bwrap') + assert ' '.join([ + '--bind', + os.path.realpath(settings.PROJECTS_ROOT), + os.path.realpath(settings.PROJECTS_ROOT) + ]) in ' '.join(args) + assert extra_vars["scm_revision_output"].startswith(settings.PROJECTS_ROOT) + return ['successful', 0] + + self.run_pexpect.side_effect = run_pexpect_side_effect self.task.run(self.pk) - assert self.run_pexpect.call_count == 1 - call_args, call_kwargs = self.run_pexpect.call_args_list[0] - args, cwd, env, stdout = call_args - - assert ' '.join(args).startswith('bwrap') - ' '.join([ - '--bind', - settings.PROJECTS_ROOT, - settings.PROJECTS_ROOT, - ]) in ' '.join(args) - assert '"scm_revision_output": "/projects/tmp' in ' '.join(args) - def test_username_and_password_auth(self, scm_type): ssh = CredentialType.defaults['ssh']() self.instance.scm_type = scm_type From 88c243c92ae7ebf2fbad087411f5896c3d8749fd Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Wed, 11 Apr 2018 18:14:08 -0400 Subject: [PATCH 2/6] mark all unsafe launch-time extra vars as !unsafe see: https://github.com/ansible/tower/issues/1338 see: https://bugzilla.redhat.com/show_bug.cgi?id=1565865 --- awx/main/conf.py | 9 ++ awx/main/models/credential.py | 4 +- awx/main/tasks.py | 18 ++- .../tests/unit/models/test_survey_models.py | 6 +- awx/main/tests/unit/test_tasks.py | 134 +++++++++++++++++- awx/main/tests/unit/utils/test_safe_yaml.py | 59 ++++++++ awx/main/utils/safe_yaml.py | 66 +++++++++ awx/settings/defaults.py | 3 + 8 files changed, 290 insertions(+), 9 deletions(-) create mode 100644 awx/main/tests/unit/utils/test_safe_yaml.py create mode 100644 awx/main/utils/safe_yaml.py 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 From 7074dcd677be72718dd5e0c490635c5cd9862f64 Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Fri, 13 Apr 2018 16:32:39 -0400 Subject: [PATCH 3/6] don't allow usage of jinja templates in certain ansible CLI flags see: https://github.com/ansible/tower/issues/1338 --- awx/main/tasks.py | 16 +++++++-------- awx/main/tests/unit/test_tasks.py | 34 +++++++++++++++++++++++++++++++ awx/main/utils/safe_yaml.py | 17 ++++++++++++++++ 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/awx/main/tasks.py b/awx/main/tasks.py index b9af133218..7f68ed80ec 100644 --- a/awx/main/tasks.py +++ b/awx/main/tasks.py @@ -58,7 +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.safe_yaml import safe_dump, sanitize_jinja 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 @@ -1151,7 +1151,7 @@ class RunJob(BaseTask): args = ['ansible-playbook', '-i', self.build_inventory(job, **kwargs)] if job.job_type == 'check': args.append('--check') - args.extend(['-u', ssh_username]) + args.extend(['-u', sanitize_jinja(ssh_username)]) if 'ssh_password' in kwargs.get('passwords', {}): args.append('--ask-pass') if job.become_enabled: @@ -1159,9 +1159,9 @@ class RunJob(BaseTask): if job.diff_mode: args.append('--diff') if become_method: - args.extend(['--become-method', become_method]) + args.extend(['--become-method', sanitize_jinja(become_method)]) if become_username: - args.extend(['--become-user', become_username]) + args.extend(['--become-user', sanitize_jinja(become_username)]) if 'become_password' in kwargs.get('passwords', {}): args.append('--ask-become-pass') # Support prompting for a vault password. @@ -2203,7 +2203,7 @@ class RunAdHocCommand(BaseTask): args = ['ansible', '-i', self.build_inventory(ad_hoc_command, **kwargs)] if ad_hoc_command.job_type == 'check': args.append('--check') - args.extend(['-u', ssh_username]) + args.extend(['-u', sanitize_jinja(ssh_username)]) if 'ssh_password' in kwargs.get('passwords', {}): args.append('--ask-pass') # We only specify sudo/su user and password if explicitly given by the @@ -2211,9 +2211,9 @@ class RunAdHocCommand(BaseTask): if ad_hoc_command.become_enabled: args.append('--become') if become_method: - args.extend(['--become-method', become_method]) + args.extend(['--become-method', sanitize_jinja(become_method)]) if become_username: - args.extend(['--become-user', become_username]) + args.extend(['--become-user', sanitize_jinja(become_username)]) if 'become_password' in kwargs.get('passwords', {}): args.append('--ask-become-pass') @@ -2248,7 +2248,7 @@ class RunAdHocCommand(BaseTask): args.extend(['-e', '@%s' % (extra_vars_path)]) args.extend(['-m', ad_hoc_command.module_name]) - args.extend(['-a', ad_hoc_command.module_args]) + args.extend(['-a', sanitize_jinja(ad_hoc_command.module_args)]) if ad_hoc_command.limit: args.append(ad_hoc_command.limit) diff --git a/awx/main/tests/unit/test_tasks.py b/awx/main/tests/unit/test_tasks.py index ffbd90a625..e81fe794a5 100644 --- a/awx/main/tests/unit/test_tasks.py +++ b/awx/main/tests/unit/test_tasks.py @@ -525,6 +525,13 @@ class TestAdhocRun(TestJobExecution): extra_vars={'awx_foo': 'awx-bar'} ) + def test_options_jinja_usage(self): + self.instance.module_args = '{{ ansible_ssh_pass }}' + with pytest.raises(Exception): + self.task.run(self.pk) + update_model_call = self.task.update_model.call_args[1] + assert 'Jinja variables are not allowed' in update_model_call['result_traceback'] + def test_created_by_extra_vars(self): self.instance.created_by = User(pk=123, username='angry-spud') @@ -647,6 +654,33 @@ class TestJobCredentials(TestJobExecution): ] } + def test_username_jinja_usage(self): + ssh = CredentialType.defaults['ssh']() + credential = Credential( + pk=1, + credential_type=ssh, + inputs = {'username': '{{ ansible_ssh_pass }}'} + ) + self.instance.credential = credential + with pytest.raises(Exception): + self.task.run(self.pk) + update_model_call = self.task.update_model.call_args[1] + assert 'Jinja variables are not allowed' in update_model_call['result_traceback'] + + @pytest.mark.parametrize("flag", ['become_username', 'become_method']) + def test_become_jinja_usage(self, flag): + ssh = CredentialType.defaults['ssh']() + credential = Credential( + pk=1, + credential_type=ssh, + inputs = {'username': 'joe', flag: '{{ ansible_ssh_pass }}'} + ) + self.instance.credential = credential + with pytest.raises(Exception): + self.task.run(self.pk) + update_model_call = self.task.update_model.call_args[1] + assert 'Jinja variables are not allowed' in update_model_call['result_traceback'] + def test_ssh_passwords(self, field, password_name, expected_flag): ssh = CredentialType.defaults['ssh']() credential = Credential( diff --git a/awx/main/utils/safe_yaml.py b/awx/main/utils/safe_yaml.py index 6b8bdfd2b4..af572e9fd5 100644 --- a/awx/main/utils/safe_yaml.py +++ b/awx/main/utils/safe_yaml.py @@ -1,3 +1,4 @@ +import re import six import yaml @@ -64,3 +65,19 @@ def safe_dump(x, safe_dict=None): default_flow_style=False, )) return ''.join(yamls) + + +def sanitize_jinja(arg): + """ + For some string, prevent usage of Jinja-like flags + """ + if isinstance(arg, six.string_types): + # If the argument looks like it contains Jinja expressions + # {{ x }} ... + if re.search('\{\{[^}]+}}', arg) is not None: + raise ValueError('Inline Jinja variables are not allowed.') + # If the argument looks like it contains Jinja statements/control flow... + # {% if x.foo() %} ... + if re.search('\{%[^%]+%}', arg) is not None: + raise ValueError('Inline Jinja variables are not allowed.') + return arg From 73043019480a9e407223c1df35060eb6c27b0e53 Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Tue, 17 Apr 2018 10:24:14 -0400 Subject: [PATCH 4/6] don't bother building a safe extra vars namespace; it's a file path now --- awx/main/models/credential.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/awx/main/models/credential.py b/awx/main/models/credential.py index fbdcbcafe8..5eddacd225 100644 --- a/awx/main/models/credential.py +++ b/awx/main/models/credential.py @@ -581,10 +581,8 @@ class CredentialType(CommonModelNameNotUnique): safe_env[env_var] = Template(tmpl).render(**safe_namespace) extra_vars = {} - safe_extra_vars = {} for var_name, tmpl in self.injectors.get('extra_vars', {}).items(): extra_vars[var_name] = Template(tmpl).render(**namespace) - safe_extra_vars[var_name] = Template(tmpl).render(**safe_namespace) def build_extra_vars_file(vars, private_dir): handle, path = tempfile.mkstemp(dir = private_dir) @@ -594,12 +592,9 @@ class CredentialType(CommonModelNameNotUnique): os.chmod(path, stat.S_IRUSR) return path + path = build_extra_vars_file(extra_vars, private_data_dir) if extra_vars: - path = build_extra_vars_file(extra_vars, private_data_dir) args.extend(['-e', '@%s' % path]) - - if safe_extra_vars: - path = build_extra_vars_file(safe_extra_vars, private_data_dir) safe_args.extend(['-e', '@%s' % path]) From fe47b75aad6787f41037e3c1d1f0628efa875a1c Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Tue, 17 Apr 2018 12:08:07 -0400 Subject: [PATCH 5/6] use a three-prong setting for Jinja extra vars policy --- awx/main/conf.py | 20 ++++++++++++++++---- awx/main/tasks.py | 10 ++++++---- awx/settings/defaults.py | 2 +- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/awx/main/conf.py b/awx/main/conf.py index 0ae74a6dfc..3dc3453fb1 100644 --- a/awx/main/conf.py +++ b/awx/main/conf.py @@ -133,10 +133,22 @@ register( ) 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 + 'ALLOW_JINJA_IN_EXTRA_VARS', + field_class=fields.ChoiceField, + choices=[ + ('always', _('Always')), + ('never', _('Never')), + ('template', _('Only On Job Template Definitions')), + ], + required=True, + label=_('When can extra variables contain Jinja templates?'), + help_text=_( + 'Ansible allows variable substitution via the Jinja2 templating ' + 'language for --extra-vars. This poses a potential security ' + 'risk where Tower users with the ability to specify extra vars at job ' + 'launch time can use Jinja2 templates to run arbitrary Python. It is ' + 'recommended that this value be set to "template" or "never".' + ), category=_('Jobs'), category_slug='jobs', ) diff --git a/awx/main/tasks.py b/awx/main/tasks.py index 7f68ed80ec..ce85ab4d83 100644 --- a/awx/main/tasks.py +++ b/awx/main/tasks.py @@ -626,7 +626,10 @@ 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(safe_dump(vars, kwargs.get('safe_dict', {}) or None)) + if settings.ALLOW_JINJA_IN_EXTRA_VARS == 'always': + f.write(yaml.safe_dump(vars)) + else: + f.write(safe_dump(vars, kwargs.get('safe_dict', {}) or None)) f.close() os.chmod(path, stat.S_IRUSR) return path @@ -909,8 +912,7 @@ class BaseTask(LogErrorsTask): except Exception: if status != 'canceled': tb = traceback.format_exc() - if settings.DEBUG: - logger.exception('%s Exception occurred while running task', instance.log_format) + logger.exception('%s Exception occurred while running task', instance.log_format) finally: try: stdout_handle.flush() @@ -1221,7 +1223,7 @@ class RunJob(BaseTask): # 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: + if job.job_template and settings.ALLOW_JINJA_IN_EXTRA_VARS == 'template': safe_dict = job.job_template.extra_vars_dict extra_vars_path = self.build_extra_vars_file( vars=extra_vars, diff --git a/awx/settings/defaults.py b/awx/settings/defaults.py index 66e17a0174..54b6110812 100644 --- a/awx/settings/defaults.py +++ b/awx/settings/defaults.py @@ -586,7 +586,7 @@ CAPTURE_JOB_EVENT_HOSTS = False 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 +ALLOW_JINJA_IN_EXTRA_VARS = 'template' # Enable bubblewrap support for running jobs (playbook runs only). # Note: This setting may be overridden by database settings. From 835f2eebc38785aa0876524b44c4f70ec96720ad Mon Sep 17 00:00:00 2001 From: Ryan Petrello Date: Tue, 17 Apr 2018 15:39:37 -0400 Subject: [PATCH 6/6] make extra var YAML serialization more robust to non-dict extra vars --- awx/main/tests/unit/utils/test_safe_yaml.py | 26 ++++++++++++++++ awx/main/utils/safe_yaml.py | 34 ++++++++++++--------- 2 files changed, 45 insertions(+), 15 deletions(-) diff --git a/awx/main/tests/unit/utils/test_safe_yaml.py b/awx/main/tests/unit/utils/test_safe_yaml.py index 1c06ff5ca1..7f528ea595 100644 --- a/awx/main/tests/unit/utils/test_safe_yaml.py +++ b/awx/main/tests/unit/utils/test_safe_yaml.py @@ -1,11 +1,25 @@ +# -*- coding: utf-8 -*- + from copy import deepcopy +import pytest +import yaml from awx.main.utils.safe_yaml import safe_dump +@pytest.mark.parametrize('value', [None, 1, 1.5, []]) +def test_native_types(value): + # Native non-string types should dump the same way that `yaml.safe_dump` does + assert safe_dump(value) == yaml.safe_dump(value) + + def test_empty(): assert safe_dump({}) == '' +def test_raw_string(): + assert safe_dump('foo') == "!unsafe 'foo'\n" + + def test_kv_int(): assert safe_dump({'a': 1}) == "!unsafe 'a': 1\n" @@ -18,6 +32,10 @@ def test_kv_unsafe(): assert safe_dump({'a': 'b'}) == "!unsafe 'a': !unsafe 'b'\n" +def test_kv_unsafe_unicode(): + assert safe_dump({'a': u'🐉'}) == '!unsafe \'a\': !unsafe "\\U0001F409"\n' + + def test_kv_unsafe_in_list(): assert safe_dump({'a': ['b']}) == "!unsafe 'a':\n- !unsafe 'b'\n" @@ -57,3 +75,11 @@ def test_safe_marking_deep_nesting(): yaml = safe_dump(deep, deepcopy(deep)) for x in ('a', 'b', 'c', 'd', 'e'): assert "!unsafe '{}'".format(x) not in yaml + + +def test_deep_diff_unsafe_marking(): + deep = {'a': [1, [{'b': {'c': [{'d': 'e'}]}}]]} + jt_vars = deepcopy(deep) + deep['a'][1][0]['b']['z'] = 'not safe' + yaml = safe_dump(deep, jt_vars) + assert "!unsafe 'z'" in yaml diff --git a/awx/main/utils/safe_yaml.py b/awx/main/utils/safe_yaml.py index af572e9fd5..1b958b69d0 100644 --- a/awx/main/utils/safe_yaml.py +++ b/awx/main/utils/safe_yaml.py @@ -50,21 +50,25 @@ def safe_dump(x, safe_dict=None): 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) + if isinstance(x, dict): + 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) + else: + return yaml.dump_all([x], None, Dumper=SafeStringDumper, default_flow_style=False) def sanitize_jinja(arg):