From e9d29b1fe4285d90d7a4506b80260a9e24c3bcea Mon Sep 17 00:00:00 2001 From: Sam Doran Date: Mon, 14 Oct 2019 12:20:07 -0400 Subject: [PATCH] Properly mask no_log values is sub parameters during failure (#63405) * Get no_log parameters from subspec * Add changelog and unit tests * Handle list of dicts in suboptions Add fancy error message (this will probably haunt me) * Update unit tests to test for list of dicts in suboptions * Add integration tests * Validate parameters in dict and list In case it comes in as a string * Make changes based on feedback, fix tests * Simplify validators since we only need to validate dicts Add test for suboptions passed in as strings to ensure they get validated properly and turned into a dictionary. ci_complete * Add a few more integration tests --- .../no-log-sub-options-invalid-parameter.yaml | 2 + lib/ansible/module_utils/basic.py | 7 +- lib/ansible/module_utils/common/parameters.py | 26 ++ .../targets/no_log/library/module.py | 45 ++++ .../targets/no_log/no_log_suboptions.yml | 24 ++ .../no_log/no_log_suboptions_invalid.yml | 45 ++++ test/integration/targets/no_log/runme.sh | 6 + .../parameters/test_list_no_log_values.py | 225 ++++++++++++++++-- 8 files changed, 359 insertions(+), 21 deletions(-) create mode 100644 changelogs/fragments/no-log-sub-options-invalid-parameter.yaml create mode 100644 test/integration/targets/no_log/library/module.py create mode 100644 test/integration/targets/no_log/no_log_suboptions.yml create mode 100644 test/integration/targets/no_log/no_log_suboptions_invalid.yml diff --git a/changelogs/fragments/no-log-sub-options-invalid-parameter.yaml b/changelogs/fragments/no-log-sub-options-invalid-parameter.yaml new file mode 100644 index 00000000000..79019d64cfe --- /dev/null +++ b/changelogs/fragments/no-log-sub-options-invalid-parameter.yaml @@ -0,0 +1,2 @@ +bugfixes: + - '**security issue** - properly hide parameters marked with ``no_log`` in suboptions when invalid parameters are passed to the module (CVE-2019-14858)' diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py index 0cc1d521653..a96ce6aa5de 100644 --- a/lib/ansible/module_utils/basic.py +++ b/lib/ansible/module_utils/basic.py @@ -1447,7 +1447,11 @@ class AnsibleModule(object): if param is None: param = self.params - self.no_log_values.update(list_no_log_values(spec, param)) + try: + self.no_log_values.update(list_no_log_values(spec, param)) + except TypeError as te: + self.fail_json(msg="Failure when processing no_log parameters. Module invocation will be hidden. " + "%s" % to_native(te), invocation={'module_args': 'HIDDEN DUE TO FAILURE'}) self._deprecations.extend(list_deprecations(spec, param)) def _check_arguments(self, check_invalid_arguments, spec=None, param=None, legal_inputs=None): @@ -1722,7 +1726,6 @@ class AnsibleModule(object): self._set_fallbacks(spec, param) options_aliases = self._handle_aliases(spec, param, option_prefix=new_prefix) - self._handle_no_log_values(spec, param) options_legal_inputs = list(spec.keys()) + list(options_aliases.keys()) self._check_arguments(self.check_invalid_arguments, spec, param, options_legal_inputs) diff --git a/lib/ansible/module_utils/common/parameters.py b/lib/ansible/module_utils/common/parameters.py index a395a2b607a..9b41bc724af 100644 --- a/lib/ansible/module_utils/common/parameters.py +++ b/lib/ansible/module_utils/common/parameters.py @@ -8,10 +8,12 @@ __metaclass__ = type from ansible.module_utils._text import to_native from ansible.module_utils.common._collections_compat import Mapping from ansible.module_utils.common.collections import is_iterable +from ansible.module_utils.common.validation import check_type_dict from ansible.module_utils.six import ( binary_type, integer_types, + string_types, text_type, ) @@ -86,6 +88,30 @@ def list_no_log_values(argument_spec, params): if no_log_object: no_log_values.update(_return_datastructure_name(no_log_object)) + # Get no_log values from suboptions + sub_argument_spec = arg_opts.get('options') + if sub_argument_spec is not None: + wanted_type = arg_opts.get('type') + sub_parameters = params.get(arg_name) + + if sub_parameters is not None: + if wanted_type == 'dict' or (wanted_type == 'list' and arg_opts.get('elements', '') == 'dict'): + # Sub parameters can be a dict or list of dicts. Ensure parameters are always a list. + if not isinstance(sub_parameters, list): + sub_parameters = [sub_parameters] + + for sub_param in sub_parameters: + # Validate dict fields in case they came in as strings + + if isinstance(sub_param, string_types): + sub_param = check_type_dict(sub_param) + + if not isinstance(sub_param, Mapping): + raise TypeError("Value '{1}' in the sub parameter field '{0}' must by a {2}, " + "not '{1.__class__.__name__}'".format(arg_name, sub_param, wanted_type)) + + no_log_values.update(list_no_log_values(sub_argument_spec, sub_param)) + return no_log_values diff --git a/test/integration/targets/no_log/library/module.py b/test/integration/targets/no_log/library/module.py new file mode 100644 index 00000000000..d4f3c565cff --- /dev/null +++ b/test/integration/targets/no_log/library/module.py @@ -0,0 +1,45 @@ +#!/usr/bin/python +# -*- coding: utf-8 -*- +# Copyright (c) 2019 Ansible Project +# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt) + +from __future__ import absolute_import, division, print_function +__metaclass__ = type + +from ansible.module_utils.basic import AnsibleModule + + +def main(): + module = AnsibleModule( + argument_spec={ + 'state': {}, + 'secret': {'no_log': True}, + 'subopt_dict': { + 'type': 'dict', + 'options': { + 'str_sub_opt1': {'no_log': True}, + 'str_sub_opt2': {}, + 'nested_subopt': { + 'type': 'dict', + 'options': { + 'n_subopt1': {'no_log': True}, + } + } + } + }, + 'subopt_list': { + 'type': 'list', + 'elements': 'dict', + 'options': { + 'subopt1': {'no_log': True}, + 'subopt2': {}, + } + } + + } + ) + module.exit_json(msg='done') + + +if __name__ == '__main__': + main() diff --git a/test/integration/targets/no_log/no_log_suboptions.yml b/test/integration/targets/no_log/no_log_suboptions.yml new file mode 100644 index 00000000000..e67ecfe21b5 --- /dev/null +++ b/test/integration/targets/no_log/no_log_suboptions.yml @@ -0,0 +1,24 @@ +- name: test no log with suboptions + hosts: testhost + gather_facts: no + + tasks: + - name: Task with suboptions + module: + secret: GLAMOROUS + subopt_dict: + str_sub_opt1: AFTERMATH + str_sub_opt2: otherstring + nested_subopt: + n_subopt1: MANPOWER + + subopt_list: + - subopt1: UNTAPPED + subopt2: thridstring + + - subopt1: CONCERNED + + - name: Task with suboptions as string + module: + secret: MARLIN + subopt_dict: str_sub_opt1=FLICK diff --git a/test/integration/targets/no_log/no_log_suboptions_invalid.yml b/test/integration/targets/no_log/no_log_suboptions_invalid.yml new file mode 100644 index 00000000000..933a8a9bb27 --- /dev/null +++ b/test/integration/targets/no_log/no_log_suboptions_invalid.yml @@ -0,0 +1,45 @@ +- name: test no log with suboptions + hosts: testhost + gather_facts: no + ignore_errors: yes + + tasks: + - name: Task with suboptions and invalid parameter + module: + secret: SUPREME + invalid: param + subopt_dict: + str_sub_opt1: IDIOM + str_sub_opt2: otherstring + nested_subopt: + n_subopt1: MOCKUP + + subopt_list: + - subopt1: EDUCATED + subopt2: thridstring + - subopt1: FOOTREST + + - name: Task with suboptions as string with invalid parameter + module: + secret: FOOTREST + invalid: param + subopt_dict: str_sub_opt1=CRAFTY + + - name: Task with suboptions with dict instead of list + module: + secret: FELINE + subopt_dict: + str_sub_opt1: CRYSTAL + str_sub_opt2: otherstring + nested_subopt: + n_subopt1: EXPECTANT + subopt_list: + foo: bar + + - name: Task with suboptions with incorrect data type + module: + secret: AGROUND + subopt_dict: 9068.21361 + subopt_list: + - subopt1: GOLIATH + - subopt1: FREEFALL diff --git a/test/integration/targets/no_log/runme.sh b/test/integration/targets/no_log/runme.sh index 474e755e13d..bb5c048fc9a 100755 --- a/test/integration/targets/no_log/runme.sh +++ b/test/integration/targets/no_log/runme.sh @@ -13,3 +13,9 @@ set -eux # no log disabled, should produce 0 censored [ "$(ansible-playbook dynamic.yml -i ../../inventory -vvvvv "$@" -e unsafe_show_logs=yes|grep -c 'output has been hidden')" = "0" ] + +# test no log for sub options +[ "$(ansible-playbook no_log_suboptions.yml -i ../../inventory -vvvvv "$@" | grep -Ec '(MANPOWER|UNTAPPED|CONCERNED|MARLIN|FLICK)')" = "0" ] + +# test invalid data passed to a suboption +[ "$(ansible-playbook no_log_suboptions_invalid.yml -i ../../inventory -vvvvv "$@" | grep -Ec '(SUPREME|IDIOM|MOCKUP|EDUCATED|FOOTREST|CRAFTY|FELINE|CRYSTAL|EXPECTANT|AGROUND|GOLIATH|FREEFALL)')" = "0" ] diff --git a/test/units/module_utils/common/parameters/test_list_no_log_values.py b/test/units/module_utils/common/parameters/test_list_no_log_values.py index 7c9b31c28d9..1b740555935 100644 --- a/test/units/module_utils/common/parameters/test_list_no_log_values.py +++ b/test/units/module_utils/common/parameters/test_list_no_log_values.py @@ -11,31 +11,218 @@ from ansible.module_utils.common.parameters import list_no_log_values @pytest.fixture -def params(): - return { - 'secret': 'undercookwovennativity', - 'other_secret': 'cautious-slate-makeshift', - 'state': 'present', - 'value': 5, - } +def argument_spec(): + # Allow extra specs to be passed to the fixture, which will be added to the output + def _argument_spec(extra_opts=None): + spec = { + 'secret': {'type': 'str', 'no_log': True}, + 'other_secret': {'type': 'str', 'no_log': True}, + 'state': {'type': 'str'}, + 'value': {'type': 'int'}, + } + if extra_opts: + spec.update(extra_opts) -def test_list_no_log_values(params): - argument_spec = { - 'secret': {'type': 'str', 'no_log': True}, - 'other_secret': {'type': 'str', 'no_log': True}, - 'state': {'type': 'str'}, - 'value': {'type': 'int'}, - } - result = set(('undercookwovennativity', 'cautious-slate-makeshift')) - assert result == list_no_log_values(argument_spec, params) + return spec + + return _argument_spec + + +@pytest.fixture +def module_parameters(): + # Allow extra parameters to be passed to the fixture, which will be added to the output + def _module_parameters(extra_params=None): + params = { + 'secret': 'under', + 'other_secret': 'makeshift', + 'state': 'present', + 'value': 5, + } + + if extra_params: + params.update(extra_params) + + return params + + return _module_parameters -def test_list_no_log_values_no_secrets(params): +def test_list_no_log_values_no_secrets(module_parameters): argument_spec = { 'other_secret': {'type': 'str', 'no_log': False}, 'state': {'type': 'str'}, 'value': {'type': 'int'}, } - result = set() - assert result == list_no_log_values(argument_spec, params) + expected = set() + assert expected == list_no_log_values(argument_spec, module_parameters) + + +def test_list_no_log_values(argument_spec, module_parameters): + expected = set(('under', 'makeshift')) + assert expected == list_no_log_values(argument_spec(), module_parameters()) + + +@pytest.mark.parametrize('extra_params', [ + {'subopt1': 1}, + {'subopt1': 3.14159}, + {'subopt1': ['one', 'two']}, + {'subopt1': ('one', 'two')}, +]) +def test_list_no_log_values_invalid_suboptions(argument_spec, module_parameters, extra_params): + extra_opts = { + 'subopt1': { + 'type': 'dict', + 'options': { + 'sub_1_1': {}, + } + } + } + + with pytest.raises(TypeError, match=r"(Value '.*?' in the sub parameter field '.*?' must by a dict, not '.*?')" + r"|(dictionary requested, could not parse JSON or key=value)"): + list_no_log_values(argument_spec(extra_opts), module_parameters(extra_params)) + + +def test_list_no_log_values_suboptions(argument_spec, module_parameters): + extra_opts = { + 'subopt1': { + 'type': 'dict', + 'options': { + 'sub_1_1': {'no_log': True}, + 'sub_1_2': {'type': 'list'}, + } + } + } + + extra_params = { + 'subopt1': { + 'sub_1_1': 'bagel', + 'sub_1_2': ['pebble'], + } + } + + expected = set(('under', 'makeshift', 'bagel')) + assert expected == list_no_log_values(argument_spec(extra_opts), module_parameters(extra_params)) + + +def test_list_no_log_values_sub_suboptions(argument_spec, module_parameters): + extra_opts = { + 'sub_level_1': { + 'type': 'dict', + 'options': { + 'l1_1': {'no_log': True}, + 'l1_2': {}, + 'l1_3': { + 'type': 'dict', + 'options': { + 'l2_1': {'no_log': True}, + 'l2_2': {}, + } + } + } + } + } + + extra_params = { + 'sub_level_1': { + 'l1_1': 'saucy', + 'l1_2': 'napped', + 'l1_3': { + 'l2_1': 'corporate', + 'l2_2': 'tinsmith', + } + } + } + + expected = set(('under', 'makeshift', 'saucy', 'corporate')) + assert expected == list_no_log_values(argument_spec(extra_opts), module_parameters(extra_params)) + + +def test_list_no_log_values_suboptions_list(argument_spec, module_parameters): + extra_opts = { + 'subopt1': { + 'type': 'list', + 'elements': 'dict', + 'options': { + 'sub_1_1': {'no_log': True}, + 'sub_1_2': {}, + } + } + } + + extra_params = { + 'subopt1': [ + { + 'sub_1_1': ['playroom', 'luxury'], + 'sub_1_2': 'deuce', + }, + { + 'sub_1_2': ['squishier', 'finished'], + } + ] + } + + expected = set(('under', 'makeshift', 'playroom', 'luxury')) + assert expected == list_no_log_values(argument_spec(extra_opts), module_parameters(extra_params)) + + +def test_list_no_log_values_sub_suboptions_list(argument_spec, module_parameters): + extra_opts = { + 'subopt1': { + 'type': 'list', + 'elements': 'dict', + 'options': { + 'sub_1_1': {'no_log': True}, + 'sub_1_2': {}, + 'subopt2': { + 'type': 'list', + 'elements': 'dict', + 'options': { + 'sub_2_1': {'no_log': True, 'type': 'list'}, + 'sub_2_2': {}, + } + } + } + } + } + + extra_params = { + 'subopt1': { + 'sub_1_1': ['playroom', 'luxury'], + 'sub_1_2': 'deuce', + 'subopt2': [ + { + 'sub_2_1': ['basis', 'gave'], + 'sub_2_2': 'liquid', + }, + { + 'sub_2_1': ['composure', 'thumping'] + }, + ] + } + } + + expected = set(('under', 'makeshift', 'playroom', 'luxury', 'basis', 'gave', 'composure', 'thumping')) + assert expected == list_no_log_values(argument_spec(extra_opts), module_parameters(extra_params)) + + +@pytest.mark.parametrize('extra_params, expected', ( + ({'subopt_dict': 'dict_subopt1=rekindle-scandal,dict_subopt2=subgroupavenge'}, ('rekindle-scandal',)), + ({'subopt_dict': 'dict_subopt1=aversion-mutable dict_subopt2=subgroupavenge'}, ('aversion-mutable',)), + ({'subopt_dict': ['dict_subopt1=blip-marine,dict_subopt2=subgroupavenge', 'dict_subopt1=tipping,dict_subopt2=hardening']}, ('blip-marine', 'tipping')), +)) +def test_string_suboptions_as_string(argument_spec, module_parameters, extra_params, expected): + extra_opts = { + 'subopt_dict': { + 'type': 'dict', + 'options': { + 'dict_subopt1': {'no_log': True}, + 'dict_subopt2': {}, + }, + }, + } + + result = set(('under', 'makeshift')) + result.update(expected) + assert result == list_no_log_values(argument_spec(extra_opts), module_parameters(extra_params))