-
Notifications
You must be signed in to change notification settings - Fork 80
🔒️(backend) fix two WOPI security bypasses #795
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| from django.db import transaction | ||
| from django.http import StreamingHttpResponse | ||
|
|
||
| from lasuite.malware_detection import malware_detection | ||
| from rest_framework import viewsets | ||
| from rest_framework.decorators import action | ||
| from rest_framework.response import Response | ||
|
|
@@ -35,6 +36,8 @@ | |
| X_WOPI_LOCK = "X-WOPI-Lock" | ||
| S3_VERSION_ID = "VersionId" | ||
|
|
||
| ILLEGAL_FILENAME_CHARS = ("/", "\\") | ||
|
|
||
|
|
||
| class WopiViewSet(viewsets.ViewSet): | ||
| """ | ||
|
|
@@ -185,8 +188,12 @@ | |
| s3_client = default_storage.connection.meta.client | ||
| default_storage.save(item.file_key, file) | ||
| item.size = file.size | ||
| # Keep the item READY during re-analysis: non-creators cannot open | ||
| # non-READY files in WOPI. | ||
| item.save(update_fields=["size", "updated_at"]) | ||
|
|
||
| malware_detection.analyse_file(item.file_key, item_id=item.id) | ||
|
|
||
| head_response = s3_client.head_object(Bucket=default_storage.bucket_name, Key=item.file_key) | ||
| return Response( | ||
| status=200, | ||
|
|
@@ -332,25 +339,40 @@ | |
| return Response(status=401) | ||
|
|
||
| new_filename = request.META.get("HTTP_X_WOPI_REQUESTEDNAME") | ||
|
|
||
| if not new_filename: | ||
| invalid_filename_error = "No filename provided" | ||
| else: | ||
| # Convert it to utf-7 to avoid issues with special characters | ||
| new_filename = new_filename.encode("ascii").decode("utf-7") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why utf-7, why not utf-8 as utf-7 is considered obsolete?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. utf-7 is part of wopi specs… |
||
| new_filename_with_extension = f"{new_filename}{splitext(item.filename)[1]}" | ||
| _, target_extension = splitext(new_filename_with_extension) | ||
|
|
||
| invalid_filename_error = None | ||
| if any(char in new_filename for char in ILLEGAL_FILENAME_CHARS): | ||
| invalid_filename_error = "Invalid filename" | ||
| elif settings.RESTRICT_UPLOAD_FILE_TYPE and target_extension.lower() not in { | ||
| extension.lower() for extension in settings.FILE_EXTENSIONS_ALLOWED | ||
| }: | ||
| logger.info( | ||
| "rename_file: file extension not allowed %r for filename %r", | ||
| target_extension, | ||
| new_filename_with_extension, | ||
| ) | ||
|
Check warning on line 360 in src/backend/wopi/viewsets.py
|
||
| invalid_filename_error = "This file extension is not allowed" | ||
|
|
||
| if invalid_filename_error: | ||
| return Response( | ||
| status=400, | ||
| headers={X_WOPI_INVALIDFILENAMERROR: "No filename provided"}, | ||
| headers={X_WOPI_INVALIDFILENAMERROR: invalid_filename_error}, | ||
| ) | ||
|
|
||
| # Convert it to utf-7 to avoid issues with special characters | ||
| new_filename = new_filename.encode("ascii").decode("utf-7") | ||
| lock_service = LockService(item) | ||
| if lock_service.is_locked(): | ||
| current_lock_value = lock_service.get_lock(default="") | ||
| lock_value = request.META.get(HTTP_X_WOPI_LOCK) | ||
| if current_lock_value != lock_value: | ||
| return Response(status=409, headers={X_WOPI_LOCK: current_lock_value}) | ||
|
|
||
| _, current_extension = splitext(item.filename) | ||
| new_filename_with_extension = f"{new_filename}{current_extension}" | ||
|
|
||
| parent_path = item.path[:-1] | ||
| # Filter on siblings with the desired filename | ||
| queryset = ( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach leave a window during which a dangerous file is in READY state.
That being said, to avoid degrading live edition I understand we have no better choice for now.
The fine grade solution is indeed only allowing wopi editors to access the wopi API via this PR: #416