[stable-2.7] Properly mask no_log values is sub parameters during failure (#63405)

(cherry picked from commit 156330b485)

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

7 files changed, 151 insertions, 1 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 2b33aa3ee9..d2a43e7f87 100644
--- a/lib/ansible/module_utils/basic.py
+++ b/lib/ansible/module_utils/basic.py
@@ -1665,6 +1665,34 @@ class AnsibleModule(object):
                 if no_log_object:
                     self.no_log_values.update(return_values(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 = param.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 = self._check_type_dict(sub_param)
+
+                            try:
+                                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))
+                            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._handle_no_log_values(sub_argument_spec, sub_param)
+
             if arg_opts.get('removed_in_version') is not None and arg_name in param:
                 self._deprecations.append({
                     'msg': "Param '%s' is deprecated. See the module docs for more information" % arg_name,
@@ -2032,7 +2060,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/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_local.yml b/test/integration/targets/no_log/no_log_local.yml
index aacf7de276..41b1858e62 100644
--- a/test/integration/targets/no_log/no_log_local.yml
+++ b/test/integration/targets/no_log/no_log_local.yml
@@ -47,6 +47,7 @@
       poll: 1
       shell: echo "DO_NOT_LOG_ASYNC_TASK_SUCCEEDED"
       no_log: true
+      ignore_errors: yes
 
 - name: play-level no_log set
   hosts: testhost
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" ]