diff --git a/java/ql/lib/change-notes/2026-08-02-apache-commons-xml-factories.md b/java/ql/lib/change-notes/2026-08-02-apache-commons-xml-factories.md new file mode 100644 index 000000000000..6ec4686065be --- /dev/null +++ b/java/ql/lib/change-notes/2026-08-02-apache-commons-xml-factories.md @@ -0,0 +1,5 @@ +--- +category: feature +--- +* Factories returned by the Apache Commons XML (`org.apache.commons.xml.XmlFactories`) hardening library are now recognized as safely configured by the XXE query. +* A new extensible class `SafeXmlFactorySource` was added to `semmle.code.java.security.XmlParsers` for modeling sources of pre-hardened JAXP factories. diff --git a/java/ql/lib/semmle/code/java/frameworks/apache/CommonsXml.qll b/java/ql/lib/semmle/code/java/frameworks/apache/CommonsXml.qll index 163bd773dad0..aa6d840a0782 100644 --- a/java/ql/lib/semmle/code/java/frameworks/apache/CommonsXml.qll +++ b/java/ql/lib/semmle/code/java/frameworks/apache/CommonsXml.qll @@ -90,3 +90,25 @@ private module SafeDigesterFlowConfig implements DataFlow::ConfigSig { } private module SafeDigesterFlow = DataFlow::Global; + +/** + * A call to one of the `org.apache.commons.xml.XmlFactories.newXxxFactory()` methods + * of the Apache Commons XML library. + * + * Every such method returns a fresh JAXP factory that has already been hardened against + * XML external entity (XXE) attacks, so any parser created from it is treated as safe. + * + * `newXPathFactory` is matched for completeness, but the XXE model has no `XPathFactory` + * safety chain (the XXE sink for XPath is the document being evaluated, not the factory), + * so it currently has no effect on XXE results. + */ +private class CommonsXmlSafeXmlFactory extends SafeXmlFactorySource, MethodCall { + CommonsXmlSafeXmlFactory() { + this.getMethod().getDeclaringType().hasQualifiedName("org.apache.commons.xml", "XmlFactories") and + this.getMethod() + .hasName([ + "newDocumentBuilderFactory", "newSAXParserFactory", "newXMLInputFactory", + "newTransformerFactory", "newSchemaFactory", "newXPathFactory" + ]) + } +} diff --git a/java/ql/lib/semmle/code/java/security/XmlParsers.qll b/java/ql/lib/semmle/code/java/security/XmlParsers.qll index 602076996a77..4a30ded19e3a 100644 --- a/java/ql/lib/semmle/code/java/security/XmlParsers.qll +++ b/java/ql/lib/semmle/code/java/security/XmlParsers.qll @@ -56,6 +56,15 @@ abstract class ParserConfig extends MethodCall { } } +/** + * An expression that evaluates to a JAXP parser factory (such as a + * `DocumentBuilderFactory` or `SAXParserFactory`) that has already been hardened + * against XML external entity (XXE) attacks, for example by a helper library. + * + * Extend this class to model additional sources of pre-hardened JAXP factories. + */ +abstract class SafeXmlFactorySource extends Expr { } + /* * https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#jaxp-documentbuilderfactory-saxparserfactory-and-dom4j */ @@ -156,6 +165,8 @@ private class DocumentBuilderConstruction extends MethodCall { private predicate safeDocumentBuilderFactoryNode(DataFlow::Node src) { src.asExpr() instanceof SafeDocumentBuilderFactory + or + src.asExpr().(SafeXmlFactorySource).getType() instanceof DocumentBuilderFactory } private module SafeDocumentBuilderFactoryToDocumentBuilderConstructionFlow = @@ -219,6 +230,8 @@ class XmlInputFactoryStreamReader extends XmlParserCall { private predicate safeXmlInputFactoryNode(DataFlow::Node src) { src.asExpr() instanceof SafeXmlInputFactory + or + src.asExpr().(SafeXmlFactorySource).getType() instanceof XmlInputFactory } private module SafeXmlInputFactoryToXmlInputFactoryReaderFlow = @@ -456,6 +469,8 @@ class SafeSaxParserFactory extends VarAccess { private predicate safeSaxParserFactoryNode(DataFlow::Node src) { src.asExpr() instanceof SafeSaxParserFactory + or + src.asExpr().(SafeXmlFactorySource).getType() instanceof SaxParserFactory } private module SafeSaxParserFactoryToNewSaxParserFlow = @@ -831,6 +846,8 @@ class TransformerFactoryConfig extends TransformerConfig { private predicate safeTransformerFactoryNode(DataFlow::Node src) { src.asExpr() instanceof SafeTransformerFactory + or + src.asExpr().(SafeXmlFactorySource).getType() instanceof TransformerFactory } private module SafeTransformerFactoryFlow = DataFlow::SimpleGlobal; @@ -920,6 +937,8 @@ class SchemaFactoryNewSchema extends XmlParserCall { private predicate safeSchemaFactoryNode(DataFlow::Node src) { src.asExpr() instanceof SafeSchemaFactory + or + src.asExpr().(SafeXmlFactorySource).getType() instanceof SchemaFactory } private module SafeSchemaFactoryToSchemaFactoryNewSchemaFlow = diff --git a/java/ql/test/query-tests/security/CWE-611/XmlFactoriesTests.java b/java/ql/test/query-tests/security/CWE-611/XmlFactoriesTests.java new file mode 100644 index 000000000000..ebb1b54e3382 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-611/XmlFactoriesTests.java @@ -0,0 +1,62 @@ +import java.net.Socket; + +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.SAXParser; +import javax.xml.parsers.SAXParserFactory; +import javax.xml.stream.XMLInputFactory; +import javax.xml.transform.Transformer; +import javax.xml.transform.TransformerFactory; +import javax.xml.transform.stream.StreamSource; +import javax.xml.validation.Schema; +import javax.xml.validation.SchemaFactory; + +import org.xml.sax.XMLReader; +import org.xml.sax.helpers.DefaultHandler; + +import org.apache.commons.xml.XmlFactories; + +// Every factory returned by `org.apache.commons.xml.XmlFactories` is already hardened against +// XXE, so the parsers created from them must not be reported. +public class XmlFactoriesTests { + + public void hardenedDocumentBuilder(Socket sock) throws Exception { + DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); + DocumentBuilder builder = factory.newDocumentBuilder(); + builder.parse(sock.getInputStream()); // safe + } + + public void hardenedDocumentBuilderChained(Socket sock) throws Exception { + XmlFactories.newDocumentBuilderFactory().newDocumentBuilder().parse(sock.getInputStream()); // safe + } + + public void hardenedSaxParser(Socket sock) throws Exception { + SAXParserFactory factory = XmlFactories.newSAXParserFactory(); + SAXParser parser = factory.newSAXParser(); + parser.parse(sock.getInputStream(), new DefaultHandler()); // safe + } + + public void hardenedSaxParserXmlReader(Socket sock) throws Exception { + SAXParser parser = XmlFactories.newSAXParserFactory().newSAXParser(); + XMLReader reader = parser.getXMLReader(); + reader.parse(new org.xml.sax.InputSource(sock.getInputStream())); // safe + } + + public void hardenedXmlInputFactory(Socket sock) throws Exception { + XMLInputFactory factory = XmlFactories.newXMLInputFactory(); + factory.createXMLStreamReader(sock.getInputStream()); // safe + factory.createXMLEventReader(sock.getInputStream()); // safe + } + + public void hardenedTransformer(Socket sock) throws Exception { + TransformerFactory tf = XmlFactories.newTransformerFactory(); + Transformer transformer = tf.newTransformer(); + transformer.transform(new StreamSource(sock.getInputStream()), null); // safe + tf.newTransformer(new StreamSource(sock.getInputStream())); // safe + } + + public void hardenedSchema(Socket sock) throws Exception { + SchemaFactory factory = XmlFactories.newSchemaFactory(); + Schema schema = factory.newSchema(new StreamSource(sock.getInputStream())); // safe + } +} diff --git a/java/ql/test/query-tests/security/CWE-611/options b/java/ql/test/query-tests/security/CWE-611/options index 190e6b2af0c6..1d6e3c40b307 100644 --- a/java/ql/test/query-tests/security/CWE-611/options +++ b/java/ql/test/query-tests/security/CWE-611/options @@ -1 +1 @@ -//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/jdom-1.1.3:${testdir}/../../../stubs/dom4j-2.1.1:${testdir}/../../../stubs/simple-xml-2.7.1:${testdir}/../../../stubs/jaxb-api-2.3.1:${testdir}/../../../stubs/jaxen-1.2.0:${testdir}/../../../stubs/apache-commons-digester3-3.2:${testdir}/../../../stubs/servlet-api-2.4/:${testdir}/../../../stubs/rundeck-api-java-client-13.2:${testdir}/../../../stubs/springframework-5.8.x/:${testdir}/../../../stubs/mdht-1.2.0/:${testdir}/../../../stubs/woodstox-core-6.4.0 +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/jdom-1.1.3:${testdir}/../../../stubs/dom4j-2.1.1:${testdir}/../../../stubs/simple-xml-2.7.1:${testdir}/../../../stubs/jaxb-api-2.3.1:${testdir}/../../../stubs/jaxen-1.2.0:${testdir}/../../../stubs/apache-commons-digester3-3.2:${testdir}/../../../stubs/servlet-api-2.4/:${testdir}/../../../stubs/rundeck-api-java-client-13.2:${testdir}/../../../stubs/springframework-5.8.x/:${testdir}/../../../stubs/mdht-1.2.0/:${testdir}/../../../stubs/woodstox-core-6.4.0:${testdir}/../../../stubs/apache-commons-xml-0.1.0 diff --git a/java/ql/test/stubs/apache-commons-xml-0.1.0/org/apache/commons/xml/XmlFactories.java b/java/ql/test/stubs/apache-commons-xml-0.1.0/org/apache/commons/xml/XmlFactories.java new file mode 100644 index 000000000000..733a3f5aad33 --- /dev/null +++ b/java/ql/test/stubs/apache-commons-xml-0.1.0/org/apache/commons/xml/XmlFactories.java @@ -0,0 +1,39 @@ +// Minimal stub of org.apache.commons.xml.XmlFactories for testing purposes + +package org.apache.commons.xml; + +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.SAXParserFactory; +import javax.xml.stream.XMLInputFactory; +import javax.xml.transform.TransformerFactory; +import javax.xml.validation.SchemaFactory; +import javax.xml.xpath.XPathFactory; + +public final class XmlFactories { + + public static DocumentBuilderFactory newDocumentBuilderFactory() { + return null; + } + + public static SAXParserFactory newSAXParserFactory() { + return null; + } + + public static SchemaFactory newSchemaFactory() { + return null; + } + + public static TransformerFactory newTransformerFactory() { + return null; + } + + public static XMLInputFactory newXMLInputFactory() { + return null; + } + + public static XPathFactory newXPathFactory() { + return null; + } + + private XmlFactories() {} +}