[2.9] CVE-2020-1746 - Remove the params module option from ldap_attr and ldap_entry (#68714)

* Remove the params module option from ldap_attr and ldap_entry

Module options that circumvent Ansible's option handling were disallowed
in:
https://meetbot.fedoraproject.org/ansible-meeting/2017-09-28/ansible_dev_meeting.2017-09-28-15.00.log.html

Additionally, this particular usage can be insecure if bind_pw is set
this way as the password could end up in a logfile or displayed on
stdout.

Fixes CVE-2020-1746

(cherry picked from commit 0ff609f1bc5e391fa25710b9a0baaf669f347eb1)

* Fix formatting for option names

Co-Authored-By: Felix Fontein <felix@fontein.de>

* Fix fail_json

* fix indentation error

Co-authored-by: Toshio Kuratomi <a.badger@gmail.com>
Co-authored-by: Felix Fontein <felix@fontein.de>

7 files changed, 102 insertions, 30 deletions

diff --git a/changelogs/fragments/ldap-params-removal.yml b/changelogs/fragments/ldap-params-removal.yml
new file mode 100644
index 0000000000..8e28b6aa37
--- /dev/null
+++ b/changelogs/fragments/ldap-params-removal.yml
@@ -0,0 +1,8 @@
+removed_features:
+  - "ldap_attr, ldap_entry - The ``params`` option has been removed in
+    Ansible-2.10 as it circumvents Ansible's option handling.  Setting
+    ``bind_pw`` with the ``params`` option was disallowed in Ansible-2.7, 2.8,
+    and 2.9 as it was insecure.  For information about this policy, see the
+discussion at:
+    https://meetbot.fedoraproject.org/ansible-meeting/2017-09-28/ansible_dev_meeting.2017-09-28-15.00.log.html
+    This fixes CVE-2020-1746"
diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.7.rst b/docs/docsite/rst/porting_guides/porting_guide_2.7.rst
index ac412bb4fc..816f77bc5c 100644
--- a/docs/docsite/rst/porting_guides/porting_guide_2.7.rst
+++ b/docs/docsite/rst/porting_guides/porting_guide_2.7.rst
@@ -103,6 +103,15 @@ hashing algorithm being used with this filter.
 Deprecated
 ==========
 
+Expedited Deprecation: Removal of the params module option in ``ldap_attr`` and ``ldap_entry``
+----------------------------------------------------------------------------------------------
+
+The ``params`` module option in ``ldap_attr`` and ``ldap_entry`` are deprecated on a short cycle (to
+be removed in Ansible-2.10) due to circumventing Ansible's normal option handling.  In particular,
+if the ``bind_pw`` option is set with ``params``, the value of the option could end up being placed
+in a logfile or displayed on stdout.
+
+
 Expedited Deprecation: Use of ``__file__`` in ``AnsibleModule``
 ---------------------------------------------------------------
 
@@ -192,6 +201,11 @@ The following modules will be removed in Ansible 2.11. Please update your playbo
 Noteworthy module changes
 -------------------------
 
+* **Security Issue** Setting ``bind_pw`` with the ``params`` option for the ``ldap_entry`` and
+  ``ldap_attr`` modules has been disallowed.  If ``bind_pw`` was set with ``params``, the value
+  could have ended up in a logfile or displayed on stdout.  Set ``bind_pw`` directly, with the
+  modules' options instead.
+
 * Check mode is now supported in the ``command`` and ``shell`` modules. However, only when ``creates`` or ``removes`` is
   specified. If either of these are specified, the module will check for existence of the file and report the correct
   changed status, if they are not included the module will skip like it had done previously.
diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.8.rst b/docs/docsite/rst/porting_guides/porting_guide_2.8.rst
index 5f376b5881..d4879d353e 100644
--- a/docs/docsite/rst/porting_guides/porting_guide_2.8.rst
+++ b/docs/docsite/rst/porting_guides/porting_guide_2.8.rst
@@ -324,6 +324,11 @@ By default in Ansible 2.7, or with ``AGNOSTIC_BECOME_PROMPT=False`` in Ansible 2
 Deprecated
 ==========
 
+* The ``params`` module option in ``ldap_attr`` and ``ldap_entry`` are deprecated on a short cycle
+  (to be removed in Ansible-2.10) due to circumventing Ansible's normal option handling.  In
+  particular, if the ``bind_pw`` option is set with ``params``, the value of the option could end up
+  being placed in a logfile or displayed on stdout.
+
 * Setting the async directory using ``ANSIBLE_ASYNC_DIR`` as an task/play environment key is deprecated and will be
   removed in Ansible 2.12. You can achieve the same result by setting ``ansible_async_dir`` as a variable like::
 
@@ -406,6 +411,10 @@ The following modules will be removed in Ansible 2.12. Please update your playbo
 Noteworthy module changes
 -------------------------
 
+* **Security Issue** Setting ``bind_pw`` with the ``params`` option for the ``ldap_entry`` and
+  ``ldap_attr`` modules has been disallowed.  If ``bind_pw`` was set with ``params``, the value
+  could have ended up in a logfile or displayed on stdout.  Set ``bind_pw`` directly, with the
+  modules' options instead.
 * The ``foreman`` and ``katello`` modules have been deprecated in favor of a set of modules that are broken out per entity with better idempotency in mind.
 * The ``foreman`` and ``katello`` modules replacement is officially part of the Foreman Community and supported there.
 * The ``tower_credential`` module originally required the ``ssh_key_data`` to be the path to a ssh_key_file.
diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.9.rst b/docs/docsite/rst/porting_guides/porting_guide_2.9.rst
index 603c8fe4f1..423823f253 100644
--- a/docs/docsite/rst/porting_guides/porting_guide_2.9.rst
+++ b/docs/docsite/rst/porting_guides/porting_guide_2.9.rst
@@ -42,7 +42,10 @@ Command Line
 Deprecated
 ==========
 
-No notable changes
+- The ``params`` module option in ``ldap_attr`` and ``ldap_entry`` are deprecated on a short cycle (to be
+  removed in Ansible-2.10) due to circumventing Ansible's normal option handling.  In particular, if
+  the ``bind_pw`` option is set with ``params``, the value of the option could end up being placed in
+  a logfile or displayed on stdout.
 
 
 Collection loader changes
@@ -691,6 +694,10 @@ be removed in Ansible 2.13. Please update update your playbooks accordingly.
 Noteworthy module changes
 -------------------------
 
+* **Security Issue** Setting ``bind_pw`` with the ``params`` option for the ``ldap_entry`` and
+  ``ldap_attr`` modules has been disallowed.  If ``bind_pw`` was set with ``params``, the value could
+  have ended up in a logfile or displayed on stdout.  Set ``bind_pw`` directly, with the modules'
+  options instead.
 * :ref:`vmware_cluster <vmware_cluster_module>` was refactored for easier maintenance/bugfixes. Use the three new, specialized modules to configure clusters. Configure DRS with :ref:`vmware_cluster_drs <vmware_cluster_drs_module>`, HA with :ref:`vmware_cluster_ha <vmware_cluster_ha_module>` and vSAN with :ref:`vmware_cluster_vsan <vmware_cluster_vsan_module>`.
 * :ref:`vmware_dvswitch <vmware_dvswitch_module>` accepts ``folder`` parameter to place dvswitch in user defined folder. This option makes ``datacenter`` as an optional parameter.
 * :ref:`vmware_datastore_cluster <vmware_datastore_cluster_module>` accepts ``folder`` parameter to place datastore cluster in user defined folder. This option makes ``datacenter`` as an optional parameter.
diff --git a/lib/ansible/modules/net_tools/ldap/ldap_attr.py b/lib/ansible/modules/net_tools/ldap/ldap_attr.py
index d6c43816c9..80bc8f79f6 100644
--- a/lib/ansible/modules/net_tools/ldap/ldap_attr.py
+++ b/lib/ansible/modules/net_tools/ldap/ldap_attr.py
@@ -35,6 +35,9 @@ notes:
     rules. This should work out in most cases, but it is theoretically
     possible to see spurious changes when target and actual values are
     semantically identical but lexically distinct.
+  - "The I(params) parameter is deprecated in Ansible-2.7 due to circumventing Ansible's parameter
+     handling. The I(params) parameter started disallowing setting the I(bind_pw) parameter in Ansible-2.7
+     as it was insecure to set the parameter that way."
 version_added: '2.3'
 author:
   - Jiri Tyr (@jtyr)
@@ -62,10 +65,6 @@ options:
         a list of strings (see examples).
     type: raw
     required: true
-  params:
-    description:
-    - Additional module parameters.
-    type: dict
 extends_documentation_fragment:
 - ldap.documentation
 '''
@@ -133,13 +132,15 @@ EXAMPLES = r'''
 #   server_uri: ldap://localhost/
 #   bind_dn: cn=admin,dc=example,dc=com
 #   bind_pw: password
+#
+# In the example below, 'args' is a task keyword, passed at the same level as the module
 - name: Get rid of an unneeded attribute
   ldap_attr:
     dn: uid=jdoe,ou=people,dc=example,dc=com
     name: shadowExpire
     values: []
     state: exact
-    params: "{{ ldap_auth }}"
+  args: "{{ ldap_auth }}"
 '''
 
 RETURN = r'''
@@ -151,6 +152,7 @@ modlist:
 '''
 
 import traceback
+from distutils.version import LooseVersion
 
 from ansible.module_utils.basic import AnsibleModule, missing_required_lib
 from ansible.module_utils._text import to_native, to_bytes
@@ -250,11 +252,28 @@ def main():
         module.fail_json(msg=missing_required_lib('python-ldap'),
                          exception=LDAP_IMP_ERR)
 
-    # Update module parameters with user's parameters if defined
-    if 'params' in module.params and isinstance(module.params['params'], dict):
-        module.params.update(module.params['params'])
-        # Remove the params
-        module.params.pop('params', None)
+    # For Ansible-2.9.x and below, allow the params module parameter with a warning
+    if LooseVersion(module.ansible_version) < LooseVersion('2.10'):
+        if module.params['params']:
+            module.deprecate("The `params` option to ldap_attr will be removed in Ansible 2.10"
+                             " since it circumvents Ansible's option handling", version='2.10')
+
+            # However, the bind_pw parameter contains a password so it **must** go through the normal
+            # argument parsing even though removing it breaks backwards compat.
+            if 'bind_pw' in module.params['params']:
+                module.fail_json(msg="Using `bind_pw` with the `params` option has been disallowed since"
+                                 " it is insecure.  Use the `bind_pw` option directly.  The `params`"
+                                 " option will be removed in Ansible-2.10")
+
+            # Update module parameters with user's parameters if defined
+            module.params.update(module.params['params'])
+            # Remove params itself
+            module.params.pop('params', None)
+    else:
+        # For Ansible 2.10 and above
+        if module.params['params']:
+            module.fail_json(msg="The `params` option to ldap_attr was removed in Ansible-2.10 since"
+                             " it circumvents Ansible's option handling")
 
     # Instantiate the LdapAttr object
     ldap = LdapAttr(module)
diff --git a/lib/ansible/modules/net_tools/ldap/ldap_entry.py b/lib/ansible/modules/net_tools/ldap/ldap_entry.py
index 0a354b6c46..cfc572fadc 100644
--- a/lib/ansible/modules/net_tools/ldap/ldap_entry.py
+++ b/lib/ansible/modules/net_tools/ldap/ldap_entry.py
@@ -32,6 +32,9 @@ notes:
     rule allowing root to modify the server configuration. If you need to use
     a simple bind to access your server, pass the credentials in I(bind_dn)
     and I(bind_pw).
+  - "The I(params) parameter is deprecated in Ansible-2.7 due to circumventing Ansible's parameter
+     handling. The I(params) parameter started disallowing setting the I(bind_pw) parameter in Ansible-2.7
+     as it was insecure to set the parameter that way."
 version_added: '2.3'
 author:
   - Jiri Tyr (@jtyr)
@@ -48,11 +51,6 @@ options:
       - If I(state=present), value or list of values to use when creating
         the entry. It can either be a string or an actual list of
         strings.
-  params:
-    description:
-      - List of options which allows to overwrite any of the task or the
-        I(attributes) options. To remove an option, set the value of the option
-        to C(null).
   state:
     description:
       - The target state of the entry.
@@ -94,11 +92,13 @@ EXAMPLES = """
 #   server_uri: ldap://localhost/
 #   bind_dn: cn=admin,dc=example,dc=com
 #   bind_pw: password
+#
+# In the example below, 'args' is a task keyword, passed at the same level as the module
 - name: Get rid of an old entry
   ldap_entry:
     dn: ou=stuff,dc=example,dc=com
     state: absent
-    params: "{{ ldap_auth }}"
+  args: "{{ ldap_auth }}"
 """
 
 
@@ -107,6 +107,7 @@ RETURN = """
 """
 
 import traceback
+from distutils.version import LooseVersion
 
 from ansible.module_utils.basic import AnsibleModule, missing_required_lib
 from ansible.module_utils.six import string_types
@@ -204,6 +205,29 @@ def main():
         module.fail_json(msg=missing_required_lib('python-ldap'),
                          exception=LDAP_IMP_ERR)
 
+    # For Ansible-2.9.x and below, allow the params module parameter with a warning
+    if LooseVersion(module.ansible_version) < LooseVersion('2.10'):
+        if module.params['params']:
+            module.deprecate("The `params` option to ldap_attr will be removed in Ansible 2.10"
+                             " since it circumvents Ansible's option handling", version='2.10')
+
+            # However, the bind_pw parameter contains a password so it **must** go through the normal
+            # argument parsing even though removing it breaks backwards compat.
+            if 'bind_pw' in module.params['params']:
+                module.fail_json(msg="Using `bind_pw` with the `params` option has been disallowed since"
+                                 " it is insecure.  Use the `bind_pw` option directly.  The `params`"
+                                 " option will be removed in Ansible-2.10")
+
+            # Update module parameters with user's parameters if defined
+            module.params.update(module.params['params'])
+            # Remove params itself
+            module.params.pop('params', None)
+    else:
+        # For Ansible 2.10 and above
+        if module.params['params']:
+            module.fail_json(msg="The `params` option to ldap_attr was removed in Ansible-2.10 since"
+                             " it circumvents Ansible's option handling")
+
     state = module.params['state']
 
     # Check if objectClass is present when needed
@@ -217,17 +241,6 @@ def main():
                 isinstance(module.params['objectClass'], list))):
         module.fail_json(msg="objectClass must be either a string or a list.")
 
-    # Update module parameters with user's parameters if defined
-    if 'params' in module.params and isinstance(module.params['params'], dict):
-        for key, val in module.params['params'].items():
-            if key in module.argument_spec:
-                module.params[key] = val
-            else:
-                module.params['attributes'][key] = val
-
-        # Remove the params
-        module.params.pop('params', None)
-
     # Instantiate the LdapEntry object
     ldap = LdapEntry(module)
 
diff --git a/test/sanity/ignore.txt b/test/sanity/ignore.txt
index b7a5d20760..672e279dfd 100644
--- a/test/sanity/ignore.txt
+++ b/test/sanity/ignore.txt
@@ -2407,9 +2407,11 @@ lib/ansible/modules/net_tools/dnsmadeeasy.py validate-modules:doc-missing-type
 lib/ansible/modules/net_tools/ip_netns.py validate-modules:doc-missing-type
 lib/ansible/modules/net_tools/ipinfoio_facts.py validate-modules:parameter-type-not-in-doc
 lib/ansible/modules/net_tools/ipinfoio_facts.py validate-modules:doc-missing-type
-lib/ansible/modules/net_tools/ldap/ldap_attr.py validate-modules:parameter-type-not-in-doc
-lib/ansible/modules/net_tools/ldap/ldap_entry.py validate-modules:parameter-type-not-in-doc
+lib/ansible/modules/net_tools/ldap/ldap_attr.py validate-modules:parameter-type-not-in-doc  # This triggers when a parameter is undocumented
+lib/ansible/modules/net_tools/ldap/ldap_attr.py validate-modules:undocumented-parameter # Parameter removed but reason for removal is shown by custom code
 lib/ansible/modules/net_tools/ldap/ldap_entry.py validate-modules:doc-missing-type
+lib/ansible/modules/net_tools/ldap/ldap_entry.py validate-modules:parameter-type-not-in-doc  # This triggers when a parameter is undocumented
+lib/ansible/modules/net_tools/ldap/ldap_entry.py validate-modules:undocumented-parameter # Parameter removed but reason for removal is shown by custom code
 lib/ansible/modules/net_tools/ldap/ldap_passwd.py validate-modules:doc-missing-type
 lib/ansible/modules/net_tools/netbox/netbox_device.py validate-modules:parameter-type-not-in-doc
 lib/ansible/modules/net_tools/netbox/netbox_device.py validate-modules:doc-missing-type