From 5cb96087e6e8270229d6a801287a01398a9d1bf0 Mon Sep 17 00:00:00 2001 From: Sam Doran Date: Thu, 30 Jul 2020 13:10:23 -0400 Subject: [PATCH] Fix warning for new default permissions when mode is not specified (#70976) (#70985) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../67794-default-permissions-warning-fix.yml | 4 +++ .../rst/porting_guides/porting_guide_2.10.rst | 2 +- lib/ansible/module_utils/basic.py | 6 ++-- .../targets/module_utils_basic/aliases | 1 + .../library/test_perm_warning.py | 36 +++++++++++++++++++ .../targets/module_utils_basic/meta/main.yml | 2 ++ .../targets/module_utils_basic/tasks/main.yml | 33 +++++++++++++++++ 7 files changed, 80 insertions(+), 4 deletions(-) create mode 100644 changelogs/fragments/67794-default-permissions-warning-fix.yml create mode 100644 test/integration/targets/module_utils_basic/aliases create mode 100644 test/integration/targets/module_utils_basic/library/test_perm_warning.py create mode 100644 test/integration/targets/module_utils_basic/meta/main.yml create mode 100644 test/integration/targets/module_utils_basic/tasks/main.yml 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 00000000000..7a69f0e7a25 --- /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 5d150d8bf1b..fbe971a3a92 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 bad1b05721a..28066518b86 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 00000000000..70a7b7a9f32 --- /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 00000000000..7b6eee61ce9 --- /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 00000000000..1810d4bec98 --- /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 00000000000..02761a96f5b --- /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