-
Notifications
You must be signed in to change notification settings - Fork 0
Java: model org.apache.commons.xml XmlFactories as safe XXE sources #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: qa/agent-github-codeql/pr-10-22269/base
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| category: feature | ||
| --- | ||
| * Factories returned by the Apache Commons Secure XML (`org.apache.commons.xml.secure`) hardening library's `SecureDocumentBuilderFactory`, `SecureSAXParserFactory`, `SecureXMLInputFactory`, `SecureTransformerFactory` and `SecureSchemaFactory` classes 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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,3 +90,33 @@ private module SafeDigesterFlowConfig implements DataFlow::ConfigSig { | |
| } | ||
|
|
||
| private module SafeDigesterFlow = DataFlow::Global<SafeDigesterFlowConfig>; | ||
|
|
||
| /** | ||
| * A call to one of the static factory methods of the `org.apache.commons.xml.secure` | ||
| * `SecureXxxFactory` classes of the Apache Commons Secure XML library. | ||
| * | ||
| * These methods mirror the JAXP factory entry points (`newInstance`, `newDefaultInstance`, | ||
| * `newNSInstance`, `newFactory`, ...) and every one of them 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. | ||
| * | ||
| * `SecureXPathFactory` 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 CommonsSecureXmlFactory extends SafeXmlFactorySource, MethodCall { | ||
| CommonsSecureXmlFactory() { | ||
| this.getMethod() | ||
| .getDeclaringType() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shipwright · HIGH The new 'CommonsSecureXmlFactory' class matches any method named 'newInstance', 'newDefaultInstance', 'newNSInstance', 'newDefaultNSInstance', 'newFactory', or 'newDefaultFactory' Impact: The new 'CommonsSecureXmlFactory' class matches any method named 'newInstance', 'newDefaultInstance', 'newNSInstance', 'newDefaultNSInstance', 'newFactory', or 'newDefaultFactory' declared on the secure factory classes, without constraining the return type. This includes overloads such as 'SecureDocumentBuilderFactory.newInstance(String, ClassLoader)' and 'SecureSchemaFactory.newInstance(String, String, ClassLoader)… Suggested fix: Fix the review finding before release. |
||
| .hasQualifiedName("org.apache.commons.xml.secure", | ||
| [ | ||
| "SecureDocumentBuilderFactory", "SecureSAXParserFactory", "SecureXMLInputFactory", | ||
| "SecureTransformerFactory", "SecureSchemaFactory", "SecureXPathFactory" | ||
| ]) and | ||
| this.getMethod() | ||
| .hasName([ | ||
| "newDefaultInstance", "newDefaultNSInstance", "newInstance", "newNSInstance", | ||
| "newDefaultFactory", "newFactory" | ||
| ]) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import java.net.Socket; | ||
|
|
||
| import javax.xml.XMLConstants; | ||
| 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.secure.SecureDocumentBuilderFactory; | ||
| import org.apache.commons.xml.secure.SecureSAXParserFactory; | ||
| import org.apache.commons.xml.secure.SecureSchemaFactory; | ||
| import org.apache.commons.xml.secure.SecureTransformerFactory; | ||
| import org.apache.commons.xml.secure.SecureXMLInputFactory; | ||
|
|
||
| // Every factory returned by the `org.apache.commons.xml.secure.SecureXxxFactory` classes is | ||
| // already hardened against XXE, so the parsers created from them must not be reported. | ||
| public class SecureXmlFactoriesTests { | ||
|
|
||
| public void hardenedDocumentBuilder(Socket sock) throws Exception { | ||
| DocumentBuilderFactory factory = SecureDocumentBuilderFactory.newInstance(); | ||
| DocumentBuilder builder = factory.newDocumentBuilder(); | ||
| builder.parse(sock.getInputStream()); // safe | ||
| } | ||
|
|
||
| public void hardenedDocumentBuilderChained(Socket sock) throws Exception { | ||
| SecureDocumentBuilderFactory.newDefaultNSInstance().newDocumentBuilder().parse(sock.getInputStream()); // safe | ||
| } | ||
|
|
||
| public void hardenedSaxParser(Socket sock) throws Exception { | ||
| SAXParserFactory factory = SecureSAXParserFactory.newInstance(); | ||
| SAXParser parser = factory.newSAXParser(); | ||
| parser.parse(sock.getInputStream(), new DefaultHandler()); // safe | ||
| } | ||
|
|
||
| public void hardenedSaxParserXmlReader(Socket sock) throws Exception { | ||
| SAXParser parser = SecureSAXParserFactory.newNSInstance().newSAXParser(); | ||
| XMLReader reader = parser.getXMLReader(); | ||
| reader.parse(new org.xml.sax.InputSource(sock.getInputStream())); // safe | ||
| } | ||
|
|
||
| public void hardenedXmlInputFactory(Socket sock) throws Exception { | ||
| XMLInputFactory factory = SecureXMLInputFactory.newFactory(); | ||
| factory.createXMLStreamReader(sock.getInputStream()); // safe | ||
| factory.createXMLEventReader(sock.getInputStream()); // safe | ||
| } | ||
|
|
||
| public void hardenedXmlInputFactoryDefault(Socket sock) throws Exception { | ||
| XMLInputFactory factory = SecureXMLInputFactory.newDefaultFactory(); | ||
| factory.createXMLStreamReader(sock.getInputStream()); // safe | ||
| } | ||
|
|
||
| public void hardenedTransformer(Socket sock) throws Exception { | ||
| TransformerFactory tf = SecureTransformerFactory.newInstance(); | ||
| Transformer transformer = tf.newTransformer(); | ||
| transformer.transform(new StreamSource(sock.getInputStream()), null); // safe | ||
| tf.newTransformer(new StreamSource(sock.getInputStream())); // safe | ||
| } | ||
|
|
||
| public void hardenedTransformerDefault(Socket sock) throws Exception { | ||
| TransformerFactory tf = SecureTransformerFactory.newDefaultInstance(); | ||
| tf.newTransformer(new StreamSource(sock.getInputStream())); // safe | ||
| } | ||
|
|
||
| public void hardenedSchema(Socket sock) throws Exception { | ||
| SchemaFactory factory = SecureSchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); | ||
| Schema schema = factory.newSchema(new StreamSource(sock.getInputStream())); // safe | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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-secure-xml-1.0.0 |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shipwright · CRITICAL
CommonsSecureXmlFactory matches all overloads of newInstance/newFactory on the secure factory classes without constraining return type or parameters.
Impact: CommonsSecureXmlFactory matches all overloads of newInstance/newFactory on the secure factory classes without constraining return type or parameters. Overloads accepting a caller-supplied factory class name and ClassLoader (e.g. SecureDocumentBuilderFactory.newInstance(String, ClassLoader), SecureSchemaFactory.newInstance(String, String, ClassLoader)) are treated as safe XXE sources even though they can return an ar…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.