stricter permissions on atomic_move when creating new file (#68970)

fixes #67794
  updated some tests that expected previous defaults
  CVE-2020-1736

4 files changed, 13 insertions, 11 deletions

diff --git a/changelogs/fragments/atomic_move_permissions.yml b/changelogs/fragments/atomic_move_permissions.yml
new file mode 100644
index 0000000000..17fe585745
--- /dev/null
+++ b/changelogs/fragments/atomic_move_permissions.yml
@@ -0,0 +1,2 @@
+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 9703ea782e..3ca1253e82 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 = 0o0666       # default file permission bits
+_DEFAULT_PERM = 0o0660       # 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 d9895368a3..0de6ca14ae 100644
--- a/test/integration/targets/apt_repository/tasks/mode.yaml
+++ b/test/integration/targets/apt_repository/tasks/mode.yaml
@@ -41,6 +41,7 @@
   apt_repository:
     repo: "{{ test_repo_spec }}"
     state: present
+    mode: 0644
   register: no_mode_results
 
 - name: Gather no mode stat
@@ -127,4 +128,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'"
\ No newline at end of file
+    that: "mode_given_yaml_literal_600.stat.mode == '1130'"
diff --git a/test/units/module_utils/basic/test_atomic_move.py b/test/units/module_utils/basic/test_atomic_move.py
index 7bd9496edd..58ad6e5df9 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 = 0o0644
+    stat1.st_mode = 0o0640
     stat1.st_uid = 0
     stat1.st_gid = 0
     stat1.st_flags = 0
@@ -80,7 +80,8 @@ 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')
-    assert atomic_mocks['chmod'].call_args_list == [mocker.call(b'/path/to/dest', basic.DEFAULT_PERM & ~18)]
+    # 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)]
 
     if selinux:
         assert atomic_am.selinux_default_context.call_args_list == [mocker.call('/path/to/dest')]
@@ -101,7 +102,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', basic.DEFAULT_PERM & ~18)]
+    assert atomic_mocks['chmod'].call_args_list == [mocker.call(b'/path/to/src', 416)]
 
     if selinux:
         assert atomic_am.set_context_if_different.call_args_list == [mocker.call('/path/to/dest', mock_context, False)]
@@ -124,10 +125,9 @@ 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,9 +152,8 @@ 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')
-    # 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)]
+    # atomic_move() will set a default permission value when it cannot retrieve the
+    # existing file's permissions.
     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')]
 
@@ -211,7 +210,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', basic.DEFAULT_PERM & ~18)]
+    assert atomic_mocks['chmod'].call_args_list == [mocker.call(b'/path/to/dest', 416)]
 
     if selinux:
         assert atomic_am.selinux_default_context.call_args_list == [mocker.call('/path/to/dest')]