[stable-2.8] 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
(cherry picked from commit e9d29b1fe4)

Co-authored-by: Sam Doran <sdoran@redhat.com>

8 files changed, 359 insertions, 21 deletions

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 0000000000..79019d64cf
--- /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 88df2d77d3..af4fa48e72 100644
--- a/lib/ansible/module_utils/basic.py
+++ b/lib/ansible/module_utils/basic.py
@@ -1429,7 +1429,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):
@@ -1699,7 +1703,6 @@ class AnsibleModule(object):
                     self._set_fallbacks(spec, param)
                     options_aliases = self._handle_aliases(spec, param)
 
-                    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 71285fbd2d..40580d8ee8 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 0000000000..d4f3c565cf
--- /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 0000000000..e67ecfe21b
--- /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 0000000000..933a8a9bb2
--- /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 474e755e13..bb5c048fc9 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 7c9b31c28d..1b74055593 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))