Fix warning for new default permissions when mode is not specified (#70976) (#70985)

Follow up to #70221
Related to #67794
CVE-2020-1736

When set_mode_if_different() is called with mode of 'None', ensure we issue
a warning about the change in default permissions.

Add integration tests to ensure the warning works properly.

* Fix tests
- actually use custom module 🤦‍♂️
- verify file permission on created files
- use remote_tmp_dir so we're ready for split controller
- improve test module so we can skip the call to set_fs_attributes_if_different()
- fix tests for CentOS 6

(cherry picked from commit dc79528cc6609ccef41a4e9708973b992851ab31)

7 files changed, 80 insertions, 4 deletions

diff --git a/changelogs/fragments/67794-default-permissions-warning-fix.yml b/changelogs/fragments/67794-default-permissions-warning-fix.yml
new file mode 100644
index 0000000000..7a69f0e7a2
--- /dev/null
+++ b/changelogs/fragments/67794-default-permissions-warning-fix.yml
@@ -0,0 +1,4 @@
+bugfixes:
+  - >
+    Fix warning for default permission change when no mode is specified. Follow up
+    to https://github.com/ansible/ansible/issues/67794. (CVE-2020-1736)
diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.10.rst b/docs/docsite/rst/porting_guides/porting_guide_2.10.rst
index 5d150d8bf1..fbe971a3a9 100644
--- a/docs/docsite/rst/porting_guides/porting_guide_2.10.rst
+++ b/docs/docsite/rst/porting_guides/porting_guide_2.10.rst
@@ -58,7 +58,7 @@ A new warning will be displayed when all of the following conditions are true:
 
     - The file at the final destination, not the temporary file, does not exist
     - A module supports setting ``mode`` but it was not specified for the task
-    - The module calls ``atomic_move()`` but does not later call ``set_fs_attributes_if_different()`` or ``set_mode_if_different()``
+    - The module calls ``atomic_move()`` but does not later call ``set_fs_attributes_if_different()`` or ``set_mode_if_different()`` with a ``mode`` specified
 
 The following modules call ``atomic_move()`` but do not call ``set_fs_attributes_if_different()``  or ``set_mode_if_different()`` and do not support setting ``mode``. This means for files they create, the default permissions have changed and there is no indication:
 
diff --git a/lib/ansible/module_utils/basic.py b/lib/ansible/module_utils/basic.py
index bad1b05721..28066518b8 100644
--- a/lib/ansible/module_utils/basic.py
+++ b/lib/ansible/module_utils/basic.py
@@ -1125,6 +1125,9 @@ class AnsibleModule(object):
 
     def set_mode_if_different(self, path, mode, changed, diff=None, expand=True):
 
+        if mode is None:
+            return changed
+
         # Remove paths so we do not warn about creating with default permissions
         # since we are calling this method on the path and setting the specified mode.
         try:
@@ -1132,9 +1135,6 @@ class AnsibleModule(object):
         except KeyError:
             pass
 
-        if mode is None:
-            return changed
-
         b_path = to_bytes(path, errors='surrogate_or_strict')
         if expand:
             b_path = os.path.expanduser(os.path.expandvars(b_path))
diff --git a/test/integration/targets/module_utils_basic/aliases b/test/integration/targets/module_utils_basic/aliases
new file mode 100644
index 0000000000..70a7b7a9f3
--- /dev/null
+++ b/test/integration/targets/module_utils_basic/aliases
@@ -0,0 +1 @@
+shippable/posix/group5
diff --git a/test/integration/targets/module_utils_basic/library/test_perm_warning.py b/test/integration/targets/module_utils_basic/library/test_perm_warning.py
new file mode 100644
index 0000000000..7b6eee61ce
--- /dev/null
+++ b/test/integration/targets/module_utils_basic/library/test_perm_warning.py
@@ -0,0 +1,36 @@
+#!/usr/bin/python
+# -*- coding: utf-8 -*-
+# Copyright (c) 2020 Ansible Project
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import absolute_import, division, print_function
+__metaclass__ = type
+
+import tempfile
+
+from ansible.module_utils.basic import AnsibleModule
+
+
+def main():
+    module = AnsibleModule(
+        argument_spec={
+            'dest': {'type': 'path'},
+            'call_fs_attributes': {'type': 'bool', 'default': True},
+        },
+        add_file_common_args=True,
+    )
+
+    results = {}
+
+    with tempfile.NamedTemporaryFile(delete=False) as tf:
+        file_args = module.load_file_common_arguments(module.params)
+        module.atomic_move(tf.name, module.params['dest'])
+
+        if module.params['call_fs_attributes']:
+            results['changed'] = module.set_fs_attributes_if_different(file_args, True)
+
+    module.exit_json(**results)
+
+
+if __name__ == '__main__':
+    main()
diff --git a/test/integration/targets/module_utils_basic/meta/main.yml b/test/integration/targets/module_utils_basic/meta/main.yml
new file mode 100644
index 0000000000..1810d4bec9
--- /dev/null
+++ b/test/integration/targets/module_utils_basic/meta/main.yml
@@ -0,0 +1,2 @@
+dependencies:
+  - setup_remote_tmp_dir
diff --git a/test/integration/targets/module_utils_basic/tasks/main.yml b/test/integration/targets/module_utils_basic/tasks/main.yml
new file mode 100644
index 0000000000..02761a96f5
--- /dev/null
+++ b/test/integration/targets/module_utils_basic/tasks/main.yml
@@ -0,0 +1,33 @@
+- name: Run task with no mode
+  test_perm_warning:
+    dest: "{{ remote_tmp_dir }}/endangerdisown"
+  register: no_mode_results
+
+- name: Run task with mode
+  test_perm_warning:
+    mode: '0644'
+    dest: "{{ remote_tmp_dir }}/groveestablish"
+  register: with_mode_results
+
+- name: Run task without calling set_fs_attributes_if_different()
+  test_perm_warning:
+    call_fs_attributes: no
+    dest: "{{ remote_tmp_dir }}/referabletank"
+  register: skip_fs_attributes
+
+- stat:
+    path: "{{ remote_tmp_dir }}/{{ item }}"
+  loop:
+    - endangerdisown
+    - groveestablish
+  register: files
+
+- name: Ensure we get a warning when appropriate
+  assert:
+    that:
+      - no_mode_results.warnings | default([], True) | length == 1
+      - "'created with default permissions' in no_mode_results.warnings[0]"
+      - files.results[0]['stat']['mode'] == '0600'
+      - files.results[1]['stat']['mode'] == '0644'
+      - with_mode_results.warnings is not defined  # The Jinja version on CentOS 6 does not support default([], True)
+      - skip_fs_attributes.warnings | default([], True) | length == 1