stricter permissions on atomic_move when creating new file (#68970)
fixes #67794 updated some tests that expected previous defaults CVE-2020-1736
This commit is contained in:
parent
de6b047fc3
commit
566f2467f6
4 changed files with 13 additions and 11 deletions
2
changelogs/fragments/atomic_move_permissions.yml
Normal file
2
changelogs/fragments/atomic_move_permissions.yml
Normal file
|
@ -0,0 +1,2 @@
|
|||
bugfixes:
|
||||
- stricter permissions when atomic_move creates a file due to target not existing yet CVE-2020-1736
|
|
@ -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):
|
||||
|
|
|
@ -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'"
|
||||
that: "mode_given_yaml_literal_600.stat.mode == '1130'"
|
||||
|
|
|
@ -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')]
|
||||
|
|
Loading…
Reference in a new issue