From d4219841dd87d23c2241651a24f7ed49b59e0bba Mon Sep 17 00:00:00 2001 From: robinvandermolen Date: Tue, 25 Feb 2025 11:04:48 +0100 Subject: [PATCH 1/2] :safety_vest: [open-formulieren/security-issues#35] SVG sanitation Added svg sanitation to the SVGOrImageField class, using the bleach library. The sanitation eliminates script tags, event handlers and non-essential tags and attributes (like Animation stuff, data-*, href) The allowed tags and attributes are derived from the MDN documentation about SVG content. --- src/openforms/utils/fields.py | 5 +- src/openforms/utils/form_fields.py | 8 +- src/openforms/utils/sanitizer.py | 196 +++++++++++++++++++++++++++++ 3 files changed, 203 insertions(+), 6 deletions(-) create mode 100644 src/openforms/utils/sanitizer.py diff --git a/src/openforms/utils/fields.py b/src/openforms/utils/fields.py index 4f2f314184..2dd9619133 100644 --- a/src/openforms/utils/fields.py +++ b/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( **{ diff --git a/src/openforms/utils/form_fields.py b/src/openforms/utils/form_fields.py index 3e0d8a6b21..17606a35a6 100644 --- a/src/openforms/utils/form_fields.py +++ b/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 diff --git a/src/openforms/utils/sanitizer.py b/src/openforms/utils/sanitizer.py new file mode 100644 index 0000000000..b3e188a5eb --- /dev/null +++ b/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, + ) From cd1f6a654334295a5e6f30558b6dd52b451c3f5d Mon Sep 17 00:00:00 2001 From: robinvandermolen Date: Tue, 25 Feb 2025 12:36:57 +0100 Subject: [PATCH 2/2] :white_check_mark: [open-formulieren/security-issues#35] Testing svg sanitizer --- .../config/tests/test_global_configuration.py | 52 ++++++++++- src/openforms/config/tests/test_theme.py | 48 ++++++++++ src/openforms/utils/tests/test_sanitizer.py | 89 +++++++++++++++++++ 3 files changed, 188 insertions(+), 1 deletion(-) create mode 100644 src/openforms/utils/tests/test_sanitizer.py diff --git a/src/openforms/config/tests/test_global_configuration.py b/src/openforms/config/tests/test_global_configuration.py index f1cc69f668..6acadb5658 100644 --- a/src/openforms/config/tests/test_global_configuration.py +++ b/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 = """ + + + + + + + + """ + sanitized_svg_content = b""" + + + // + alert("I am malicious >:)") + // + + + + + """ + + 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): diff --git a/src/openforms/config/tests/test_theme.py b/src/openforms/config/tests/test_theme.py index c21683ec9d..886fd505ca 100644 --- a/src/openforms/config/tests/test_theme.py +++ b/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 = """ + + + + + + + + """ + sanitized_svg_content = b""" + + + // + alert("I am malicious >:)") + // + + + + + """ + 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() diff --git a/src/openforms/utils/tests/test_sanitizer.py b/src/openforms/utils/tests/test_sanitizer.py new file mode 100644 index 0000000000..787fd3f846 --- /dev/null +++ b/src/openforms/utils/tests/test_sanitizer.py @@ -0,0 +1,89 @@ +import io + +from django.test import SimpleTestCase + +from ..sanitizer import sanitize_svg_content, sanitize_svg_file + + +class SanitizeSvgFileTests(SimpleTestCase): + def test_sanitize_svg_content_removes_script_tags(self): + bad_svg_content = """ + + + + + + + + """ + sanitized_svg_content = b""" + + + // + alert("I am malicious >:)") + // + + + + + """ + + temp_file = io.BytesIO(bad_svg_content.encode("utf-8")) + + # Assert that sanitize_svg_content removed the script tag + sanitized_svg_file = sanitize_svg_file(temp_file) + self.assertEqual(sanitized_svg_file.read(), sanitized_svg_content) + + +class SanitizeSvgContentTests(SimpleTestCase): + def test_sanitize_svg_content_removes_script_tags(self): + bad_svg_content = """ + + + + + + + + """ + sanitized_svg_content = """ + + + // + alert("I am malicious >:)") + // + + + + + """ + + # Assert that sanitize_svg_content removed the script tag + sanitized_bad_svg_content = sanitize_svg_content(bad_svg_content) + self.assertEqual(sanitized_svg_content, sanitized_bad_svg_content) + + def test_sanitize_svg_content_removes_event_handlers(self): + bad_svg_content = """ + + + + + + + """ + sanitized_svg_content = """ + + + + + + + """ + + # Assert that sanitize_svg_content removed the event handlers + sanitized_bad_svg_content = sanitize_svg_content(bad_svg_content) + self.assertEqual(sanitized_svg_content, sanitized_bad_svg_content)