From 5f44c23ac69db7d6d933d77f177e07603cf05f8b Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Thu, 21 Sep 2023 13:33:35 +0200 Subject: [PATCH] Merge pull request from GHSA-hc5c-r8m5-2gfh Done by forcing a download instead of displaying inline. Normal accessing via an image tag is not affected and is safe. --- news/1.bugfix | 5 ++ src/plone/restapi/services/users/get.py | 33 ++++++++++ src/plone/restapi/tests/image.svg | 3 + .../restapi/tests/test_services_users.py | 66 +++++++++++++++++++ 4 files changed, 107 insertions(+) create mode 100644 news/1.bugfix create mode 100644 src/plone/restapi/tests/image.svg diff --git a/news/1.bugfix b/news/1.bugfix new file mode 100644 index 0000000000..37a92b93ea --- /dev/null +++ b/news/1.bugfix @@ -0,0 +1,5 @@ +Fix stored XSS (Cross Site Scripting) for SVG image in user portrait. +Done by forcing a download instead of displaying inline. +Normal accessing via an image tag is not affected and is safe. +See `security advisory `_. +[maurits] diff --git a/src/plone/restapi/services/users/get.py b/src/plone/restapi/services/users/get.py index 10ed48af4a..bef0c3ef0e 100644 --- a/src/plone/restapi/services/users/get.py +++ b/src/plone/restapi/services/users/get.py @@ -2,6 +2,9 @@ from Acquisition import aq_inner from itertools import chain from plone.app.workflow.browser.sharing import merge_search_results +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.utils import stream_data from plone.restapi.interfaces import ISerializeToJson from plone.restapi.services import Service @@ -13,6 +16,7 @@ from typing import Iterable from typing import Sequence from urllib.parse import parse_qs +from urllib.parse import quote from zExceptions import BadRequest from zope.component import getMultiAdapter from zope.component import queryMultiAdapter @@ -220,6 +224,14 @@ def reply(self): @implementer(IPublishTraverse) class PortraitGet(Service): + # 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): super().__init__(context, request) self.params = [] @@ -237,6 +249,18 @@ def _get_user_id(self): raise Exception("Must supply exactly one parameter (user id)") return self.params[0] + def _should_force_download(self, portrait): + # If this returns True, the caller should set the Content-Disposition header. + mimetype = portrait.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 + def render(self): if len(self.params) == 1: # Retrieve the user portrait @@ -254,6 +278,15 @@ def render(self): self.request.response.setStatus(404) return None + if self._should_force_download(portrait): + # We need a filename, even a dummy one if needed. + ext = portrait.content_type.split("/")[-1].split("+")[0] + filename = f"{portrait.getId()}.{ext}" + filename = quote(filename.encode("utf8")) + self.request.response.setHeader( + "Content-Disposition", f"attachment; filename*=UTF-8''{filename}" + ) + self.request.response.setStatus(200) self.request.response.setHeader("Content-Type", portrait.content_type) diff --git a/src/plone/restapi/tests/image.svg b/src/plone/restapi/tests/image.svg new file mode 100644 index 0000000000..18d6411909 --- /dev/null +++ b/src/plone/restapi/tests/image.svg @@ -0,0 +1,3 @@ + + + \ No newline at end of file diff --git a/src/plone/restapi/tests/test_services_users.py b/src/plone/restapi/tests/test_services_users.py index b08170eafc..6de014a8cb 100644 --- a/src/plone/restapi/tests/test_services_users.py +++ b/src/plone/restapi/tests/test_services_users.py @@ -16,6 +16,7 @@ from zope.component import getAdapter from zope.component import getUtility +import base64 import os import re import transaction @@ -530,6 +531,30 @@ def test_update_portrait(self): self.assertEqual(response.status_code, 204) transaction.commit() + def _update_portrait_with_svg(self): + here = os.path.dirname(__file__) + # icon from https://icons.getbootstrap.com/icons/person/ + path = os.path.join(here, "image.svg") + with open(path, "rb") as image: + data = base64.encodebytes(image.read()) + + payload = { + "portrait": { + "filename": "image.svg", + "encoding": "base64", + "data": data, + "content-type": "image/svg+xml", + } + } + self.api_session.auth = ("noam", "password") + response = self.api_session.patch("/@users/noam", json=payload) + + self.assertEqual(response.status_code, 204) + transaction.commit() + + def test_update_portrait_with_svg(self): + self._update_portrait_with_svg() + user = self.api_session.get("/@users/noam").json() self.assertTrue(user.get("portrait").endswith("/@portrait/noam")) @@ -1072,6 +1097,23 @@ def test_get_own_user_portrait(self): self.assertEqual(response.headers["Content-Type"], "image/gif") noam_api_session.close() + def test_get_own_user_portrait_with_svg(self): + self._update_portrait_with_svg() + + noam_api_session = RelativeSession(self.portal_url, test=self) + noam_api_session.headers.update({"Accept": "application/json"}) + noam_api_session.auth = ("noam", "password") + + response = noam_api_session.get("/@portrait") + + self.assertEqual(200, response.status_code) + self.assertEqual(response.headers["Content-Type"], "image/svg+xml") + self.assertEqual( + response.headers["Content-Disposition"], + "attachment; filename*=UTF-8''noam.svg", + ) + noam_api_session.close() + def test_get_own_user_portrait_logged_out(self): response = self.anon_api_session.get( "/@portrait", @@ -1089,6 +1131,9 @@ def test_get_user_portrait_not_set(self): def test_get_user_portrait(self): with self.makeRealImage() as image: pm = api.portal.get_tool("portal_membership") + # Note: if you would set an SVG in this way, this would give a + # PIL.UnidentifiedImageError, which is what happens in ClassicUI + # as well. pm.changeMemberPortrait(image, "noam") transaction.commit() @@ -1098,6 +1143,26 @@ def test_get_user_portrait(self): self.assertEqual(200, response.status_code) self.assertEqual(response.headers["Content-Type"], "image/gif") + self.assertIsNone(response.headers.get("Content-Disposition")) + + def test_get_user_portrait_with_svg(self): + # If we would upload an SVG in the same way as in + # test_get_user_portrait, with pm.changeMemberPortrait, + # this would actually give PIL.UnidentifiedImageError, + # which is what happens in ClassicUI as well. + # So update it with a restapi call instead. + self._update_portrait_with_svg() + + response = self.api_session.get( + "/@portrait/noam", + ) + + self.assertEqual(200, response.status_code) + self.assertEqual(response.headers["Content-Type"], "image/svg+xml") + self.assertEqual( + response.headers["Content-Disposition"], + "attachment; filename*=UTF-8''noam.svg", + ) def test_get_user_portrait_anonymous(self): with self.makeRealImage() as image: @@ -1111,6 +1176,7 @@ def test_get_user_portrait_anonymous(self): self.assertEqual(200, response.status_code) self.assertEqual(response.headers["Content-Type"], "image/gif") + self.assertIsNone(response.headers.get("Content-Disposition")) def test_get_user_portrait_if_email_login_enabled(self): # enable use_email_as_login