From 54f5524760423ae4a59c7ff024cc561f8f61da82 Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Thu, 10 Jun 2021 15:04:56 +0530 Subject: [PATCH 1/7] Fix race condition during Windows directory creation --- monkey/monkey_island/cc/environment/utils.py | 21 ++++++++++++------- .../cc/environment/windows_permissions.py | 11 ++++------ 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index dbed504f2..bbf2a0ba6 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -8,6 +8,8 @@ def is_windows_os() -> bool: if is_windows_os(): + import win32file + import monkey_island.cc.environment.windows_permissions as windows_permissions LOG = logging.getLogger(__name__) @@ -15,11 +17,13 @@ LOG = logging.getLogger(__name__) def create_secure_directory(path: str, create_parent_dirs: bool): if not os.path.isdir(path): - _create_secure_directory(path, create_parent_dirs) - set_secure_permissions(path) + if is_windows_os(): + _create_secure_directory_windows(path) + else: + _create_secure_directory_linux(path, create_parent_dirs) -def _create_secure_directory(path: str, create_parent_dirs: bool): +def _create_secure_directory_linux(path: str, create_parent_dirs: bool): try: if create_parent_dirs: # Don't split directory creation and permission setting @@ -35,10 +39,13 @@ def _create_secure_directory(path: str, create_parent_dirs: bool): raise ex -def set_secure_permissions(dir_path: str): +def _create_secure_directory_windows(path: str): + security_descriptor = windows_permissions.get_sd_for_owner_only_perms() try: - if is_windows_os(): - windows_permissions.set_perms_to_owner_only(folder_path=dir_path) + win32file.CreateDirectory(path, security_descriptor) except Exception as ex: - LOG.error(f"Permissions could not be set successfully for {dir_path}: {str(ex)}") + LOG.error( + f'Could not create a directory at "{path}" (maybe environmental variables could not be ' + f"resolved?): {str(ex)}" + ) raise ex diff --git a/monkey/monkey_island/cc/environment/windows_permissions.py b/monkey/monkey_island/cc/environment/windows_permissions.py index 225e52370..02640e734 100644 --- a/monkey/monkey_island/cc/environment/windows_permissions.py +++ b/monkey/monkey_island/cc/environment/windows_permissions.py @@ -4,12 +4,10 @@ import win32con import win32security -def set_perms_to_owner_only(folder_path: str) -> None: +def get_sd_for_owner_only_perms() -> None: user = get_user_pySID_object() + security_descriptor = win32security.SECURITY_DESCRIPTOR() - security_descriptor = win32security.GetFileSecurity( - folder_path, win32security.DACL_SECURITY_INFORMATION - ) dacl = win32security.ACL() dacl.AddAccessAllowedAce( win32security.ACL_REVISION, @@ -17,9 +15,8 @@ def set_perms_to_owner_only(folder_path: str) -> None: user, ) security_descriptor.SetSecurityDescriptorDacl(1, dacl, 0) - win32security.SetFileSecurity( - folder_path, win32security.DACL_SECURITY_INFORMATION, security_descriptor - ) + + return security_descriptor def get_user_pySID_object(): From 1fa2ffe8f75009aedf8d5d51feb090d76b13d8e0 Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Thu, 10 Jun 2021 15:16:01 +0530 Subject: [PATCH 2/7] Fix Windows directory creation --- monkey/monkey_island/cc/environment/utils.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index bbf2a0ba6..77d1216f4 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -9,6 +9,7 @@ def is_windows_os() -> bool: if is_windows_os(): import win32file + import win32security import monkey_island.cc.environment.windows_permissions as windows_permissions @@ -40,9 +41,10 @@ def _create_secure_directory_linux(path: str, create_parent_dirs: bool): def _create_secure_directory_windows(path: str): - security_descriptor = windows_permissions.get_sd_for_owner_only_perms() try: - win32file.CreateDirectory(path, security_descriptor) + security_attributes = win32security.SECURITY_ATTRIBUTES() + security_attributes.SECURITY_DESCRIPTOR = windows_permissions.get_sd_for_owner_only_perms() + win32file.CreateDirectory(path, security_attributes) except Exception as ex: LOG.error( f'Could not create a directory at "{path}" (maybe environmental variables could not be ' From 74111f80e9d560c8307c6fee85435c321e5f4914 Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Thu, 10 Jun 2021 15:19:40 +0530 Subject: [PATCH 3/7] Remove `create_parents_dir` parameter when creating directories Can't create parents on Windows using pywin32. Removed it completely so that behavior is consistent across OSes. --- monkey/monkey_island/cc/environment/utils.py | 15 +++++-------- monkey/monkey_island/cc/setup/config_setup.py | 4 ++-- .../cc/setup/mongo/mongo_setup.py | 2 +- .../cc/environment/test_utils.py | 22 +++++-------------- 4 files changed, 14 insertions(+), 29 deletions(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index 77d1216f4..866cf5d4f 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -16,22 +16,19 @@ if is_windows_os(): LOG = logging.getLogger(__name__) -def create_secure_directory(path: str, create_parent_dirs: bool): +def create_secure_directory(path: str): if not os.path.isdir(path): if is_windows_os(): _create_secure_directory_windows(path) else: - _create_secure_directory_linux(path, create_parent_dirs) + _create_secure_directory_linux(path) -def _create_secure_directory_linux(path: str, create_parent_dirs: bool): +def _create_secure_directory_linux(path: str): try: - if create_parent_dirs: - # Don't split directory creation and permission setting - # because it will temporarily create an accessible directory which anyone can use. - os.makedirs(path, mode=0o700) - else: - os.mkdir(path, mode=0o700) + # Don't split directory creation and permission setting + # because it will temporarily create an accessible directory which anyone can use. + os.mkdir(path, mode=0o700) except Exception as ex: LOG.error( f'Could not create a directory at "{path}" (maybe environmental variables could not be ' diff --git a/monkey/monkey_island/cc/setup/config_setup.py b/monkey/monkey_island/cc/setup/config_setup.py index d1e3e984b..103137a91 100644 --- a/monkey/monkey_island/cc/setup/config_setup.py +++ b/monkey/monkey_island/cc/setup/config_setup.py @@ -18,7 +18,7 @@ def setup_data_dir(island_args: IslandCmdArgs) -> Tuple[IslandConfigOptions, str def _setup_config_by_cmd_arg(server_config_path) -> Tuple[IslandConfigOptions, str]: server_config_path = file_utils.expand_path(server_config_path) config = server_config_handler.load_server_config_from_file(server_config_path) - create_secure_directory(config.data_dir, create_parent_dirs=True) + create_secure_directory(config.data_dir) return config, server_config_path @@ -26,7 +26,7 @@ def _setup_default_config() -> Tuple[IslandConfigOptions, str]: default_config = server_config_handler.load_server_config_from_file(DEFAULT_SERVER_CONFIG_PATH) default_data_dir = default_config.data_dir - create_secure_directory(default_data_dir, create_parent_dirs=False) + create_secure_directory(default_data_dir) server_config_path = server_config_handler.create_default_server_config_file(default_data_dir) config = server_config_handler.load_server_config_from_file(server_config_path) diff --git a/monkey/monkey_island/cc/setup/mongo/mongo_setup.py b/monkey/monkey_island/cc/setup/mongo/mongo_setup.py index e62bbcdb7..0ab8ca0c0 100644 --- a/monkey/monkey_island/cc/setup/mongo/mongo_setup.py +++ b/monkey/monkey_island/cc/setup/mongo/mongo_setup.py @@ -35,7 +35,7 @@ def _create_db_dir(db_dir_parent_path) -> str: db_dir = os.path.join(db_dir_parent_path, DB_DIR_NAME) logger.info(f"Database content directory: {db_dir}.") - create_secure_directory(db_dir, create_parent_dirs=False) + create_secure_directory(db_dir) return db_dir diff --git a/monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py b/monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py index e8287c3a6..47e4ac8f6 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py +++ b/monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py @@ -20,38 +20,26 @@ def test_path(tmpdir): return path -def test_create_secure_directory__parent_dirs(test_path_nested): - create_secure_directory(test_path_nested, create_parent_dirs=True) - assert os.path.isdir(test_path_nested) - - def test_create_secure_directory__already_created(test_path): os.mkdir(test_path) assert os.path.isdir(test_path) - create_secure_directory(test_path, create_parent_dirs=False) + create_secure_directory(test_path) def test_create_secure_directory__no_parent_dir(test_path_nested): with pytest.raises(Exception): - create_secure_directory(test_path_nested, create_parent_dirs=False) - - -@pytest.mark.skipif(is_windows_os(), reason="Tests Posix (not Windows) permissions.") -def test_create_secure_directory__perm_linux(test_path_nested): - create_secure_directory(test_path_nested, create_parent_dirs=True) - st = os.stat(test_path_nested) - return bool(st.st_mode & stat.S_IRWXU) + create_secure_directory(test_path_nested) @pytest.mark.skipif(not is_windows_os(), reason="Tests Windows (not Posix) permissions.") def test_create_secure_directory__perm_windows(test_path): - import win32api # noqa: E402 - import win32security # noqa: E402 + import win32api + import win32security FULL_CONTROL = 2032127 ACE_TYPE_ALLOW = 0 - create_secure_directory(test_path, create_parent_dirs=False) + create_secure_directory(test_path) user_sid, _, _ = win32security.LookupAccountName("", win32api.GetUserName()) security_descriptor = win32security.GetNamedSecurityInfo( From 7643102ccde50c63d51b1bbd5831be2c6f5b7917 Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Thu, 10 Jun 2021 16:55:56 +0530 Subject: [PATCH 4/7] Rename function to not use abbreviations --- monkey/monkey_island/cc/environment/utils.py | 4 +++- monkey/monkey_island/cc/environment/windows_permissions.py | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index 866cf5d4f..fb239221c 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -40,7 +40,9 @@ def _create_secure_directory_linux(path: str): def _create_secure_directory_windows(path: str): try: security_attributes = win32security.SECURITY_ATTRIBUTES() - security_attributes.SECURITY_DESCRIPTOR = windows_permissions.get_sd_for_owner_only_perms() + security_attributes.SECURITY_DESCRIPTOR = ( + windows_permissions.get_security_descriptor_for_owner_only_perms() + ) win32file.CreateDirectory(path, security_attributes) except Exception as ex: LOG.error( diff --git a/monkey/monkey_island/cc/environment/windows_permissions.py b/monkey/monkey_island/cc/environment/windows_permissions.py index 02640e734..f090083f6 100644 --- a/monkey/monkey_island/cc/environment/windows_permissions.py +++ b/monkey/monkey_island/cc/environment/windows_permissions.py @@ -4,7 +4,7 @@ import win32con import win32security -def get_sd_for_owner_only_perms() -> None: +def get_security_descriptor_for_owner_only_perms() -> None: user = get_user_pySID_object() security_descriptor = win32security.SECURITY_DESCRIPTOR() From 92a71451fb13d7d4c6b4fdf23eaac7ab6ac75318 Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Thu, 10 Jun 2021 17:01:57 +0530 Subject: [PATCH 5/7] Remove unused import in test_utils.py --- .../tests/unit_tests/monkey_island/cc/environment/test_utils.py | 1 - 1 file changed, 1 deletion(-) diff --git a/monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py b/monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py index 47e4ac8f6..aa4458c38 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py +++ b/monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py @@ -1,5 +1,4 @@ import os -import stat import pytest From 5d8db4b112c7c89e1232ddd4183b74faee37249d Mon Sep 17 00:00:00 2001 From: Shreya Malviya Date: Thu, 10 Jun 2021 17:03:01 +0530 Subject: [PATCH 6/7] Update log message in monkey/monkey_island/cc/environment/utils.py Co-authored-by: Mike Salvatore --- monkey/monkey_island/cc/environment/utils.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index fb239221c..62c8bb7d2 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -46,7 +46,6 @@ def _create_secure_directory_windows(path: str): win32file.CreateDirectory(path, security_attributes) except Exception as ex: LOG.error( - f'Could not create a directory at "{path}" (maybe environmental variables could not be ' - f"resolved?): {str(ex)}" + f'Could not create a directory at "{path}": {str(ex)}" ) raise ex From f04f307f7818d40868c245ddc39d74e15d0aa47c Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Thu, 10 Jun 2021 17:12:34 +0530 Subject: [PATCH 7/7] Add unit test for Linux directory permissions (removed accidentally previously) --- monkey/monkey_island/cc/environment/utils.py | 2 +- .../unit_tests/monkey_island/cc/environment/test_utils.py | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index 62c8bb7d2..8efbb4492 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -46,6 +46,6 @@ def _create_secure_directory_windows(path: str): win32file.CreateDirectory(path, security_attributes) except Exception as ex: LOG.error( - f'Could not create a directory at "{path}": {str(ex)}" + f'Could not create a directory at "{path}": {str(ex)}' ) raise ex diff --git a/monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py b/monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py index aa4458c38..4d933af76 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py +++ b/monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py @@ -1,4 +1,5 @@ import os +import stat import pytest @@ -30,6 +31,13 @@ def test_create_secure_directory__no_parent_dir(test_path_nested): create_secure_directory(test_path_nested) +@pytest.mark.skipif(is_windows_os(), reason="Tests Posix (not Windows) permissions.") +def test_create_secure_directory__perm_linux(test_path): + create_secure_directory(test_path) + st = os.stat(test_path) + return bool(st.st_mode & stat.S_IRWXU) + + @pytest.mark.skipif(not is_windows_os(), reason="Tests Windows (not Posix) permissions.") def test_create_secure_directory__perm_windows(test_path): import win32api