Skip to content

CVE-2020-1746 - Remove the params module option from ldap_attr and ldap_etnry #67866

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

abadger
Copy link
Contributor

@abadger abadger commented Feb 28, 2020

Fix for CVE-2020-1746

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.

ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
COMPONENT NAME
  • lib/ansible/modules/net_tools/ldap/ldap_entry.py
  • lib/ansible/modules/net_tools/ldap/_ldap_attr.py

@abadger
Copy link
Contributor Author

abadger commented Feb 28, 2020

/cc @acozine @samccann

@ansibot
Copy link
Contributor

ansibot commented Feb 28, 2020

cc @jtyr
click here for bot help

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. deprecated This issue/PR relates to a deprecated module. docs This issue/PR relates to or includes documentation. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. net_tools Net-tools category support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Feb 28, 2020
@samdoran samdoran added the security Related to a vulnerability or CVE label Feb 28, 2020
@ansibot
Copy link
Contributor

ansibot commented Feb 28, 2020

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

lib/ansible/modules/net_tools/ldap/_ldap_attr.py:0:0: parameter-type-not-in-doc: Argument 'params' in argument_spec defines type as 'dict' but documentation doesn't define type
lib/ansible/modules/net_tools/ldap/_ldap_attr.py:0:0: undocumented-parameter: Argument 'params' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/net_tools/ldap/ldap_entry.py:0:0: undocumented-parameter: Argument 'params' is listed in the argument_spec, but not documented in the module documentation

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. and removed needs_triage Needs a first human triage before being processed. labels Feb 28, 2020
@ansibot
Copy link
Contributor

ansibot commented Feb 28, 2020

@ansibot ansibot added test This PR relates to tests. and removed ci_verified Changes made in this PR are causing tests to fail. labels Feb 28, 2020
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the shorter cycle from a community point of view

# 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'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this so the same code can be backported to previous Ansible releases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. we removed params from yum_repository and jenkins_plugin on a short cycle before but didn't backport the changes. Since this is a security issue, we should backport it. I tried to make the change do less for 2.9 and earlier (only disallowing bind_pw usage).

@ansibot
Copy link
Contributor

ansibot commented Feb 28, 2020

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/net_tools/ldap/_ldap_attr.py:0:0: parameter-type-not-in-doc: Argument 'params' in argument_spec defines type as 'dict' but documentation doesn't define type

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Feb 28, 2020
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Feb 28, 2020
Copy link
Contributor

@samccann samccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just some docs nits. I changed 'via' to 'with' but you could use 'through' instead if you think that reads better. We're trying to avoid Latin as it causes translation problems.

@abadger abadger changed the title [WIP] Remove the params module option from ldap_attr and ldap_etnry CVE-2020-1746 - Remove the params module option from ldap_attr and ldap_etnry Feb 29, 2020
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Feb 29, 2020
@abadger
Copy link
Contributor Author

abadger commented Feb 29, 2020

@felixfontein (Maybe @drybjed ) Does this look okay to you?

@drybjed
Copy link
Contributor

drybjed commented Feb 29, 2020

@abadger Looks good to me. 👍

Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@felixfontein
Copy link
Contributor

I also tested that ldap_entry still works (when not using params). We don't use ldap_attr anymore (we switched to ldap_attrs some time ago) so I can't easily test that one.

Copy link
Contributor

@samccann samccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs portion LGTM

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
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 11, 2020
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Mar 27, 2020
# 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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fail_json requires a msg keyword argument. Needs to be updated for the other places fail_json was added too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh.... I'll fix that here and also open an issue to change that behaviour in fail_json. It's a mandatory parameter so there's no reason it shouldn't be accessible as a positional arg too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix pushed

@s-hertel
Copy link
Contributor

s-hertel commented Apr 6, 2020

This was updated for community.general in ansible-collections/community.general#113. The three backports are linked above.

@abadger If you have a chance to review those I'd appreciate it.

@s-hertel s-hertel closed this Apr 6, 2020
@ansible ansible locked and limited conversation to collaborators May 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. deprecated This issue/PR relates to a deprecated module. docs This issue/PR relates to or includes documentation. module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. net_tools Net-tools category security Related to a vulnerability or CVE support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants