From f60c2f1dde008cfa3ba8d3a50431647c469c3334 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 21 Jun 2022 13:16:00 -0400 Subject: [PATCH 1/8] Island: Add ValueError to IFileRepository docstring --- .../cc/repository/file_storage/i_file_repository.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py b/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py index bb9e345dc..237cc2c0d 100644 --- a/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py +++ b/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py @@ -20,6 +20,7 @@ class IFileRepository(metaclass=abc.ABCMeta): :param unsafe_file_name: An unsanitized file name that will identify the file :param file_contents: The data to be stored in the file + :raises ValueError: If the file name is an attempted directory traversal """ pass @@ -32,6 +33,7 @@ class IFileRepository(metaclass=abc.ABCMeta): :return: A file-like object providing access to the file's contents :raises FileNotFoundError: if the file does not exist :raises RetrievalError: if the file exists but cannot be retrieved + :raises ValueError: If the file name is an attempted directory traversal """ pass @@ -44,6 +46,7 @@ class IFileRepository(metaclass=abc.ABCMeta): idempotent and will succeed if the file to be deleted does not exist. :param unsafe_file_name: An unsanitized file name that identifies the file to be deleted + :raises ValueError: If the file name is an attempted directory traversal """ pass From 3446dbf0aa6611715cfc185fefe67a578a76402d Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 21 Jun 2022 13:18:35 -0400 Subject: [PATCH 2/8] Island: Export StorageError from monkey_island.cc.repository --- monkey/monkey_island/cc/repository/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/monkey/monkey_island/cc/repository/__init__.py b/monkey/monkey_island/cc/repository/__init__.py index 8fadb9ccf..3a8c552e3 100644 --- a/monkey/monkey_island/cc/repository/__init__.py +++ b/monkey/monkey_island/cc/repository/__init__.py @@ -1,4 +1,4 @@ -from .errors import RetrievalError +from .errors import RetrievalError, StorageError from .file_storage import FileNotFoundError, IFileRepository, LocalStorageFileRepository from .i_agent_binary_repository import IAgentBinaryRepository from .agent_binary_repository import AgentBinaryRepository From 63404c7bed2c0f3ffed701e92f747b3808059cc0 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 21 Jun 2022 13:19:56 -0400 Subject: [PATCH 3/8] Island: Raise StorageError in LocalStorageFileRepository.save_file() --- .../cc/repository/file_storage/i_file_repository.py | 1 + .../file_storage/local_storage_file_repository.py | 9 ++++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py b/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py index 237cc2c0d..73d3b4759 100644 --- a/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py +++ b/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py @@ -21,6 +21,7 @@ class IFileRepository(metaclass=abc.ABCMeta): :param unsafe_file_name: An unsanitized file name that will identify the file :param file_contents: The data to be stored in the file :raises ValueError: If the file name is an attempted directory traversal + :raises StorageError: If an error was encountered while attempting to store the file """ pass diff --git a/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py b/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py index cb18c6ba8..a766746ae 100644 --- a/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py +++ b/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py @@ -4,7 +4,7 @@ from pathlib import Path from typing import BinaryIO from common.utils.file_utils import get_all_regular_files_in_directory -from monkey_island.cc.repository import RetrievalError +from monkey_island.cc.repository import RetrievalError, StorageError from monkey_island.cc.server_utils.file_utils import create_secure_directory from . import IFileRepository, i_file_repository @@ -35,8 +35,11 @@ class LocalStorageFileRepository(IFileRepository): safe_file_path = self._get_safe_file_path(unsafe_file_name) logger.debug(f"Saving file contents to {safe_file_path}") - with open(safe_file_path, "wb") as dest: - shutil.copyfileobj(file_contents, dest) + try: + with open(safe_file_path, "wb") as dest: + shutil.copyfileobj(file_contents, dest) + except Exception as err: + raise StorageError(f"Error while attempting to store {unsafe_file_name}: {err}") def open_file(self, unsafe_file_name: str) -> BinaryIO: safe_file_path = self._get_safe_file_path(unsafe_file_name) From cd1481e4fe8f70f651c0c0ce52a75643d7dfd8e3 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 21 Jun 2022 13:22:29 -0400 Subject: [PATCH 4/8] Island: Add monkey_island.cc.repository.RemovalError --- monkey/monkey_island/cc/repository/__init__.py | 2 +- monkey/monkey_island/cc/repository/errors.py | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/monkey/monkey_island/cc/repository/__init__.py b/monkey/monkey_island/cc/repository/__init__.py index 3a8c552e3..52e6f6873 100644 --- a/monkey/monkey_island/cc/repository/__init__.py +++ b/monkey/monkey_island/cc/repository/__init__.py @@ -1,4 +1,4 @@ -from .errors import RetrievalError, StorageError +from .errors import RemovalError, RetrievalError, StorageError from .file_storage import FileNotFoundError, IFileRepository, LocalStorageFileRepository from .i_agent_binary_repository import IAgentBinaryRepository from .agent_binary_repository import AgentBinaryRepository diff --git a/monkey/monkey_island/cc/repository/errors.py b/monkey/monkey_island/cc/repository/errors.py index 586f23cb2..aeb9fa23c 100644 --- a/monkey/monkey_island/cc/repository/errors.py +++ b/monkey/monkey_island/cc/repository/errors.py @@ -1,3 +1,11 @@ +class RemovalError(RuntimeError): + """ + Raised when a repository encounters an error while attempting to remove data. + """ + + pass + + class RetrievalError(RuntimeError): """ Raised when a repository encounters an error while attempting to retrieve data. From da1339e41069a87d939637aa12895a93a662f504 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 21 Jun 2022 13:24:22 -0400 Subject: [PATCH 5/8] Island: Raise RemovalError in LocalStorageFileRepository.delete_file() --- .../cc/repository/file_storage/i_file_repository.py | 1 + .../repository/file_storage/local_storage_file_repository.py | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py b/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py index 73d3b4759..cfebe5c05 100644 --- a/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py +++ b/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py @@ -48,6 +48,7 @@ class IFileRepository(metaclass=abc.ABCMeta): :param unsafe_file_name: An unsanitized file name that identifies the file to be deleted :raises ValueError: If the file name is an attempted directory traversal + :raises RemovalError: If an error was encountered while attempting to remove a file """ pass diff --git a/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py b/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py index a766746ae..d18e029f4 100644 --- a/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py +++ b/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py @@ -4,7 +4,7 @@ from pathlib import Path from typing import BinaryIO from common.utils.file_utils import get_all_regular_files_in_directory -from monkey_island.cc.repository import RetrievalError, StorageError +from monkey_island.cc.repository import RemovalError, RetrievalError, StorageError from monkey_island.cc.server_utils.file_utils import create_secure_directory from . import IFileRepository, i_file_repository @@ -66,6 +66,8 @@ class LocalStorageFileRepository(IFileRepository): except FileNotFoundError: # This method is idempotent. pass + except Exception as err: + raise RemovalError(f"Error while attempting to remove {unsafe_file_name}: {err}") def _get_safe_file_path(self, unsafe_file_name: str): # Remove any path information from the file name. From 21be5fc2be5d89ee2543b58e13843c21469e62dc Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 21 Jun 2022 13:28:44 -0400 Subject: [PATCH 6/8] Island: raise RemovalError in Local...FileRepository.delete_all_files() --- .../cc/repository/file_storage/i_file_repository.py | 2 ++ .../file_storage/local_storage_file_repository.py | 9 ++++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py b/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py index cfebe5c05..c0897aa1a 100644 --- a/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py +++ b/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py @@ -56,5 +56,7 @@ class IFileRepository(metaclass=abc.ABCMeta): def delete_all_files(self): """ Delete all files that have been stored using this service. + + :raises RemovalError: If an error was encountered while attempting to remove a file """ pass diff --git a/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py b/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py index d18e029f4..564582c76 100644 --- a/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py +++ b/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py @@ -82,6 +82,9 @@ class LocalStorageFileRepository(IFileRepository): return safe_file_path def delete_all_files(self): - for file in get_all_regular_files_in_directory(self._storage_directory): - logger.debug(f"Deleting {file}") - file.unlink() + try: + for file in get_all_regular_files_in_directory(self._storage_directory): + logger.debug(f"Deleting {file}") + file.unlink() + except Exception as err: + raise RemovalError(f"Error while attempting to clear the repository: {err}") From 40b192accc14bc684aa5ea78f0897b2f3f9246e0 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 21 Jun 2022 13:46:15 -0400 Subject: [PATCH 7/8] Island: Wrap directory traversal errors in repository errors A given `IFileRepository` may have no concept of directories. `LocalStorageFileRepository` should wrap the ValueErrors raised to prevent directory traversal in repository errors. --- .../repository/file_storage/i_file_repository.py | 3 --- .../file_storage/local_storage_file_repository.py | 14 +++++++------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py b/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py index c0897aa1a..baa52fdda 100644 --- a/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py +++ b/monkey/monkey_island/cc/repository/file_storage/i_file_repository.py @@ -20,7 +20,6 @@ class IFileRepository(metaclass=abc.ABCMeta): :param unsafe_file_name: An unsanitized file name that will identify the file :param file_contents: The data to be stored in the file - :raises ValueError: If the file name is an attempted directory traversal :raises StorageError: If an error was encountered while attempting to store the file """ pass @@ -34,7 +33,6 @@ class IFileRepository(metaclass=abc.ABCMeta): :return: A file-like object providing access to the file's contents :raises FileNotFoundError: if the file does not exist :raises RetrievalError: if the file exists but cannot be retrieved - :raises ValueError: If the file name is an attempted directory traversal """ pass @@ -47,7 +45,6 @@ class IFileRepository(metaclass=abc.ABCMeta): idempotent and will succeed if the file to be deleted does not exist. :param unsafe_file_name: An unsanitized file name that identifies the file to be deleted - :raises ValueError: If the file name is an attempted directory traversal :raises RemovalError: If an error was encountered while attempting to remove a file """ pass diff --git a/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py b/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py index 564582c76..09fd14453 100644 --- a/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py +++ b/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py @@ -32,19 +32,19 @@ class LocalStorageFileRepository(IFileRepository): self._storage_directory = storage_directory def save_file(self, unsafe_file_name: str, file_contents: BinaryIO): - safe_file_path = self._get_safe_file_path(unsafe_file_name) - - logger.debug(f"Saving file contents to {safe_file_path}") try: + safe_file_path = self._get_safe_file_path(unsafe_file_name) + + logger.debug(f"Saving file contents to {safe_file_path}") with open(safe_file_path, "wb") as dest: shutil.copyfileobj(file_contents, dest) except Exception as err: raise StorageError(f"Error while attempting to store {unsafe_file_name}: {err}") def open_file(self, unsafe_file_name: str) -> BinaryIO: - safe_file_path = self._get_safe_file_path(unsafe_file_name) - try: + safe_file_path = self._get_safe_file_path(unsafe_file_name) + logger.debug(f"Opening {safe_file_path}") return open(safe_file_path, "rb") except FileNotFoundError as err: @@ -58,9 +58,9 @@ class LocalStorageFileRepository(IFileRepository): ) def delete_file(self, unsafe_file_name: str): - safe_file_path = self._get_safe_file_path(unsafe_file_name) - try: + safe_file_path = self._get_safe_file_path(unsafe_file_name) + logger.debug(f"Deleting {safe_file_path}") safe_file_path.unlink() except FileNotFoundError: From f9775f5b5407c7233d3f3902434e9792b5d78cdb Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 21 Jun 2022 14:02:19 -0400 Subject: [PATCH 8/8] Island: Improve directory traversal error message --- .../repository/file_storage/local_storage_file_repository.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py b/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py index 09fd14453..4c7662b0c 100644 --- a/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py +++ b/monkey/monkey_island/cc/repository/file_storage/local_storage_file_repository.py @@ -76,7 +76,9 @@ class LocalStorageFileRepository(IFileRepository): # This is a paranoid check to avoid directory traversal attacks. 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") + raise ValueError( + f'The file named "{unsafe_file_name}" cannot be safely retrieved or written' + ) logger.debug(f"Untrusted file name {unsafe_file_name} sanitized: {safe_file_path}") return safe_file_path