From b39440e871e9f25a6090cc7dd36eeadff3f46813 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Sat, 19 Jun 2021 08:55:53 -0400 Subject: [PATCH 1/4] island: Return a fd instead of PyHandle during windows file creation Fixes #1252 --- monkey/monkey_island/cc/server_utils/file_utils.py | 9 +++++---- .../monkey_island/cc/server_utils/test_file_utils.py | 3 +-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/file_utils.py b/monkey/monkey_island/cc/server_utils/file_utils.py index 995501c09..1cda9a6d3 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -79,8 +79,7 @@ def _get_file_descriptor_for_new_secure_file_linux(path: str) -> int: def _get_file_descriptor_for_new_secure_file_windows(path: str) -> int: try: file_access = win32file.GENERIC_READ | win32file.GENERIC_WRITE - # subsequent open operations on the object will succeed only if read access is requested - file_sharing = win32file.FILE_SHARE_READ + file_sharing = win32file.FILE_SHARE_READ | win32file.FILE_SHARE_WRITE security_attributes = win32security.SECURITY_ATTRIBUTES() security_attributes.SECURITY_DESCRIPTOR = ( windows_permissions.get_security_descriptor_for_owner_only_perms() @@ -88,7 +87,7 @@ def _get_file_descriptor_for_new_secure_file_windows(path: str) -> int: file_creation = win32file.CREATE_NEW # fails if file exists file_attributes = win32file.FILE_FLAG_BACKUP_SEMANTICS - fd = win32file.CreateFile( + handle = win32file.CreateFile( path, file_access, file_sharing, @@ -98,7 +97,9 @@ def _get_file_descriptor_for_new_secure_file_windows(path: str) -> int: _get_null_value_for_win32(), ) - return fd + detached_handle = handle.Detach() + + return win32file._open_osfhandle(detached_handle, os.O_RDWR) except Exception as ex: LOG.error(f'Could not create a file at "{path}": {str(ex)}') 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 894f1e6b3..756c6452d 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 @@ -12,7 +12,6 @@ from monkey_island.cc.server_utils.file_utils import ( if is_windows_os(): import win32api - import win32file import win32security FULL_CONTROL = 2032127 @@ -125,7 +124,7 @@ def test_get_file_descriptor_for_new_secure_file__perm_linux(test_path): @pytest.mark.skipif(not is_windows_os(), reason="Tests Windows (not Posix) permissions.") def test_get_file_descriptor_for_new_secure_file__perm_windows(test_path): - win32file.CloseHandle(get_file_descriptor_for_new_secure_file(test_path)) + os.close(get_file_descriptor_for_new_secure_file(test_path)) acl, user_sid = _get_acl_and_sid_from_path(test_path) From 51aa0d1564b83ad0aae4e27cad538ac17f5dced8 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Sat, 19 Jun 2021 09:12:04 -0400 Subject: [PATCH 2/4] island: Refactor get_file_descriptor_for_new_secure_file as contextmgr get_file_descriptor_for_new_secure_file() has been refactored as a contextmanager. Additionally, it has been renamed to open_new_securely_permissioned_file(). The function can now be used similarly to open(). Example: with open_new_securely_permissioned_file(file_path, "wb") as f: f.write(data) --- .../cc/server_utils/encryptor.py | 5 ++- .../cc/server_utils/file_utils.py | 12 +++++-- .../cc/server_utils/test_file_utils.py | 32 +++++++++++++------ 3 files changed, 34 insertions(+), 15 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/encryptor.py b/monkey/monkey_island/cc/server_utils/encryptor.py index 657a06e87..06a973526 100644 --- a/monkey/monkey_island/cc/server_utils/encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryptor.py @@ -6,7 +6,7 @@ import os from Crypto import Random # noqa: DUO133 # nosec: B413 from Crypto.Cipher import AES # noqa: DUO133 # nosec: B413 -from monkey_island.cc.server_utils.file_utils import get_file_descriptor_for_new_secure_file +from monkey_island.cc.server_utils.file_utils import open_new_securely_permissioned_file __author__ = "itay.mizeretz" @@ -27,8 +27,7 @@ class Encryptor: def _init_key(self, password_file_path: str): self._cipher_key = Random.new().read(self._BLOCK_SIZE) - get_file_descriptor_for_new_secure_file(path=password_file_path) - with open(password_file_path, "wb") as f: + with open_new_securely_permissioned_file(password_file_path, "wb") as f: f.write(self._cipher_key) def _load_existing_key(self, password_file): diff --git a/monkey/monkey_island/cc/server_utils/file_utils.py b/monkey/monkey_island/cc/server_utils/file_utils.py index 1cda9a6d3..ebd28595b 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -2,6 +2,8 @@ import logging import os import platform import stat +from contextlib import contextmanager +from typing import Generator LOG = logging.getLogger(__name__) @@ -54,11 +56,15 @@ def _create_secure_directory_windows(path: str): raise ex -def get_file_descriptor_for_new_secure_file(path: str) -> int: +@contextmanager +def open_new_securely_permissioned_file(path: str, mode: str = "w") -> Generator: if is_windows_os(): - return _get_file_descriptor_for_new_secure_file_windows(path) + fd = _get_file_descriptor_for_new_secure_file_windows(path) else: - return _get_file_descriptor_for_new_secure_file_linux(path) + fd = _get_file_descriptor_for_new_secure_file_linux(path) + + with open(fd, mode) as f: + yield f def _get_file_descriptor_for_new_secure_file_linux(path: str) -> int: 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 756c6452d..9a9ada29d 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 @@ -6,8 +6,8 @@ import pytest from monkey_island.cc.server_utils.file_utils import ( create_secure_directory, expand_path, - get_file_descriptor_for_new_secure_file, is_windows_os, + open_new_securely_permissioned_file, ) if is_windows_os(): @@ -98,22 +98,26 @@ def test_create_secure_directory__perm_windows(test_path): assert ace_inheritance == ACE_INHERIT_OBJECT_AND_CONTAINER -def test_get_file_descriptor_for_new_secure_file__already_exists(test_path): +def test_open_new_securely_permissioned_file__already_exists(test_path): os.close(os.open(test_path, os.O_CREAT, stat.S_IRWXU)) assert os.path.isfile(test_path) with pytest.raises(Exception): - get_file_descriptor_for_new_secure_file(test_path) + with open_new_securely_permissioned_file(test_path): + pass -def test_get_file_descriptor_for_new_secure_file__no_parent_dir(test_path_nested): +def test_open_new_securely_permissioned_file__no_parent_dir(test_path_nested): with pytest.raises(Exception): - get_file_descriptor_for_new_secure_file(test_path_nested) + with open_new_securely_permissioned_file(test_path_nested): + pass @pytest.mark.skipif(is_windows_os(), reason="Tests Posix (not Windows) permissions.") -def test_get_file_descriptor_for_new_secure_file__perm_linux(test_path): - os.close(get_file_descriptor_for_new_secure_file(test_path)) +def test_open_new_securely_permissioned_file__perm_linux(test_path): + with open_new_securely_permissioned_file(test_path): + pass + st = os.stat(test_path) expected_mode = stat.S_IRUSR | stat.S_IWUSR @@ -123,8 +127,9 @@ def test_get_file_descriptor_for_new_secure_file__perm_linux(test_path): @pytest.mark.skipif(not is_windows_os(), reason="Tests Windows (not Posix) permissions.") -def test_get_file_descriptor_for_new_secure_file__perm_windows(test_path): - os.close(get_file_descriptor_for_new_secure_file(test_path)) +def test_open_new_securely_permissioned_file__perm_windows(test_path): + with open_new_securely_permissioned_file(test_path): + pass acl, user_sid = _get_acl_and_sid_from_path(test_path) @@ -140,3 +145,12 @@ def test_get_file_descriptor_for_new_secure_file__perm_windows(test_path): assert ace_sid == user_sid assert ace_permissions == FULL_CONTROL and ace_access_mode == ACE_ACCESS_MODE_GRANT_ACCESS assert ace_inheritance == ACE_INHERIT_OBJECT_AND_CONTAINER + + +def test_open_new_securely_permissioned_file__write(test_path): + TEST_STR = b"Hello World" + with open_new_securely_permissioned_file(test_path, "wb") as f: + f.write(TEST_STR) + + with open(test_path, "rb") as f: + assert f.read() == TEST_STR From 2d18a68787dc17a3291255c13ee84d99e377d8ac Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 21 Jun 2021 09:26:39 -0400 Subject: [PATCH 3/4] island: Fix return type hint for _get_null_value_for_win32() The _get_null_value_for_win32() function does not return None, it returns a PyHANDLE object. For the moment, I'm unable to determine the correct way to import PyHANDLE so that it can be specified in the type hint. Since type hints aren't actually enforced, it's not worth the effort to fully solve this at the present time, so the type hint has just been removed. --- monkey/monkey_island/cc/server_utils/file_utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/monkey/monkey_island/cc/server_utils/file_utils.py b/monkey/monkey_island/cc/server_utils/file_utils.py index ebd28595b..7c4ab59fb 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -112,6 +112,6 @@ def _get_file_descriptor_for_new_secure_file_windows(path: str) -> int: raise ex -def _get_null_value_for_win32() -> None: +def _get_null_value_for_win32(): # https://stackoverflow.com/questions/46800142/in-python-with-pywin32-win32job-the-createjobobject-function-how-do-i-pass-nu # noqa: E501 return win32job.CreateJobObject(None, "") From 02ed22bab77e57c342dacba2dc473e43d624fb90 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Mon, 21 Jun 2021 13:43:20 -0400 Subject: [PATCH 4/4] island: Remove FILE_SHARE_WRITE from windows permissions Granting FILE_SHARE_WRITE on mongo_key.bin is unnecessary. Since mongo_key.bin is the only file that is created using _get_file_descriptor_for_new_secure_file_windows() at the moment, we won't grant FILE_SHARE_WRITE. --- monkey/monkey_island/cc/server_utils/file_utils.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/monkey/monkey_island/cc/server_utils/file_utils.py b/monkey/monkey_island/cc/server_utils/file_utils.py index 7c4ab59fb..e429eb464 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -85,7 +85,12 @@ def _get_file_descriptor_for_new_secure_file_linux(path: str) -> int: def _get_file_descriptor_for_new_secure_file_windows(path: str) -> int: try: file_access = win32file.GENERIC_READ | win32file.GENERIC_WRITE - file_sharing = win32file.FILE_SHARE_READ | win32file.FILE_SHARE_WRITE + + # Enables other processes to open this file with read-only access. + # Attempts by other processes to open the file for writing while this + # process still holds it open will fail. + file_sharing = win32file.FILE_SHARE_READ + security_attributes = win32security.SECURITY_ATTRIBUTES() security_attributes.SECURITY_DESCRIPTOR = ( windows_permissions.get_security_descriptor_for_owner_only_perms()