From 08cd631c953ae6cbf8a019b40ee8073d8b39e97d Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Wed, 27 Apr 2022 09:52:02 -0400 Subject: [PATCH 1/5] Island: Make PBA_*_FILENAME_PATH tuples --- monkey/common/config_value_paths.py | 6 ++++-- monkey/monkey_island/cc/resources/pba_file_upload.py | 6 ++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/monkey/common/config_value_paths.py b/monkey/common/config_value_paths.py index c998f44fa..8f43960e0 100644 --- a/monkey/common/config_value_paths.py +++ b/monkey/common/config_value_paths.py @@ -10,5 +10,7 @@ SUBNET_SCAN_LIST_PATH = ["basic_network", "scope", "subnet_scan_list"] LOCAL_NETWORK_SCAN_PATH = ["basic_network", "scope", "local_network_scan"] LM_HASH_LIST_PATH = ["internal", "exploits", "exploit_lm_hash_list"] NTLM_HASH_LIST_PATH = ["internal", "exploits", "exploit_ntlm_hash_list"] -PBA_LINUX_FILENAME_PATH = ["monkey", "post_breach", "PBA_linux_filename"] -PBA_WINDOWS_FILENAME_PATH = ["monkey", "post_breach", "PBA_windows_filename"] + +# TODO: These are tuples so that they are immutable. Make the rest of these paths touples as well. +PBA_LINUX_FILENAME_PATH = ("monkey", "post_breach", "PBA_linux_filename") +PBA_WINDOWS_FILENAME_PATH = ("monkey", "post_breach", "PBA_windows_filename") diff --git a/monkey/monkey_island/cc/resources/pba_file_upload.py b/monkey/monkey_island/cc/resources/pba_file_upload.py index 2a27b324a..153c3774b 100644 --- a/monkey/monkey_island/cc/resources/pba_file_upload.py +++ b/monkey/monkey_island/cc/resources/pba_file_upload.py @@ -1,4 +1,3 @@ -import copy import logging from http import HTTPStatus @@ -43,10 +42,9 @@ class FileUpload(flask_restful.Resource): # Verify that file_name is indeed a file from config if target_os == LINUX_PBA_TYPE: - # TODO: Make these paths Tuples so we don't need to copy them - filename = ConfigService.get_config_value(copy.deepcopy(PBA_LINUX_FILENAME_PATH)) + filename = ConfigService.get_config_value(PBA_LINUX_FILENAME_PATH) else: - filename = ConfigService.get_config_value(copy.deepcopy(PBA_WINDOWS_FILENAME_PATH)) + filename = ConfigService.get_config_value(PBA_WINDOWS_FILENAME_PATH) try: file = self._file_storage_service.open_file(filename) From 1476efa38332e0b7ce46fb6579ff427d94c277c8 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Wed, 27 Apr 2022 12:44:58 -0400 Subject: [PATCH 2/5] Island: Change unsafe -> untrusted in log message --- .../monkey_island/cc/services/directory_file_storage_service.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/monkey/monkey_island/cc/services/directory_file_storage_service.py b/monkey/monkey_island/cc/services/directory_file_storage_service.py index a4be91522..48a037a68 100644 --- a/monkey/monkey_island/cc/services/directory_file_storage_service.py +++ b/monkey/monkey_island/cc/services/directory_file_storage_service.py @@ -66,7 +66,7 @@ class DirectoryFileStorageService(IFileStorageService): if self._storage_directory.resolve() not in safe_file_path.parents: raise ValueError(f"The file named {unsafe_file_name} can not be safely retrieved") - logger.debug(f"Unsafe file name {unsafe_file_name} sanitized: {safe_file_path}") + logger.debug(f"Untrusted file name {unsafe_file_name} sanitized: {safe_file_path}") return safe_file_path def delete_all_files(self): From 3a5d28cc59b1fe2c2e1cd8bf012285890922b1d1 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Wed, 27 Apr 2022 12:59:08 -0400 Subject: [PATCH 3/5] Island: Decouple FileUpload service from FilePond --- .../cc/resources/pba_file_upload.py | 38 ++++++------------- 1 file changed, 11 insertions(+), 27 deletions(-) diff --git a/monkey/monkey_island/cc/resources/pba_file_upload.py b/monkey/monkey_island/cc/resources/pba_file_upload.py index 153c3774b..cc6826f01 100644 --- a/monkey/monkey_island/cc/resources/pba_file_upload.py +++ b/monkey/monkey_island/cc/resources/pba_file_upload.py @@ -3,8 +3,7 @@ from http import HTTPStatus import flask_restful from flask import Response, make_response, request, send_file -from werkzeug.datastructures import FileStorage -from werkzeug.utils import secure_filename +from werkzeug.utils import secure_filename as sanitize_filename from common.config_value_paths import PBA_LINUX_FILENAME_PATH, PBA_WINDOWS_FILENAME_PATH from monkey_island.cc.resources.auth.auth import jwt_required @@ -21,19 +20,18 @@ WINDOWS_PBA_TYPE = "PBAwindows" class FileUpload(flask_restful.Resource): """ - File upload endpoint used to exchange files with filepond component on the front-end + File upload endpoint used to send/receive Custom PBA files """ def __init__(self, file_storage_service: IFileStorageService): self._file_storage_service = file_storage_service - # TODO: Fix references/coupling to filepond # TODO: Add comment explaining why this is basically a duplicate of the endpoint in the # PBAFileDownload resource. @jwt_required def get(self, target_os): """ - Sends file to filepond + Sends file to the requester :param target_os: Indicates which file to send, linux or windows :return: Returns file contents """ @@ -59,38 +57,24 @@ class FileUpload(flask_restful.Resource): @jwt_required def post(self, target_os): """ - Receives user's uploaded file from filepond + Receives user's uploaded file :param target_os: Type indicates which file was received, linux or windows :return: Returns flask response object with uploaded file's filename """ if self._is_target_os_supported(target_os): return Response(status=HTTPStatus.UNPROCESSABLE_ENTITY, mimetype="text/plain") - filename = self._upload_pba_file( - # TODO: This "filepond" string can be changed to be more generic in the `react-filepond` - # component. - request.files["filepond"], - (target_os == LINUX_PBA_TYPE), - ) - - response = Response(response=filename, status=200, mimetype="text/plain") - return response - - def _upload_pba_file(self, file_storage: FileStorage, is_linux=True): - """ - Uploads PBA file to island's file system - :param request_: Request object containing PBA file - :param is_linux: Boolean indicating if this file is for windows or for linux - :return: filename string - """ - filename = secure_filename(file_storage.filename) - self._file_storage_service.save_file(filename, file_storage.stream) + file_storage = next(request.files.values()) # For now, assume there's only one file + safe_filename = sanitize_filename(file_storage.filename) + self._file_storage_service.save_file(safe_filename, file_storage.stream) ConfigService.set_config_value( - (PBA_LINUX_FILENAME_PATH if is_linux else PBA_WINDOWS_FILENAME_PATH), filename + (PBA_LINUX_FILENAME_PATH if target_os == LINUX_PBA_TYPE else PBA_WINDOWS_FILENAME_PATH), + safe_filename, ) - return filename + response = Response(response=safe_filename, status=200, mimetype="text/plain") + return response @jwt_required def delete(self, target_os): From 13ca4b6f8c0f071fc6ba7bfe40545a7bdd536d8d Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Wed, 27 Apr 2022 13:19:16 -0400 Subject: [PATCH 4/5] Island: Add comment about seemingly duplicate endpoints --- monkey/monkey_island/cc/resources/pba_file_upload.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/monkey/monkey_island/cc/resources/pba_file_upload.py b/monkey/monkey_island/cc/resources/pba_file_upload.py index cc6826f01..233cf9e67 100644 --- a/monkey/monkey_island/cc/resources/pba_file_upload.py +++ b/monkey/monkey_island/cc/resources/pba_file_upload.py @@ -26,8 +26,10 @@ class FileUpload(flask_restful.Resource): def __init__(self, file_storage_service: IFileStorageService): self._file_storage_service = file_storage_service - # TODO: Add comment explaining why this is basically a duplicate of the endpoint in the - # PBAFileDownload resource. + # This endpoint is basically a duplicate of PBAFileDownload.get(). They serve slightly different + # purposes. This endpoint is authenticated, whereas the one in PBAFileDownload can not be (at + # the present time). In the future, consider whether or not they should be merged, or if they + # serve truly distinct purposes @jwt_required def get(self, target_os): """ From c4edec80a6581bb10e476697b949e2f4e37c8fcc Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Wed, 27 Apr 2022 19:08:37 -0400 Subject: [PATCH 5/5] Common: Fix typo touples -> tuples Co-authored-by: ilija-lazoroski --- monkey/common/config_value_paths.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/monkey/common/config_value_paths.py b/monkey/common/config_value_paths.py index 8f43960e0..5edb7a8b9 100644 --- a/monkey/common/config_value_paths.py +++ b/monkey/common/config_value_paths.py @@ -11,6 +11,6 @@ LOCAL_NETWORK_SCAN_PATH = ["basic_network", "scope", "local_network_scan"] LM_HASH_LIST_PATH = ["internal", "exploits", "exploit_lm_hash_list"] NTLM_HASH_LIST_PATH = ["internal", "exploits", "exploit_ntlm_hash_list"] -# TODO: These are tuples so that they are immutable. Make the rest of these paths touples as well. +# TODO: These are tuples so that they are immutable. Make the rest of these paths tuples as well. PBA_LINUX_FILENAME_PATH = ("monkey", "post_breach", "PBA_linux_filename") PBA_WINDOWS_FILENAME_PATH = ("monkey", "post_breach", "PBA_windows_filename")