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