Merge pull request #529 from actinium15/issue/526

#526 Preventing SSRF due to external resource refs in SVGs

imageio/imageio-batik/pom.xml

@@ -14,6 +14,21 @@
         See the <a href="http://xmlgraphics.apache.org/batik/">Batik Home page</a>
         for more information.]]>
     </description>
+    <build>
+        <plugins>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-surefire-plugin</artifactId>
+                <configuration>
+                    <systemPropertyVariables>
+                        <com.twelvemonkeys.imageio.plugins.svg.allowexternalresources>
+                            true
+                        </com.twelvemonkeys.imageio.plugins.svg.allowexternalresources>
+                    </systemPropertyVariables>
+                </configuration>
+            </plugin>
+        </plugins>
+    </build>
 
     <dependencies>
         <dependency>
@@ -88,6 +103,6 @@
     </dependencies>
 
     <properties>
-        <batik.version>1.9</batik.version>
+        <batik.version>1.12</batik.version>
     </properties>
 </project>

imageio/imageio-batik/src/main/java/com/twelvemonkeys/imageio/plugins/svg/SVGImageReader.java

@@ -79,7 +79,12 @@ import java.util.Map;
  * @see <A href="http://www.mail-archive.com/batik-dev@xml.apache.org/msg00992.html">batik-dev</A>
  */
 public class SVGImageReader extends ImageReaderBase {
+
+    final static boolean DEFAULT_ALLOW_EXTERNAL_RESOURCES =
+            "true".equalsIgnoreCase(System.getProperty("com.twelvemonkeys.imageio.plugins.svg.allowexternalresources"));
+
     private Rasterizer rasterizer;
+    private boolean allowExternalResources = DEFAULT_ALLOW_EXTERNAL_RESOURCES;
 
     /**
      * Creates an {@code SVGImageReader}.
@@ -116,6 +121,9 @@ public class SVGImageReader extends ImageReaderBase {
         if (pParam instanceof SVGReadParam) {
             SVGReadParam svgParam = (SVGReadParam) pParam;
 
+            // set the external-resource-resolution preference
+            allowExternalResources = svgParam.isAllowExternalResources();
+
             // Get the base URI
             // This must be done before converting the params to hints
             String baseURI = svgParam.getBaseURI();
@@ -641,6 +649,14 @@ public class SVGImageReader extends ImageReaderBase {
             public void displayMessage(String message) {
                 processWarningOccurred(message.replaceAll("[\\r\\n]+", " "));
             }
+
+            @Override
+            public ExternalResourceSecurity getExternalResourceSecurity(ParsedURL resourceURL, ParsedURL docURL) {
+                if (allowExternalResources) {
+                    return super.getExternalResourceSecurity(resourceURL, docURL);
+                }
+                return new NoLoadExternalResourceSecurity();
+            }
         }
     }
 }

imageio/imageio-batik/src/main/java/com/twelvemonkeys/imageio/plugins/svg/SVGReadParam.java

@@ -41,6 +41,11 @@ import java.awt.*;
 public class SVGReadParam extends ImageReadParam {
     private Paint background;
     private String baseURI;
+    private boolean allowExternalResources = SVGImageReader.DEFAULT_ALLOW_EXTERNAL_RESOURCES;
+
+    public SVGReadParam() {
+        super();
+    }
 
     public Paint getBackgroundColor() {
         return background;
@@ -58,6 +63,14 @@ public class SVGReadParam extends ImageReadParam {
         baseURI = pBaseURI;
     }
 
+    public void setAllowExternalResources(boolean allow) {
+        allowExternalResources = allow;
+    }
+
+    public boolean isAllowExternalResources() {
+        return allowExternalResources;
+    }
+
     @Override
     public boolean canSetSourceRenderSize() {
         return true;

imageio/imageio-batik/src/test/java/com/twelvemonkeys/imageio/plugins/svg/SVGImageReaderTest.java

@@ -49,7 +49,6 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.net.URISyntaxException;
 import java.net.URL;
-import java.nio.Buffer;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
@@ -67,6 +66,7 @@ import static org.mockito.Mockito.*;
  * @version $Id: SVGImageReaderTest.java,v 1.0 Apr 1, 2008 10:39:17 PM haraldk Exp$
  */
 public class SVGImageReaderTest extends ImageReaderAbstractTest<SVGImageReader> {
+
     private SVGImageReaderSpi provider = new SVGImageReaderSpi();
 
     protected List<TestData> getTestData() {
@@ -228,6 +228,7 @@ public class SVGImageReaderTest extends ImageReaderAbstractTest<SVGImageReader>
             reader.addIIOReadWarningListener(listener);
 
             SVGReadParam param = reader.getDefaultReadParam();
+            param.setAllowExternalResources(true);
             param.setBaseURI(resource.toURI().toASCIIString());
             BufferedImage image = reader.read(0, param);
 
@@ -248,6 +249,8 @@ public class SVGImageReaderTest extends ImageReaderAbstractTest<SVGImageReader>
         // Asking for metadata, width, height etc, before attempting to read using a param,
         // will cause the document to be parsed without a base URI.
         // This will work, but may not use the CSS...
+        // since the param is not available before the read operation is invoked,
+        // this test-case MUST use the system-property for backwards compatibility
         URL resource = getClassLoaderResource("/svg/barChart.svg");
 
         SVGImageReader reader = createReader();
@@ -286,18 +289,17 @@ public class SVGImageReaderTest extends ImageReaderAbstractTest<SVGImageReader>
     public void testEmbeddedNoBaseURI() throws IOException {
         // With no base URI, we will throw an exception, about the missing embedded resource
         URL resource = getClassLoaderResource("/svg/barChart.svg");
-
         SVGImageReader reader = createReader();
 
         TestData data = new TestData(resource, (Dimension) null);
         try (ImageInputStream stream = data.getInputStream()) {
             reader.setInput(stream);
 
-            BufferedImage image = reader.read(0);
+            SVGReadParam params = reader.getDefaultReadParam();
+            params.setAllowExternalResources(true);
+            reader.read(0, params);
 
-            assertNotNull(image);
+            assertTrue("reader.read should've thrown an exception, but didn't", false);
-            assertEquals(450, image.getWidth());
-            assertEquals(500, image.getHeight());
         }
         catch (IIOException allowed) {
             assertTrue(allowed.getMessage().contains("batikLogo.svg")); // The embedded resource we don't find
@@ -306,4 +308,28 @@ public class SVGImageReaderTest extends ImageReaderAbstractTest<SVGImageReader>
             reader.dispose();
         }
     }
+
+    @Test(expected = SecurityException.class)
+    public void testDisallowedExternalResources() throws URISyntaxException, IOException {
+        // system-property set to true in surefire-plugin-settings in the pom
+        URL resource = getClassLoaderResource("/svg/barChart.svg");
+        SVGImageReader reader = createReader();
+
+        TestData data = new TestData(resource, (Dimension) null);
+        try (ImageInputStream stream = data.getInputStream()) {
+            reader.setInput(stream);
+
+            SVGReadParam param = reader.getDefaultReadParam();
+            param.setBaseURI(resource.toURI().toASCIIString());
+            param.setAllowExternalResources(false);
+            // even when the system-property is set to true,
+            // `reader.read` for `/svg/barChart.svg` should raise
+            // a SecurityException when External Resources are blocked
+            // because the API invocation gets preference
+            reader.read(0, param);
+        }
+        finally {
+            reader.dispose();
+        }
+    }
 }