Merge pull request #5120 from open-formulieren/issue/svg-sanitize

svg sanitizing

src/openforms/config/tests/test_global_configuration.py

@@ -12,7 +12,7 @@
 import clamd
 from django_webtest import WebTest
 from maykin_2fa.test import disable_admin_mfa
-from webtest import Form as WebTestForm
+from webtest import Form as WebTestForm, Upload
 
 from openforms.accounts.tests.factories import SuperUserFactory
 from openforms.tests.utils import NOOP_CACHES
@@ -165,6 +165,56 @@ def test_virus_scan_enabled_cant_connect(self):
             "Cannot connect to ClamAV: Cannot connect!",
         )
 
+    @override_settings(LANGUAGE_CODE="en")
+    def test_svg_sanitation_favicon(self):
+        bad_svg_content = """
+        <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 50 50">
+            <circle cx="25" cy="25" r="25" fill="green" onclick="alert(\'forget about me?\')" />
+            <script>//<![CDATA[
+                alert("I am malicious >:)")
+            //]]></script>
+            <g>
+                <rect class="btn" x="0" y="0" width="10" height="10" fill="red" onload="alert(\'click!\')" />
+            </g>
+        </svg>
+        """
+        sanitized_svg_content = b"""
+        <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 50 50">
+            <circle cx="25" cy="25" r="25" fill="green"></circle>
+            //
+                alert("I am malicious &gt;:)")
+            //
+            <g>
+                <rect x="0" y="0" width="10" height="10" fill="red"></rect>
+            </g>
+        </svg>
+        """
+
+        config = GlobalConfiguration.get_solo()
+
+        with self.subTest(part="admin config"):
+            url = reverse("admin:config_globalconfiguration_change", args=(1,))
+            change_page = self.app.get(url)
+
+            upload = Upload(
+                "bad_svg.svg", bad_svg_content.encode("utf-8"), "image/svg+xml"
+            )
+
+            form = change_page.forms["globalconfiguration_form"]
+            form["favicon"] = upload
+            _ensure_arrayfields(form, config=config)
+            response = form.submit()
+
+            self.assertEqual(response.status_code, 302)
+            config.refresh_from_db()
+            self.assertEqual(config.favicon, "logo/bad_svg.svg")
+
+        with self.subTest(part="assert favicon sanitized"):
+            with config.favicon.file.open("r") as favicon_file:
+                # Assert that the logo is completely sanitized
+                decoded_logo = favicon_file.read()
+                self.assertEqual(decoded_logo, sanitized_svg_content)
+
 
 class GlobalConfirmationEmailTests(TestCase):
     def setUp(self):

src/openforms/config/tests/test_theme.py

@@ -77,6 +77,54 @@ def test_upload_svg(self):
                 style_tag.text(),
             )
 
+    def test_upload_malicious_svg(self):
+        bad_svg_content = """
+        <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 50 50">
+            <circle cx="25" cy="25" r="25" fill="green" onclick="alert('forget about me?')" />
+            <script>//<![CDATA[
+                alert("I am malicious >:)")
+            //]]></script>
+            <g>
+                <rect class="btn" x="0" y="0" width="10" height="10" fill="red" onload="alert('click!')" />
+            </g>
+        </svg>
+        """
+        sanitized_svg_content = b"""
+        <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 50 50">
+            <circle cx="25" cy="25" r="25" fill="green"></circle>
+            //
+                alert("I am malicious &gt;:)")
+            //
+            <g>
+                <rect x="0" y="0" width="10" height="10" fill="red"></rect>
+            </g>
+        </svg>
+        """
+        theme = ThemeFactory.create()
+
+        with self.subTest(part="admin config"):
+            url = reverse("admin:config_theme_change", args=(theme.pk,))
+
+            change_page = self.app.get(url)
+
+            upload = Upload(
+                "bad_svg.svg", bad_svg_content.encode("utf-8"), "image/svg+xml"
+            )
+
+            form = change_page.forms["theme_form"]
+            form["logo"] = upload
+            response = form.submit()
+
+            self.assertEqual(response.status_code, 302)
+            theme.refresh_from_db()
+            self.assertEqual(theme.logo, "logo/bad_svg.svg")
+
+        with self.subTest(part="assert logo sanitized"):
+            with theme.logo.file.open("r") as logo_file:
+                # Assert that the logo is completely sanitized
+                decoded_logo = logo_file.read()
+                self.assertEqual(decoded_logo, sanitized_svg_content)
+
     def test_upload_png(self):
         logo = Path(settings.DJANGO_PROJECT_DIR) / "static" / "img" / "digid.png"
         theme = ThemeFactory.create()

src/openforms/utils/fields.py

@@ -14,9 +14,8 @@ class StringUUIDField(models.UUIDField):
 
 
 class SVGOrImageField(models.ImageField):
-    # SVG's are not regular "images", so they get different treatment. Note that
-    # we're not doing extended sanitization *yet* here, so be careful when using
-    # this field.
+    # SVG's are not regular "images", so they get different treatment. Sanitization is
+    # done in the form_class, form_fields.SVGOrImageField.
     def formfield(self, **kwargs):
         return super().formfield(
             **{

src/openforms/utils/form_fields.py

@@ -9,6 +9,8 @@
 from django.db import models
 from django.forms import CheckboxSelectMultiple, ImageField, MultipleChoiceField
 
+from .sanitizer import sanitize_svg_file
+
 image_or_svg_extension_validator = FileExtensionValidator(
     allowed_extensions=["svg"] + list(get_available_image_extensions())
 )
@@ -44,10 +46,10 @@ def to_python(self, data):
         # get the extension
         extension = get_extension(data)
 
-        # SVG's are not regular "images", so they get different treatment. Note that
-        # we're not doing extended sanitization *yet* here, so be careful when using
-        # this field.
+        # SVG's are not regular "images", so they get different treatment.
         if extension == "svg":
+            data = sanitize_svg_file(data)
+
             # we call the parent of the original ImageField instead of calling to_python
             # of ImageField (the direct superclass), as that one tries to validate the
             # image with PIL

src/openforms/utils/sanitizer.py

@@ -0,0 +1,196 @@
+from django.core.files import File
+
+import bleach
+
+ALLOWED_SVG_TAGS = (
+    "circle",
+    "clipPath",
+    "defs",
+    "desc",
+    "ellipse",
+    "feBlend",
+    "feColorMatrix",
+    "feComponentTransfer",
+    "feComposite",
+    "feConvolveMatrix",
+    "feDiffuseLighting",
+    "feDisplacementMap",
+    "feDistantLight",
+    "feDropShadow",
+    "feFlood",
+    "feFuncA",
+    "feFuncB",
+    "feFuncG",
+    "feFuncR",
+    "feGaussianBlur",
+    "feImage",
+    "feMerge",
+    "feMergeNode",
+    "feMorphology",
+    "feOffset",
+    "fePointLight",
+    "feSpecularLighting",
+    "feSpotLight",
+    "feTile",
+    "feTurbulence",
+    "filter",
+    "foreignObject",
+    "g",
+    "image",
+    "line",
+    "linearGradient",
+    "marker",
+    "mask",
+    "metadata",
+    "mpath",
+    "path",
+    "pattern",
+    "polygon",
+    "polyline",
+    "radialGradient",
+    "rect",
+    "set",
+    "stop",
+    "style",
+    "svg",
+    "symbol",
+    "text",
+    "textPath",
+    "title",
+    "tspan",
+    "use",
+    "view",
+    # --- Not allowing 'a', 'animate*' and 'script' tags
+)
+
+ALLOWED_SVG_ATTRIBUTES = {
+    "*": [
+        # --- Basic presentation attributes
+        "alignment-baseline",
+        "baseline-shift",
+        "clip",
+        "clip-path",
+        "clip-rule",
+        "color",
+        "color-interpolation",
+        "color-interpolation-filters",
+        "cursor",
+        "cx",
+        "cy",
+        "d",
+        "direction",
+        "display",
+        "dominant-baseline",
+        "fill",
+        "fill-opacity",
+        "fill-rule",
+        "filter",
+        "flood-color",
+        "flood-opacity",
+        "font-family",
+        "font-size",
+        "font-size-adjust",
+        "font-stretch",
+        "font-style",
+        "font-variant",
+        "font-weight",
+        "glyph-orientation-horizontal",
+        "glyph-orientation-vertical",
+        "height",
+        "image-rendering",
+        "letter-spacing",
+        "lighting-color",
+        "marker-end",
+        "marker-mid",
+        "marker-start",
+        "mask",
+        "mask-type",
+        "opacity",
+        "overflow",
+        "pointer-events",
+        "r",
+        "rx",
+        "ry",
+        "shape-rendering",
+        "stop-color",
+        "stop-opacity",
+        "stroke",
+        "stroke-dasharray",
+        "stroke-dashoffset",
+        "stroke-linecap",
+        "stroke-linejoin",
+        "stroke-miterlimit",
+        "stroke-opacity",
+        "stroke-width",
+        "text-anchor",
+        "text-decoration",
+        "text-overflow",
+        "text-rendering",
+        "transform",
+        "transform-origin",
+        "unicode-bidi",
+        "vector-effect",
+        "visibility",
+        "white-space",
+        "width",
+        "word-spacing",
+        "writing-mode",
+        "x",
+        "y",
+        # --- Filter attributes
+        "amplitude",
+        "exponent",
+        "intercept",
+        "offset",
+        "slope",
+        "tableValues",
+        "type",
+        # --- Not allowing 'href', 'data-*', Animation and some other attributes
+    ],
+    "svg": ["xmlns", "viewBox"],
+}
+
+
+def sanitize_svg_file(data: File) -> File:
+    """
+    Defuse an uploaded SVG file.
+
+    The entire file content will be replaced with a sanitized version. All tags and
+    attributes that aren't explicitly allowed, are removed from the SVG content.
+
+    :arg data: the uploaded SVG file, opened in binary mode.
+    """
+    # Making sure that the file is reset properly
+    data.seek(0)
+
+    file_content = data.read().decode("utf-8")
+    sanitized_content = sanitize_svg_content(file_content)
+
+    # Replace svg file content with the bleached variant.
+    # `truncate(0)` doesn't reset the point, so start with a seek(0) to make sure the
+    # content is as expected.
+    data.seek(0)
+    data.truncate(0)
+    data.write(sanitized_content.encode("utf-8"))
+
+    # Reset pointer
+    data.seek(0)
+    return data
+
+
+def sanitize_svg_content(svg_content: str) -> str:
+    """
+    Strip (potentially) dangerous elements and attributes from the provided SVG string.
+
+    All tags and attributes that aren't explicitly allowed, are removed from the SVG
+    content.
+
+    :arg svg_content: decoded SVG content.
+    """
+
+    return bleach.clean(
+        svg_content,
+        tags=ALLOWED_SVG_TAGS,
+        attributes=ALLOWED_SVG_ATTRIBUTES,
+        strip=True,
+    )