Wagtail vulnerable to denial-of-service via memory exhaustion when uploading large files

@@ -295,6 +295,7 @@ Changelog
 ~~~~~~~~~~~~~~~~~~
 
  * Fix: CVE-2023-28836 - Stored XSS attack via ModelAdmin views (Thibaud Colas)
+ * Fix: CVE-2023-28837 - Denial-of-service via memory exhaustion when uploading large files (Jake Howard)
  * Fix: Fix radio and checkbox elements shrinking when using a long label (Sage Abdullah)
  * Fix: Fix select elements expanding beyond their container when using a long option label (Sage Abdullah)
  * Fix: Fix timezone handling of `TemplateResponse`s for users with a custom timezone (Stefan Hammer, Sage Abdullah)

@@ -17,6 +17,16 @@ This release addresses a stored cross-site scripting (XSS) vulnerability on Mode
 
 Many thanks to Thibaud Colas for reporting this issue. For further details, please see [the CVE-2023-28836 security advisory](https://github.com/wagtail/wagtail/security/advisories/GHSA-5286-f2rf-35c2).
 
+
+### CVE-2023-28837: Denial-of-service via memory exhaustion when uploading large files
+
+This release addresses a memory exhaustion bug in Wagtail's handling of uploaded images and documents. For both images and documents, files are loaded into memory during upload for additional processing. A user with access to upload images or documents through the Wagtail admin interface could upload a file so large that it results in a crash or denial of service.
+
+The vulnerability is not exploitable by an ordinary site visitor without access to the Wagtail admin. It can only be exploited by admin users with permission to upload images or documents.
+
+Many thanks to Jake Howard for reporting this issue. For further details, please see [the CVE-2023-28837 security advisory](https://github.com/wagtail/wagtail/security/advisories/GHSA-33pv-vcgh-jfg9).
+
+
 ### Bug fixes
 
 * Fix radio and checkbox elements shrinking when using a long label (Sage Abdullah)

@@ -106,6 +106,7 @@ Changelog
 ~~~~~~~~~~~~~~~~~~
 
  * Fix: CVE-2023-28836 - Stored XSS attack via ModelAdmin views (Thibaud Colas)
+ * Fix: CVE-2023-28837 - Denial-of-service via memory exhaustion when uploading large files (Jake Howard)
  * Fix: Fix radio and checkbox elements shrinking when using a long label (Sage Abdullah)
  * Fix: Fix select elements expanding beyond their container when using a long option label (Sage Abdullah)
  * Fix: Fix timezone handling of `TemplateResponse`s for users with a custom timezone (Stefan Hammer, Sage Abdullah)

@@ -17,6 +17,16 @@ This release addresses a stored cross-site scripting (XSS) vulnerability on Mode
 
 Many thanks to Thibaud Colas for reporting this issue. For further details, please see [the CVE-2023-28836 security advisory](https://github.com/wagtail/wagtail/security/advisories/GHSA-5286-f2rf-35c2).
 
+
+### CVE-2023-28837: Denial-of-service via memory exhaustion when uploading large files
+
+This release addresses a memory exhaustion bug in Wagtail's handling of uploaded images and documents. For both images and documents, files are loaded into memory during upload for additional processing. A user with access to upload images or documents through the Wagtail admin interface could upload a file so large that it results in a crash or denial of service.
+
+The vulnerability is not exploitable by an ordinary site visitor without access to the Wagtail admin. It can only be exploited by admin users with permission to upload images or documents.
+
+Many thanks to Jake Howard for reporting this issue. For further details, please see [the CVE-2023-28837 security advisory](https://github.com/wagtail/wagtail/security/advisories/GHSA-33pv-vcgh-jfg9).
+
+
 ### Bug fixes
 
 * Fix radio and checkbox elements shrinking when using a long label (Sage Abdullah)

@@ -146,11 +146,11 @@ def to_python(self, data):
         if f is None:
             return None
 
-        # We need to get a file object for Pillow. When we get a path, we need to open
-        # the file first. And we have to read the data into memory to pass to Willow.
+        # Get the file content ready for Willow
         if hasattr(data, "temporary_file_path"):
-            with open(data.temporary_file_path(), "rb") as fh:
-                file = BytesIO(fh.read())
+            # Django's `TemporaryUploadedFile` is enough of a file to satisfy Willow
+            # Willow doesn't support opening images by path https://github.com/wagtail/Willow/issues/108
+            file = data
         else:
             if hasattr(data, "read"):
                 file = BytesIO(data.read())

@@ -3,7 +3,7 @@
 import urllib
 
 from django.contrib.auth.models import Group, Permission
-from django.core.files.uploadedfile import SimpleUploadedFile
+from django.core.files.uploadedfile import SimpleUploadedFile, TemporaryUploadedFile
 from django.template.defaultfilters import filesizeformat
 from django.template.loader import render_to_string
 from django.test import RequestFactory, TestCase, override_settings
@@ -567,6 +567,44 @@ def test_add_svg(self):
         images = Image.objects.filter(title="Test image")
         self.assertEqual(images.count(), 1)
 
+    def test_add_temporary_uploaded_file(self):
+        """
+        Test that uploading large files (spooled to the filesystem) work as expected
+        """
+        test_image_file = get_test_image_file()
+        uploaded_file = TemporaryUploadedFile(
+            "test.png", "image/png", test_image_file.size, "utf-8"
+        )
+        uploaded_file.write(test_image_file.file.getvalue())
+        uploaded_file.seek(0)
+
+        response = self.post(
+            {
+                "title": "Test image",
+                "file": uploaded_file,
+            }
+        )
+
+        # Should redirect back to index
+        self.assertRedirects(response, reverse("wagtailimages:index"))
+
+        # Check that the image was created
+        images = Image.objects.filter(title="Test image")
+        self.assertEqual(images.count(), 1)
+
+        # Test that size was populated correctly
+        image = images.first()
+        self.assertEqual(image.width, 640)
+        self.assertEqual(image.height, 480)
+
+        # Test that the file_size/hash fields were set
+        self.assertTrue(image.file_size)
+        self.assertTrue(image.file_hash)
+
+        # Test that it was placed in the root collection
+        root_collection = Collection.get_first_root_node()
+        self.assertEqual(image.collection, root_collection)
+
     @override_settings(
         DEFAULT_FILE_STORAGE="wagtail.test.dummy_external_storage.DummyExternalStorage"
     )

@@ -1,4 +1,3 @@
-import hashlib
 import os.path
 import urllib
 from contextlib import contextmanager
@@ -15,6 +14,7 @@
 from wagtail.models import CollectionMember, ReferenceIndex
 from wagtail.search import index
 from wagtail.search.queryset import SearchableQuerySetMixin
+from wagtail.utils.file import hash_filelike
 
 
 class DocumentQuerySet(SearchableQuerySetMixin, models.QuerySet):
@@ -122,14 +122,13 @@ def get_file_size(self):
 
         return self.file_size
 
-    def _set_file_hash(self, file_contents):
-        self.file_hash = hashlib.sha1(file_contents).hexdigest()
+    def _set_file_hash(self):
+        with self.open_file() as f:
+            self.file_hash = hash_filelike(f)
 
     def get_file_hash(self):
         if self.file_hash == "":
-            with self.open_file() as f:
-                self._set_file_hash(f.read())
-
+            self._set_file_hash()
             self.save(update_fields=["file_hash"])
 
         return self.file_hash
@@ -141,7 +140,7 @@ def _set_document_file_metadata(self):
         self.file_size = self.file.size
 
         # Set new document file hash
-        self._set_file_hash(self.file.read())
+        self._set_file_hash()
         self.file.seek(0)
 
     def __str__(self):

@@ -126,6 +126,19 @@ def test_content_type(self):
             "application/octet-stream", self.extensionless_document.content_type
         )
 
+    def test_file_hash(self):
+        self.assertEqual(
+            self.document.get_file_hash(), "7d8c4778b182e4f3bd442408c64a6e22a4b0ed85"
+        )
+        self.assertEqual(
+            self.pdf_document.get_file_hash(),
+            "7d8c4778b182e4f3bd442408c64a6e22a4b0ed85",
+        )
+        self.assertEqual(
+            self.extensionless_document.get_file_hash(),
+            "7d8c4778b182e4f3bd442408c64a6e22a4b0ed85",
+        )
+
     def test_content_disposition(self):
         self.assertEqual(
             """attachment; filename=example.doc; filename*=UTF-8''example.doc""",

@@ -38,6 +38,7 @@
 from wagtail.models import CollectionMember, ReferenceIndex
 from wagtail.search import index
 from wagtail.search.queryset import SearchableQuerySetMixin
+from wagtail.utils.file import hash_filelike
 
 logger = logging.getLogger("wagtail.images")
 
@@ -267,14 +268,13 @@ class AbstractImage(ImageFileMixin, CollectionMember, index.Indexed, models.Mode
 
     objects = ImageQuerySet.as_manager()
 
-    def _set_file_hash(self, file_contents):
-        self.file_hash = hashlib.sha1(file_contents).hexdigest()
+    def _set_file_hash(self):
+        with self.open_file() as f:
+            self.file_hash = hash_filelike(f)
 
     def get_file_hash(self):
         if self.file_hash == "":
-            with self.open_file() as f:
-                self._set_file_hash(f.read())
-
+            self._set_file_hash()
             self.save(update_fields=["file_hash"])
 
         return self.file_hash
@@ -286,7 +286,7 @@ def _set_image_file_metadata(self):
         self.file_size = self.file.size
 
         # Set new image file hash
-        self._set_file_hash(self.file.read())
+        self._set_file_hash()
         self.file.seek(0)
 
     def get_upload_to(self, filename):

@@ -112,6 +112,11 @@ def test_get_file_size_on_missing_file_raises_sourceimageioerror(self):
         with self.assertRaises(SourceImageIOError):
             self.image.get_file_size()
 
+    def test_file_hash(self):
+        self.assertEqual(
+            self.image.get_file_hash(), "4dd0211870e130b7e1690d2ec53c499a54a48fef"
+        )
+
 
 class TestImageQuerySet(TestCase):
     def test_search_method(self):

@@ -1,8 +1,11 @@
 # -*- coding: utf-8 -*
 import pickle
+from io import BytesIO, StringIO
+from pathlib import Path
 
 from django.contrib.contenttypes.models import ContentType
 from django.core.exceptions import ImproperlyConfigured, SuspiciousOperation
+from django.core.files.uploadedfile import SimpleUploadedFile
 from django.test import SimpleTestCase, TestCase, override_settings
 from django.utils.text import slugify
 from django.utils.translation import _trans
@@ -23,6 +26,7 @@
     string_to_ascii,
 )
 from wagtail.models import Page, Site
+from wagtail.utils.file import hash_filelike
 from wagtail.utils.utils import deep_update
 
 
@@ -508,3 +512,58 @@ def test_deep_update(self):
                 "starship": "enterprise",
             },
         )
+
+
+class HashFileLikeTestCase(SimpleTestCase):
+    test_file = Path.cwd() / "LICENSE"
+
+    def test_hashes_io(self):
+        self.assertEqual(
+            hash_filelike(BytesIO(b"test")), "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3"
+        )
+        self.assertEqual(
+            hash_filelike(StringIO("test")), "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3"
+        )
+
+    def test_hashes_file(self):
+        with self.test_file.open(mode="r") as f:
+            self.assertEqual(
+                hash_filelike(f), "9e58400061ca660ef7b5c94338a5205627c77eda"
+            )
+
+    def test_hashes_file_bytes(self):
+        with self.test_file.open(mode="rb") as f:
+            self.assertEqual(
+                hash_filelike(f), "9e58400061ca660ef7b5c94338a5205627c77eda"
+            )
+
+    def test_hashes_django_uploaded_file(self):
+        """
+        Check Django's file shims can be hashed as-is.
+        `SimpleUploadedFile` inherits the base `UploadedFile`, but is easiest to test against
+        """
+        self.assertEqual(
+            hash_filelike(SimpleUploadedFile("example.txt", b"test")),
+            "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3",
+        )
+
+    def test_hashes_large_file(self):
+        class FakeLargeFile:
+            """
+            A class that pretends to be a huge file (~1.3GB)
+            """
+
+            def __init__(self):
+                self.iterations = 20000
+
+            def read(self, bytes):
+                self.iterations -= 1
+                if not self.iterations:
+                    return b""
+
+                return b"A" * bytes
+
+        self.assertEqual(
+            hash_filelike(FakeLargeFile()),
+            "187cc1db32624dccace20d042f6d631f1a483020",
+        )

@@ -0,0 +1,35 @@
+from hashlib import sha1
+from io import UnsupportedOperation
+
+from django.utils.encoding import force_bytes
+
+HASH_READ_SIZE = 65536  # 64k
+
+
+def hash_filelike(filelike):
+    """
+    Compute the hash of a file-like object, without loading it all into memory.
+    """
+    file_pos = 0
+    if hasattr(filelike, "tell"):
+        file_pos = filelike.tell()
+
+    try:
+        # Reset file handler to the start of the file so we hash it all
+        filelike.seek(0)
+    except (AttributeError, UnsupportedOperation):
+        pass
+
+    hasher = sha1()
+    while True:
+        data = filelike.read(HASH_READ_SIZE)
+        if not data:
+            break
+        # Use `force_bytes` to account for files opened as text
+        hasher.update(force_bytes(data))
+
+    if hasattr(filelike, "seek"):
+        # Reset the file handler to where it was before
+        filelike.seek(file_pos)
+
+    return hasher.hexdigest()