Merge pull request from GHSA-jpf9-646h-4px7

* Mitigate a CSRF vulnerability in export and backup-related endpoints

While Django has built-in CSRF protection (which we use), it does not cover
GET requests, and AFAICS, there is no way to force it to do that.
Unfortunately, the many endpoints that initiate dataset exports and backups
do accept GET requests _and_ initiate side effects, making them susceptible.

The proper fix for this issue would be to redesign those endpoints to use
POST requests, but a) that's more complicated, and b) we should still keep
the old endpoints for backwards compatibility.

So apply a less proper fix, which is to disable session authentication for
the affected endpoints. It's a bit complex, because in some cases
(particularly when `action=download`) we _need_ session authentication to
work, because the UI redirects the user to such endpoints.

In addition, modify the handling logic for these endpoints in order to
ensure that when `action=download`, no side effects are triggered.
Previously, `action=download` would still queue an RQ job if none existed.

Even after this, `action=download` will still have two small side effects:

* An existing RQ job will be deleted if its results are out of date.
  I don't think this is a problem, because such a job cannot be used anyway.

* A completed RQ job will be deleted too. This is a problematic design,
  but I don't think an attacker can achieve anything by exploiting this. If
  an attacker maliciously redirects the user to an `action=download` URL,
  then they'll just download the export/backup as usual.

Some tests were making export requests incorrectly, so fix them.

* Add test for the CSRF workaround

changelog.d/20240523_192101_roman_export_backup_csrf.md

@@ -0,0 +1,4 @@
+### Security
+
+- Mitigated a CSRF vulnerability in backup and export-related endpoints
+  (<https://github.com/cvat-ai/cvat/security/advisories/GHSA-jpf9-646h-4px7>)

cvat/apps/dataset_manager/tests/test_rest_api_formats.py

@@ -317,7 +317,7 @@ def _create_annotations_in_job(self, task, job_id,  name_ann, key_get_values):
     def _download_file(self, url, data, user, file_name):
         response = self._get_request_with_data(url, data, user)
         self.assertEqual(response.status_code, status.HTTP_202_ACCEPTED)
-        response = self._get_request_with_data(url, data, user)
+        response = self._get_request_with_data(url, {**data, "action": "download"}, user)
         self.assertEqual(response.status_code, status.HTTP_200_OK)
 
         content = BytesIO(b"".join(response.streaming_content))
@@ -659,7 +659,6 @@ def test_api_v2_dump_and_upload_annotations_with_objects_are_different_images(se
                     file_zip_name = osp.join(test_dir, f'{test_name}_{upload_type}.zip')
                     data = {
                         "format": dump_format_name,
-                        "action": "download",
                     }
                     self._download_file(url, data, self.admin, file_zip_name)
                     self.assertEqual(osp.exists(file_zip_name), True)
@@ -700,7 +699,6 @@ def test_api_v2_dump_and_upload_annotations_with_objects_are_different_video(sel
 
                     data = {
                         "format": dump_format_name,
-                        "action": "download",
                     }
                     self._download_file(url, data, self.admin, file_zip_name)
                     self.assertEqual(osp.exists(file_zip_name), True)
@@ -732,7 +730,6 @@ def test_api_v2_dump_and_upload_with_objects_type_is_track_and_outside_property(
             file_zip_name = osp.join(test_dir, f'{test_name}.zip')
             data = {
                 "format": dump_format_name,
-                "action": "download",
             }
             self._download_file(url, data, self.admin, file_zip_name)
             self.assertEqual(osp.exists(file_zip_name), True)
@@ -756,7 +753,6 @@ def test_api_v2_dump_and_upload_with_objects_type_is_track_and_keyframe_property
 
             data = {
                 "format": dump_format_name,
-                "action": "download",
             }
             self._download_file(url, data, self.admin, file_zip_name)
             self.assertEqual(osp.exists(file_zip_name), True)
@@ -781,7 +777,6 @@ def test_api_v2_dump_upload_annotations_from_several_jobs(self):
             file_zip_name = osp.join(test_dir, f'{test_name}.zip')
             data = {
                 "format": dump_format_name,
-                "action": "download",
             }
             self._download_file(url, data, self.admin, file_zip_name)
             self.assertEqual(osp.exists(file_zip_name), True)
@@ -817,7 +812,6 @@ def test_api_v2_dump_annotations_from_several_jobs(self):
                     file_zip_name = osp.join(test_dir, f'{test_name}.zip')
                     data = {
                         "format": dump_format_name,
-                        "action": "download",
                     }
                     self._download_file(url, data, self.admin, file_zip_name)
                     self.assertEqual(osp.exists(file_zip_name), True)
@@ -905,7 +899,6 @@ def test_api_v2_dump_empty_frames(self):
                     file_zip_name = osp.join(test_dir, f'empty_{dump_format_name}.zip')
                     data = {
                         "format": dump_format_name,
-                        "action": "download",
                     }
                     self._download_file(url, data, self.admin, file_zip_name)
                     self.assertEqual(osp.exists(file_zip_name), True)
@@ -983,7 +976,6 @@ def test_api_v2_rewriting_annotations(self):
                     file_zip_name = osp.join(test_dir, f'{test_name}_{dump_format_name}.zip')
                     data = {
                         "format": dump_format_name,
-                        "action": "download",
                     }
                     self._download_file(url, data, self.admin, file_zip_name)
                     self.assertEqual(osp.exists(file_zip_name), True)
@@ -1027,7 +1019,6 @@ def test_api_v2_tasks_annotations_dump_and_upload_many_jobs_with_datumaro(self):
 
                     data = {
                         "format": dump_format_name,
-                        "action": "download",
                     }
                     self._download_file(url, data, self.admin, file_zip_name)
                     self._check_downloaded_file(file_zip_name)
@@ -1100,7 +1091,6 @@ def test_api_v2_tasks_annotations_dump_and_upload_with_datumaro(self):
                         file_zip_name = osp.join(test_dir, f'{test_name}_{dump_format_name}.zip')
                         data = {
                             "format": dump_format_name,
-                            "action": "download",
                         }
                         self._download_file(url, data, self.admin, file_zip_name)
                         self._check_downloaded_file(file_zip_name)
@@ -1128,7 +1118,6 @@ def test_api_v2_check_duplicated_polygon_points(self):
         task_id = task["id"]
         data = {
             "format": "CVAT for video 1.1",
-            "action": "download",
         }
         annotation_name = "CVAT for video 1.1 polygon"
         self._create_annotations(task, annotation_name, "default")
@@ -1170,7 +1159,6 @@ def test_api_v2_check_widerface_with_all_attributes(self):
                 url = self._generate_url_dump_tasks_annotations(task_id)
                 data = {
                     "format": dump_format_name,
-                    "action": "download",
                 }
                 with TestDir() as test_dir:
                     file_zip_name = osp.join(test_dir, f'{test_name}_{dump_format_name}.zip')
@@ -1207,7 +1195,6 @@ def test_api_v2_check_mot_with_shapes_only(self):
                 url = self._generate_url_dump_tasks_annotations(task_id)
                 data = {
                     "format": format_name,
-                    "action": "download",
                 }
                 with TestDir() as test_dir:
                     file_zip_name = osp.join(test_dir, f'{test_name}_{format_name}.zip')
@@ -1245,7 +1232,6 @@ def test_api_v2_check_attribute_import_in_tracks(self):
                 url = self._generate_url_dump_tasks_annotations(task_id)
                 data = {
                     "format": dump_format_name,
-                    "action": "download",
                 }
                 with TestDir() as test_dir:
                     file_zip_name = osp.join(test_dir, f'{test_name}_{dump_format_name}.zip')

cvat/apps/engine/backup.py

@@ -952,14 +952,12 @@ def export(db_instance, request, queue_name):
         field_name=StorageType.TARGET
     )
 
+    last_instance_update_time = timezone.localtime(db_instance.updated_date)
+
     queue = django_rq.get_queue(queue_name)
     rq_id = f"export:{obj_type}.id{db_instance.pk}-by-{request.user}"
     rq_job = queue.fetch_job(rq_id)
 
-    last_instance_update_time = timezone.localtime(db_instance.updated_date)
-    timestamp = datetime.strftime(last_instance_update_time, "%Y_%m_%d_%H_%M_%S")
-    location = location_conf.get('location')
-
     if rq_job:
         rq_request = rq_job.meta.get('request', None)
         request_time = rq_request.get("timestamp", None) if rq_request else None
@@ -968,43 +966,54 @@ def export(db_instance, request, queue_name):
             # we have to enqueue dependent jobs after canceling one
             rq_job.cancel(enqueue_dependents=settings.ONE_RUNNING_JOB_IN_QUEUE_PER_USER)
             rq_job.delete()
-        else:
-            if rq_job.is_finished:
-                if location == Location.LOCAL:
-                    file_path = rq_job.return_value()
-
-                    if not file_path:
-                        return Response('A result for exporting job was not found for finished RQ job', status=status.HTTP_500_INTERNAL_SERVER_ERROR)
-
-                    elif not os.path.exists(file_path):
-                        return Response('The result file does not exist in export cache', status=status.HTTP_500_INTERNAL_SERVER_ERROR)
-
-                    filename = filename or build_backup_file_name(
-                        class_name=obj_type,
-                        identifier=db_instance.name,
-                        timestamp=timestamp,
-                        extension=os.path.splitext(file_path)[1]
-                    )
-
-                    if action == "download":
-                        rq_job.delete()
-                        return sendfile(request, file_path, attachment=True,
-                            attachment_filename=filename)
-
-                    return Response(status=status.HTTP_201_CREATED)
-
-                elif location == Location.CLOUD_STORAGE:
-                    rq_job.delete()
-                    return Response(status=status.HTTP_200_OK)
-                else:
-                    raise NotImplementedError()
-            elif rq_job.is_failed:
-                exc_info = rq_job.meta.get('formatted_exception', str(rq_job.exc_info))
+            rq_job = None
+
+    timestamp = datetime.strftime(last_instance_update_time, "%Y_%m_%d_%H_%M_%S")
+    location = location_conf.get('location')
+
+    if action == "download":
+        if location != Location.LOCAL:
+            return Response('Action "download" is only supported for a local backup location', status=status.HTTP_400_BAD_REQUEST)
+
+        if not rq_job or not rq_job.is_finished:
+            return Response('Backup has not finished', status=status.HTTP_400_BAD_REQUEST)
+
+        file_path = rq_job.return_value()
+
+        if not file_path:
+            return Response('A result for exporting job was not found for finished RQ job', status=status.HTTP_500_INTERNAL_SERVER_ERROR)
+
+        elif not os.path.exists(file_path):
+            return Response('The result file does not exist in export cache', status=status.HTTP_500_INTERNAL_SERVER_ERROR)
+
+        filename = filename or build_backup_file_name(
+            class_name=obj_type,
+            identifier=db_instance.name,
+            timestamp=timestamp,
+            extension=os.path.splitext(file_path)[1]
+        )
+
+        rq_job.delete()
+        return sendfile(request, file_path, attachment=True,
+            attachment_filename=filename)
+
+    if rq_job:
+        if rq_job.is_finished:
+            if location == Location.LOCAL:
+                return Response(status=status.HTTP_201_CREATED)
+
+            elif location == Location.CLOUD_STORAGE:
                 rq_job.delete()
-                return Response(exc_info,
-                    status=status.HTTP_500_INTERNAL_SERVER_ERROR)
+                return Response(status=status.HTTP_200_OK)
             else:
-                return Response(status=status.HTTP_202_ACCEPTED)
+                raise NotImplementedError()
+        elif rq_job.is_failed:
+            exc_info = rq_job.meta.get('formatted_exception', str(rq_job.exc_info))
+            rq_job.delete()
+            return Response(exc_info,
+                status=status.HTTP_500_INTERNAL_SERVER_ERROR)
+        else:
+            return Response(status=status.HTTP_202_ACCEPTED)
 
     ttl = dm.views.PROJECT_CACHE_TTL.total_seconds()
     user_id = request.user.id

cvat/apps/engine/mixins.py

@@ -12,13 +12,15 @@
 from pathlib import Path
 from tempfile import NamedTemporaryFile
 from unittest import mock
-from typing import Optional, Callable, Dict, Any
+from typing import Optional, Callable, Dict, Any, Mapping
 
 import django_rq
 from attr.converters import to_bool
 from django.conf import settings
 from rest_framework import mixins, status
+from rest_framework.authentication import SessionAuthentication
 from rest_framework.response import Response
+from rest_framework.views import APIView
 
 from cvat.apps.engine.location import StorageType, get_location_configuration
 from cvat.apps.engine.log import ServerLogManager
@@ -498,3 +500,36 @@ def perform_update(self, serializer):
     def partial_update(self, request, *args, **kwargs):
         with mock.patch.object(self, 'update', new=self._update, create=True):
             return mixins.UpdateModelMixin.partial_update(self, request=request, *args, **kwargs)
+
+class CsrfWorkaroundMixin(APIView):
+    """
+    Disables session authentication for GET/HEAD requests
+    for which csrf_workaround_is_needed returns True.
+
+    csrf_workaround_is_needed is supposed to be overridden by each view.
+
+    This only exists to mitigate CSRF attacks on several known endpoints that
+    perform side effects in response to GET requests. Do not use this in
+    new code: instead, make sure that all endpoints with side effects use
+    a method other than GET/HEAD. Then Django's built-in CSRF protection
+    will cover them.
+    """
+
+    @staticmethod
+    def csrf_workaround_is_needed(query_params: Mapping[str, str]) -> bool:
+        return False
+
+    def get_authenticators(self):
+        authenticators = super().get_authenticators()
+
+        if (
+            self.request and
+            # Don't apply the workaround for requests from unit tests, since
+            # they can only use session authentication.
+            not getattr(self.request, "_dont_enforce_csrf_checks", False) and
+            self.request.method in ("GET", "HEAD") and
+            self.csrf_workaround_is_needed(self.request.GET)
+        ):
+            authenticators = [a for a in authenticators if not isinstance(a, SessionAuthentication)]
+
+        return authenticators

cvat/apps/engine/tests/test_rest_api_3D.py

@@ -153,7 +153,7 @@ def _delete_request(self, path, user):
     def _download_file(self, url, data, user, file_name):
         response = self._get_request_with_data(url, data, user)
         self.assertEqual(response.status_code, status.HTTP_202_ACCEPTED)
-        response = self._get_request_with_data(url, data, user)
+        response = self._get_request_with_data(url, {**data, "action": "download"}, user)
         self.assertEqual(response.status_code, status.HTTP_200_OK)
 
         content = BytesIO(b"".join(response.streaming_content))
@@ -583,7 +583,6 @@ def test_api_v2_rewrite_annotation(self):
                     file_name = osp.join(test_dir, f"{format_name}.zip")
                     data = {
                         "format": format_name,
-                        "action": "download",
                     }
                     self._download_file(url, data, self.admin, file_name)
                     self.assertTrue(osp.exists(file_name))
@@ -622,7 +621,6 @@ def test_api_v2_dump_and_upload_empty_annotation(self):
                     file_name = osp.join(test_dir, f"{format_name}.zip")
                     data = {
                         "format": format_name,
-                        "action": "download",
                     }
                     self._download_file(url, data, self.admin, file_name)
                     self.assertTrue(osp.exists(file_name))
@@ -663,7 +661,6 @@ def test_api_v2_dump_and_upload_several_jobs(self):
                     file_name = osp.join(test_dir, f"{format_name}.zip")
                     data = {
                         "format": format_name,
-                        "action": "download",
                     }
                     self._download_file(url, data, self.admin, file_name)
 
@@ -687,7 +684,6 @@ def test_api_v2_upload_annotation_with_attributes(self):
                     file_name = osp.join(test_dir, f"{format_name}.zip")
                     data = {
                         "format": format_name,
-                        "action": "download",
                     }
                     self._download_file(url, data, self.admin, file_name)
                     self.assertTrue(osp.exists(file_name))