From 40b192accc14bc684aa5ea78f0897b2f3f9246e0 Mon Sep 17 00:00:00 2001 From: Mike Salvatore Date: Tue, 21 Jun 2022 13:46:15 -0400 Subject: [PATCH] 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: