From 01b5963559c867c611bd5f0090050588d75c1dde Mon Sep 17 00:00:00 2001 From: nscuro Date: Mon, 24 Aug 2026 17:35:57 +0200 Subject: [PATCH] Stop parsing entire XML BOM just to determine the schema version Determining the schema version previously involved parsing the full BOM, just to iterate over its XML namespaces. This caused XML parsing overhead and unnecessary memory usage. This change switches the mechanism to streaming, making the procedure dirt-cheap, as it keeps only a small subset of data in-memory, and short-circuits when the desired information has been found. Signed-off-by: nscuro --- .../java/org/cyclonedx/parsers/XmlParser.java | 105 ++++++------------ .../org/cyclonedx/util/XmlFactoryUtils.java | 20 ++++ .../org/cyclonedx/BomXmlGeneratorTest.java | 2 +- .../cyclonedx/parsers/XercesFallbackTest.java | 4 +- .../org/cyclonedx/parsers/XmlParserTest.java | 16 +++ .../resources/security/xxe-protection.xml | 2 +- 6 files changed, 77 insertions(+), 72 deletions(-) diff --git a/src/main/java/org/cyclonedx/parsers/XmlParser.java b/src/main/java/org/cyclonedx/parsers/XmlParser.java index 6f6411605d..06a7ee79e8 100644 --- a/src/main/java/org/cyclonedx/parsers/XmlParser.java +++ b/src/main/java/org/cyclonedx/parsers/XmlParser.java @@ -25,10 +25,6 @@ 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; @@ -36,10 +32,12 @@ 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 NAMESPACE_TO_VERSION_MAP = new HashMap<>(); + private static final Map 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 namespaces = extractAllNamespaceDeclarations(in); - - for (String namespaceUri : namespaces) { - String versionString = NAMESPACE_TO_VERSION_MAP.get(namespaceUri); - if (versionString != null) { - return versionString; - } - } - return null; - } - - private List extractAllNamespaceDeclarations(final InputSource in) - throws ParserConfigurationException, IOException, SAXException - { - Document doc = createSecureDocument(in); - - // Extract all namespaces, including the default namespace - Listnamespaces = 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 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); } } diff --git a/src/main/java/org/cyclonedx/util/XmlFactoryUtils.java b/src/main/java/org/cyclonedx/util/XmlFactoryUtils.java index 0a7310d165..7cc63f7a74 100644 --- a/src/main/java/org/cyclonedx/util/XmlFactoryUtils.java +++ b/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 diff --git a/src/test/java/org/cyclonedx/BomXmlGeneratorTest.java b/src/test/java/org/cyclonedx/BomXmlGeneratorTest.java index 02bc8f3840..70805d30c3 100644 --- a/src/test/java/org/cyclonedx/BomXmlGeneratorTest.java +++ b/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 diff --git a/src/test/java/org/cyclonedx/parsers/XercesFallbackTest.java b/src/test/java/org/cyclonedx/parsers/XercesFallbackTest.java index 13bbf74a8a..ed69d1c02e 100644 --- a/src/test/java/org/cyclonedx/parsers/XercesFallbackTest.java +++ b/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 diff --git a/src/test/java/org/cyclonedx/parsers/XmlParserTest.java b/src/test/java/org/cyclonedx/parsers/XmlParserTest.java index 0afaf1355d..2bd179688e 100644 --- a/src/test/java/org/cyclonedx/parsers/XmlParserTest.java +++ b/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 { diff --git a/src/test/resources/security/xxe-protection.xml b/src/test/resources/security/xxe-protection.xml index 3de89c238d..d89cbdb99a 100644 --- a/src/test/resources/security/xxe-protection.xml +++ b/src/test/resources/security/xxe-protection.xml @@ -3,7 +3,7 @@ - Example Application &xxe;3 + Example Application &xxe; 2.1.0 This is an example application