diff --git a/changelogs/fragments/subversion_password.yaml b/changelogs/fragments/subversion_password.yaml new file mode 100644 index 00000000000..42e09fb1a07 --- /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/<pid>/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 c7625f62026..1e60529a062 100644 --- a/lib/ansible/modules/source_control/subversion.py +++ b/lib/ansible/modules/source_control/subversion.py @@ -56,7 +56,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, @@ -111,6 +113,8 @@ EXAMPLES = ''' import os import re +from distutils.version import LooseVersion + from ansible.module_utils.basic import AnsibleModule @@ -124,6 +128,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 = [ @@ -132,12 +140,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() diff --git a/test/integration/targets/subversion/aliases b/test/integration/targets/subversion/aliases index 270f301a721..d3e8e8020c5 100644 --- a/test/integration/targets/subversion/aliases +++ b/test/integration/targets/subversion/aliases @@ -1,3 +1,4 @@ +setup/always/setup_passlib shippable/posix/group2 skip/aix 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 c7b09a44c2e..00000000000 --- a/test/integration/targets/subversion/meta/main.yml +++ /dev/null @@ -1,3 +0,0 @@ -dependencies: - - prepare_tests - - setup_passlib diff --git a/test/integration/targets/subversion/defaults/main.yml b/test/integration/targets/subversion/roles/subversion/defaults/main.yml similarity index 91% rename from test/integration/targets/subversion/defaults/main.yml rename to test/integration/targets/subversion/roles/subversion/defaults/main.yml index 86500a31e49..af5ea0263ca 100644 --- a/test/integration/targets/subversion/defaults/main.yml +++ b/test/integration/targets/subversion/roles/subversion/defaults/main.yml @@ -1,5 +1,6 @@ --- apache_port: 11386 # cannot use 80 as httptester overrides this +output_dir: "{{ lookup('env', 'OUTPUT_DIR') }}" subversion_test_dir: '{{ output_dir }}/svn-test' subversion_server_dir: /tmp/ansible-svn # cannot use a path in the home dir without userdir or granting exec permission to the apache user subversion_repo_name: ansible-test-repo diff --git a/test/integration/targets/subversion/files/create_repo.sh b/test/integration/targets/subversion/roles/subversion/files/create_repo.sh similarity index 100% rename from test/integration/targets/subversion/files/create_repo.sh rename to test/integration/targets/subversion/roles/subversion/files/create_repo.sh 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 00000000000..9be43b4c256 --- /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 00000000000..0d6acb8a571 --- /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/tasks/setup.yml b/test/integration/targets/subversion/roles/subversion/tasks/setup.yml similarity index 87% rename from test/integration/targets/subversion/tasks/setup.yml rename to test/integration/targets/subversion/roles/subversion/tasks/setup.yml index 63d33d68b0d..5c9c5cb5410 100644 --- a/test/integration/targets/subversion/tasks/setup.yml +++ b/test/integration/targets/subversion/roles/subversion/tasks/setup.yml @@ -1,11 +1,11 @@ --- -- 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' +- name: clean out the checkout dir + file: + path: '{{ subversion_test_dir }}' + state: '{{ item }}' + loop: + - absent + - directory - name: install SVN pre-reqs package: diff --git a/test/integration/targets/subversion/tasks/setup_selinux.yml b/test/integration/targets/subversion/roles/subversion/tasks/setup_selinux.yml similarity index 100% rename from test/integration/targets/subversion/tasks/setup_selinux.yml rename to test/integration/targets/subversion/roles/subversion/tasks/setup_selinux.yml diff --git a/test/integration/targets/subversion/tasks/tests.yml b/test/integration/targets/subversion/roles/subversion/tasks/tests.yml similarity index 100% rename from test/integration/targets/subversion/tasks/tests.yml rename to test/integration/targets/subversion/roles/subversion/tasks/tests.yml 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 00000000000..50ebd441e73 --- /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/templates/subversion.conf.j2 b/test/integration/targets/subversion/roles/subversion/templates/subversion.conf.j2 similarity index 100% rename from test/integration/targets/subversion/templates/subversion.conf.j2 rename to test/integration/targets/subversion/roles/subversion/templates/subversion.conf.j2 diff --git a/test/integration/targets/subversion/runme.sh b/test/integration/targets/subversion/runme.sh new file mode 100755 index 00000000000..f505e58168c --- /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 00000000000..c67d7b89b1b --- /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 3ce734a7a36..00000000000 --- a/test/integration/targets/subversion/tasks/main.yml +++ /dev/null @@ -1,27 +0,0 @@ ---- -- name: clean out the checkout dir - file: - path: '{{ subversion_test_dir }}' - state: '{{ item }}' - loop: - - absent - - directory - -- name: setup subversion server - include_tasks: setup.yml - -- block: - - name: verify that subversion is installed so this test can continue - shell: which svn - - - name: run tests - include_tasks: tests.yml - - always: - - 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