Zope vulnerable to Stored Cross Site Scripting with SVG images

@@ -10,6 +10,16 @@ https://zope.readthedocs.io/en/2.13/CHANGES.html
 4.8.10 (unreleased)
 -------------------
 
+- Allow only some image types to be displayed inline. Force download for
+  others, especially SVG images.  By default we use a list of allowed types.
+  You can switch a to a list of denied types by setting OS environment variable
+  ``OFS_IMAGE_USE_DENYLIST=1``.  You can override the allowed list with
+  environment variable ``ALLOWED_INLINE_MIMETYPES`` and the disallowed list
+  with ``DISALLOWED_INLINE_MIMETYPES``.  Separate multiple entries by either
+  comma or space.  This change only affects direct URL access.
+  ``<img src="image.svg" />`` works the same as before. (CVE-2023-42458)
+  See `security advisory <https://github.com/zopefoundation/Zope/security/advisories/GHSA-wm8q-9975-xh5v>`_.
+
 - Tighten down the ZMI frame source logic to only allow site-local sources.
   Problem reported by Miguel Segovia Gil.
 

@@ -13,16 +13,19 @@
 """Image object
 """
 
+import os
 import struct
 from email.generator import _make_boundary
 from io import BytesIO
 from io import TextIOBase
+from mimetypes import guess_extension
 from tempfile import TemporaryFile
 from warnings import warn
 
 from six import PY2
 from six import binary_type
 from six import text_type
+from six.moves.urllib.parse import quote
 
 import ZPublisher.HTTPRequest
 from AccessControl.class_init import InitializeClass
@@ -61,6 +64,64 @@
     from cgi import escape
 
 
+def _get_list_from_env(name, default=None):
+    """Get list from environment variable.
+
+    Supports splitting on comma or white space.
+    Use the default as fallback only when the variable is not set.
+    So if the env variable is set to an empty string, this will ignore the
+    default and return an empty list.
+    """
+    value = os.environ.get(name)
+    if value is None:
+        return default or []
+    value = value.strip()
+    if "," in value:
+        return value.split(",")
+    return value.split()
+
+
+# We have one list for allowed, and one for disallowed inline mimetypes.
+# This is for security purposes.
+# By default we use the allowlist.  We give integrators the option to choose
+# the denylist via an environment variable.
+ALLOWED_INLINE_MIMETYPES = _get_list_from_env(
+    "ALLOWED_INLINE_MIMETYPES",
+    default=[
+        "image/gif",
+        # The mimetypes registry lists several for jpeg 2000:
+        "image/jp2",
+        "image/jpeg",
+        "image/jpeg2000-image",
+        "image/jpeg2000",
+        "image/jpx",
+        "image/png",
+        "image/webp",
+        "image/x-icon",
+        "image/x-jpeg2000-image",
+        "text/plain",
+        # By popular request we allow PDF:
+        "application/pdf",
+    ]
+)
+DISALLOWED_INLINE_MIMETYPES = _get_list_from_env(
+    "DISALLOWED_INLINE_MIMETYPES",
+    default=[
+        "application/javascript",
+        "application/x-javascript",
+        "text/javascript",
+        "text/html",
+        "image/svg+xml",
+        "image/svg+xml-compressed",
+    ]
+)
+try:
+    USE_DENYLIST = os.environ.get("OFS_IMAGE_USE_DENYLIST")
+    USE_DENYLIST = bool(int(USE_DENYLIST))
+except (ValueError, TypeError, AttributeError):
+    USE_DENYLIST = False
+
+
 manage_addFileForm = DTMLFile(
     'dtml/imageAdd',
     globals(),
@@ -120,6 +181,13 @@ class File(
     Cacheable
 ):
     """A File object is a content object for arbitrary files."""
+    # You can control which mimetypes may be shown inline
+    # and which must always be downloaded, for security reasons.
+    # Make the configuration available on the class.
+    # Then subclasses can override this.
+    allowed_inline_mimetypes = ALLOWED_INLINE_MIMETYPES
+    disallowed_inline_mimetypes = DISALLOWED_INLINE_MIMETYPES
+    use_denylist = USE_DENYLIST
 
     meta_type = 'File'
     zmi_icon = 'far fa-file-archive'
@@ -418,6 +486,19 @@ def _range_request_handler(self, REQUEST, RESPONSE):
                         b'\r\n--' + boundary.encode('ascii') + b'--\r\n')
                     return True
 
+    def _should_force_download(self):
+        # If this returns True, the caller should set a
+        # Content-Disposition header with filename.
+        mimetype = self.content_type
+        if not mimetype:
+            return False
+        if self.use_denylist:
+            # We explicitly deny a few mimetypes, and allow the rest.
+            return mimetype in self.disallowed_inline_mimetypes
+        # Use the allowlist.
+        # We only explicitly allow a few mimetypes, and deny the rest.
+        return mimetype not in self.allowed_inline_mimetypes
+
     @security.protected(View)
     def index_html(self, REQUEST, RESPONSE):
         """
@@ -456,6 +537,24 @@ def index_html(self, REQUEST, RESPONSE):
         RESPONSE.setHeader('Content-Length', self.size)
         RESPONSE.setHeader('Accept-Ranges', 'bytes')
 
+        if self._should_force_download():
+            # We need a filename, even a dummy one if needed.
+            filename = self.getId()
+            if "." not in filename:
+                # This either returns None or ".some_extension"
+                ext = guess_extension(self.content_type, strict=False)
+                if not ext:
+                    # image/svg+xml -> svg
+                    ext = "." + self.content_type.split("/")[-1].split("+")[0]
+                filename += ext
+            if not isinstance(filename, bytes):
+                filename = filename.encode("utf8")
+            filename = quote(filename)
+            RESPONSE.setHeader(
+                "Content-Disposition",
+                "attachment; filename*=UTF-8''{}".format(filename),
+            )
+
         if self.ZCacheable_isCachingEnabled():
             result = self.ZCacheable_get(default=None)
             if result is not None:

@@ -368,6 +368,7 @@ def testViewImageOrFile(self):
         response = request.RESPONSE
         result = self.file.index_html(request, response)
         self.assertEqual(result, self.data)
+        self.assertIsNone(response.getHeader("Content-Disposition"))
 
     def test_interfaces(self):
         from OFS.Image import Image
@@ -382,6 +383,61 @@ def test_text_representation_is_tag(self):
                          ' alt="" title="" height="16" width="16" />')
 
 
+class SVGTests(ImageTests):
+    content_type = 'image/svg+xml'
+
+    def testViewImageOrFile(self):
+        request = self.app.REQUEST
+        response = request.RESPONSE
+        result = self.file.index_html(request, response)
+        self.assertEqual(result, self.data)
+        self.assertEqual(
+            response.getHeader("Content-Disposition"),
+            "attachment; filename*=UTF-8''file.svg",
+        )
+
+    def testViewImageOrFileNonAscii(self):
+        try:
+            factory = getattr(self.app, self.factory)
+            factory('hällo',
+                    file=self.data, content_type=self.content_type)
+            transaction.commit()
+        except Exception:
+            transaction.abort()
+            self.connection.close()
+            raise
+        transaction.begin()
+        image = getattr(self.app, 'hällo')
+        request = self.app.REQUEST
+        response = request.RESPONSE
+        result = image.index_html(request, response)
+        self.assertEqual(result, self.data)
+        self.assertEqual(
+            response.getHeader("Content-Disposition"),
+            "attachment; filename*=UTF-8''h%C3%A4llo.svg",
+        )
+
+    def testViewImageOrFile_with_denylist(self):
+        request = self.app.REQUEST
+        response = request.RESPONSE
+        self.file.use_denylist = True
+        result = self.file.index_html(request, response)
+        self.assertEqual(result, self.data)
+        self.assertEqual(
+            response.getHeader("Content-Disposition"),
+            "attachment; filename*=UTF-8''file.svg",
+        )
+
+    def testViewImageOrFile_with_empty_denylist(self):
+        request = self.app.REQUEST
+        response = request.RESPONSE
+        self.file.use_denylist = True
+        self.file.disallowed_inline_mimetypes = []
+        result = self.file.index_html(request, response)
+        self.assertEqual(result, self.data)
+        self.assertIsNone(response.getHeader("Content-Disposition"))
+
+
 class FileEditTests(Testing.ZopeTestCase.FunctionalTestCase):
     """Browser testing ..Image.File"""
 

@@ -11,6 +11,16 @@ https://github.com/zopefoundation/Zope/blob/4.x/CHANGES.rst
 5.8.5 (unreleased)
 ------------------
 
+- Allow only some image types to be displayed inline. Force download for
+  others, especially SVG images.  By default we use a list of allowed types.
+  You can switch a to a list of denied types by setting OS environment variable
+  ``OFS_IMAGE_USE_DENYLIST=1``.  You can override the allowed list with
+  environment variable ``ALLOWED_INLINE_MIMETYPES`` and the disallowed list
+  with ``DISALLOWED_INLINE_MIMETYPES``.  Separate multiple entries by either
+  comma or space.  This change only affects direct URL access.
+  ``<img src="image.svg" />`` works the same as before. (CVE-2023-42458)
+  See `security advisory <https://github.com/zopefoundation/Zope/security/advisories/GHSA-wm8q-9975-xh5v>`_.
+
 - Tighten down the ZMI frame source logic to only allow site-local sources.
   Problem reported by Miguel Segovia Gil.
 

@@ -14,9 +14,12 @@
 """
 
 import html
+import os
 import struct
 from email.generator import _make_boundary
 from io import BytesIO
+from mimetypes import guess_extension
+from urllib.parse import quote
 from xml.dom import minidom
 
 import ZPublisher.HTTPRequest
@@ -48,6 +51,64 @@
 from ZPublisher.HTTPRequest import FileUpload
 
 
+def _get_list_from_env(name, default=None):
+    """Get list from environment variable.
+
+    Supports splitting on comma or white space.
+    Use the default as fallback only when the variable is not set.
+    So if the env variable is set to an empty string, this will ignore the
+    default and return an empty list.
+    """
+    value = os.environ.get(name)
+    if value is None:
+        return default or []
+    value = value.strip()
+    if "," in value:
+        return value.split(",")
+    return value.split()
+
+
+# We have one list for allowed, and one for disallowed inline mimetypes.
+# This is for security purposes.
+# By default we use the allowlist.  We give integrators the option to choose
+# the denylist via an environment variable.
+ALLOWED_INLINE_MIMETYPES = _get_list_from_env(
+    "ALLOWED_INLINE_MIMETYPES",
+    default=[
+        "image/gif",
+        # The mimetypes registry lists several for jpeg 2000:
+        "image/jp2",
+        "image/jpeg",
+        "image/jpeg2000-image",
+        "image/jpeg2000",
+        "image/jpx",
+        "image/png",
+        "image/webp",
+        "image/x-icon",
+        "image/x-jpeg2000-image",
+        "text/plain",
+        # By popular request we allow PDF:
+        "application/pdf",
+    ]
+)
+DISALLOWED_INLINE_MIMETYPES = _get_list_from_env(
+    "DISALLOWED_INLINE_MIMETYPES",
+    default=[
+        "application/javascript",
+        "application/x-javascript",
+        "text/javascript",
+        "text/html",
+        "image/svg+xml",
+        "image/svg+xml-compressed",
+    ]
+)
+try:
+    USE_DENYLIST = os.environ.get("OFS_IMAGE_USE_DENYLIST")
+    USE_DENYLIST = bool(int(USE_DENYLIST))
+except (ValueError, TypeError, AttributeError):
+    USE_DENYLIST = False
+
+
 manage_addFileForm = DTMLFile(
     'dtml/imageAdd',
     globals(),
@@ -107,6 +168,13 @@ class File(
     Cacheable
 ):
     """A File object is a content object for arbitrary files."""
+    # You can control which mimetypes may be shown inline
+    # and which must always be downloaded, for security reasons.
+    # Make the configuration available on the class.
+    # Then subclasses can override this.
+    allowed_inline_mimetypes = ALLOWED_INLINE_MIMETYPES
+    disallowed_inline_mimetypes = DISALLOWED_INLINE_MIMETYPES
+    use_denylist = USE_DENYLIST
 
     meta_type = 'File'
     zmi_icon = 'far fa-file-archive'
@@ -403,6 +471,19 @@ def _range_request_handler(self, REQUEST, RESPONSE):
                         b'\r\n--' + boundary.encode('ascii') + b'--\r\n')
                     return True
 
+    def _should_force_download(self):
+        # If this returns True, the caller should set a
+        # Content-Disposition header with filename.
+        mimetype = self.content_type
+        if not mimetype:
+            return False
+        if self.use_denylist:
+            # We explicitly deny a few mimetypes, and allow the rest.
+            return mimetype in self.disallowed_inline_mimetypes
+        # Use the allowlist.
+        # We only explicitly allow a few mimetypes, and deny the rest.
+        return mimetype not in self.allowed_inline_mimetypes
+
     @security.protected(View)
     def index_html(self, REQUEST, RESPONSE):
         """
@@ -441,6 +522,22 @@ def index_html(self, REQUEST, RESPONSE):
         RESPONSE.setHeader('Content-Length', self.size)
         RESPONSE.setHeader('Accept-Ranges', 'bytes')
 
+        if self._should_force_download():
+            # We need a filename, even a dummy one if needed.
+            filename = self.getId()
+            if "." not in filename:
+                # This either returns None or ".some_extension"
+                ext = guess_extension(self.content_type, strict=False)
+                if not ext:
+                    # image/svg+xml -> svg
+                    ext = "." + self.content_type.split("/")[-1].split("+")[0]
+                filename += f"{ext}"
+            filename = quote(filename.encode("utf8"))
+            RESPONSE.setHeader(
+                "Content-Disposition",
+                f"attachment; filename*=UTF-8''{filename}",
+            )
+
         if self.ZCacheable_isCachingEnabled():
             result = self.ZCacheable_get(default=None)
             if result is not None:

@@ -373,6 +373,7 @@ def testViewImageOrFile(self):
         response = request.RESPONSE
         result = self.file.index_html(request, response)
         self.assertEqual(result, self.data)
+        self.assertIsNone(response.getHeader("Content-Disposition"))
 
     def test_interfaces(self):
         from OFS.Image import Image
@@ -387,6 +388,61 @@ def test_text_representation_is_tag(self):
                          ' alt="" title="" height="16" width="16" />')
 
 
+class SVGTests(ImageTests):
+    content_type = 'image/svg+xml'
+
+    def testViewImageOrFile(self):
+        request = self.app.REQUEST
+        response = request.RESPONSE
+        result = self.file.index_html(request, response)
+        self.assertEqual(result, self.data)
+        self.assertEqual(
+            response.getHeader("Content-Disposition"),
+            "attachment; filename*=UTF-8''file.svg",
+        )
+
+    def testViewImageOrFileNonAscii(self):
+        try:
+            factory = getattr(self.app, self.factory)
+            factory('hällo',
+                    file=self.data, content_type=self.content_type)
+            transaction.commit()
+        except Exception:
+            transaction.abort()
+            self.connection.close()
+            raise
+        transaction.begin()
+        image = getattr(self.app, 'hällo')
+        request = self.app.REQUEST
+        response = request.RESPONSE
+        result = image.index_html(request, response)
+        self.assertEqual(result, self.data)
+        self.assertEqual(
+            response.getHeader("Content-Disposition"),
+            "attachment; filename*=UTF-8''h%C3%A4llo.svg",
+        )
+
+    def testViewImageOrFile_with_denylist(self):
+        request = self.app.REQUEST
+        response = request.RESPONSE
+        self.file.use_denylist = True
+        result = self.file.index_html(request, response)
+        self.assertEqual(result, self.data)
+        self.assertEqual(
+            response.getHeader("Content-Disposition"),
+            "attachment; filename*=UTF-8''file.svg",
+        )
+
+    def testViewImageOrFile_with_empty_denylist(self):
+        request = self.app.REQUEST
+        response = request.RESPONSE
+        self.file.use_denylist = True
+        self.file.disallowed_inline_mimetypes = []
+        result = self.file.index_html(request, response)
+        self.assertEqual(result, self.data)
+        self.assertIsNone(response.getHeader("Content-Disposition"))
+
+
 class FileEditTests(Testing.ZopeTestCase.FunctionalTestCase):
     """Browser testing ..Image.File"""