Stop parsing entire XML BOM just to determine the schema version

src/main/java/org/cyclonedx/parsers/XmlParser.java

@@ -25,21 +25,19 @@
 import org.cyclonedx.exception.ParseException;
 import org.cyclonedx.model.Bom;
 import org.cyclonedx.util.XmlFactoryUtils;
-import org.w3c.dom.Document;
-import org.w3c.dom.NamedNodeMap;
-import org.w3c.dom.Node;
-import org.w3c.dom.NodeList;
 import org.xml.sax.ErrorHandler;
 import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
 import org.xml.sax.SAXParseException;
 import org.xml.sax.XMLReader;
 
 import javax.xml.XMLConstants;
-import javax.xml.parsers.DocumentBuilder;
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.parsers.SAXParserFactory;
+import javax.xml.stream.XMLInputFactory;
+import javax.xml.stream.XMLStreamConstants;
+import javax.xml.stream.XMLStreamException;
+import javax.xml.stream.XMLStreamReader;
 import javax.xml.transform.Source;
 import javax.xml.transform.sax.SAXSource;
 import javax.xml.transform.stream.StreamSource;
@@ -52,7 +50,6 @@
 import java.io.Reader;
 import java.lang.reflect.Field;
 import java.nio.file.Files;
-import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.LinkedList;
 import java.util.List;
@@ -72,11 +69,11 @@ public XmlParser() {
         mapper = new XmlMapper();
     }
 
-    private static final Map<String, String> NAMESPACE_TO_VERSION_MAP = new HashMap<>();
+    private static final Map<String, String> SCHEMA_VERSION_BY_NAMESPACE = new HashMap<>();
 
     static {
         for (Version version : Version.values()) {
-            NAMESPACE_TO_VERSION_MAP.put(version.getNamespace(), version.getVersionString());
+            SCHEMA_VERSION_BY_NAMESPACE.put(version.getNamespace(), version.getVersionString());
         }
     }
 
@@ -85,10 +82,13 @@ public XmlParser() {
      */
     public Bom parse(final File file) throws ParseException {
         try {
-            final String schemaVersion = identifySchemaVersion(new InputSource(Files.newInputStream(file.toPath())));
+            final String schemaVersion;
+            try (final InputStream fis = Files.newInputStream(file.toPath())) {
+                schemaVersion = identifySchemaVersion(fis);
+            }
 
             return injectSchemaVersion(mapper.readValue(file, Bom.class), schemaVersion);
-        } catch (IOException | ParserConfigurationException | SAXException e) {
+        } catch (IOException | XMLStreamException e) {
             throw new ParseException(e);
         }
     }
@@ -98,10 +98,10 @@ public Bom parse(final File file) throws ParseException {
      */
     public Bom parse(final byte[] bomBytes) throws ParseException {
         try {
-            final String schemaVersion = identifySchemaVersion(new InputSource(new ByteArrayInputStream(bomBytes)));
+            final String schemaVersion = identifySchemaVersion(new ByteArrayInputStream(bomBytes));
 
             return injectSchemaVersion(mapper.readValue(bomBytes, Bom.class), schemaVersion);
-        } catch (IOException | ParserConfigurationException | SAXException e) {
+        } catch (IOException | XMLStreamException e) {
             throw new ParseException(e);
         }
     }
@@ -337,66 +337,35 @@ public boolean isValid(final InputStream inputStream, final Version schemaVersio
         return validate(inputStream, schemaVersion).isEmpty();
     }
 
-    private String identifySchemaVersion(final InputSource in)
-        throws ParserConfigurationException, IOException, SAXException
-    {
-
-        List<String> namespaces = extractAllNamespaceDeclarations(in);
-
-        for (String namespaceUri : namespaces) {
-            String versionString = NAMESPACE_TO_VERSION_MAP.get(namespaceUri);
-            if (versionString != null) {
-                return versionString;
-            }
-        }
-        return null;
-    }
-
-    private List<String> extractAllNamespaceDeclarations(final InputSource in)
-        throws ParserConfigurationException, IOException, SAXException
-    {
-        Document doc = createSecureDocument(in);
-
-        // Extract all namespaces, including the default namespace
-        List<String>namespaces = new ArrayList<>();
-        extractNamespaces(doc.getDocumentElement(), namespaces);
-
-        return namespaces;
-    }
+    private String identifySchemaVersion(InputStream in) throws XMLStreamException {
+        final XMLStreamReader reader = createSecureXmlStreamReader(in);
+        try {
+            while (reader.hasNext()) {
+                if (reader.next() != XMLStreamConstants.START_ELEMENT) {
+                    continue;
+                }
 
-    private void extractNamespaces(Node node, List<String> namespaces) {
-        if (node.getNodeType() == Node.ELEMENT_NODE) {
-            NamedNodeMap attributes = node.getAttributes();
-            for (int i = 0; i < attributes.getLength(); i++) {
-                Node attr = attributes.item(i);
-                if (attr.getNodeName().equals("xmlns")) {
-                    namespaces.add(attr.getNodeValue());
+                for (int i = 0; i < reader.getNamespaceCount(); i++) {
+                    if (reader.getNamespacePrefix(i) == null) {
+                        final String schemaVersion = SCHEMA_VERSION_BY_NAMESPACE.get(reader.getNamespaceURI(i));
+                        if (schemaVersion != null) {
+                            return schemaVersion;
+                        }
+                    }
                 }
             }
-        }
-        NodeList children = node.getChildNodes();
-        for (int i = 0; i < children.getLength(); i++) {
-            extractNamespaces(children.item(i), namespaces);
+
+            return null;
+        } finally {
+            reader.close();
         }
     }
 
-    private Document createSecureDocument(InputSource in) throws ParserConfigurationException, IOException, SAXException
-    {
-        //https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#xpathexpression
-        DocumentBuilderFactory df = XmlFactoryUtils.newDocumentBuilderFactory();
-        try {
-            df.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
-            df.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
-        } catch (IllegalArgumentException e) {
-            // JAXP 1.5 secure-processing attributes are not supported by outdated
-            // DocumentBuilderFactory implementations (e.g. Xerces 2.x found on the classpath).
-            // Secure processing alone does not prevent XXE there, so compensate by disallowing
-            // DOCTYPE declarations entirely; if that is unsupported too, fail rather than
-            // parse insecurely
-            df.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
-        }
-        df.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
-        DocumentBuilder builder = df.newDocumentBuilder();
-        return builder.parse(in);
+    private XMLStreamReader createSecureXmlStreamReader(InputStream in) throws XMLStreamException {
+        //https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#xmlinputfactory-a-stax-parser
+        final XMLInputFactory factory = XmlFactoryUtils.newXMLInputFactory();
+        factory.setProperty(XMLInputFactory.SUPPORT_DTD, false);
+        factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
+        return factory.createXMLStreamReader(in);
     }
 }

src/main/java/org/cyclonedx/util/XmlFactoryUtils.java

@@ -21,6 +21,7 @@
 import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.SAXParserFactory;
+import javax.xml.stream.XMLInputFactory;
 import javax.xml.validation.SchemaFactory;
 
 /**
@@ -85,6 +86,25 @@ public static SAXParserFactory newSAXParserFactory() {
         return SAXParserFactory.newInstance();
     }
 
+    /**
+     * Creates a new {@link XMLInputFactory}, preferring the JDK's built-in implementation
+     * unless one is explicitly requested via the {@code javax.xml.stream.XMLInputFactory}
+     * system property.
+     *
+     * @return a new {@link XMLInputFactory}
+     */
+    public static XMLInputFactory newXMLInputFactory() {
+        if (System.getProperty(XMLInputFactory.class.getName()) == null) {
+            try {
+                return (XMLInputFactory) XMLInputFactory.class.getMethod("newDefaultFactory").invoke(null);
+            } catch (ReflectiveOperationException e) {
+                // Java 8: fall back to the standard lookup below
+            }
+        }
+
+        return XMLInputFactory.newInstance();
+    }
+
     /**
      * Creates a new {@link SchemaFactory} for W3C XML Schema, preferring the JDK's built-in
      * implementation unless one is explicitly requested via the

src/test/java/org/cyclonedx/BomXmlGeneratorTest.java

@@ -758,7 +758,7 @@ public void testIssue408Regression_externalReferenceBom() throws Exception {
     public void testXxeProtection() {
         assertThatExceptionOfType(ParseException.class)
                 .isThrownBy(() -> createCommonBomXml("/security/xxe-protection.xml"))
-                .withMessageContaining("not allowed due to restriction set by the accessExternalDTD property");
+                .withMessageContaining("Undeclared general entity \"xxe\"");
     }
 
     @Test

src/test/java/org/cyclonedx/parsers/XercesFallbackTest.java

@@ -82,10 +82,10 @@ void generateShouldWorkWithXerces() throws Exception {
     @Test
     void parseShouldNotBeVulnerableToXxeWithXerces() throws Exception {
         final byte[] bomBytes = resource("/security/xxe-protection.xml");
-        // the doctype must be rejected before any external entity can be resolved
+        // No DTD is processed, so the entity is never declared, and not resolved.
         assertThatExceptionOfType(ParseException.class)
                 .isThrownBy(() -> new XmlParser().parse(bomBytes))
-                .withMessageContaining("DOCTYPE");
+                .withMessageContaining("Undeclared general entity \"xxe\"");
     }
 
     @Test

src/test/java/org/cyclonedx/parsers/XmlParserTest.java

@@ -84,6 +84,7 @@
 import java.util.stream.Collectors;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
@@ -1195,6 +1196,21 @@ public void schema17_patent() throws Exception {
         assertEquals(PatentAssertion.AssertionType.EXCLUSIVE_RIGHTS, pa3.getAssertionType());
     }
 
+    @Test
+    void parseShouldNotBeVulnerableToXxe() throws Exception {
+        final byte[] bomBytes;
+        try (final InputStream bomInputStream = getClass().getResourceAsStream("/security/xxe-protection.xml")) {
+            assertThat(bomInputStream).isNotNull();
+            bomBytes = bomInputStream.readAllBytes();
+        }
+
+        // No DTD is processed, so the entity is never declared, let alone resolved. Were it
+        // resolved, the parse would instead fail on the file the entity points to
+        assertThatExceptionOfType(ParseException.class)
+                .isThrownBy(() -> new XmlParser().parse(bomBytes))
+                .withMessageContaining("Undeclared general entity \"xxe\"");
+    }
+
     @Test
     @DefaultLocale("en-US") // Validator exception message is localized.
     void validateShouldNotBeVulnerableToXxe() throws Exception {

src/test/resources/security/xxe-protection.xml

@@ -3,7 +3,7 @@
 <bom xmlns="http://cyclonedx.org/schema/bom/1.5" serialNumber="urn:uuid:3e671687-395b-41f5-a30f-a58921a69b79">
     <components>
         <component type="application">
-            <name>Example Application &#x26;xxe;3</name>
+            <name>Example Application &xxe;</name>
             <version>2.1.0</version>
             <description>This is an example application</description>
             <licenses>