Merge pull request from GHSA-jj7c-jrv4-c65x

Force download of image scales when inline is not allowed for mime type [6.1.x]

news/1.bugfix

@@ -0,0 +1,4 @@
+Fix stored XSS (Cross Site Scripting) for SVG images.
+Done by forcing a download instead of displaying inline.
+See `security advisory <https://github.com/plone/plone.namedfile/security/advisories/GHSA-jj7c-jrv4-c65x>`_.
+[maurits]

plone/namedfile/scaling.py

@@ -4,6 +4,9 @@
 from io import BytesIO
 from plone.memoize import ram
 from plone.namedfile.file import FILECHUNK_CLASSES
+from plone.namedfile.browser import ALLOWED_INLINE_MIMETYPES
+from plone.namedfile.browser import DISALLOWED_INLINE_MIMETYPES
+from plone.namedfile.browser import USE_DENYLIST
 from plone.namedfile.interfaces import IAvailableSizes
 from plone.namedfile.interfaces import IStableImageScale
 from plone.namedfile.picture import get_picture_variants
@@ -75,6 +78,14 @@ class ImageScale(BrowserView):
     __allow_access_to_unprotected_subobjects__ = 1
     data = None
 
+    # 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
+
     def __init__(self, context, request, **info):
         self.context = context
         self.request = request
@@ -167,10 +178,37 @@ def validate_access(self):
         fieldname = getattr(self.data, "fieldname", getattr(self, "fieldname", None))
         guarded_getattr(self.context, fieldname)
 
+    def _should_force_download(self):
+        # If this returns True, the caller should call set_headers with a filename.
+        if not hasattr(self.data, "contentType"):
+            return
+        mimetype = self.data.contentType
+        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
+
+    def set_headers(self, response=None):
+        # set headers for the image
+        image = self.data
+        if response is None:
+            response = self.request.response
+        filename = None
+        if self._should_force_download():
+            # We MUST pass a filename, even a dummy one if needed.
+            filename = getattr(image, "filename", getattr(self, "filename", None))
+            if filename is None:
+                filename = getattr(image, "fieldname", getattr(self, "fieldname", None))
+                if filename is None:
+                    filename = "image.ext"
+        set_headers(image, response, filename=filename)
+
     def index_html(self):
         """download the image"""
         self.validate_access()
-        set_headers(self.data, self.request.response)
+        self.set_headers()
         return stream_data(self.data)
 
     def manage_DAVget(self):
@@ -190,7 +228,7 @@ def HEAD(self, REQUEST, RESPONSE=None):
         without transfer of the image itself
         """
         self.validate_access()
-        set_headers(self.data, REQUEST.response)
+        self.set_headers(response=REQUEST.response)
         return ""
 
     HEAD.__roles__ = ("Anonymous",)

plone/namedfile/tests/test_display_file.py

@@ -3,12 +3,15 @@
 from plone.app.testing import SITE_OWNER_PASSWORD
 from plone.namedfile import field
 from plone.namedfile import file
+from plone.namedfile.interfaces import IAvailableSizes
 from plone.namedfile.interfaces import IImageScaleTraversable
 from plone.namedfile.testing import PLONE_NAMEDFILE_FUNCTIONAL_TESTING
 from plone.namedfile.tests import getFile
 from plone.testing.zope import Browser
 from Products.CMFPlone.utils import safe_unicode
+from zope.annotation import IAnnotations
 from zope.annotation import IAttributeAnnotatable
+from zope.component import getSiteManager
 from zope.interface import implementer
 
 import transaction
@@ -46,6 +49,11 @@ def get_disposition_header(browser):
     return browser.headers.get(name, None)
 
 
+def custom_available_sizes():
+    # Define available image scales.
+    return {"custom": (10, 10)}
+
+
 class TestAttackVectorNamedImage(unittest.TestCase):
     layer = PLONE_NAMEDFILE_FUNCTIONAL_TESTING
     field_class = file.NamedImage
@@ -56,6 +64,12 @@ def setUp(self):
         item = DummyContent()
         self.layer["app"]._setOb("item", item)
         self.item = self.layer["app"].item
+        sm = getSiteManager()
+        sm.registerUtility(component=custom_available_sizes, provided=IAvailableSizes)
+
+    def tearDown(self):
+        sm = getSiteManager()
+        sm.unregisterUtility(provided=IAvailableSizes)
 
     def get_admin_browser(self):
         browser = Browser(self.layer["app"])
@@ -98,30 +112,73 @@ def assert_display_inline_is_download(self, base_url):
         self.assertIn("attachment", header)
         self.assertIn("filename", header)
 
+    def assert_scale_view_works(self, base_url):
+        # Test that accessing a scale view shows the image inline.
+        browser = self.get_anon_browser()
+        browser.open(base_url + "/@@images/{}".format(self.field_name))
+        self.assertIsNone(get_disposition_header(browser))
+
+        # Note: the 'custom' scale is defined in an adapter above.
+        browser.open(base_url + "/@@images/{}/custom".format(self.field_name))
+        self.assertIsNone(get_disposition_header(browser))
+
+        unique_scale_id = list(IAnnotations(self.item)["plone.scale"].keys())[0]
+        browser.open(base_url + "/@@images/{}".format(unique_scale_id))
+        self.assertIsNone(get_disposition_header(browser))
+
+    def assert_scale_view_is_download(self, base_url):
+        # Test that accessing a scale view turns into a download.
+        browser = self.get_anon_browser()
+        browser.open(base_url + "/@@images/{}".format(self.field_name))
+        header = get_disposition_header(browser)
+        self.assertIsNotNone(header)
+        self.assertIn("attachment", header)
+        self.assertIn("filename", header)
+
+        browser.open(base_url + "/@@images/{}/custom".format(self.field_name))
+        header = get_disposition_header(browser)
+        self.assertIsNotNone(header)
+        self.assertIn("attachment", header)
+        self.assertIn("filename", header)
+
+        unique_scale_id = list(IAnnotations(self.item)["plone.scale"].keys())[0]
+        browser.open(base_url + "/@@images/{}".format(unique_scale_id))
+        header = get_disposition_header(browser)
+        self.assertIsNotNone(header)
+        self.assertIn("attachment", header)
+        self.assertIn("filename", header)
+
     def test_png_image(self):
         setattr(self.item, self.field_name, self._named_file("image.png"))
         transaction.commit()
         base_url = self.item.absolute_url()
         self.assert_download_works(base_url)
         self.assert_display_inline_works(base_url)
+        if self.field_name == "image":
+            self.assert_scale_view_works(base_url)
 
     def test_svg_image(self):
         setattr(self.item, self.field_name, self._named_file("image.svg"))
         transaction.commit()
         base_url = self.item.absolute_url()
         self.assert_download_works(base_url)
         self.assert_display_inline_is_download(base_url)
+        if self.field_name == "image":
+            self.assert_scale_view_is_download(base_url)
 
     def test_filename_none(self):
-        # A 'None' filename None probably does not happen during normal upload,
-        # but if an attacker manages this, even @@download will show inline.
+        # A 'None' filename probably does not happen during normal upload,
+        # but if an attacker manages this, even @@download would show inline.
+        # We prevent this.
         data = self._named_file("image.svg")
         data.filename = None
         setattr(self.item, self.field_name, data)
         transaction.commit()
         base_url = self.item.absolute_url()
         self.assert_download_works(base_url)
         self.assert_display_inline_is_download(base_url)
+        if self.field_name == "image":
+            self.assert_scale_view_is_download(base_url)
 
     def test_filename_empty(self):
         # An empty filename is probably no problem, but let's check.
@@ -132,6 +189,8 @@ def test_filename_empty(self):
         base_url = self.item.absolute_url()
         self.assert_download_works(base_url)
         self.assert_display_inline_is_download(base_url)
+        if self.field_name == "image":
+            self.assert_scale_view_is_download(base_url)
 
 
 class TestAttackVectorNamedBlobImage(TestAttackVectorNamedImage):