Ticket #19286: #19286_--_External_entities_security_vulnerability_(e_g__tagging_presets).patch

src/org/openstreetmap/josm/tools/XmlUtils.java

@@ -35 +35 @@
  */
 public final class XmlUtils {
 
+    private static final String FEATURE_DISALLOW_DOCTYPE_DECL = "http://apache.org/xml/features/disallow-doctype-decl";
+
     private XmlUtils() {
         // Hide default constructor for utils classes
     }
@@ -100 +102 @@
     public static SAXParser newSafeSAXParser() throws ParserConfigurationException, SAXException {
         SAXParserFactory parserFactory = SAXParserFactory.newInstance();
         parserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+        parserFactory.setFeature(FEATURE_DISALLOW_DOCTYPE_DECL, true);
         parserFactory.setNamespaceAware(true);
         return parserFactory.newSAXParser();
     }

test/data/dom_external_entity.xml

@@ +1 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE updateProfile [
+  <!ENTITY file SYSTEM "file:///etc/passwd">
+]>
+<root>
+    &file;
+</root>

test/data/preset_external_entity.xml

@@ +1 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE updateProfile [
+  <!ENTITY file SYSTEM "file:///etc/passwd">
+]>
+<presets xmlns="http://josm.openstreetmap.de/tagging-preset-1.0">
+    &file;
+</presets>

test/unit/org/openstreetmap/josm/gui/tagging/presets/TaggingPresetReaderTest.java

@@ -5 +5 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import java.io.IOException;
 import java.util.Collection;
@@ -65 +66 @@
         assertEquals("[A1, A2, A3, B1, B2, B3, C1, C2, C3]", keys.toString());
     }
 
+    /**
+     * Test external entity resolving.
+     * See #19286
+     */
+    @Test
+    public void testExternalEntityResolving() throws IOException {
+        try {
+            TaggingPresetReader.readAll(TestUtils.getTestDataRoot() + "preset_external_entity.xml", true);
+            fail("Reading a file with external entities should throw an SAXParseException!");
+        } catch (SAXException e) {
+            Assert.assertEquals("DOCTYPE is disallowed when the feature \"http://apache.org/xml/features/disallow-doctype-decl\" set to true.", e.getMessage());
+        }
+    }
+
     /**
      * Validate internal presets
      * See #9027

test/unit/org/openstreetmap/josm/tools/XmlUtilsTest.java

@@ +1 @@
+// License: GPL. For details, see LICENSE file.
+package org.openstreetmap.josm.tools;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import org.junit.Rule;
+import org.junit.Test;
+import org.openstreetmap.josm.TestUtils;
+import org.openstreetmap.josm.testutils.JOSMTestRules;
+import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
+import org.xml.sax.helpers.DefaultHandler;
+
+import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.transform.Transformer;
+import javax.xml.transform.TransformerException;
+import javax.xml.transform.stream.StreamResult;
+import javax.xml.transform.stream.StreamSource;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.StringWriter;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
+
+/**
+ * Unit tests of {@link XmlUtils} class.
+ */
+public class XmlUtilsTest {
+
+    /**
+     * Use default, basic test rules.
+     */
+    @Rule
+    @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
+    public JOSMTestRules rules = new JOSMTestRules();
+
+    @Test
+    public void testExternalEntitiesParsingDom() throws IOException, ParserConfigurationException {
+        try {
+            final String source = TestUtils.getTestDataRoot() + "dom_external_entity.xml";
+            XmlUtils.parseSafeDOM(new FileInputStream(source));
+            fail("Parsing a document with external entities should not be allowed.");
+        } catch (SAXException e) {
+            assertEquals("External Entity: Failed to read external document 'passwd', because 'file' access is not allowed due to restriction set by the accessExternalDTD property.", e.getMessage());
+        }
+    }
+
+    @Test
+    public void testExternalEntitiesSaxParser() throws IOException, ParserConfigurationException {
+        try {
+            final String source = TestUtils.getTestDataRoot() + "dom_external_entity.xml";
+            final DefaultHandler handler = new DefaultHandler();
+            XmlUtils.parseSafeSAX(new InputSource(new FileInputStream(source)), handler);
+            fail("Parsing a document with external entities should not be allowed.");
+        } catch (SAXException e) {
+            assertEquals("External Entity: Failed to read external document 'passwd', because 'file' access is not allowed due to restriction set by the accessExternalDTD property.", e.getMessage());
+        }
+    }
+
+    @Test
+    public void testExternalEntitiesTransformer() throws IOException {
+        try {
+            final String source = TestUtils.getTestDataRoot() + "dom_external_entity.xml";
+            final Transformer transformer = XmlUtils.newSafeTransformerFactory().newTransformer();
+            transformer.transform(new StreamSource(new FileInputStream(source)), new StreamResult(new StringWriter()));
+            fail("Parsing a document with external entities should not be allowed.");
+        } catch (TransformerException e) {
+            assertNotNull(e.getCause());
+            assertEquals("External Entity: Failed to read external document 'passwd', because 'file' access is not allowed due to restriction set by the accessExternalDTD property.", e.getCause().getMessage());
+        }
+    }
+}
+ No newline at end of file