From 07a890a857ff14c6077fa5eabab42bdf99061162 Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Fri, 1 Sep 2023 00:57:26 +0200 Subject: [PATCH] Force download of image scales when inline is not allowed for this mimetype. We already did this for the `@@display-file` view in 2021, but now it is done for image scales as well. See [security advisory](https://github.com/plone/plone.namedfile/security/advisories/GHSA-jj7c-jrv4-c65x). --- news/1.bugfix | 4 ++ plone/namedfile/scaling.py | 42 ++++++++++++++- plone/namedfile/tests/test_display_file.py | 63 +++++++++++++++++++++- 3 files changed, 105 insertions(+), 4 deletions(-) create mode 100644 news/1.bugfix diff --git a/news/1.bugfix b/news/1.bugfix new file mode 100644 index 00000000..c6cf8bbf --- /dev/null +++ b/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 `_. +[maurits] diff --git a/plone/namedfile/scaling.py b/plone/namedfile/scaling.py index ffa064dd..47503ddc 100644 --- a/plone/namedfile/scaling.py +++ b/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",) diff --git a/plone/namedfile/tests/test_display_file.py b/plone/namedfile/tests/test_display_file.py index 86251a8e..4ab467d3 100644 --- a/plone/namedfile/tests/test_display_file.py +++ b/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,12 +112,50 @@ 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")) @@ -111,10 +163,13 @@ def test_svg_image(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) 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) @@ -122,6 +177,8 @@ def test_filename_none(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) 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):