Skip to content

Commit

Permalink
🔒️ Enforce valid type on copy/move of uploads
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
foosel committed Aug 16, 2022
1 parent a33622e commit 3e3c118
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 32 deletions.
12 changes: 12 additions & 0 deletions src/octoprint/filemanager/storage.py
Expand Up @@ -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:
Expand Down Expand Up @@ -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)
Expand Down
11 changes: 11 additions & 0 deletions src/octoprint/server/__init__.py
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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,
),
),
Expand Down
77 changes: 45 additions & 32 deletions src/octoprint/server/api/files.py
Expand Up @@ -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",
Expand Down

0 comments on commit 3e3c118

Please sign in to comment.