CVE-2022-2872: Unrestricted Upload of File with Dangerous Type (CWE-434)
value: Unrestricted Upload of File with Dangerous Type in GitHub repository octoprint/octoprint prior to 1.8.3.
CWE description: The product allows the upload or transfer of dangerous file types that are automatically processed within its environment.
CVSS v3 base score: 5.4
Published: 2022-09-21T10:15Z
Repository: https://github.com/octoprint/octoprint
Language: Python
Files changed: src/octoprint/filemanager/storage.py, src/octoprint/server/__init__.py, src/octoprint/server/api/files.py
Commit date: 2022-08-17 00:45:31 +0200
Commit message: 🔒️ Enforce valid type on copy/move of uploads

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.

Diff: 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
@@ -952,10 +952,16 @@ class LocalFileStorage(StorageInterface):
     def copy_file(self, source, destination):
         source_data, destination_data = self._get_source_destination_data(
             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:
             raise StorageError(
                 "Could not copy %s in %s to %s in %s"
@@ -981,10 +987,16 @@ class LocalFileStorage(StorageInterface):
     def move_file(self, source, destination, allow_overwrite=False):
         source_data, destination_data = self._get_source_destination_data(
             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)
             return self.path_in_storage(destination_data["fullpath"])
 
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
@@ -38,10 +38,11 @@ from flask_login import (  # noqa: F401
 )
 from watchdog.observers import Observer
 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
 from octoprint.systemcommands import system_command_manager
 from octoprint.util.json import JsonEncoding
@@ -744,10 +745,19 @@ class Server:
             "path_validation": util.tornado.path_validation_factory(
                 lambda path: not octoprint.util.is_hidden_path(path), status_code=404
             )
         }
 
+        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)
         )
         timelapse_path_validator = {
@@ -838,10 +848,11 @@ class Server:
                         "name_generator": download_name_generator,
                     },
                     download_permission_validator,
                     download_handler_kwargs,
                     no_hidden_files_validator,
+                    only_known_types_validator,
                     additional_mime_types,
                 ),
             ),
             # log files
             (
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
@@ -1136,46 +1136,59 @@ def gcodeFileCommand(filename, target):
             is_folder = fileManager.folder_exists(target, filename)
 
             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",
                 target=target,
                 filename=destination,

Vulnerable code:             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:
                    # deselect the file if it's currently selected
                    currentOrigin, currentFilename = _getCurrentFile()
                    if currentFilename is not None and filename == currentFilename:
                        printer.unselect_file()

                        fileManager.move_file(target, filename, destination)
                        fileManager.move_folder(target, filename, destination)
Fixed code:         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,
            )

        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,
            )

import octoprint.filemanager
        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,
            )
        }

                    only_known_types_validator,
            try:
                if command == "copy":
                    # destination already there? error...
                    if _verifyFileExists(target, destination) or _verifyFolderExists(
                        target, destination
                    ):
                        fileManager.copy_file(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}",
                    )