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 <sdoran@redhat.com>

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 @@ 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()

test/integration/targets/subversion/aliases

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

test/integration/targets/subversion/meta/main.yml

@@ -1,3 +0,0 @@
-dependencies:
-  - prepare_tests
-  - setup_passlib

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/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:

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

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

test/integration/targets/subversion/tasks/main.yml

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