Prevent XSS in SVG with malicious script: Use Content-Security-Policy…

… instead of Content-Disposition header (#50) additionally, make this behavior configurable via OSGi configuration, as it may prevent special use cases e.g. SVGs animatd via JavaScript. by default, the feature is enabled.
wcm-io · Apr 25, 2024 · 4ab53a8 · 4ab53a8
1 parent bbde9be
commit 4ab53a8
Show file tree

Hide file tree

Showing 5 changed files with 74 additions and 7 deletions.
diff --git a/changes.xml b/changes.xml
@@ -23,6 +23,13 @@
     xsi:schemaLocation="http://maven.apache.org/changes/1.0.0 http://maven.apache.org/plugins/maven-changes-plugin/xsd/changes-1.0.0.xsd">
   <body>
 
+    <release version="2.0.6" date="not released">
+      <action type="fix" dev="sseifert" issue="50">
+        MediaFileServlet: Use Content-Security-Policy instead of Content-Disposition header to prevent XSS attacks in SVG files.
+        Make this behavior configurable via OSGi configuration, as it may prevent special use cases e.g. SVGs animatd via JavaScript.
+      </action>
+    </release>
+
     <release version="2.0.4" date="2024-04-17">
       <action type="fix" dev="sseifert" issue="49">
         Fix failing to resolve media when enforceVirtualRenditions is enabled and auto cropping is used at the same time.

diff --git a/src/main/java/io/wcm/handler/media/impl/AbstractMediaFileServlet.java b/src/main/java/io/wcm/handler/media/impl/AbstractMediaFileServlet.java
@@ -20,6 +20,7 @@
 package io.wcm.handler.media.impl;
 
 import static io.wcm.handler.media.impl.MediaFileServletConstants.HEADER_CONTENT_DISPOSITION;
+import static io.wcm.handler.media.impl.MediaFileServletConstants.HEADER_CONTENT_SECURITY_POLICY;
 import static io.wcm.handler.media.impl.MediaFileServletConstants.SELECTOR_DOWNLOAD;
 
 import java.io.IOException;
@@ -159,9 +160,9 @@ protected void sendBinaryData(byte @NotNull [] binaryData, @NotNull String conte
     }
 
     // special handling for SVG images which are not treated as download:
-    // force content disposition header to prevent stored XSS attack with malicious JavaScript in SVG file
-    else if (StringUtils.equals(contentType, ContentType.SVG)) {
-      setContentDispositionAttachmentHeader(request, response);
+    // set content security policy to prevent stored XSS attack with malicious JavaScript in SVG file
+    if (StringUtils.equals(contentType, ContentType.SVG)) {
+      setSVGContentSecurityPolicy(response);
     }
 
     // write binary data
@@ -181,4 +182,8 @@ private void setContentDispositionAttachmentHeader(@NotNull SlingHttpServletRequ
     response.setHeader(HEADER_CONTENT_DISPOSITION, dispositionHeader.toString());
   }
 
+  protected void setSVGContentSecurityPolicy(@NotNull SlingHttpServletResponse response) {
+    response.setHeader(HEADER_CONTENT_SECURITY_POLICY, "sandbox");
+  }
+
 }
diff --git a/src/main/java/io/wcm/handler/media/impl/MediaFileServlet.java b/src/main/java/io/wcm/handler/media/impl/MediaFileServlet.java
@@ -21,8 +21,14 @@
 
 import javax.servlet.Servlet;
 
+import org.apache.sling.api.SlingHttpServletResponse;
 import org.apache.sling.api.servlets.HttpConstants;
+import org.jetbrains.annotations.NotNull;
+import org.osgi.service.component.annotations.Activate;
 import org.osgi.service.component.annotations.Component;
+import org.osgi.service.metatype.annotations.AttributeDefinition;
+import org.osgi.service.metatype.annotations.Designate;
+import org.osgi.service.metatype.annotations.ObjectClassDefinition;
 
 import com.day.cq.commons.jcr.JcrConstants;
 
@@ -37,9 +43,34 @@
     "sling.servlet.resourceTypes=" + JcrConstants.NT_RESOURCE,
     "sling.servlet.methods=" + HttpConstants.METHOD_GET
 })
+@Designate(ocd = MediaFileServlet.Config.class)
 public final class MediaFileServlet extends AbstractMediaFileServlet {
   private static final long serialVersionUID = 1L;
 
-  // inherits all functionality
+  private boolean svgContentSecurityPolicy;
+
+  @ObjectClassDefinition(
+      name = "wcm.io Media Handler Media File Servlet",
+      description = "Configures delivery of media file binaries.")
+  @interface Config {
+
+    @AttributeDefinition(
+        name = "SVG Content Security Policy",
+        description = "Apply XSS protection when serving SVG files by setting Content-Security-Policy to 'sandbox'.")
+    boolean svgContentSecurityPolicy() default true;
+
+  }
+
+  @Activate
+  private void activate(Config config) {
+    this.svgContentSecurityPolicy = config.svgContentSecurityPolicy();
+  }
+
+  @Override
+  protected void setSVGContentSecurityPolicy(@NotNull SlingHttpServletResponse response) {
+    if (this.svgContentSecurityPolicy) {
+      super.setSVGContentSecurityPolicy(response);
+    }
+  }
 
 }
diff --git a/src/main/java/io/wcm/handler/media/impl/MediaFileServletConstants.java b/src/main/java/io/wcm/handler/media/impl/MediaFileServletConstants.java
@@ -34,6 +34,11 @@ public final class MediaFileServletConstants {
    */
   public static final String HEADER_CONTENT_DISPOSITION = "Content-Disposition";
 
+  /**
+   * Content disposition header
+   */
+  public static final String HEADER_CONTENT_SECURITY_POLICY = "Content-Security-Policy";
+
   /**
    * Selector
    */

diff --git a/src/test/java/io/wcm/handler/media/impl/MediaFileServletTest.java b/src/test/java/io/wcm/handler/media/impl/MediaFileServletTest.java
@@ -20,6 +20,7 @@
 package io.wcm.handler.media.impl;
 
 import static io.wcm.handler.media.impl.MediaFileServletConstants.HEADER_CONTENT_DISPOSITION;
+import static io.wcm.handler.media.impl.MediaFileServletConstants.HEADER_CONTENT_SECURITY_POLICY;
 import static io.wcm.handler.media.impl.MediaFileServletConstants.SELECTOR_DOWNLOAD;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
@@ -48,7 +49,7 @@ class MediaFileServletTest {
 
   @BeforeEach
   void setUp() {
-    underTest = new MediaFileServlet();
+    underTest = context.registerInjectActivateService(MediaFileServlet.class);
     context.currentResource(context.load().binaryFile("/sample_image_215x102.jpg", "/content/sample_image.jpg"));
   }
 
@@ -73,8 +74,26 @@ void testGet_SVG() throws Exception {
     assertEquals(ContentType.SVG, context.response().getContentType());
     assertEquals(EXPECTED_CONTENT_LENGTH_SVG, context.response().getOutput().length);
     assertEquals(EXPECTED_CONTENT_LENGTH_SVG, context.response().getContentLength());
-    // forced content disposition header for SVG to prevent stored XSS
-    assertEquals("attachment;", context.response().getHeader(HEADER_CONTENT_DISPOSITION));
+    // forced content security policy for SVG to prevent stored XSS
+    assertEquals("sandbox", context.response().getHeader(HEADER_CONTENT_SECURITY_POLICY));
+  }
+
+  @Test
+  void testGet_SVG_DisableContentSecurityPolicy() throws Exception {
+    // disable content security policy for SVG files
+    underTest = context.registerInjectActivateService(MediaFileServlet.class,
+        "svgContentSecurityPolicy", false);
+
+    context.currentResource(context.load().binaryFile("/filetype/sample.svg", "/content/sample.svg"));
+
+    underTest.service(context.request(), context.response());
+
+    assertEquals(HttpServletResponse.SC_OK, context.response().getStatus());
+    assertEquals(ContentType.SVG, context.response().getContentType());
+    assertEquals(EXPECTED_CONTENT_LENGTH_SVG, context.response().getOutput().length);
+    assertEquals(EXPECTED_CONTENT_LENGTH_SVG, context.response().getContentLength());
+    // no CSP
+    assertNull(context.response().getHeader(HEADER_CONTENT_SECURITY_POLICY));
   }
 
   @Test