diff options
author | Brian Coca <bcoca@users.noreply.github.com> | 2020-04-16 12:51:17 -0400 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-04-16 12:51:17 -0400 |
commit | 57336600404cbf2ffe286ec8d35a303be854d140 (patch) | |
tree | 97d8301424f8b708891a3913540b908d6209960d | |
parent | 566f2467f6ca6e0e817ea793ef802116ad469858 (diff) | |
download | ansible-revert-68970-safer_default_perms.tar.gz |
Revert "stricter permissions on atomic_move when creating new file (#68970)"revert-68970-safer_default_perms
This reverts commit 566f2467f6ca6e0e817ea793ef802116ad469858.
-rw-r--r-- | changelogs/fragments/atomic_move_permissions.yml | 2 | ||||
-rw-r--r-- | lib/ansible/module_utils/common/file.py | 2 | ||||
-rw-r--r-- | test/integration/targets/apt_repository/tasks/mode.yaml | 3 | ||||
-rw-r--r-- | test/units/module_utils/basic/test_atomic_move.py | 17 |
4 files changed, 11 insertions, 13 deletions
diff --git a/changelogs/fragments/atomic_move_permissions.yml b/changelogs/fragments/atomic_move_permissions.yml deleted file mode 100644 index 17fe585745..0000000000 --- a/changelogs/fragments/atomic_move_permissions.yml +++ /dev/null @@ -1,2 +0,0 @@ -bugfixes: - - stricter permissions when atomic_move creates a file due to target not existing yet CVE-2020-1736 diff --git a/lib/ansible/module_utils/common/file.py b/lib/ansible/module_utils/common/file.py index 3ca1253e82..9703ea782e 100644 --- a/lib/ansible/module_utils/common/file.py +++ b/lib/ansible/module_utils/common/file.py @@ -59,7 +59,7 @@ PERMS_RE = re.compile(r'[^rwxXstugo]') _PERM_BITS = 0o7777 # file mode permission bits _EXEC_PERM_BITS = 0o0111 # execute permission bits -_DEFAULT_PERM = 0o0660 # default file permission bits +_DEFAULT_PERM = 0o0666 # default file permission bits def is_executable(path): diff --git a/test/integration/targets/apt_repository/tasks/mode.yaml b/test/integration/targets/apt_repository/tasks/mode.yaml index 0de6ca14ae..d9895368a3 100644 --- a/test/integration/targets/apt_repository/tasks/mode.yaml +++ b/test/integration/targets/apt_repository/tasks/mode.yaml @@ -41,7 +41,6 @@ apt_repository: repo: "{{ test_repo_spec }}" state: present - mode: 0644 register: no_mode_results - name: Gather no mode stat @@ -128,4 +127,4 @@ # See https://github.com/ansible/ansible/issues/16370 - name: Assert mode_given_yaml_literal_600 is correct assert: - that: "mode_given_yaml_literal_600.stat.mode == '1130'" + that: "mode_given_yaml_literal_600.stat.mode == '1130'"
\ No newline at end of file diff --git a/test/units/module_utils/basic/test_atomic_move.py b/test/units/module_utils/basic/test_atomic_move.py index 58ad6e5df9..7bd9496edd 100644 --- a/test/units/module_utils/basic/test_atomic_move.py +++ b/test/units/module_utils/basic/test_atomic_move.py @@ -63,7 +63,7 @@ def atomic_mocks(mocker, monkeypatch): @pytest.fixture def fake_stat(mocker): stat1 = mocker.MagicMock() - stat1.st_mode = 0o0640 + stat1.st_mode = 0o0644 stat1.st_uid = 0 stat1.st_gid = 0 stat1.st_flags = 0 @@ -80,8 +80,7 @@ def test_new_file(atomic_am, atomic_mocks, mocker, selinux): atomic_am.atomic_move('/path/to/src', '/path/to/dest') atomic_mocks['rename'].assert_called_with(b'/path/to/src', b'/path/to/dest') - # 416 is what we expect with default perms set to 0640 - assert atomic_mocks['chmod'].call_args_list == [mocker.call(b'/path/to/dest', 416)] + assert atomic_mocks['chmod'].call_args_list == [mocker.call(b'/path/to/dest', basic.DEFAULT_PERM & ~18)] if selinux: assert atomic_am.selinux_default_context.call_args_list == [mocker.call('/path/to/dest')] @@ -102,7 +101,7 @@ def test_existing_file(atomic_am, atomic_mocks, fake_stat, mocker, selinux): atomic_am.atomic_move('/path/to/src', '/path/to/dest') atomic_mocks['rename'].assert_called_with(b'/path/to/src', b'/path/to/dest') - assert atomic_mocks['chmod'].call_args_list == [mocker.call(b'/path/to/src', 416)] + assert atomic_mocks['chmod'].call_args_list == [mocker.call(b'/path/to/src', basic.DEFAULT_PERM & ~18)] if selinux: assert atomic_am.set_context_if_different.call_args_list == [mocker.call('/path/to/dest', mock_context, False)] @@ -125,9 +124,10 @@ def test_no_tty_fallback(atomic_am, atomic_mocks, fake_stat, mocker): atomic_am.atomic_move('/path/to/src', '/path/to/dest') atomic_mocks['rename'].assert_called_with(b'/path/to/src', b'/path/to/dest') + assert atomic_mocks['chmod'].call_args_list == [mocker.call(b'/path/to/src', basic.DEFAULT_PERM & ~18)] + assert atomic_am.set_context_if_different.call_args_list == [mocker.call('/path/to/dest', mock_context, False)] assert atomic_am.selinux_context.call_args_list == [mocker.call('/path/to/dest')] - atomic_am.atomic_move('/path/to/src', '/path/to/dest') @pytest.mark.parametrize('stdin', [{}], indirect=['stdin']) @@ -152,8 +152,9 @@ def test_existing_file_stat_perms_failure(atomic_am, atomic_mocks, mocker): atomic_am.atomic_move('/path/to/src', '/path/to/dest') atomic_mocks['rename'].assert_called_with(b'/path/to/src', b'/path/to/dest') - # atomic_move() will set a default permission value when it cannot retrieve the - # existing file's permissions. + # FIXME: Should atomic_move() set a default permission value when it cannot retrieve the + # existing file's permissions? (Right now it's up to the calling code. + # assert atomic_mocks['chmod'].call_args_list == [mocker.call(b'/path/to/src', basic.DEFAULT_PERM & ~18)] assert atomic_am.set_context_if_different.call_args_list == [mocker.call('/path/to/dest', mock_context, False)] assert atomic_am.selinux_context.call_args_list == [mocker.call('/path/to/dest')] @@ -210,7 +211,7 @@ def test_rename_perms_fail_temp_succeeds(atomic_am, atomic_mocks, fake_stat, moc atomic_am.atomic_move('/path/to/src', '/path/to/dest') assert atomic_mocks['rename'].call_args_list == [mocker.call(b'/path/to/src', b'/path/to/dest'), mocker.call(b'/path/to/tempfile', b'/path/to/dest')] - assert atomic_mocks['chmod'].call_args_list == [mocker.call(b'/path/to/dest', 416)] + assert atomic_mocks['chmod'].call_args_list == [mocker.call(b'/path/to/dest', basic.DEFAULT_PERM & ~18)] if selinux: assert atomic_am.selinux_default_context.call_args_list == [mocker.call('/path/to/dest')] |