[2.7] CVE-2020-1739 - provide password securely for subversion module or warn (#68913)

* 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>
(cherry picked from commit d91658ec0c8434c82c3ef98bfe9eb4e1027a43a3)

* Create the OUTPUT_DIR and make sure it is removed at the end

* fix sanity test

16 files changed, 137 insertions, 52 deletions

diff --git a/changelogs/fragments/subversion_password.yaml b/changelogs/fragments/subversion_password.yaml
new file mode 100644
index 0000000000..42e09fb1a0
--- /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 2cc7ff7bd6..75c1313eb9 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,
@@ -110,6 +112,8 @@ EXAMPLES = '''
 import os
 import re
 
+from distutils.version import LooseVersion
+
 from ansible.module_utils.basic import AnsibleModule
 
 
@@ -123,6 +127,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 = [
@@ -131,12 +139,20 @@ 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 a021224b73..264de18e86 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/osx
 destructive
diff --git a/test/integration/targets/subversion/meta/main.yml b/test/integration/targets/subversion/meta/main.yml
deleted file mode 100644
index c7b09a44c2..0000000000
--- 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
index 86500a31e4..af5ea0263c 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
index cc7f40741a..cc7f40741a 100644
--- a/test/integration/targets/subversion/files/create_repo.sh
+++ b/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 0000000000..9be43b4c25
--- /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 0000000000..0d6acb8a57
--- /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
index 1ec5b5fc24..5c9c5cb541 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:
@@ -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
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 0000000000..a9ffa712f9
--- /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/tasks/tests.yml b/test/integration/targets/subversion/roles/subversion/tasks/tests.yml
index 8421f9deca..8421f9deca 100644
--- a/test/integration/targets/subversion/tasks/tests.yml
+++ b/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 0000000000..50ebd441e7
--- /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
index 07e7083a6b..07e7083a6b 100644
--- a/test/integration/targets/subversion/templates/subversion.conf.j2
+++ b/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 0000000000..323863155c
--- /dev/null
+++ b/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
diff --git a/test/integration/targets/subversion/runme.yml b/test/integration/targets/subversion/runme.yml
new file mode 100644
index 0000000000..c67d7b89b1
--- /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 3ce734a7a3..0000000000
--- 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