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

@@ -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 <https://github.com/plone/plone.restapi/security/advisories/GHSA-hc5c-r8m5-2gfh>`_.
+[maurits]

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)
 

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