From 937dbac4d04c5d4af13a176ebe9888a3054c8530 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Thu, 10 Jun 2021 06:50:00 -0400 Subject: [PATCH] island: Remove SSL permissions checks These checks prevent the docker container from working properly, as the default SSL cert must have at least 444 permissions. --- monkey/common/utils/exceptions.py | 4 - .../cc/server_utils/file_utils.py | 52 ----------- .../setup/island_config_options_validator.py | 21 ----- .../cc/server_utils/test_file_utils.py | 35 -------- .../test_island_config_options_validator.py | 86 ++----------------- 5 files changed, 6 insertions(+), 192 deletions(-) diff --git a/monkey/common/utils/exceptions.py b/monkey/common/utils/exceptions.py index 632c08991..8396b423b 100644 --- a/monkey/common/utils/exceptions.py +++ b/monkey/common/utils/exceptions.py @@ -52,7 +52,3 @@ class FindingWithoutDetailsError(Exception): class DomainControllerNameFetchError(FailedExploitationError): """ Raise on failed attempt to extract domain controller's name """ - - -class InsecurePermissionsError(Exception): - """ Raise when a file does not have permissions that are secure enough """ diff --git a/monkey/monkey_island/cc/server_utils/file_utils.py b/monkey/monkey_island/cc/server_utils/file_utils.py index 668aa4356..225fb8732 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -1,57 +1,5 @@ import os -from monkey_island.cc.environment.utils import is_windows_os - def expand_path(path: str) -> str: return os.path.expandvars(os.path.expanduser(path)) - - -def has_expected_permissions(path: str, expected_permissions: int) -> bool: - if is_windows_os(): - return _has_expected_windows_permissions(path, expected_permissions) - - return _has_expected_linux_permissions(path, expected_permissions) - - -def _has_expected_linux_permissions(path: str, expected_permissions: int) -> bool: - file_mode = os.stat(path).st_mode - file_permissions = file_mode & 0o777 - - return file_permissions == expected_permissions - - -def _has_expected_windows_permissions(path: str, expected_permissions: int) -> bool: - import win32api # noqa: E402 - import win32security # noqa: E402 - - FULL_CONTROL = 2032127 - ACE_TYPE_ALLOW = 0 - ACE_TYPE_DENY = 1 - - admins_sid, _, _ = win32security.LookupAccountName("", "Administrators") - user_sid, _, _ = win32security.LookupAccountName("", win32api.GetUserName()) - - security_descriptor = win32security.GetNamedSecurityInfo( - path, win32security.SE_FILE_OBJECT, win32security.DACL_SECURITY_INFORMATION - ) - - acl = security_descriptor.GetSecurityDescriptorDacl() - - for i in range(acl.GetAceCount()): - ace = acl.GetAce(i) - ace_type, _ = ace[0] # 0 for allow, 1 for deny - permissions = ace[1] - sid = ace[-1] - - if sid == user_sid: - if not (permissions == expected_permissions and ace_type == ACE_TYPE_ALLOW): - return False - elif sid == admins_sid: - continue - # TODO: consider removing; so many system accounts/groups exist, it's likely to fail - else: - if not (permissions == FULL_CONTROL and ace_type == ACE_TYPE_DENY): - return False - - return True diff --git a/monkey/monkey_island/cc/setup/island_config_options_validator.py b/monkey/monkey_island/cc/setup/island_config_options_validator.py index 5febe16ab..032eeb2e2 100644 --- a/monkey/monkey_island/cc/setup/island_config_options_validator.py +++ b/monkey/monkey_island/cc/setup/island_config_options_validator.py @@ -1,34 +1,13 @@ import os -from common.utils.exceptions import InsecurePermissionsError -from monkey_island.cc.environment.utils import is_windows_os -from monkey_island.cc.server_utils.file_utils import has_expected_permissions from monkey_island.cc.setup.island_config_options import IslandConfigOptions def raise_on_invalid_options(options: IslandConfigOptions): - LINUX_READ_ONLY_BY_USER = 0o400 - WINDOWS_READ_ONLY = 1179817 - _raise_if_not_isfile(options.crt_path) - _raise_if_incorrect_permissions(options.crt_path, LINUX_READ_ONLY_BY_USER, WINDOWS_READ_ONLY) - _raise_if_not_isfile(options.key_path) - _raise_if_incorrect_permissions(options.key_path, LINUX_READ_ONLY_BY_USER, WINDOWS_READ_ONLY) def _raise_if_not_isfile(f: str): if not os.path.isfile(f): raise FileNotFoundError(f"{f} does not exist or is not a regular file.") - - -def _raise_if_incorrect_permissions( - f: str, linux_expected_permissions: int, windows_expected_permissions: int -): - expected_permissions = ( - windows_expected_permissions if is_windows_os() else linux_expected_permissions - ) - if not has_expected_permissions(f, expected_permissions): - raise InsecurePermissionsError( - f"The file {f} has incorrect permissions. Expected: {expected_permissions}" - ) diff --git a/monkey/tests/unit_tests/monkey_island/cc/server_utils/test_file_utils.py b/monkey/tests/unit_tests/monkey_island/cc/server_utils/test_file_utils.py index 6297aada9..cff716135 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/server_utils/test_file_utils.py +++ b/monkey/tests/unit_tests/monkey_island/cc/server_utils/test_file_utils.py @@ -1,7 +1,4 @@ import os -import subprocess - -import pytest from monkey_island.cc.server_utils import file_utils @@ -18,35 +15,3 @@ def test_expand_vars(patched_home_env): expected_path = os.path.join(patched_home_env, "test") assert file_utils.expand_path(input_path) == expected_path - - -@pytest.mark.skipif(os.name != "posix", reason="Tests Posix (not Windows) permissions.") -def test_has_expected_permissions_true_linux(tmpdir, create_empty_tmp_file): - file_name = create_empty_tmp_file("test") - os.chmod(file_name, 0o754) - - assert file_utils.has_expected_permissions(file_name, 0o754) - - -@pytest.mark.skipif(os.name != "posix", reason="Tests Posix (not Windows) permissions.") -def test_has_expected_permissions_false_linux(tmpdir, create_empty_tmp_file): - file_name = create_empty_tmp_file("test") - os.chmod(file_name, 0o755) - - assert not file_utils.has_expected_permissions(file_name, 0o700) - - -@pytest.mark.skipif(os.name == "posix", reason="Tests Windows (not Posix) permissions.") -def test_has_expected_permissions_true_windows(tmpdir, create_empty_tmp_file): - file_name = create_empty_tmp_file("test") - subprocess.run(f"echo y| cacls {file_name} /p %USERNAME%:F", shell=True) # noqa: DUO116 - - assert file_utils.has_expected_permissions(file_name, 2032127) - - -@pytest.mark.skipif(os.name == "posix", reason="Tests Windows (not Posix) permissions.") -def test_has_expected_permissions_false_windows(tmpdir, create_empty_tmp_file): - file_name = create_empty_tmp_file("test") - subprocess.run(f"echo y| cacls {file_name} /p %USERNAME%:R", shell=True) # noqa: DUO116 - - assert not file_utils.has_expected_permissions(file_name, 2032127) diff --git a/monkey/tests/unit_tests/monkey_island/cc/setup/test_island_config_options_validator.py b/monkey/tests/unit_tests/monkey_island/cc/setup/test_island_config_options_validator.py index b6d9eeb85..9fb132305 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/setup/test_island_config_options_validator.py +++ b/monkey/tests/unit_tests/monkey_island/cc/setup/test_island_config_options_validator.py @@ -1,16 +1,11 @@ import os -import subprocess from collections.abc import Callable import pytest -from common.utils.exceptions import InsecurePermissionsError from monkey_island.cc.setup.island_config_options import IslandConfigOptions from monkey_island.cc.setup.island_config_options_validator import raise_on_invalid_options -LINUX_READ_ONLY_BY_USER = 0o400 -LINUX_RWX_BY_ALL = 0o777 - def certificate_test_island_config_options(crt_file, key_file): return IslandConfigOptions( @@ -24,24 +19,13 @@ def certificate_test_island_config_options(crt_file, key_file): @pytest.fixture -def linux_island_config_options(create_read_only_linux_file: Callable): - crt_file = create_read_only_linux_file("test.crt") - key_file = create_read_only_linux_file("test.key") +def linux_island_config_options(create_empty_tmp_file: Callable): + crt_file = create_empty_tmp_file("test.crt") + key_file = create_empty_tmp_file("test.key") return certificate_test_island_config_options(crt_file, key_file) -@pytest.fixture -def create_read_only_linux_file(tmpdir: str, create_empty_tmp_file: Callable) -> Callable: - def inner(file_name: str) -> str: - full_file_path = create_empty_tmp_file(file_name) - os.chmod(full_file_path, LINUX_READ_ONLY_BY_USER) - - return full_file_path - - return inner - - @pytest.mark.skipif(os.name != "posix", reason="Tests Posix (not Windows) permissions.") def test_linux_valid_crt_and_key_paths(linux_island_config_options): try: @@ -59,14 +43,6 @@ def test_linux_crt_path_does_not_exist(linux_island_config_options): raise_on_invalid_options(linux_island_config_options) -@pytest.mark.skipif(os.name != "posix", reason="Tests Posix (not Windows) permissions.") -def test_linux_crt_path_insecure_permissions(linux_island_config_options): - os.chmod(linux_island_config_options.crt_path, LINUX_RWX_BY_ALL) - - with pytest.raises(InsecurePermissionsError): - raise_on_invalid_options(linux_island_config_options) - - @pytest.mark.skipif(os.name != "posix", reason="Tests Posix (not Windows) permissions.") def test_linux_key_path_does_not_exist(linux_island_config_options): os.remove(linux_island_config_options.key_path) @@ -75,42 +51,14 @@ def test_linux_key_path_does_not_exist(linux_island_config_options): raise_on_invalid_options(linux_island_config_options) -@pytest.mark.skipif(os.name != "posix", reason="Tests Posix (not Windows) permissions.") -def test_linux_key_path_insecure_permissions(linux_island_config_options): - os.chmod(linux_island_config_options.key_path, LINUX_RWX_BY_ALL) - - with pytest.raises(InsecurePermissionsError): - raise_on_invalid_options(linux_island_config_options) - - @pytest.fixture -def windows_island_config_options(tmpdir: str, create_read_only_windows_file: Callable): - crt_file = create_read_only_windows_file("test.crt") - key_file = create_read_only_windows_file("test.key") +def windows_island_config_options(tmpdir: str, create_empty_tmp_file: Callable): + crt_file = create_empty_tmp_file("test.crt") + key_file = create_empty_tmp_file("test.key") return certificate_test_island_config_options(crt_file, key_file) -@pytest.fixture -def create_read_only_windows_file(tmpdir: str, create_empty_tmp_file: Callable) -> Callable: - def inner(file_name: str) -> str: - full_file_path = create_empty_tmp_file(file_name) - cmd_to_change_permissions = get_windows_cmd_to_change_permissions(full_file_path, "R") - subprocess.run(cmd_to_change_permissions, shell=True) # noqa DUO116 - - return full_file_path - - return inner - - -def get_windows_cmd_to_change_permissions(file_name, permissions): - """ - :param file_name: name of file - :param permissions: can be: N (None), R (Read), W (Write), C (Change (write)), F (Full control) - """ - return f"echo y| cacls {file_name} /p %USERNAME%:{permissions}" - - @pytest.mark.skipif(os.name == "posix", reason="Tests Windows (not Posix) permissions.") def test_windows_valid_crt_and_key_paths(windows_island_config_options): try: @@ -128,31 +76,9 @@ def test_windows_crt_path_does_not_exist(windows_island_config_options): raise_on_invalid_options(windows_island_config_options) -@pytest.mark.skipif(os.name == "posix", reason="Tests Windows (not Posix) permissions.") -def test_windows_crt_path_insecure_permissions(windows_island_config_options): - cmd_to_change_permissions = get_windows_cmd_to_change_permissions( - windows_island_config_options.crt_path, "W" - ) - subprocess.run(cmd_to_change_permissions, shell=True) # noqa DUO116 - - with pytest.raises(InsecurePermissionsError): - raise_on_invalid_options(windows_island_config_options) - - @pytest.mark.skipif(os.name == "posix", reason="Tests Windows (not Posix) permissions.") def test_windows_key_path_does_not_exist(windows_island_config_options): os.remove(windows_island_config_options.key_path) with pytest.raises(FileNotFoundError): raise_on_invalid_options(windows_island_config_options) - - -@pytest.mark.skipif(os.name == "posix", reason="Tests Windows (not Posix) permissions.") -def test_windows_key_path_insecure_permissions(windows_island_config_options): - cmd_to_change_permissions = get_windows_cmd_to_change_permissions( - windows_island_config_options.key_path, "W" - ) - subprocess.run(cmd_to_change_permissions, shell=True) # noqa DUO116 - - with pytest.raises(InsecurePermissionsError): - raise_on_invalid_options(windows_island_config_options)