[2.7] CVE-2020-1739 - provide password securely for subversion module or warn

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)

lib/ansible/modules/source_control/subversion.py

@@ -56,7 +56,9 @@
       - 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,
@@ -110,6 +112,8 @@
 import os
 import re
 
+from distutils.version import LooseVersion
+
 from ansible.module_utils.basic import AnsibleModule
 
 
@@ -123,6 +127,10 @@ def __init__(self, module, dest, repo, revision, username, password, svn_path):
         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 = [
@@ -131,12 +139,20 @@ def _exec(self, args, check_rc=True):
             '--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:

test/integration/targets/subversion/aliases

@@ -1,3 +1,4 @@
+setup/always/setup_passlib
 shippable/posix/group2
 skip/osx
 destructive

test/integration/targets/subversion/defaults/main.yml → 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

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

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

test/integration/targets/subversion/tasks/setup.yml → 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:
@@ -24,18 +24,8 @@
     path: '{{ subversion_server_dir }}'
     state: directory
 
-- name: set SELinux security context for SVN folder
-  sefcontext:
-    target: '{{ subversion_server_dir }}(/.*)?'
-    setype: '{{ item }}'
-    state: present
-  when: ansible_selinux.status == "enabled"
-  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 }}
+- name: setup selinux when enabled
+  include_tasks: setup_selinux.yml
   when: ansible_selinux.status == "enabled"
 
 - name: template out configuration file

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 }}

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 }}'

test/integration/targets/subversion/runme.sh

@@ -0,0 +1,37 @@
+#!/usr/bin/env bash
+
+set -eu
+
+OUTPUT_DIR=$(mktemp -d)
+
+cleanup() {
+    set +e  # Ensure cleanup completes
+    echo "Cleanup"
+    ansible-playbook runme.yml -e "output_dir=${OUTPUT_DIR}" "$@" --tags cleanup
+    echo "Removing the temporary test output directory"
+    rm -rf "${OUTPUT_DIR}"
+    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

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