From 26a55dbc301db417f47cafda6fe0f983b5690088 Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Thu, 21 Sep 2023 10:01:18 +0200 Subject: [PATCH] Merge pull request from GHSA-wm8q-9975-xh5v * 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``. This change only affects direct URL access. ```` works the same as before. See security advisory: https://github.com/zopefoundation/Zope/security/advisories/GHSA-wm8q-9975-xh5v * svg download: only encode filename when it is not bytes. On Python 2.7 it is already bytes. * Use guess_extension as first guess for the extension of an image. * Support overriding ALLOWED_INLINE_MIMETYPES and DISALLOWED_INLINE_MIMETYPES. * Test that filename of attachment gets encoded correctly. * Add CVE --------- Co-authored-by: Michael Howitz --- CHANGES.rst | 10 ++++ src/OFS/Image.py | 99 +++++++++++++++++++++++++++++++ src/OFS/tests/testFileAndImage.py | 56 +++++++++++++++++ 3 files changed, 165 insertions(+) diff --git a/CHANGES.rst b/CHANGES.rst index 5e1b1efa3c..707413ba9e 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -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. + ```` works the same as before. (CVE-2023-42458) + See `security advisory `_. + - Tighten down the ZMI frame source logic to only allow site-local sources. Problem reported by Miguel Segovia Gil. diff --git a/src/OFS/Image.py b/src/OFS/Image.py index 0e1caf53ef..1dfe7b30e4 100644 --- a/src/OFS/Image.py +++ b/src/OFS/Image.py @@ -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: diff --git a/src/OFS/tests/testFileAndImage.py b/src/OFS/tests/testFileAndImage.py index 264834fcc9..12276ec3f7 100644 --- a/src/OFS/tests/testFileAndImage.py +++ b/src/OFS/tests/testFileAndImage.py @@ -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"""