From 641ff410128a959884b035a2c720c3ba61bf2064 Mon Sep 17 00:00:00 2001 From: Sloane Hertel Date: Mon, 13 Apr 2020 10:21:10 -0400 Subject: [PATCH] subversion module - provide password securely when possible or warn (#67829) * subversion module - provide password securely with svn command line option --password-from-stdin when possible, and provide a warning otherwise. * Update lib/ansible/modules/source_control/subversion.py. * Add a test. Co-authored-by: Sam Doran (cherry picked from commit d91658ec0c8434c82c3ef98bfe9eb4e1027a43a3) --- changelogs/fragments/subversion_password.yaml | 9 ++ .../modules/source_control/subversion.py | 21 ++- test/integration/targets/subversion/aliases | 1 + .../targets/subversion/meta/main.yml | 2 - .../roles/subversion/tasks/cleanup.yml | 8 ++ .../roles/subversion/tasks/main.yml | 20 +++ .../roles/subversion/tasks/setup_selinux.yml | 11 ++ .../roles/subversion/tasks/warnings.yml | 7 + test/integration/targets/subversion/runme.sh | 32 +++++ test/integration/targets/subversion/runme.yml | 15 +++ .../targets/subversion/tasks/main.yml | 123 ------------------ 11 files changed, 121 insertions(+), 128 deletions(-) create mode 100644 changelogs/fragments/subversion_password.yaml delete mode 100644 test/integration/targets/subversion/meta/main.yml create mode 100644 test/integration/targets/subversion/roles/subversion/tasks/cleanup.yml create mode 100644 test/integration/targets/subversion/roles/subversion/tasks/main.yml create mode 100644 test/integration/targets/subversion/roles/subversion/tasks/setup_selinux.yml create mode 100644 test/integration/targets/subversion/roles/subversion/tasks/warnings.yml create mode 100755 test/integration/targets/subversion/runme.sh create mode 100644 test/integration/targets/subversion/runme.yml delete mode 100644 test/integration/targets/subversion/tasks/main.yml diff --git a/changelogs/fragments/subversion_password.yaml b/changelogs/fragments/subversion_password.yaml new file mode 100644 index 0000000..42e09fb --- /dev/null +++ b/changelogs/fragments/subversion_password.yaml @@ -0,0 +1,9 @@ +bugfixes: +- > + **security issue** - The ``subversion`` module provided the password + via the svn command line option ``--password`` and can be retrieved + from the host's /proc//cmdline file. Update the module to use + the secure ``--password-from-stdin`` option instead, and add a warning + in the module and in the documentation if svn version is too old to + support it. + (CVE-2020-1739) diff --git a/lib/ansible/modules/source_control/subversion.py b/lib/ansible/modules/source_control/subversion.py index 3b2547d..e9112cb 100644 --- a/lib/ansible/modules/source_control/subversion.py +++ b/lib/ansible/modules/source_control/subversion.py @@ -49,7 +49,9 @@ options: - C(--username) parameter passed to svn. password: description: - - C(--password) parameter passed to svn. + - C(--password) parameter passed to svn when svn is less than version 1.10.0. This is not secure and + the password will be leaked to argv. + - C(--password-from-stdin) parameter when svn is greater or equal to version 1.10.0. executable: description: - Path to svn executable to use. If not supplied, @@ -103,6 +105,8 @@ EXAMPLES = ''' import os import re +from distutils.version import LooseVersion + from ansible.module_utils.basic import AnsibleModule @@ -116,6 +120,10 @@ class Subversion(object): self.password = password self.svn_path = svn_path + def has_option_password_from_stdin(self): + rc, version, err = self.module.run_command([self.svn_path, '--version', '--quiet'], check_rc=True) + return LooseVersion(version) >= LooseVersion('1.10.0') + def _exec(self, args, check_rc=True): '''Execute a subversion command, and return output. If check_rc is False, returns the return code instead of the output.''' bits = [ @@ -124,12 +132,19 @@ class Subversion(object): '--trust-server-cert', '--no-auth-cache', ] + stdin_data = None if self.username: bits.extend(["--username", self.username]) if self.password: - bits.extend(["--password", self.password]) + if self.has_option_password_from_stdin(): + bits.append("--password-from-stdin") + stdin_data = self.password + else: + self.module.warn("The authentication provided will be used on the svn command line and is not secure. " + "To securely pass credentials, upgrade svn to version 1.10.0 or greater.") + bits.extend(["--password", self.password]) bits.extend(args) - rc, out, err = self.module.run_command(bits, check_rc) + rc, out, err = self.module.run_command(bits, check_rc, data=stdin_data) if check_rc: return out.splitlines() else: diff --git a/test/integration/targets/subversion/aliases b/test/integration/targets/subversion/aliases index c364b48..8be436d 100644 --- a/test/integration/targets/subversion/aliases +++ b/test/integration/targets/subversion/aliases @@ -1,2 +1,3 @@ +setup/always/setup_passlib posix/ci/group2 skip/osx diff --git a/test/integration/targets/subversion/meta/main.yml b/test/integration/targets/subversion/meta/main.yml deleted file mode 100644 index 07faa21..0000000 --- a/test/integration/targets/subversion/meta/main.yml +++ /dev/null @@ -1,2 +0,0 @@ -dependencies: - - prepare_tests diff --git a/test/integration/targets/subversion/roles/subversion/tasks/cleanup.yml b/test/integration/targets/subversion/roles/subversion/tasks/cleanup.yml new file mode 100644 index 0000000..9be43b4 --- /dev/null +++ b/test/integration/targets/subversion/roles/subversion/tasks/cleanup.yml @@ -0,0 +1,8 @@ +--- +- name: stop apache after tests + shell: "kill -9 $(cat '{{ subversion_server_dir }}/apache.pid')" + +- name: remove tmp subversion server dir + file: + path: '{{ subversion_server_dir }}' + state: absent diff --git a/test/integration/targets/subversion/roles/subversion/tasks/main.yml b/test/integration/targets/subversion/roles/subversion/tasks/main.yml new file mode 100644 index 0000000..0d6acb8 --- /dev/null +++ b/test/integration/targets/subversion/roles/subversion/tasks/main.yml @@ -0,0 +1,20 @@ +--- +- name: setup subversion server + import_tasks: setup.yml + tags: setup + +- name: verify that subversion is installed so this test can continue + shell: which svn + tags: always + +- name: run tests + import_tasks: tests.yml + tags: tests + +- name: run warning + import_tasks: warnings.yml + tags: warnings + +- name: clean up + import_tasks: cleanup.yml + tags: cleanup diff --git a/test/integration/targets/subversion/roles/subversion/tasks/setup_selinux.yml b/test/integration/targets/subversion/roles/subversion/tasks/setup_selinux.yml new file mode 100644 index 0000000..a9ffa71 --- /dev/null +++ b/test/integration/targets/subversion/roles/subversion/tasks/setup_selinux.yml @@ -0,0 +1,11 @@ +- name: set SELinux security context for SVN folder + sefcontext: + target: '{{ subversion_server_dir }}(/.*)?' + setype: '{{ item }}' + state: present + with_items: + - httpd_sys_content_t + - httpd_sys_rw_content_t + +- name: apply new SELinux context to filesystem + command: restorecon -irv {{ subversion_server_dir | quote }} diff --git a/test/integration/targets/subversion/roles/subversion/tasks/warnings.yml b/test/integration/targets/subversion/roles/subversion/tasks/warnings.yml new file mode 100644 index 0000000..50ebd44 --- /dev/null +++ b/test/integration/targets/subversion/roles/subversion/tasks/warnings.yml @@ -0,0 +1,7 @@ +--- +- name: checkout using a password to test for a warning when using svn lt 1.10.0 + subversion: + repo: '{{ subversion_repo_auth_url }}' + dest: '{{ subversion_test_dir }}/svn' + username: '{{ subversion_username }}' + password: '{{ subversion_password }}' diff --git a/test/integration/targets/subversion/runme.sh b/test/integration/targets/subversion/runme.sh new file mode 100755 index 0000000..f505e58 --- /dev/null +++ b/test/integration/targets/subversion/runme.sh @@ -0,0 +1,32 @@ +#!/usr/bin/env bash + +set -eu + +cleanup() { + echo "Cleanup" + ansible-playbook runme.yml -e "output_dir=${OUTPUT_DIR}" "$@" --tags cleanup + echo "Done" +} + +trap cleanup INT TERM EXIT + +export ANSIBLE_ROLES_PATH=roles/ + +# Ensure subversion is set up +ansible-playbook runme.yml "$@" -v --tags setup + +# Test functionality +ansible-playbook runme.yml "$@" -v --tags tests + +# Test a warning is displayed for versions < 1.10.0 when a password is provided +ansible-playbook runme.yml "$@" --tags warnings 2>&1 | tee out.txt + +version="$(svn --version -q)" +secure=$(python -c "from distutils.version import LooseVersion; print(LooseVersion('$version') >= LooseVersion('1.10.0'))") + +if [[ "${secure}" = "False" ]] && [[ "$(grep -c 'To securely pass credentials, upgrade svn to version 1.10.0' out.txt)" -eq 1 ]]; then + echo "Found the expected warning" +elif [[ "${secure}" = "False" ]]; then + echo "Expected a warning" + exit 1 +fi diff --git a/test/integration/targets/subversion/runme.yml b/test/integration/targets/subversion/runme.yml new file mode 100644 index 0000000..c67d7b8 --- /dev/null +++ b/test/integration/targets/subversion/runme.yml @@ -0,0 +1,15 @@ +--- +- hosts: localhost + tasks: + - name: load OS specific vars + include_vars: '{{ item }}' + with_first_found: + - files: + - '{{ ansible_distribution }}-{{ ansible_distribution_major_version }}.yml' + - '{{ ansible_os_family }}.yml' + paths: '../vars' + tags: always + + - include_role: + name: subversion + tags: always diff --git a/test/integration/targets/subversion/tasks/main.yml b/test/integration/targets/subversion/tasks/main.yml deleted file mode 100644 index 5631adb..0000000 --- a/test/integration/targets/subversion/tasks/main.yml +++ /dev/null @@ -1,123 +0,0 @@ -# test code for the svn module -# (c) 2014, Michael DeHaan - -# This file is part of Ansible -# -# Ansible is free software: you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# Ansible is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with Ansible. If not, see . - -- name: set where to extract the repo - set_fact: checkout_dir={{ output_dir }}/svn - -- name: set what repo to use - set_fact: repo=https://github.com/jimi-c/test_role - -- name: clean out the output_dir - shell: rm -rf {{ output_dir }}/* - -- name: install subversion - package: - name: subversion - when: ansible_distribution != "MacOSX" - -- name: verify that subversion is installed so this test can continue - shell: which svn - -# checks out every branch so using a small repo - -- name: initial checkout - subversion: repo={{ repo }} dest={{ checkout_dir }} - register: subverted - -- debug: var=subverted - -- shell: ls {{ checkout_dir }} - -# FIXME: the before/after logic here should be fixed to make them hashes, see GitHub 6078 -# looks like this: { -# "after": [ -# "Revision: 9", -# "URL: https://github.com/jimi-c/test_role" -# ], -# "before": null, -# "changed": true, -# "item": "" -# } - -- name: verify information about the initial clone - assert: - that: - - "'after' in subverted" - - "subverted.after.1 == 'URL: https://github.com/jimi-c/test_role'" - - "not subverted.before" - - "subverted.changed" - -- name: repeated checkout - subversion: repo={{ repo }} dest={{ checkout_dir }} - register: subverted2 - -- name: verify on a reclone things are marked unchanged - assert: - that: - - "not subverted2.changed" - -- name: check for tags - stat: path={{ checkout_dir }}/tags - register: tags - -- name: check for trunk - stat: path={{ checkout_dir }}/trunk - register: trunk - -- name: check for branches - stat: path={{ checkout_dir }}/branches - register: branches - -- name: assert presence of tags/trunk/branches - assert: - that: - - "tags.stat.isdir" - - "trunk.stat.isdir" - - "branches.stat.isdir" - -- name: checkout with quotes in username - subversion: repo={{ repo }} dest={{ checkout_dir }} username="quoteme'''" - register: subverted3 - -- debug: var=subverted3 - -- name: checkout with export - subversion: repo={{ repo }} dest={{ output_dir }}/svn-export export=True - register: subverted4 - -- name: check for tags - stat: path={{ output_dir }}/svn-export/tags - register: export_tags - -- name: check for trunk - stat: path={{ output_dir }}/svn-export/trunk - register: export_trunk - -- name: check for branches - stat: path={{ output_dir }}/svn-export/branches - register: export_branches - -- name: assert presence of tags/trunk/branches in export - assert: - that: - - "export_tags.stat.isdir" - - "export_trunk.stat.isdir" - - "export_branches.stat.isdir" - - "subverted4.changed" - -# TBA: test for additional options or URL variants welcome -- 2.27.0