From 992a4a945148aab5912eacad1431fdab0e622534 Mon Sep 17 00:00:00 2001 From: Toshio Kuratomi Date: Fri, 28 Feb 2020 09:16:44 -0800 Subject: [PATCH 1/5] 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) --- changelogs/fragments/ldap-params-removal.yml | 8 ++++ .../rst/porting_guides/porting_guide_2.7.rst | 14 ++++++ .../modules/net_tools/ldap/ldap_attr.py | 31 ++++++++++-- .../modules/net_tools/ldap/ldap_entry.py | 47 ++++++++++++------- 4 files changed, 79 insertions(+), 21 deletions(-) create mode 100644 changelogs/fragments/ldap-params-removal.yml diff --git a/changelogs/fragments/ldap-params-removal.yml b/changelogs/fragments/ldap-params-removal.yml new file mode 100644 index 00000000000000..8e28b6aa377898 --- /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 3c253837d8582f..6c0896a83226d1 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/lib/ansible/modules/net_tools/ldap/ldap_attr.py b/lib/ansible/modules/net_tools/ldap/ldap_attr.py index 8265d20ba62dda..a450e6a1bf4039 100644 --- a/lib/ansible/modules/net_tools/ldap/ldap_attr.py +++ b/lib/ansible/modules/net_tools/ldap/ldap_attr.py @@ -38,6 +38,9 @@ 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 C(params) parameter was removed in Ansible-2.10 due to circumventing Ansible's parameter + handling. The C(params) parameter started disallowing setting the 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) @@ -131,13 +134,15 @@ # 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 }}" """ @@ -150,6 +155,7 @@ """ import traceback +from distutils.version import LooseVersion from ansible.module_utils.basic import AnsibleModule from ansible.module_utils._text import to_native, to_bytes @@ -249,11 +255,28 @@ def main(): module.fail_json( msg="Missing required 'ldap' module (pip install python-ldap)") - # Update module parameters with user's parameters if defined - if 'params' in module.params and isinstance(module.params['params'], dict): + # 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("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 the params + # Remove params itself module.params.pop('params', None) + else: + # For Ansible 2.10 and above + if module.params['params']: + module.fail_json("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 5fca7034a35c18..90a01272aa391f 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 @@ 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 C(params) parameter was removed in Ansible-2.10 due to circumventing Ansible's parameter + handling. The C(params) parameter started disallowing setting the 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 @@ - 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 @@ # 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 @@ """ import traceback +from distutils.version import LooseVersion from ansible.module_utils.basic import AnsibleModule from ansible.module_utils.six import string_types @@ -202,6 +203,29 @@ def main(): module.fail_json( msg="Missing required 'ldap' module (pip install python-ldap).") + # 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("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("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 @@ -215,17 +239,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) From 4cab46b6c250803bc9fc9d8a151f2ed969dc970f Mon Sep 17 00:00:00 2001 From: s-hertel Date: Mon, 6 Apr 2020 12:44:01 -0400 Subject: [PATCH 2/5] Fix formatting for option names Co-Authored-By: Felix Fontein --- lib/ansible/modules/net_tools/ldap/ldap_attr.py | 6 +++--- lib/ansible/modules/net_tools/ldap/ldap_entry.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/ansible/modules/net_tools/ldap/ldap_attr.py b/lib/ansible/modules/net_tools/ldap/ldap_attr.py index a450e6a1bf4039..cdc32acf398980 100644 --- a/lib/ansible/modules/net_tools/ldap/ldap_attr.py +++ b/lib/ansible/modules/net_tools/ldap/ldap_attr.py @@ -38,9 +38,9 @@ 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 C(params) parameter was removed in Ansible-2.10 due to circumventing Ansible's parameter - handling. The C(params) parameter started disallowing setting the bind_pw parameter in - Ansible-2.7 as it was insecure to set the parameter that way." + - "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) diff --git a/lib/ansible/modules/net_tools/ldap/ldap_entry.py b/lib/ansible/modules/net_tools/ldap/ldap_entry.py index 90a01272aa391f..d8380553570973 100644 --- a/lib/ansible/modules/net_tools/ldap/ldap_entry.py +++ b/lib/ansible/modules/net_tools/ldap/ldap_entry.py @@ -32,9 +32,9 @@ 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 C(params) parameter was removed in Ansible-2.10 due to circumventing Ansible's parameter - handling. The C(params) parameter started disallowing setting the bind_pw parameter in - Ansible-2.7 as it was insecure to set the parameter that way." + - "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) From f7349079c782e216c32529c0acd4f521f3da863c Mon Sep 17 00:00:00 2001 From: s-hertel Date: Mon, 6 Apr 2020 12:46:10 -0400 Subject: [PATCH 3/5] Fix fail_json --- lib/ansible/modules/net_tools/ldap/ldap_attr.py | 4 ++-- lib/ansible/modules/net_tools/ldap/ldap_entry.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/ansible/modules/net_tools/ldap/ldap_attr.py b/lib/ansible/modules/net_tools/ldap/ldap_attr.py index cdc32acf398980..3c29aaaa10aa54 100644 --- a/lib/ansible/modules/net_tools/ldap/ldap_attr.py +++ b/lib/ansible/modules/net_tools/ldap/ldap_attr.py @@ -264,7 +264,7 @@ def main(): # 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("Using `bind_pw` with the `params` option has been disallowed since" + 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") @@ -275,7 +275,7 @@ def main(): else: # For Ansible 2.10 and above if module.params['params']: - module.fail_json("The `params` option to ldap_attr was removed in Ansible-2.10 since" + 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 diff --git a/lib/ansible/modules/net_tools/ldap/ldap_entry.py b/lib/ansible/modules/net_tools/ldap/ldap_entry.py index d8380553570973..0b4c04e90a9afc 100644 --- a/lib/ansible/modules/net_tools/ldap/ldap_entry.py +++ b/lib/ansible/modules/net_tools/ldap/ldap_entry.py @@ -212,7 +212,7 @@ def main(): # 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("Using `bind_pw` with the `params` option has been disallowed since" + 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") @@ -223,7 +223,7 @@ def main(): else: # For Ansible 2.10 and above if module.params['params']: - module.fail_json("The `params` option to ldap_attr was removed in Ansible-2.10 since" + 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'] From 5344a3b72a87714405e1b37a648e6070eff00594 Mon Sep 17 00:00:00 2001 From: s-hertel Date: Mon, 6 Apr 2020 13:52:47 -0400 Subject: [PATCH 4/5] update sanity --- test/sanity/validate-modules/ignore.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/test/sanity/validate-modules/ignore.txt b/test/sanity/validate-modules/ignore.txt index c55b59a66ec8f2..736b3b686ebb1e 100644 --- a/test/sanity/validate-modules/ignore.txt +++ b/test/sanity/validate-modules/ignore.txt @@ -631,6 +631,7 @@ lib/ansible/modules/net_tools/infinity/infinity.py E326 lib/ansible/modules/net_tools/ipify_facts.py E324 lib/ansible/modules/net_tools/ipify_facts.py E325 lib/ansible/modules/net_tools/ldap/ldap_attr.py E322 +lib/ansible/modules/net_tools/ldap/ldap_entry.py E322 lib/ansible/modules/net_tools/nmcli.py E324 lib/ansible/modules/net_tools/omapi_host.py E317 lib/ansible/modules/net_tools/omapi_host.py E322 From c3fbc6b59e756b8948c45f24fdedd8ba4e911362 Mon Sep 17 00:00:00 2001 From: s-hertel Date: Thu, 9 Apr 2020 09:26:27 -0400 Subject: [PATCH 5/5] fix indentation error --- .../modules/net_tools/ldap/ldap_attr.py | 22 +++++++++---------- .../modules/net_tools/ldap/ldap_entry.py | 22 +++++++++---------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/lib/ansible/modules/net_tools/ldap/ldap_attr.py b/lib/ansible/modules/net_tools/ldap/ldap_attr.py index 3c29aaaa10aa54..ee77368d3746b5 100644 --- a/lib/ansible/modules/net_tools/ldap/ldap_attr.py +++ b/lib/ansible/modules/net_tools/ldap/ldap_attr.py @@ -261,17 +261,17 @@ def main(): 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) + # 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']: diff --git a/lib/ansible/modules/net_tools/ldap/ldap_entry.py b/lib/ansible/modules/net_tools/ldap/ldap_entry.py index 0b4c04e90a9afc..db8ad1789db084 100644 --- a/lib/ansible/modules/net_tools/ldap/ldap_entry.py +++ b/lib/ansible/modules/net_tools/ldap/ldap_entry.py @@ -209,17 +209,17 @@ def main(): 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) + # 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']: