Changeset 10404 in josm for trunk

Timestamp:

2016-06-16T19:10:53+02:00 (8 years ago)

Author:

Don-vip

Message:

findbugs security - XML Parsing Vulnerable to XXE - enable FEATURE_SECURE_PROCESSING for DOM builders

Location:

trunk/src/org/openstreetmap/josm

Files:

• data/CustomConfigurator.java (modified) (3 diffs)
• gui/oauth/TestAccessTokenTask.java (modified) (3 diffs)
• io/OsmServerUserInfoReader.java (modified) (3 diffs)
• io/imagery/WMSImagery.java (modified) (2 diffs)
• io/session/SessionReader.java (modified) (2 diffs)
• io/session/SessionWriter.java (modified) (2 diffs)
• tools/Utils.java (modified) (3 diffs)
• tools/bugreport/BugReportSender.java (modified) (2 diffs)

trunk/src/org/openstreetmap/josm/data/CustomConfigurator.java

@@ -35 +35 @@
 import javax.swing.SwingUtilities;
 import javax.xml.parsers.DocumentBuilder;
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.stream.XMLStreamException;
@@ -269 +268 @@
         try {
             String toXML = Main.pref.toXML(true);
-            InputStream is = new ByteArrayInputStream(toXML.getBytes(StandardCharsets.UTF_8));
-            DocumentBuilderFactory builderFactory = DocumentBuilderFactory.newInstance();
-            builderFactory.setValidating(false);
-            builderFactory.setNamespaceAware(false);
-            DocumentBuilder builder = builderFactory.newDocumentBuilder();
-            document = builder.parse(is);
+            DocumentBuilder builder = Utils.newSafeDOMBuilder();
+            document = builder.parse(new ByteArrayInputStream(toXML.getBytes(StandardCharsets.UTF_8)));
             exportDocument = builder.newDocument();
             root = document.getDocumentElement();
@@ -465 +460 @@
         public void openAndReadXML(InputStream is) {
             try {
-                DocumentBuilderFactory builderFactory = DocumentBuilderFactory.newInstance();
-                builderFactory.setValidating(false);
-                builderFactory.setNamespaceAware(true);
-                DocumentBuilder builder = builderFactory.newDocumentBuilder();
-                Document document = builder.parse(is);
+                Document document = Utils.parseSafeDOM(is);
                 synchronized (CustomConfigurator.class) {
                     processXML(document);

trunk/src/org/openstreetmap/josm/gui/oauth/TestAccessTokenTask.java

@@ -10 +10 @@
 
 import javax.swing.JOptionPane;
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 
@@ -26 +25 @@
 import org.openstreetmap.josm.tools.CheckParameterUtil;
 import org.openstreetmap.josm.tools.HttpClient;
+import org.openstreetmap.josm.tools.Utils;
 import org.openstreetmap.josm.tools.XmlParsingException;
 import org.w3c.dom.Document;
@@ -124 +124 @@
                 throw new OsmApiException(connection.getResponse().getResponseCode(),
                         connection.getResponse().getHeaderField("Error"), null);
-            Document d = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(connection.getResponse().getContent());
+            Document d = Utils.parseSafeDOM(connection.getResponse().getContent());
             return OsmServerUserInfoReader.buildFromXML(d);
         } catch (SAXException | ParserConfigurationException e) {

trunk/src/org/openstreetmap/josm/io/OsmServerUserInfoReader.java

@@ -9 +9 @@
 import java.util.List;
 
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.xpath.XPath;
@@ -20 +19 @@
 import org.openstreetmap.josm.data.osm.UserInfo;
 import org.openstreetmap.josm.gui.progress.ProgressMonitor;
+import org.openstreetmap.josm.tools.Utils;
 import org.openstreetmap.josm.tools.XmlParsingException;
 import org.openstreetmap.josm.tools.date.DateUtils;
@@ -175 +175 @@
             monitor.indeterminateSubTask(tr("Reading user info ..."));
             try (InputStream in = getInputStream("user/details", monitor.createSubTaskMonitor(1, true), reason)) {
-                return buildFromXML(
-                        DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(in)
-                );
+                return buildFromXML(Utils.parseSafeDOM(in));
             }
         } catch (OsmTransferException e) {

trunk/src/org/openstreetmap/josm/io/imagery/WMSImagery.java

@@ -18 +18 @@
 import javax.imageio.ImageIO;
 import javax.xml.parsers.DocumentBuilder;
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 
@@ -152 +151 @@
 
         try {
-            DocumentBuilderFactory builderFactory = DocumentBuilderFactory.newInstance();
-            builderFactory.setValidating(false);
-            builderFactory.setNamespaceAware(true);
-            DocumentBuilder builder = builderFactory.newDocumentBuilder();
+            DocumentBuilder builder = Utils.newSafeDOMBuilder();
             builder.setEntityResolver(new EntityResolver() {
                 @Override

trunk/src/org/openstreetmap/josm/io/session/SessionReader.java

@@ -29 +29 @@
 import javax.swing.JOptionPane;
 import javax.swing.SwingUtilities;
-import javax.xml.parsers.DocumentBuilder;
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 
@@ -633 +631 @@
 
         try {
-            DocumentBuilderFactory builderFactory = DocumentBuilderFactory.newInstance();
-            builderFactory.setValidating(false);
-            builderFactory.setNamespaceAware(true);
-            DocumentBuilder builder = builderFactory.newDocumentBuilder();
-            Document document = builder.parse(josIS);
-            parseJos(document, progressMonitor);
+            parseJos(Utils.parseSafeDOM(josIS), progressMonitor);
         } catch (SAXException e) {
             throw new IllegalDataException(e);

trunk/src/org/openstreetmap/josm/io/session/SessionWriter.java

@@ -19 +19 @@
 
 import javax.xml.parsers.DocumentBuilder;
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.transform.OutputKeys;
@@ -201 +200 @@
      */
     public Document createJosDocument() throws IOException {
-        DocumentBuilderFactory builderFactory = DocumentBuilderFactory.newInstance();
-        builderFactory.setValidating(false);
-        builderFactory.setNamespaceAware(true);
         DocumentBuilder builder = null;
         try {
-            builder = builderFactory.newDocumentBuilder();
+            builder = Utils.newSafeDOMBuilder();
         } catch (ParserConfigurationException e) {
-            throw new RuntimeException(e);
+            throw new IOException(e);
         }
         Document doc = builder.newDocument();

trunk/src/org/openstreetmap/josm/tools/Utils.java

@@ -64 +64 @@
 
 import javax.xml.XMLConstants;
+import javax.xml.parsers.DocumentBuilder;
+import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.parsers.SAXParser;
@@ -70 +72 @@
 import org.apache.commons.compress.compressors.bzip2.BZip2CompressorInputStream;
 import org.openstreetmap.josm.Main;
+import org.w3c.dom.Document;
 import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
@@ -1408 +1411 @@
         }
         return null;
+    }
+
+    /**
+     * Returns a new secure DOM builder, supporting XML namespaces.
+     * @return a new secure DOM builder, supporting XML namespaces
+     * @throws ParserConfigurationException if a parser cannot be created which satisfies the requested configuration.
+     * @throws ParserConfigurationException if a parser cannot be created which satisfies the requested configuration.
+     * @since 10404
+     */
+    public static DocumentBuilder newSafeDOMBuilder() throws ParserConfigurationException {
+        DocumentBuilderFactory builderFactory = DocumentBuilderFactory.newInstance();
+        builderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+        builderFactory.setNamespaceAware(true);
+        builderFactory.setValidating(false);
+        return builderFactory.newDocumentBuilder();
+    }
+
+    /**
+     * Parse the content given {@link InputStream} as XML.
+     * This method uses a secure DOM builder, supporting XML namespaces.
+     *
+     * @param is The InputStream containing the content to be parsed.
+     * @return the result DOM document
+     * @throws ParserConfigurationException if a parser cannot be created which satisfies the requested configuration.
+     * @throws IOException if any IO errors occur.
+     * @throws SAXException for SAX errors.
+     * @since 10404
+     */
+    public static Document parseSafeDOM(InputStream is) throws ParserConfigurationException, IOException, SAXException {
+        long start = System.currentTimeMillis();
+        if (Main.isDebugEnabled()) {
+            Main.debug("Starting DOM parsing of " + is);
+        }
+        Document result = newSafeDOMBuilder().parse(is);
+        if (Main.isDebugEnabled()) {
+            Main.debug("DOM parsing done in " + getDurationString(System.currentTimeMillis() - start));
+        }
+        return result;
     }
 

trunk/src/org/openstreetmap/josm/tools/bugreport/BugReportSender.java

@@ -18 +18 @@
 import javax.swing.JPanel;
 import javax.swing.SwingUtilities;
-import javax.xml.parsers.DocumentBuilder;
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.xpath.XPath;
@@ -100 +98 @@
 
             try (InputStream in = connection.getContent()) {
-                DocumentBuilder builder = DocumentBuilderFactory.newInstance().newDocumentBuilder();
-                Document document = builder.parse(in);
-                return retrieveDebugToken(document);
+                return retrieveDebugToken(Utils.parseSafeDOM(in));
             }
         } catch (IOException | SAXException | ParserConfigurationException | XPathExpressionException t) {