From 5d7d86aedcd47da98722c375ce5d54bdedc41f12 Mon Sep 17 00:00:00 2001 From: Shreya Date: Mon, 14 Jun 2021 12:22:45 +0530 Subject: [PATCH 01/31] island: Modify log message when creating secure directory on Windows --- monkey/monkey_island/cc/environment/utils.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index 8efbb4492..524b9d5a8 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -30,10 +30,7 @@ def _create_secure_directory_linux(path: str): # 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 ' - f"resolved?): {str(ex)}" - ) + LOG.error(f'Could not create a directory at "{path}": {str(ex)}') raise ex @@ -45,7 +42,5 @@ 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)}' - ) + LOG.error(f'Could not create a directory at "{path}": {str(ex)}') raise ex From ff85360639d318096c5283d87ca626177e3a04e2 Mon Sep 17 00:00:00 2001 From: Shreya Date: Mon, 14 Jun 2021 13:20:19 +0530 Subject: [PATCH 02/31] island: Add functions to create a file securely on Linux and Windows --- monkey/monkey_island/cc/environment/utils.py | 52 ++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index 524b9d5a8..7c822edd1 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -29,6 +29,7 @@ def _create_secure_directory_linux(path: str): # 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}": {str(ex)}') raise ex @@ -41,6 +42,57 @@ def _create_secure_directory_windows(path: str): windows_permissions.get_security_descriptor_for_owner_only_perms() ) win32file.CreateDirectory(path, security_attributes) + except Exception as ex: LOG.error(f'Could not create a directory at "{path}": {str(ex)}') raise ex + + +def create_secure_file(path: str): + if not os.path.isfile(path): + if is_windows_os(): + _create_secure_file_windows(path) + else: + _create_secure_file_linux(path) + + +def _create_secure_file_linux(path: str): + try: + flags = os.O_RDWR | os.O_CREAT # read/write, create new + mode = 0o700 # read/write/execute permissions to owner + + with os.open(path, flags, mode) as _: + pass + + except Exception as ex: + LOG.error(f'Could not create a file at "{path}": {str(ex)}') + raise ex + + +def _create_secure_file_windows(path: str): + try: + file_access = win32file.GENERIC_READ | win32file.GENERIC_WRITE + file_sharing = ( + win32file.FILE_SHARE_READ + ) # subsequent open operations on the object will succeed only if read access is requested + security_attributes = win32security.SECURITY_ATTRIBUTES() + security_attributes.SECURITY_DESCRIPTOR = ( + windows_permissions.get_security_descriptor_for_owner_only_perms() + ) + file_creation = win32file.CREATE_NEW # fails if file exists + file_attributes = win32file.FILE_ATTRIBUTE_NORMAL + + with win32file.CreateFile( + fileName=path, + desiredAccess=file_access, + shareMode=file_sharing, + attributes=security_attributes, + CreationDisposition=file_creation, + flagsAndAttributes=file_attributes, + hTemplateFile=win32file.NULL, + ) as _: + pass + + except Exception as ex: + LOG.error(f'Could not create a file at "{path}": {str(ex)}') + raise ex From 26ae50f90f239ed47cf4d02a10b461307d95d775 Mon Sep 17 00:00:00 2001 From: Shreya Date: Mon, 14 Jun 2021 13:21:29 +0530 Subject: [PATCH 03/31] island: Create mongo key file securely before using it --- monkey/monkey_island/cc/server_utils/encryptor.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/monkey/monkey_island/cc/server_utils/encryptor.py b/monkey/monkey_island/cc/server_utils/encryptor.py index 60ab8ead9..cfa5b751c 100644 --- a/monkey/monkey_island/cc/server_utils/encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryptor.py @@ -6,6 +6,8 @@ import os from Crypto import Random # noqa: DUO133 # nosec: B413 from Crypto.Cipher import AES # noqa: DUO133 # nosec: B413 +from monkey_island.cc.environment.utils import create_secure_file + __author__ = "itay.mizeretz" _encryptor = None @@ -21,6 +23,7 @@ class Encryptor: if os.path.exists(password_file): self._load_existing_key(password_file) else: + create_secure_file(path=password_file) self._init_key(password_file) def _init_key(self, password_file): From 8dd4bb5e17e1c796470fe1c8f32044cbd6b1fe7b Mon Sep 17 00:00:00 2001 From: Shreya Date: Mon, 14 Jun 2021 16:26:00 +0530 Subject: [PATCH 04/31] island: Use 'x' instead of '_' when creating a secure file --- monkey/monkey_island/cc/environment/utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index 7c822edd1..e454843ed 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -61,7 +61,7 @@ def _create_secure_file_linux(path: str): flags = os.O_RDWR | os.O_CREAT # read/write, create new mode = 0o700 # read/write/execute permissions to owner - with os.open(path, flags, mode) as _: + with os.open(path, flags, mode) as x: # noqa: F841 pass except Exception as ex: @@ -90,7 +90,7 @@ def _create_secure_file_windows(path: str): CreationDisposition=file_creation, flagsAndAttributes=file_attributes, hTemplateFile=win32file.NULL, - ) as _: + ) as x: # noqa: F841 pass except Exception as ex: From 8b932e194654c70cdaf4bf662184c4942984ae51 Mon Sep 17 00:00:00 2001 From: Shreya Date: Mon, 14 Jun 2021 16:49:24 +0530 Subject: [PATCH 05/31] island: Add os.O_EXCL flag so that an error is thrown if trying to create a file that exists --- monkey/monkey_island/cc/environment/utils.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index e454843ed..0a84e6aac 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -58,7 +58,9 @@ def create_secure_file(path: str): def _create_secure_file_linux(path: str): try: - flags = os.O_RDWR | os.O_CREAT # read/write, create new + flags = ( + os.O_RDWR | os.O_CREAT | os.O_EXCL + ) # read/write, create new, throw error if file exists mode = 0o700 # read/write/execute permissions to owner with os.open(path, flags, mode) as x: # noqa: F841 From 5fe0c80377c709c6e6a3c9ec87729f7026bc0a0e Mon Sep 17 00:00:00 2001 From: Shreya Date: Mon, 14 Jun 2021 17:07:30 +0530 Subject: [PATCH 06/31] island: Can't use `with` with `os.open()`, use `os.close()` to close file descriptor --- monkey/monkey_island/cc/environment/utils.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index 0a84e6aac..1fb998a5f 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -62,9 +62,7 @@ def _create_secure_file_linux(path: str): os.O_RDWR | os.O_CREAT | os.O_EXCL ) # read/write, create new, throw error if file exists mode = 0o700 # read/write/execute permissions to owner - - with os.open(path, flags, mode) as x: # noqa: F841 - pass + os.close(os.open(path, flags, mode)) except Exception as ex: LOG.error(f'Could not create a file at "{path}": {str(ex)}') From 248d57789f3a963bbf8ac5be9abcba3b27c7763e Mon Sep 17 00:00:00 2001 From: Shreya Date: Mon, 14 Jun 2021 17:50:40 +0530 Subject: [PATCH 07/31] tests: Add unit tests for securly creating a file --- .../cc/environment/test_utils.py | 51 ++++++++++++++++++- 1 file changed, 50 insertions(+), 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 4d933af76..b04b180e5 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 @@ -3,7 +3,11 @@ import stat import pytest -from monkey_island.cc.environment.utils import create_secure_directory, is_windows_os +from monkey_island.cc.environment.utils import ( + create_secure_directory, + create_secure_file, + is_windows_os, +) @pytest.fixture @@ -63,3 +67,48 @@ def test_create_secure_directory__perm_windows(test_path): assert sid == user_sid assert permissions == FULL_CONTROL and ace_type == ACE_TYPE_ALLOW + + +def test_create_secure_file__already_created(test_path): + os.close(os.open(test_path, os.O_CREAT, 0o700)) + assert os.path.isfile(test_path) + create_secure_file(test_path) + + +def test_create_secure_file__no_parent_dir(test_path_nested): + with pytest.raises(Exception): + create_secure_file(test_path_nested) + + +@pytest.mark.skipif(is_windows_os(), reason="Tests Posix (not Windows) permissions.") +def test_create_secure_file__perm_linux(test_path): + create_secure_file(test_path) + st = os.stat(test_path) + assert (st.st_mode & 0o777) == stat.S_IRWXU + + +@pytest.mark.skipif(not is_windows_os(), reason="Tests Windows (not Posix) permissions.") +def test_create_secure_file__perm_windows(test_path): + import win32api + import win32security + + FULL_CONTROL = 2032127 + ACE_TYPE_ALLOW = 0 + + create_secure_file(test_path) + + user_sid, _, _ = win32security.LookupAccountName("", win32api.GetUserName()) + security_descriptor = win32security.GetNamedSecurityInfo( + test_path, win32security.SE_FILE_OBJECT, win32security.DACL_SECURITY_INFORMATION + ) + acl = security_descriptor.GetSecurityDescriptorDacl() + + assert acl.GetAceCount() == 1 + + ace = acl.GetAce(0) + ace_type, _ = ace[0] # 0 for allow, 1 for deny + permissions = ace[1] + sid = ace[-1] + + assert sid == user_sid + assert permissions == FULL_CONTROL and ace_type == ACE_TYPE_ALLOW From 6d360ef8654d2e719478c23cb9f89c5269c16c54 Mon Sep 17 00:00:00 2001 From: Shreya Date: Mon, 14 Jun 2021 17:51:55 +0530 Subject: [PATCH 08/31] tests: Fix assertion in `test_create_secure_directory__perm_linux()` --- .../tests/unit_tests/monkey_island/cc/environment/test_utils.py | 2 +- 1 file changed, 1 insertion(+), 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 b04b180e5..b18f5d656 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 @@ -39,7 +39,7 @@ def test_create_secure_directory__no_parent_dir(test_path_nested): 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) + assert (st.st_mode & 0o777) == stat.S_IRWXU @pytest.mark.skipif(not is_windows_os(), reason="Tests Windows (not Posix) permissions.") From c0d9489100bfe51954a024ba582c829e9184619b Mon Sep 17 00:00:00 2001 From: Shreya Date: Mon, 14 Jun 2021 17:59:57 +0530 Subject: [PATCH 09/31] tests: Extract duplicate code in Windows tests in test_utils --- .../cc/environment/test_utils.py | 42 +++++++++---------- 1 file changed, 20 insertions(+), 22 deletions(-) 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 b18f5d656..248d5f850 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 @@ -9,6 +9,13 @@ from monkey_island.cc.environment.utils import ( is_windows_os, ) +if is_windows_os(): + import win32api + import win32security + + FULL_CONTROL = 2032127 + ACE_TYPE_ALLOW = 0 + @pytest.fixture def test_path_nested(tmpdir): @@ -24,6 +31,17 @@ def test_path(tmpdir): return path +def _get_acl_and_sid_from_path(path: str): + create_secure_file(path) + + sid, _, _ = win32security.LookupAccountName("", win32api.GetUserName()) + security_descriptor = win32security.GetNamedSecurityInfo( + test_path, win32security.SE_FILE_OBJECT, win32security.DACL_SECURITY_INFORMATION + ) + acl = security_descriptor.GetSecurityDescriptorDacl() + return acl, sid + + def test_create_secure_directory__already_created(test_path): os.mkdir(test_path) assert os.path.isdir(test_path) @@ -44,19 +62,9 @@ def test_create_secure_directory__perm_linux(test_path): @pytest.mark.skipif(not is_windows_os(), reason="Tests Windows (not Posix) permissions.") def test_create_secure_directory__perm_windows(test_path): - import win32api - import win32security - - FULL_CONTROL = 2032127 - ACE_TYPE_ALLOW = 0 - create_secure_directory(test_path) - user_sid, _, _ = win32security.LookupAccountName("", win32api.GetUserName()) - security_descriptor = win32security.GetNamedSecurityInfo( - test_path, win32security.SE_FILE_OBJECT, win32security.DACL_SECURITY_INFORMATION - ) - acl = security_descriptor.GetSecurityDescriptorDacl() + acl, user_sid = _get_acl_and_sid_from_path() assert acl.GetAceCount() == 1 @@ -89,19 +97,9 @@ def test_create_secure_file__perm_linux(test_path): @pytest.mark.skipif(not is_windows_os(), reason="Tests Windows (not Posix) permissions.") def test_create_secure_file__perm_windows(test_path): - import win32api - import win32security - - FULL_CONTROL = 2032127 - ACE_TYPE_ALLOW = 0 - create_secure_file(test_path) - user_sid, _, _ = win32security.LookupAccountName("", win32api.GetUserName()) - security_descriptor = win32security.GetNamedSecurityInfo( - test_path, win32security.SE_FILE_OBJECT, win32security.DACL_SECURITY_INFORMATION - ) - acl = security_descriptor.GetSecurityDescriptorDacl() + acl, user_sid = _get_acl_and_sid_from_path() assert acl.GetAceCount() == 1 From 37eda4e7adbf6e0b118e388b84f944f32afc6a2b Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Mon, 14 Jun 2021 23:15:17 +0530 Subject: [PATCH 10/31] island: Fix secure file creation on Windows --- monkey/monkey_island/cc/environment/utils.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index 1fb998a5f..2f1b161e1 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 win32job import win32security import monkey_island.cc.environment.windows_permissions as windows_permissions @@ -83,13 +84,15 @@ def _create_secure_file_windows(path: str): file_attributes = win32file.FILE_ATTRIBUTE_NORMAL with win32file.CreateFile( - fileName=path, - desiredAccess=file_access, - shareMode=file_sharing, - attributes=security_attributes, - CreationDisposition=file_creation, - flagsAndAttributes=file_attributes, - hTemplateFile=win32file.NULL, + path, + file_access, + file_sharing, + security_attributes, + file_creation, + file_attributes, + win32job.CreateJobObject( + None, "" + ), # https://stackoverflow.com/questions/46800142/in-python-with-pywin32-win32job-the-createjobobject-function-how-do-i-pass-nu # noqa: E501 ) as x: # noqa: F841 pass From 1467a53e6071e1235b304bc75476df38cf5faa3b Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Mon, 14 Jun 2021 23:24:09 +0530 Subject: [PATCH 11/31] island: Use win32file.CloseHandle() to close file descriptor on Windows --- monkey/monkey_island/cc/environment/utils.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index 2f1b161e1..86370e01f 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -83,7 +83,7 @@ def _create_secure_file_windows(path: str): file_creation = win32file.CREATE_NEW # fails if file exists file_attributes = win32file.FILE_ATTRIBUTE_NORMAL - with win32file.CreateFile( + win32file.CloseHandle(win32file.CreateFile( path, file_access, file_sharing, @@ -93,8 +93,7 @@ def _create_secure_file_windows(path: str): win32job.CreateJobObject( None, "" ), # https://stackoverflow.com/questions/46800142/in-python-with-pywin32-win32job-the-createjobobject-function-how-do-i-pass-nu # noqa: E501 - ) as x: # noqa: F841 - pass + )) except Exception as ex: LOG.error(f'Could not create a file at "{path}": {str(ex)}') From 7ddb986f159225a87322da294622d1493c800ddb Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Mon, 14 Jun 2021 23:24:52 +0530 Subject: [PATCH 12/31] tests: Fix file creation unit tests in test_utils.py --- .../unit_tests/monkey_island/cc/environment/test_utils.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 248d5f850..90b65c4ec 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 @@ -36,7 +36,7 @@ def _get_acl_and_sid_from_path(path: str): sid, _, _ = win32security.LookupAccountName("", win32api.GetUserName()) security_descriptor = win32security.GetNamedSecurityInfo( - test_path, win32security.SE_FILE_OBJECT, win32security.DACL_SECURITY_INFORMATION + path, win32security.SE_FILE_OBJECT, win32security.DACL_SECURITY_INFORMATION ) acl = security_descriptor.GetSecurityDescriptorDacl() return acl, sid @@ -64,7 +64,7 @@ def test_create_secure_directory__perm_linux(test_path): def test_create_secure_directory__perm_windows(test_path): create_secure_directory(test_path) - acl, user_sid = _get_acl_and_sid_from_path() + acl, user_sid = _get_acl_and_sid_from_path(test_path) assert acl.GetAceCount() == 1 @@ -99,7 +99,7 @@ def test_create_secure_file__perm_linux(test_path): def test_create_secure_file__perm_windows(test_path): create_secure_file(test_path) - acl, user_sid = _get_acl_and_sid_from_path() + acl, user_sid = _get_acl_and_sid_from_path(test_path) assert acl.GetAceCount() == 1 From 1170b176d3e00ad1b0d93ee375459df1745550d8 Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Mon, 14 Jun 2021 23:41:56 +0530 Subject: [PATCH 13/31] island: Fix Windows' secure file creation by using a different file flag --- monkey/monkey_island/cc/environment/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index 86370e01f..558b21b20 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -81,7 +81,7 @@ def _create_secure_file_windows(path: str): windows_permissions.get_security_descriptor_for_owner_only_perms() ) file_creation = win32file.CREATE_NEW # fails if file exists - file_attributes = win32file.FILE_ATTRIBUTE_NORMAL + file_attributes = win32file.FILE_FLAG_BACKUP_SEMANTICS win32file.CloseHandle(win32file.CreateFile( path, From 443b66e9d9f404e808ce0c91f3bb126f184ae0d6 Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Mon, 14 Jun 2021 23:43:05 +0530 Subject: [PATCH 14/31] tests: Remove accidental code in `_get_acl_and_sid_from_path()` in test_utils.py --- .../tests/unit_tests/monkey_island/cc/environment/test_utils.py | 2 -- 1 file changed, 2 deletions(-) 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 90b65c4ec..c3a8b39b1 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 @@ -32,8 +32,6 @@ def test_path(tmpdir): def _get_acl_and_sid_from_path(path: str): - create_secure_file(path) - sid, _, _ = win32security.LookupAccountName("", win32api.GetUserName()) security_descriptor = win32security.GetNamedSecurityInfo( path, win32security.SE_FILE_OBJECT, win32security.DACL_SECURITY_INFORMATION From 5ea046eda531656d8ff1a346a86bd5936bbbd9db Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Tue, 15 Jun 2021 00:06:40 +0530 Subject: [PATCH 15/31] island: Format cc/environment/utils.py with black --- monkey/monkey_island/cc/environment/utils.py | 24 +++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index 558b21b20..a9b312092 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -83,17 +83,19 @@ def _create_secure_file_windows(path: str): file_creation = win32file.CREATE_NEW # fails if file exists file_attributes = win32file.FILE_FLAG_BACKUP_SEMANTICS - win32file.CloseHandle(win32file.CreateFile( - path, - file_access, - file_sharing, - security_attributes, - file_creation, - file_attributes, - win32job.CreateJobObject( - None, "" - ), # https://stackoverflow.com/questions/46800142/in-python-with-pywin32-win32job-the-createjobobject-function-how-do-i-pass-nu # noqa: E501 - )) + win32file.CloseHandle( + win32file.CreateFile( + path, + file_access, + file_sharing, + security_attributes, + file_creation, + file_attributes, + win32job.CreateJobObject( + None, "" + ), # https://stackoverflow.com/questions/46800142/in-python-with-pywin32-win32job-the-createjobobject-function-how-do-i-pass-nu # noqa: E501 + ) + ) except Exception as ex: LOG.error(f'Could not create a file at "{path}": {str(ex)}') From d7565fc515131d0c059217f6a6858f05fc77cd8c Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Tue, 15 Jun 2021 12:20:48 +0530 Subject: [PATCH 16/31] island: Use stat.S_IRWXU in place of 0o700 in cc/environment/utils.py --- monkey/monkey_island/cc/environment/utils.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py index a9b312092..94943106a 100644 --- a/monkey/monkey_island/cc/environment/utils.py +++ b/monkey/monkey_island/cc/environment/utils.py @@ -1,6 +1,7 @@ import logging import os import platform +import stat def is_windows_os() -> bool: @@ -29,7 +30,7 @@ def _create_secure_directory_linux(path: str): try: # 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) + os.mkdir(path, mode=stat.S_IRWXU) except Exception as ex: LOG.error(f'Could not create a directory at "{path}": {str(ex)}') @@ -62,7 +63,7 @@ def _create_secure_file_linux(path: str): flags = ( os.O_RDWR | os.O_CREAT | os.O_EXCL ) # read/write, create new, throw error if file exists - mode = 0o700 # read/write/execute permissions to owner + mode = stat.S_IRWXU # read/write/execute permissions to owner os.close(os.open(path, flags, mode)) except Exception as ex: From 91873343dd94dbbb86ffcbf67feaf57cb5a833fe Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Tue, 15 Jun 2021 12:26:33 +0530 Subject: [PATCH 17/31] tests: Add comment to `test_create_secure_directory__perm_windows()` explaining when it fails --- .../tests/unit_tests/monkey_island/cc/environment/test_utils.py | 2 +- 1 file changed, 1 insertion(+), 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 c3a8b39b1..2c4262276 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 @@ -78,7 +78,7 @@ def test_create_secure_directory__perm_windows(test_path): def test_create_secure_file__already_created(test_path): os.close(os.open(test_path, os.O_CREAT, 0o700)) assert os.path.isfile(test_path) - create_secure_file(test_path) + create_secure_file(test_path) # test fails if any exceptions are thrown def test_create_secure_file__no_parent_dir(test_path_nested): From b5f092a85c20c103eaab3755fad3c25f2a247f68 Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Tue, 15 Jun 2021 12:46:18 +0530 Subject: [PATCH 18/31] island: Move code from cc/environment/utils.py to cc/server_utils/file_utils.py --- monkey/monkey_island/cc/environment/utils.py | 103 ------------------ .../monkey_island/cc/server_utils/consts.py | 2 +- .../cc/server_utils/encryptor.py | 2 +- .../cc/server_utils/file_utils.py | 102 +++++++++++++++++ .../windows_permissions.py | 0 monkey/monkey_island/cc/setup/config_setup.py | 2 +- .../cc/setup/mongo/mongo_setup.py | 2 +- 7 files changed, 106 insertions(+), 107 deletions(-) delete mode 100644 monkey/monkey_island/cc/environment/utils.py rename monkey/monkey_island/cc/{environment => server_utils}/windows_permissions.py (100%) diff --git a/monkey/monkey_island/cc/environment/utils.py b/monkey/monkey_island/cc/environment/utils.py deleted file mode 100644 index 94943106a..000000000 --- a/monkey/monkey_island/cc/environment/utils.py +++ /dev/null @@ -1,103 +0,0 @@ -import logging -import os -import platform -import stat - - -def is_windows_os() -> bool: - return platform.system() == "Windows" - - -if is_windows_os(): - import win32file - import win32job - import win32security - - import monkey_island.cc.environment.windows_permissions as windows_permissions - -LOG = logging.getLogger(__name__) - - -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) - - -def _create_secure_directory_linux(path: str): - try: - # Don't split directory creation and permission setting - # because it will temporarily create an accessible directory which anyone can use. - os.mkdir(path, mode=stat.S_IRWXU) - - except Exception as ex: - LOG.error(f'Could not create a directory at "{path}": {str(ex)}') - raise ex - - -def _create_secure_directory_windows(path: str): - try: - security_attributes = win32security.SECURITY_ATTRIBUTES() - security_attributes.SECURITY_DESCRIPTOR = ( - windows_permissions.get_security_descriptor_for_owner_only_perms() - ) - win32file.CreateDirectory(path, security_attributes) - - except Exception as ex: - LOG.error(f'Could not create a directory at "{path}": {str(ex)}') - raise ex - - -def create_secure_file(path: str): - if not os.path.isfile(path): - if is_windows_os(): - _create_secure_file_windows(path) - else: - _create_secure_file_linux(path) - - -def _create_secure_file_linux(path: str): - try: - flags = ( - os.O_RDWR | os.O_CREAT | os.O_EXCL - ) # read/write, create new, throw error if file exists - mode = stat.S_IRWXU # read/write/execute permissions to owner - os.close(os.open(path, flags, mode)) - - except Exception as ex: - LOG.error(f'Could not create a file at "{path}": {str(ex)}') - raise ex - - -def _create_secure_file_windows(path: str): - try: - file_access = win32file.GENERIC_READ | win32file.GENERIC_WRITE - file_sharing = ( - win32file.FILE_SHARE_READ - ) # subsequent open operations on the object will succeed only if read access is requested - security_attributes = win32security.SECURITY_ATTRIBUTES() - security_attributes.SECURITY_DESCRIPTOR = ( - windows_permissions.get_security_descriptor_for_owner_only_perms() - ) - file_creation = win32file.CREATE_NEW # fails if file exists - file_attributes = win32file.FILE_FLAG_BACKUP_SEMANTICS - - win32file.CloseHandle( - win32file.CreateFile( - path, - file_access, - file_sharing, - security_attributes, - file_creation, - file_attributes, - win32job.CreateJobObject( - None, "" - ), # https://stackoverflow.com/questions/46800142/in-python-with-pywin32-win32job-the-createjobobject-function-how-do-i-pass-nu # noqa: E501 - ) - ) - - except Exception as ex: - LOG.error(f'Could not create a file at "{path}": {str(ex)}') - raise ex diff --git a/monkey/monkey_island/cc/server_utils/consts.py b/monkey/monkey_island/cc/server_utils/consts.py index ef5d0733c..cfa426d93 100644 --- a/monkey/monkey_island/cc/server_utils/consts.py +++ b/monkey/monkey_island/cc/server_utils/consts.py @@ -1,7 +1,7 @@ import os from pathlib import Path -from monkey_island.cc.environment.utils import is_windows_os +from monkey_island.cc.server_utils.file_utils import is_windows_os from monkey_island.cc.server_utils import file_utils __author__ = "itay.mizeretz" diff --git a/monkey/monkey_island/cc/server_utils/encryptor.py b/monkey/monkey_island/cc/server_utils/encryptor.py index cfa5b751c..abeb34dc3 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.environment.utils import create_secure_file +from monkey_island.cc.server_utils.file_utils import create_secure_file __author__ = "itay.mizeretz" diff --git a/monkey/monkey_island/cc/server_utils/file_utils.py b/monkey/monkey_island/cc/server_utils/file_utils.py index 225fb8732..a495fb5f0 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -1,5 +1,107 @@ import os +import logging +import platform +import stat + +LOG = logging.getLogger(__name__) + + +def is_windows_os() -> bool: + return platform.system() == "Windows" + + +if is_windows_os(): + import win32file + import win32job + import win32security + + import monkey_island.cc.server_utils.windows_permissions as windows_permissions def expand_path(path: str) -> str: return os.path.expandvars(os.path.expanduser(path)) + + +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) + + +def _create_secure_directory_linux(path: str): + try: + # Don't split directory creation and permission setting + # because it will temporarily create an accessible directory which anyone can use. + os.mkdir(path, mode=stat.S_IRWXU) + + except Exception as ex: + LOG.error(f'Could not create a directory at "{path}": {str(ex)}') + raise ex + + +def _create_secure_directory_windows(path: str): + try: + security_attributes = win32security.SECURITY_ATTRIBUTES() + security_attributes.SECURITY_DESCRIPTOR = ( + windows_permissions.get_security_descriptor_for_owner_only_perms() + ) + win32file.CreateDirectory(path, security_attributes) + + except Exception as ex: + LOG.error(f'Could not create a directory at "{path}": {str(ex)}') + raise ex + + +def create_secure_file(path: str): + if not os.path.isfile(path): + if is_windows_os(): + _create_secure_file_windows(path) + else: + _create_secure_file_linux(path) + + +def _create_secure_file_linux(path: str): + try: + flags = ( + os.O_RDWR | os.O_CREAT | os.O_EXCL + ) # read/write, create new, throw error if file exists + mode = stat.S_IRWXU # read/write/execute permissions to owner + os.close(os.open(path, flags, mode)) + + except Exception as ex: + LOG.error(f'Could not create a file at "{path}": {str(ex)}') + raise ex + + +def _create_secure_file_windows(path: str): + try: + file_access = win32file.GENERIC_READ | win32file.GENERIC_WRITE + file_sharing = ( + win32file.FILE_SHARE_READ + ) # subsequent open operations on the object will succeed only if read access is requested + security_attributes = win32security.SECURITY_ATTRIBUTES() + security_attributes.SECURITY_DESCRIPTOR = ( + windows_permissions.get_security_descriptor_for_owner_only_perms() + ) + file_creation = win32file.CREATE_NEW # fails if file exists + file_attributes = win32file.FILE_FLAG_BACKUP_SEMANTICS + + win32file.CloseHandle( + win32file.CreateFile( + path, + file_access, + file_sharing, + security_attributes, + file_creation, + file_attributes, + win32job.CreateJobObject( + None, "" + ), # https://stackoverflow.com/questions/46800142/in-python-with-pywin32-win32job-the-createjobobject-function-how-do-i-pass-nu # noqa: E501 + ) + ) + + except Exception as ex: + LOG.error(f'Could not create a file at "{path}": {str(ex)}') + raise ex diff --git a/monkey/monkey_island/cc/environment/windows_permissions.py b/monkey/monkey_island/cc/server_utils/windows_permissions.py similarity index 100% rename from monkey/monkey_island/cc/environment/windows_permissions.py rename to monkey/monkey_island/cc/server_utils/windows_permissions.py diff --git a/monkey/monkey_island/cc/setup/config_setup.py b/monkey/monkey_island/cc/setup/config_setup.py index 103137a91..ef965e560 100644 --- a/monkey/monkey_island/cc/setup/config_setup.py +++ b/monkey/monkey_island/cc/setup/config_setup.py @@ -2,7 +2,7 @@ from typing import Tuple from monkey_island.cc.arg_parser import IslandCmdArgs from monkey_island.cc.environment import server_config_handler -from monkey_island.cc.environment.utils import create_secure_directory +from monkey_island.cc.server_utils.file_utils import create_secure_directory from monkey_island.cc.server_utils import file_utils from monkey_island.cc.server_utils.consts import DEFAULT_SERVER_CONFIG_PATH from monkey_island.cc.setup.island_config_options import IslandConfigOptions diff --git a/monkey/monkey_island/cc/setup/mongo/mongo_setup.py b/monkey/monkey_island/cc/setup/mongo/mongo_setup.py index 0ab8ca0c0..02e097c68 100644 --- a/monkey/monkey_island/cc/setup/mongo/mongo_setup.py +++ b/monkey/monkey_island/cc/setup/mongo/mongo_setup.py @@ -5,7 +5,7 @@ import sys import time from monkey_island.cc.database import get_db_version, is_db_server_up -from monkey_island.cc.environment.utils import create_secure_directory +from monkey_island.cc.server_utils.file_utils import create_secure_directory from monkey_island.cc.setup.mongo import mongo_connector from monkey_island.cc.setup.mongo.mongo_connector import MONGO_DB_HOST, MONGO_DB_NAME, MONGO_DB_PORT from monkey_island.cc.setup.mongo.mongo_db_process import MongoDbProcess From 5abcadc69a90c7e05c307348eeb1f54f1535f7ac Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Tue, 15 Jun 2021 12:47:34 +0530 Subject: [PATCH 19/31] tests: Move tests from test_utils.py to test_file_utils.py --- .../cc/environment/test_utils.py | 110 ----------------- .../cc/server_utils/test_file_utils.py | 115 +++++++++++++++++- 2 files changed, 112 insertions(+), 113 deletions(-) delete mode 100644 monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py 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 deleted file mode 100644 index 2c4262276..000000000 --- a/monkey/tests/unit_tests/monkey_island/cc/environment/test_utils.py +++ /dev/null @@ -1,110 +0,0 @@ -import os -import stat - -import pytest - -from monkey_island.cc.environment.utils import ( - create_secure_directory, - create_secure_file, - is_windows_os, -) - -if is_windows_os(): - import win32api - import win32security - - FULL_CONTROL = 2032127 - ACE_TYPE_ALLOW = 0 - - -@pytest.fixture -def test_path_nested(tmpdir): - path = os.path.join(tmpdir, "test1", "test2", "test3") - return path - - -@pytest.fixture -def test_path(tmpdir): - test_path = "test1" - path = os.path.join(tmpdir, test_path) - - return path - - -def _get_acl_and_sid_from_path(path: str): - sid, _, _ = win32security.LookupAccountName("", win32api.GetUserName()) - security_descriptor = win32security.GetNamedSecurityInfo( - path, win32security.SE_FILE_OBJECT, win32security.DACL_SECURITY_INFORMATION - ) - acl = security_descriptor.GetSecurityDescriptorDacl() - return acl, sid - - -def test_create_secure_directory__already_created(test_path): - os.mkdir(test_path) - assert os.path.isdir(test_path) - 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) - - -@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) - assert (st.st_mode & 0o777) == 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): - create_secure_directory(test_path) - - acl, user_sid = _get_acl_and_sid_from_path(test_path) - - assert acl.GetAceCount() == 1 - - ace = acl.GetAce(0) - ace_type, _ = ace[0] # 0 for allow, 1 for deny - permissions = ace[1] - sid = ace[-1] - - assert sid == user_sid - assert permissions == FULL_CONTROL and ace_type == ACE_TYPE_ALLOW - - -def test_create_secure_file__already_created(test_path): - os.close(os.open(test_path, os.O_CREAT, 0o700)) - assert os.path.isfile(test_path) - create_secure_file(test_path) # test fails if any exceptions are thrown - - -def test_create_secure_file__no_parent_dir(test_path_nested): - with pytest.raises(Exception): - create_secure_file(test_path_nested) - - -@pytest.mark.skipif(is_windows_os(), reason="Tests Posix (not Windows) permissions.") -def test_create_secure_file__perm_linux(test_path): - create_secure_file(test_path) - st = os.stat(test_path) - assert (st.st_mode & 0o777) == stat.S_IRWXU - - -@pytest.mark.skipif(not is_windows_os(), reason="Tests Windows (not Posix) permissions.") -def test_create_secure_file__perm_windows(test_path): - create_secure_file(test_path) - - acl, user_sid = _get_acl_and_sid_from_path(test_path) - - assert acl.GetAceCount() == 1 - - ace = acl.GetAce(0) - ace_type, _ = ace[0] # 0 for allow, 1 for deny - permissions = ace[1] - sid = ace[-1] - - assert sid == user_sid - assert permissions == FULL_CONTROL and ace_type == ACE_TYPE_ALLOW 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 cff716135..4af793353 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,17 +1,126 @@ import os -from monkey_island.cc.server_utils import file_utils +import stat + +import pytest + +from monkey_island.cc.server_utils.file_utils import ( + create_secure_directory, + create_secure_file, + is_windows_os, + expand_path +) + +if is_windows_os(): + import win32api + import win32security + + FULL_CONTROL = 2032127 + ACE_TYPE_ALLOW = 0 + def test_expand_user(patched_home_env): input_path = os.path.join("~", "test") expected_path = os.path.join(patched_home_env, "test") - assert file_utils.expand_path(input_path) == expected_path + assert expand_path(input_path) == expected_path def test_expand_vars(patched_home_env): input_path = os.path.join("$HOME", "test") expected_path = os.path.join(patched_home_env, "test") - assert file_utils.expand_path(input_path) == expected_path + assert expand_path(input_path) == expected_path + +@pytest.fixture +def test_path_nested(tmpdir): + path = os.path.join(tmpdir, "test1", "test2", "test3") + return path + + +@pytest.fixture +def test_path(tmpdir): + test_path = "test1" + path = os.path.join(tmpdir, test_path) + + return path + + +def _get_acl_and_sid_from_path(path: str): + sid, _, _ = win32security.LookupAccountName("", win32api.GetUserName()) + security_descriptor = win32security.GetNamedSecurityInfo( + path, win32security.SE_FILE_OBJECT, win32security.DACL_SECURITY_INFORMATION + ) + acl = security_descriptor.GetSecurityDescriptorDacl() + return acl, sid + + +def test_create_secure_directory__already_created(test_path): + os.mkdir(test_path) + assert os.path.isdir(test_path) + 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) + + +@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) + assert (st.st_mode & 0o777) == 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): + create_secure_directory(test_path) + + acl, user_sid = _get_acl_and_sid_from_path(test_path) + + assert acl.GetAceCount() == 1 + + ace = acl.GetAce(0) + ace_type, _ = ace[0] # 0 for allow, 1 for deny + permissions = ace[1] + sid = ace[-1] + + assert sid == user_sid + assert permissions == FULL_CONTROL and ace_type == ACE_TYPE_ALLOW + + +def test_create_secure_file__already_created(test_path): + os.close(os.open(test_path, os.O_CREAT, 0o700)) + assert os.path.isfile(test_path) + create_secure_file(test_path) # test fails if any exceptions are thrown + + +def test_create_secure_file__no_parent_dir(test_path_nested): + with pytest.raises(Exception): + create_secure_file(test_path_nested) + + +@pytest.mark.skipif(is_windows_os(), reason="Tests Posix (not Windows) permissions.") +def test_create_secure_file__perm_linux(test_path): + create_secure_file(test_path) + st = os.stat(test_path) + assert (st.st_mode & 0o777) == stat.S_IRWXU + + +@pytest.mark.skipif(not is_windows_os(), reason="Tests Windows (not Posix) permissions.") +def test_create_secure_file__perm_windows(test_path): + create_secure_file(test_path) + + acl, user_sid = _get_acl_and_sid_from_path(test_path) + + assert acl.GetAceCount() == 1 + + ace = acl.GetAce(0) + ace_type, _ = ace[0] # 0 for allow, 1 for deny + permissions = ace[1] + sid = ace[-1] + + assert sid == user_sid + assert permissions == FULL_CONTROL and ace_type == ACE_TYPE_ALLOW From e01165403ac47dfe8314ec4a23a26c225aa46940 Mon Sep 17 00:00:00 2001 From: shreyamalviya Date: Tue, 15 Jun 2021 12:51:10 +0530 Subject: [PATCH 20/31] island, tests: Run isort and black on previously changed files --- monkey/monkey_island/cc/server_utils/consts.py | 2 +- monkey/monkey_island/cc/server_utils/file_utils.py | 2 +- monkey/monkey_island/cc/setup/config_setup.py | 2 +- .../monkey_island/cc/server_utils/test_file_utils.py | 5 ++--- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/consts.py b/monkey/monkey_island/cc/server_utils/consts.py index cfa426d93..13e0c66e4 100644 --- a/monkey/monkey_island/cc/server_utils/consts.py +++ b/monkey/monkey_island/cc/server_utils/consts.py @@ -1,8 +1,8 @@ import os from pathlib import Path -from monkey_island.cc.server_utils.file_utils import is_windows_os from monkey_island.cc.server_utils import file_utils +from monkey_island.cc.server_utils.file_utils import is_windows_os __author__ = "itay.mizeretz" diff --git a/monkey/monkey_island/cc/server_utils/file_utils.py b/monkey/monkey_island/cc/server_utils/file_utils.py index a495fb5f0..368f49e95 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -1,5 +1,5 @@ -import os import logging +import os import platform import stat diff --git a/monkey/monkey_island/cc/setup/config_setup.py b/monkey/monkey_island/cc/setup/config_setup.py index ef965e560..657ffaac5 100644 --- a/monkey/monkey_island/cc/setup/config_setup.py +++ b/monkey/monkey_island/cc/setup/config_setup.py @@ -2,9 +2,9 @@ from typing import Tuple from monkey_island.cc.arg_parser import IslandCmdArgs from monkey_island.cc.environment import server_config_handler -from monkey_island.cc.server_utils.file_utils import create_secure_directory from monkey_island.cc.server_utils import file_utils from monkey_island.cc.server_utils.consts import DEFAULT_SERVER_CONFIG_PATH +from monkey_island.cc.server_utils.file_utils import create_secure_directory from monkey_island.cc.setup.island_config_options import IslandConfigOptions 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 4af793353..59b6f68a8 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,5 +1,4 @@ import os - import stat import pytest @@ -7,8 +6,8 @@ import pytest from monkey_island.cc.server_utils.file_utils import ( create_secure_directory, create_secure_file, + expand_path, is_windows_os, - expand_path ) if is_windows_os(): @@ -19,7 +18,6 @@ if is_windows_os(): ACE_TYPE_ALLOW = 0 - def test_expand_user(patched_home_env): input_path = os.path.join("~", "test") expected_path = os.path.join(patched_home_env, "test") @@ -33,6 +31,7 @@ def test_expand_vars(patched_home_env): assert expand_path(input_path) == expected_path + @pytest.fixture def test_path_nested(tmpdir): path = os.path.join(tmpdir, "test1", "test2", "test3") From e90bf526743b5324d4f2c67d83586db973ccedcd Mon Sep 17 00:00:00 2001 From: Shreya Date: Tue, 15 Jun 2021 15:51:02 +0530 Subject: [PATCH 21/31] island: Use `Path().touch()` instead of `os.open()` when securely creating a file on Linux --- monkey/monkey_island/cc/server_utils/file_utils.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/file_utils.py b/monkey/monkey_island/cc/server_utils/file_utils.py index 368f49e95..cb08be10f 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -2,6 +2,7 @@ import logging import os import platform import stat +from pathlib import Path LOG = logging.getLogger(__name__) @@ -64,11 +65,8 @@ def create_secure_file(path: str): def _create_secure_file_linux(path: str): try: - flags = ( - os.O_RDWR | os.O_CREAT | os.O_EXCL - ) # read/write, create new, throw error if file exists mode = stat.S_IRWXU # read/write/execute permissions to owner - os.close(os.open(path, flags, mode)) + Path(path).touch(mode=mode, exist_ok=False) except Exception as ex: LOG.error(f'Could not create a file at "{path}": {str(ex)}') From 8b2c3ef8a379ccf9379d38fe6f7a39ae591f66d4 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 15 Jun 2021 09:29:18 -0400 Subject: [PATCH 22/31] island: Remove execute bit from "secure" file creation --- monkey/monkey_island/cc/server_utils/file_utils.py | 2 +- .../monkey_island/cc/server_utils/test_file_utils.py | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/file_utils.py b/monkey/monkey_island/cc/server_utils/file_utils.py index cb08be10f..cb94065e4 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -65,7 +65,7 @@ def create_secure_file(path: str): def _create_secure_file_linux(path: str): try: - mode = stat.S_IRWXU # read/write/execute permissions to owner + mode = stat.S_IRUSR | stat.S_IWUSR Path(path).touch(mode=mode, exist_ok=False) except Exception as 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 59b6f68a8..0c5b7348e 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 @@ -91,7 +91,7 @@ def test_create_secure_directory__perm_windows(test_path): def test_create_secure_file__already_created(test_path): - os.close(os.open(test_path, os.O_CREAT, 0o700)) + os.close(os.open(test_path, os.O_CREAT, stat.S_IRWXU)) assert os.path.isfile(test_path) create_secure_file(test_path) # test fails if any exceptions are thrown @@ -105,7 +105,11 @@ def test_create_secure_file__no_parent_dir(test_path_nested): def test_create_secure_file__perm_linux(test_path): create_secure_file(test_path) st = os.stat(test_path) - assert (st.st_mode & 0o777) == stat.S_IRWXU + + expected_mode = stat.S_IRUSR | stat.S_IWUSR + actual_mode = st.st_mode & (stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO) + + assert expected_mode == actual_mode @pytest.mark.skipif(not is_windows_os(), reason="Tests Windows (not Posix) permissions.") From 6b4a0906c0ab320f7a35c1bb379b6eb25ed87849 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 15 Jun 2021 09:31:22 -0400 Subject: [PATCH 23/31] island: use constants for permissions mode in test_file_utils.py --- .../monkey_island/cc/server_utils/test_file_utils.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 0c5b7348e..ddb4c4936 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 @@ -70,7 +70,11 @@ def test_create_secure_directory__no_parent_dir(test_path_nested): def test_create_secure_directory__perm_linux(test_path): create_secure_directory(test_path) st = os.stat(test_path) - assert (st.st_mode & 0o777) == stat.S_IRWXU + + expected_mode = stat.S_IRWXU + actual_mode = st.st_mode & (stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO) + + assert expected_mode == actual_mode @pytest.mark.skipif(not is_windows_os(), reason="Tests Windows (not Posix) permissions.") From 14371f3fbab5fc07e32b94047c665a5b381cf2bb Mon Sep 17 00:00:00 2001 From: Shreya Date: Tue, 15 Jun 2021 18:55:15 +0530 Subject: [PATCH 24/31] island: Return file descriptor when creating secure file --- .../cc/server_utils/encryptor.py | 5 +-- .../cc/server_utils/file_utils.py | 42 ++++++++++--------- 2 files changed, 25 insertions(+), 22 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/encryptor.py b/monkey/monkey_island/cc/server_utils/encryptor.py index abeb34dc3..4f376532c 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 create_secure_file +from monkey_island.cc.server_utils.file_utils import get_file_descriptor_for_new_secure_file __author__ = "itay.mizeretz" @@ -23,12 +23,11 @@ class Encryptor: if os.path.exists(password_file): self._load_existing_key(password_file) else: - create_secure_file(path=password_file) self._init_key(password_file) def _init_key(self, password_file): self._cipher_key = Random.new().read(self._BLOCK_SIZE) - with open(password_file, "wb") as f: + with open(get_file_descriptor_for_new_secure_file(path=password_file)) 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 cb94065e4..8681bcc54 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -2,7 +2,6 @@ import logging import os import platform import stat -from pathlib import Path LOG = logging.getLogger(__name__) @@ -55,25 +54,30 @@ def _create_secure_directory_windows(path: str): raise ex -def create_secure_file(path: str): +def get_file_descriptor_for_new_secure_file(path: str): if not os.path.isfile(path): if is_windows_os(): - _create_secure_file_windows(path) + return _get_file_descriptor_for_new_secure_file_windows(path) else: - _create_secure_file_linux(path) + return _get_file_descriptor_for_new_secure_file_linux(path) -def _create_secure_file_linux(path: str): +def _get_file_descriptor_for_new_secure_file_linux(path: str): try: mode = stat.S_IRUSR | stat.S_IWUSR - Path(path).touch(mode=mode, exist_ok=False) + flags = ( + os.O_RDWR | os.O_CREAT | os.O_EXCL + ) # read/write, create new, throw error if file exists + fd = os.open(path, flags, mode) + + return fd except Exception as ex: LOG.error(f'Could not create a file at "{path}": {str(ex)}') raise ex -def _create_secure_file_windows(path: str): +def _get_file_descriptor_for_new_secure_file_windows(path: str): try: file_access = win32file.GENERIC_READ | win32file.GENERIC_WRITE file_sharing = ( @@ -86,20 +90,20 @@ def _create_secure_file_windows(path: str): file_creation = win32file.CREATE_NEW # fails if file exists file_attributes = win32file.FILE_FLAG_BACKUP_SEMANTICS - win32file.CloseHandle( - win32file.CreateFile( - path, - file_access, - file_sharing, - security_attributes, - file_creation, - file_attributes, - win32job.CreateJobObject( - None, "" - ), # https://stackoverflow.com/questions/46800142/in-python-with-pywin32-win32job-the-createjobobject-function-how-do-i-pass-nu # noqa: E501 - ) + fd = win32file.CreateFile( + path, + file_access, + file_sharing, + security_attributes, + file_creation, + file_attributes, + win32job.CreateJobObject( + None, "" + ), # https://stackoverflow.com/questions/46800142/in-python-with-pywin32-win32job-the-createjobobject-function-how-do-i-pass-nu # noqa: E501 ) + return fd + except Exception as ex: LOG.error(f'Could not create a file at "{path}": {str(ex)}') raise ex From b648452b5f1c5869de8b8573487cf6307a9068c5 Mon Sep 17 00:00:00 2001 From: Shreya Date: Tue, 15 Jun 2021 18:57:18 +0530 Subject: [PATCH 25/31] island: Fix comment and statement formatting in file_utils.py --- monkey/monkey_island/cc/server_utils/file_utils.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/file_utils.py b/monkey/monkey_island/cc/server_utils/file_utils.py index 8681bcc54..4b9d4c708 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -80,9 +80,8 @@ def _get_file_descriptor_for_new_secure_file_linux(path: str): def _get_file_descriptor_for_new_secure_file_windows(path: str): try: file_access = win32file.GENERIC_READ | win32file.GENERIC_WRITE - file_sharing = ( - win32file.FILE_SHARE_READ - ) # subsequent open operations on the object will succeed only if read access is requested + # subsequent open operations on the object will succeed only if read access is requested + file_sharing = win32file.FILE_SHARE_READ security_attributes = win32security.SECURITY_ATTRIBUTES() security_attributes.SECURITY_DESCRIPTOR = ( windows_permissions.get_security_descriptor_for_owner_only_perms() From 37889d0b87df8e7c9be2f6273adad7ed18114b11 Mon Sep 17 00:00:00 2001 From: Shreya Date: Tue, 15 Jun 2021 19:00:12 +0530 Subject: [PATCH 26/31] island: Extract code to `_get_null_value_for_win32()` in file_utils.py --- monkey/monkey_island/cc/server_utils/file_utils.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/file_utils.py b/monkey/monkey_island/cc/server_utils/file_utils.py index 4b9d4c708..0a9223d60 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -96,9 +96,7 @@ def _get_file_descriptor_for_new_secure_file_windows(path: str): security_attributes, file_creation, file_attributes, - win32job.CreateJobObject( - None, "" - ), # https://stackoverflow.com/questions/46800142/in-python-with-pywin32-win32job-the-createjobobject-function-how-do-i-pass-nu # noqa: E501 + _get_null_value_for_win32(), ) return fd @@ -106,3 +104,8 @@ def _get_file_descriptor_for_new_secure_file_windows(path: str): except Exception as ex: LOG.error(f'Could not create a file at "{path}": {str(ex)}') raise ex + + +def _get_null_value_for_win32() -> None: + # 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 22c3c5a11bd779f8d676cb4e5214c8e03422d169 Mon Sep 17 00:00:00 2001 From: Shreya Date: Tue, 15 Jun 2021 19:09:02 +0530 Subject: [PATCH 27/31] tests: Fix secure file creation tests as per latest changes --- .../monkey_island/cc/server_utils/test_file_utils.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) 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 ddb4c4936..ac0c20abe 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 @@ -5,13 +5,14 @@ import pytest from monkey_island.cc.server_utils.file_utils import ( create_secure_directory, - create_secure_file, expand_path, + get_file_descriptor_for_new_secure_file, is_windows_os, ) if is_windows_os(): import win32api + import win32file import win32security FULL_CONTROL = 2032127 @@ -97,17 +98,18 @@ def test_create_secure_directory__perm_windows(test_path): def test_create_secure_file__already_created(test_path): os.close(os.open(test_path, os.O_CREAT, stat.S_IRWXU)) assert os.path.isfile(test_path) - create_secure_file(test_path) # test fails if any exceptions are thrown + # test fails if any exceptions are thrown + get_file_descriptor_for_new_secure_file(test_path) def test_create_secure_file__no_parent_dir(test_path_nested): with pytest.raises(Exception): - create_secure_file(test_path_nested) + get_file_descriptor_for_new_secure_file(test_path_nested) @pytest.mark.skipif(is_windows_os(), reason="Tests Posix (not Windows) permissions.") def test_create_secure_file__perm_linux(test_path): - create_secure_file(test_path) + os.close(get_file_descriptor_for_new_secure_file(test_path)) st = os.stat(test_path) expected_mode = stat.S_IRUSR | stat.S_IWUSR @@ -118,7 +120,7 @@ def test_create_secure_file__perm_linux(test_path): @pytest.mark.skipif(not is_windows_os(), reason="Tests Windows (not Posix) permissions.") def test_create_secure_file__perm_windows(test_path): - create_secure_file(test_path) + win32file.CloseHandle(get_file_descriptor_for_new_secure_file(test_path)) acl, user_sid = _get_acl_and_sid_from_path(test_path) From 64ac1fe7061a0994a69048ee308fe8dd00f58030 Mon Sep 17 00:00:00 2001 From: Shreya Date: Tue, 15 Jun 2021 19:11:57 +0530 Subject: [PATCH 28/31] island: Add type hinting in file_utils.py --- monkey/monkey_island/cc/server_utils/file_utils.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/file_utils.py b/monkey/monkey_island/cc/server_utils/file_utils.py index 0a9223d60..25161ca2f 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -54,7 +54,7 @@ def _create_secure_directory_windows(path: str): raise ex -def get_file_descriptor_for_new_secure_file(path: str): +def get_file_descriptor_for_new_secure_file(path: str) -> int: if not os.path.isfile(path): if is_windows_os(): return _get_file_descriptor_for_new_secure_file_windows(path) @@ -62,7 +62,7 @@ def get_file_descriptor_for_new_secure_file(path: str): return _get_file_descriptor_for_new_secure_file_linux(path) -def _get_file_descriptor_for_new_secure_file_linux(path: str): +def _get_file_descriptor_for_new_secure_file_linux(path: str) -> int: try: mode = stat.S_IRUSR | stat.S_IWUSR flags = ( @@ -77,7 +77,7 @@ def _get_file_descriptor_for_new_secure_file_linux(path: str): raise ex -def _get_file_descriptor_for_new_secure_file_windows(path: str): +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 From 80bfd9007445609ab03cd8b042bf60a4f8443857 Mon Sep 17 00:00:00 2001 From: Shreya Date: Tue, 15 Jun 2021 19:29:49 +0530 Subject: [PATCH 29/31] island: Specify mode to open new secure file in, in encryptor.py --- monkey/monkey_island/cc/server_utils/encryptor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/monkey/monkey_island/cc/server_utils/encryptor.py b/monkey/monkey_island/cc/server_utils/encryptor.py index 4f376532c..83292be8a 100644 --- a/monkey/monkey_island/cc/server_utils/encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryptor.py @@ -27,7 +27,7 @@ class Encryptor: def _init_key(self, password_file): self._cipher_key = Random.new().read(self._BLOCK_SIZE) - with open(get_file_descriptor_for_new_secure_file(path=password_file)) as f: + with open(get_file_descriptor_for_new_secure_file(path=password_file), "wb") as f: f.write(self._cipher_key) def _load_existing_key(self, password_file): From 327ff7a626244a3b058dd40c0a00dc6fcb37841e Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 15 Jun 2021 13:12:18 -0400 Subject: [PATCH 30/31] island: Remove isfile() check from get_file_descriptor_for_new_secure_file() get_file_descriptor_for_new_secure_file() should return a file descriptor. If the file already exists, this function would return nothing, potentially causing issues with whatever relies on this function's output. --- monkey/monkey_island/cc/server_utils/file_utils.py | 9 ++++----- .../monkey_island/cc/server_utils/test_file_utils.py | 5 +++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/file_utils.py b/monkey/monkey_island/cc/server_utils/file_utils.py index 25161ca2f..995501c09 100644 --- a/monkey/monkey_island/cc/server_utils/file_utils.py +++ b/monkey/monkey_island/cc/server_utils/file_utils.py @@ -55,11 +55,10 @@ def _create_secure_directory_windows(path: str): def get_file_descriptor_for_new_secure_file(path: str) -> int: - if not os.path.isfile(path): - if is_windows_os(): - return _get_file_descriptor_for_new_secure_file_windows(path) - else: - return _get_file_descriptor_for_new_secure_file_linux(path) + if is_windows_os(): + return _get_file_descriptor_for_new_secure_file_windows(path) + else: + return _get_file_descriptor_for_new_secure_file_linux(path) 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 ac0c20abe..7e3e10ed6 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 @@ -98,8 +98,9 @@ def test_create_secure_directory__perm_windows(test_path): def test_create_secure_file__already_created(test_path): os.close(os.open(test_path, os.O_CREAT, stat.S_IRWXU)) assert os.path.isfile(test_path) - # test fails if any exceptions are thrown - get_file_descriptor_for_new_secure_file(test_path) + + with pytest.raises(Exception): + get_file_descriptor_for_new_secure_file(test_path) def test_create_secure_file__no_parent_dir(test_path_nested): From 44bdfa5508c76f87d1a8b016cb6e3dcbce53bef9 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 15 Jun 2021 13:14:31 -0400 Subject: [PATCH 31/31] island: Rename create_secure_file tests create_secure_file() was previously renamed to get_file_descriptor_for_new_secure_file(). --- .../monkey_island/cc/server_utils/test_file_utils.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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 7e3e10ed6..ab1c77ed1 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 @@ -56,7 +56,7 @@ def _get_acl_and_sid_from_path(path: str): return acl, sid -def test_create_secure_directory__already_created(test_path): +def test_create_secure_directory__already_exists(test_path): os.mkdir(test_path) assert os.path.isdir(test_path) create_secure_directory(test_path) @@ -95,7 +95,7 @@ def test_create_secure_directory__perm_windows(test_path): assert permissions == FULL_CONTROL and ace_type == ACE_TYPE_ALLOW -def test_create_secure_file__already_created(test_path): +def test_get_file_descriptor_for_new_secure_file__already_exists(test_path): os.close(os.open(test_path, os.O_CREAT, stat.S_IRWXU)) assert os.path.isfile(test_path) @@ -103,13 +103,13 @@ def test_create_secure_file__already_created(test_path): get_file_descriptor_for_new_secure_file(test_path) -def test_create_secure_file__no_parent_dir(test_path_nested): +def test_get_file_descriptor_for_new_secure_file__no_parent_dir(test_path_nested): with pytest.raises(Exception): get_file_descriptor_for_new_secure_file(test_path_nested) @pytest.mark.skipif(is_windows_os(), reason="Tests Posix (not Windows) permissions.") -def test_create_secure_file__perm_linux(test_path): +def test_get_file_descriptor_for_new_secure_file__perm_linux(test_path): os.close(get_file_descriptor_for_new_secure_file(test_path)) st = os.stat(test_path) @@ -120,7 +120,7 @@ def test_create_secure_file__perm_linux(test_path): @pytest.mark.skipif(not is_windows_os(), reason="Tests Windows (not Posix) permissions.") -def test_create_secure_file__perm_windows(test_path): +def test_get_file_descriptor_for_new_secure_file__perm_windows(test_path): win32file.CloseHandle(get_file_descriptor_for_new_secure_file(test_path)) acl, user_sid = _get_acl_and_sid_from_path(test_path)