From 3e3c11811e216fb371a33e28412df83f9701e5b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gina=20H=C3=A4u=C3=9Fge?= Date: Wed, 17 Aug 2022 00:45:31 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=EF=B8=8F=20Enforce=20valid=20type?= =?UTF-8?q?=20on=20copy/move=20of=20uploads?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also enforce that on downloads and mirror it on the API. Prevents accidental or intentional renaming of valid files to something like .html to e.g. attempt to run arbitrary JS code in the browser on "download" in the context of the OctoPrint URL. Stealing login credentials via XSS is very unlikely here given their http-only nature, however it's still better to prevent this from happening anyhow. --- src/octoprint/filemanager/storage.py | 12 +++++ src/octoprint/server/__init__.py | 11 ++++ src/octoprint/server/api/files.py | 77 ++++++++++++++++------------ 3 files changed, 68 insertions(+), 32 deletions(-) diff --git a/src/octoprint/filemanager/storage.py b/src/octoprint/filemanager/storage.py index 8cd24d38cf..aa5a8863fc 100644 --- a/src/octoprint/filemanager/storage.py +++ b/src/octoprint/filemanager/storage.py @@ -954,6 +954,12 @@ def copy_file(self, source, destination): source, destination, must_not_equal=True ) + if not octoprint.filemanager.valid_file_type(destination_data["name"]): + raise StorageError( + f"{destination_data['name']} is an unrecognized file type", + code=StorageError.INVALID_FILE, + ) + try: shutil.copy2(source_data["fullpath"], destination_data["fullpath"]) except Exception as e: @@ -983,6 +989,12 @@ def move_file(self, source, destination, allow_overwrite=False): source, destination ) + if not octoprint.filemanager.valid_file_type(destination_data["name"]): + raise StorageError( + f"{destination_data['name']} is an unrecognized file type", + code=StorageError.INVALID_FILE, + ) + # only a display rename? Update that and bail early if source_data["fullpath"] == destination_data["fullpath"]: self._set_display_metadata(destination_data) diff --git a/src/octoprint/server/__init__.py b/src/octoprint/server/__init__.py index 6f095aa150..b4a6c84d9c 100644 --- a/src/octoprint/server/__init__.py +++ b/src/octoprint/server/__init__.py @@ -40,6 +40,7 @@ from watchdog.observers.polling import PollingObserver from werkzeug.exceptions import HTTPException +import octoprint.filemanager import octoprint.util import octoprint.util.net from octoprint.server import util @@ -746,6 +747,15 @@ def download_name_generator(path): ) } + only_known_types_validator = { + "path_validation": util.tornado.path_validation_factory( + lambda path: octoprint.filemanager.valid_file_type( + os.path.basename(path) + ), + status_code=404, + ) + } + valid_timelapse = lambda path: not octoprint.util.is_hidden_path(path) and ( octoprint.timelapse.valid_timelapse(path) or octoprint.timelapse.valid_timelapse_thumbnail(path) @@ -840,6 +850,7 @@ def joined_dict(*dicts): download_permission_validator, download_handler_kwargs, no_hidden_files_validator, + only_known_types_validator, additional_mime_types, ), ), diff --git a/src/octoprint/server/api/files.py b/src/octoprint/server/api/files.py index 35c8b4da9c..40e6200c98 100644 --- a/src/octoprint/server/api/files.py +++ b/src/octoprint/server/api/files.py @@ -1138,42 +1138,55 @@ def slicing_done(target, path, select_after_slicing, print_after_slicing): if not (is_file or is_folder): abort(400, description=f"Neither file nor folder, can't {command}") - if command == "copy": - # destination already there? error... - if _verifyFileExists(target, destination) or _verifyFolderExists( - target, destination - ): - abort(409, description="File or folder does already exist") - - if is_file: - fileManager.copy_file(target, filename, destination) - else: - fileManager.copy_folder(target, filename, destination) - - elif command == "move": - with Permissions.FILES_DELETE.require(403): - if _isBusy(target, filename): - abort( - 409, - description="Trying to move a file or folder that is currently in use", - ) - - # destination already there AND not ourselves (= display rename)? error... - if ( - _verifyFileExists(target, destination) - or _verifyFolderExists(target, destination) - ) and sanitized_destination != filename: + try: + if command == "copy": + # destination already there? error... + if _verifyFileExists(target, destination) or _verifyFolderExists( + target, destination + ): abort(409, description="File or folder does already exist") - # deselect the file if it's currently selected - currentOrigin, currentFilename = _getCurrentFile() - if currentFilename is not None and filename == currentFilename: - printer.unselect_file() - if is_file: - fileManager.move_file(target, filename, destination) + fileManager.copy_file(target, filename, destination) else: - fileManager.move_folder(target, filename, destination) + fileManager.copy_folder(target, filename, destination) + + elif command == "move": + with Permissions.FILES_DELETE.require(403): + if _isBusy(target, filename): + abort( + 409, + description="Trying to move a file or folder that is currently in use", + ) + + # destination already there AND not ourselves (= display rename)? error... + if ( + _verifyFileExists(target, destination) + or _verifyFolderExists(target, destination) + ) and sanitized_destination != filename: + abort(409, description="File or folder does already exist") + + # deselect the file if it's currently selected + currentOrigin, currentFilename = _getCurrentFile() + if currentFilename is not None and filename == currentFilename: + printer.unselect_file() + + if is_file: + fileManager.move_file(target, filename, destination) + else: + fileManager.move_folder(target, filename, destination) + + except octoprint.filemanager.storage.StorageError as e: + if e.code == octoprint.filemanager.storage.StorageError.INVALID_FILE: + abort( + 415, + description=f"Could not {command} {filename} to {destination}, invalid type", + ) + else: + abort( + 500, + description=f"Could not {command} {filename} to {destination}", + ) location = url_for( ".readGcodeFile",