Merge pull request github#13166 from atorralba/atorralba/java/xpath-xxe-sink

Java: Add `XPath.evaluate` as XXE sink

Author: atorralba

java/ql/lib/semmle/code/java/security/XmlParsers.qll

@@ -655,6 +655,11 @@ class XmlReader extends RefType {
   XmlReader() { this.hasQualifiedName("org.xml.sax", "XMLReader") }
 }
 
+/** The class `org.xml.sax.InputSource`. */
+class InputSource extends Class {
+  InputSource() { this.hasQualifiedName("org.xml.sax", "InputSource") }
+}
+
 /** DEPRECATED: Alias for XmlReader */
 deprecated class XMLReader = XmlReader;
 
@@ -1164,22 +1169,34 @@ class XmlUnmarshal extends XmlParserCall {
 }
 
 /* XPathExpression: https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#xpathexpression */
-/** The class `javax.xml.xpath.XPathExpression`. */
-class XPathExpression extends RefType {
+/** The interface `javax.xml.xpath.XPathExpression`. */
+class XPathExpression extends Interface {
   XPathExpression() { this.hasQualifiedName("javax.xml.xpath", "XPathExpression") }
 }
 
-/** A call to `XPathExpression.evaluate`. */
+/** The interface `java.xml.xpath.XPath`. */
+class XPath extends Interface {
+  XPath() { this.hasQualifiedName("javax.xml.xpath", "XPath") }
+}
+
+/** A call to the method `evaluate` of the classes `XPathExpression` or `XPath`. */
 class XPathEvaluate extends XmlParserCall {
+  Argument sink;
+
   XPathEvaluate() {
     exists(Method m |
       this.getMethod() = m and
-      m.getDeclaringType() instanceof XPathExpression and
       m.hasName("evaluate")
+    |
+      m.getDeclaringType().getASourceSupertype*() instanceof XPathExpression and
+      sink = this.getArgument(0)
+      or
+      m.getDeclaringType().getASourceSupertype*() instanceof XPath and
+      sink = this.getArgument(1)
     )
   }
 
-  override Expr getSink() { result = this.getArgument(0) }
+  override Expr getSink() { result = sink }
 
   override predicate isSafe() { none() }
 }

java/ql/src/change-notes/2023-05-15-xpath-xxe-sink.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The queries `java/xxe` and `java/xxe-local` now recognize the second argument of calls to `XPath.evaluate` as a sink.

java/ql/test/query-tests/security/CWE-611/XPathExpressionTests.java

@@ -12,18 +12,33 @@ public class XPathExpressionTests {
 
   public void safeXPathExpression(Socket sock) throws Exception {
     DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
-      factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
-      DocumentBuilder builder = factory.newDocumentBuilder();
-      XPathFactory xFactory = XPathFactory.newInstance();
-      XPath path = xFactory.newXPath();
-      XPathExpression expr = path.compile("");
-      expr.evaluate(builder.parse(sock.getInputStream())); //safe
+    factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+    DocumentBuilder builder = factory.newDocumentBuilder();
+    XPathFactory xFactory = XPathFactory.newInstance();
+    XPath path = xFactory.newXPath();
+    XPathExpression expr = path.compile("");
+    expr.evaluate(builder.parse(sock.getInputStream())); // safe
   }
 
   public void unsafeExpressionTests(Socket sock) throws Exception {
-      XPathFactory xFactory = XPathFactory.newInstance();
-      XPath path = xFactory.newXPath();
-      XPathExpression expr = path.compile("");
-      expr.evaluate(new InputSource(sock.getInputStream())); //unsafe
+    XPathFactory xFactory = XPathFactory.newInstance();
+    XPath path = xFactory.newXPath();
+    XPathExpression expr = path.compile("");
+    expr.evaluate(new InputSource(sock.getInputStream())); // unsafe
+  }
+
+  public void safeXPathEvaluateTest(Socket sock) throws Exception {
+    DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
+    factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+    DocumentBuilder builder = factory.newDocumentBuilder();
+    XPathFactory xFactory = XPathFactory.newInstance();
+    XPath path = xFactory.newXPath();
+    path.evaluate("", builder.parse(sock.getInputStream()));
+  }
+
+  public void unsafeXPathEvaluateTest(Socket sock) throws Exception {
+    XPathFactory xFactory = XPathFactory.newInstance();
+    XPath path = xFactory.newXPath();
+    path.evaluate("", new InputSource(sock.getInputStream())); // unsafe
   }
 }

java/ql/test/query-tests/security/CWE-611/XXE.expected

@@ -74,7 +74,8 @@ edges
 | XMLReaderTests.java:86:34:86:54 | getInputStream(...) : InputStream | XMLReaderTests.java:86:18:86:55 | new InputSource(...) |
 | XMLReaderTests.java:94:34:94:54 | getInputStream(...) : InputStream | XMLReaderTests.java:94:18:94:55 | new InputSource(...) |
 | XMLReaderTests.java:100:34:100:54 | getInputStream(...) : InputStream | XMLReaderTests.java:100:18:100:55 | new InputSource(...) |
-| XPathExpressionTests.java:27:37:27:57 | getInputStream(...) : InputStream | XPathExpressionTests.java:27:21:27:58 | new InputSource(...) |
+| XPathExpressionTests.java:27:35:27:55 | getInputStream(...) : InputStream | XPathExpressionTests.java:27:19:27:56 | new InputSource(...) |
+| XPathExpressionTests.java:42:39:42:59 | getInputStream(...) : InputStream | XPathExpressionTests.java:42:23:42:60 | new InputSource(...) |
 nodes
 | DocumentBuilderTests.java:14:19:14:39 | getInputStream(...) | semmle.label | getInputStream(...) |
 | DocumentBuilderTests.java:28:19:28:39 | getInputStream(...) | semmle.label | getInputStream(...) |
@@ -235,8 +236,10 @@ nodes
 | XMLReaderTests.java:94:34:94:54 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
 | XMLReaderTests.java:100:18:100:55 | new InputSource(...) | semmle.label | new InputSource(...) |
 | XMLReaderTests.java:100:34:100:54 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
-| XPathExpressionTests.java:27:21:27:58 | new InputSource(...) | semmle.label | new InputSource(...) |
-| XPathExpressionTests.java:27:37:27:57 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
+| XPathExpressionTests.java:27:19:27:56 | new InputSource(...) | semmle.label | new InputSource(...) |
+| XPathExpressionTests.java:27:35:27:55 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
+| XPathExpressionTests.java:42:23:42:60 | new InputSource(...) | semmle.label | new InputSource(...) |
+| XPathExpressionTests.java:42:39:42:59 | getInputStream(...) : InputStream | semmle.label | getInputStream(...) : InputStream |
 | XmlInputFactoryTests.java:9:35:9:55 | getInputStream(...) | semmle.label | getInputStream(...) |
 | XmlInputFactoryTests.java:10:34:10:54 | getInputStream(...) | semmle.label | getInputStream(...) |
 | XmlInputFactoryTests.java:24:35:24:55 | getInputStream(...) | semmle.label | getInputStream(...) |
@@ -336,7 +339,8 @@ subpaths
 | XMLReaderTests.java:86:18:86:55 | new InputSource(...) | XMLReaderTests.java:86:34:86:54 | getInputStream(...) : InputStream | XMLReaderTests.java:86:18:86:55 | new InputSource(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XMLReaderTests.java:86:34:86:54 | getInputStream(...) | user-provided value |
 | XMLReaderTests.java:94:18:94:55 | new InputSource(...) | XMLReaderTests.java:94:34:94:54 | getInputStream(...) : InputStream | XMLReaderTests.java:94:18:94:55 | new InputSource(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XMLReaderTests.java:94:34:94:54 | getInputStream(...) | user-provided value |
 | XMLReaderTests.java:100:18:100:55 | new InputSource(...) | XMLReaderTests.java:100:34:100:54 | getInputStream(...) : InputStream | XMLReaderTests.java:100:18:100:55 | new InputSource(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XMLReaderTests.java:100:34:100:54 | getInputStream(...) | user-provided value |
-| XPathExpressionTests.java:27:21:27:58 | new InputSource(...) | XPathExpressionTests.java:27:37:27:57 | getInputStream(...) : InputStream | XPathExpressionTests.java:27:21:27:58 | new InputSource(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XPathExpressionTests.java:27:37:27:57 | getInputStream(...) | user-provided value |
+| XPathExpressionTests.java:27:19:27:56 | new InputSource(...) | XPathExpressionTests.java:27:35:27:55 | getInputStream(...) : InputStream | XPathExpressionTests.java:27:19:27:56 | new InputSource(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XPathExpressionTests.java:27:35:27:55 | getInputStream(...) | user-provided value |
+| XPathExpressionTests.java:42:23:42:60 | new InputSource(...) | XPathExpressionTests.java:42:39:42:59 | getInputStream(...) : InputStream | XPathExpressionTests.java:42:23:42:60 | new InputSource(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XPathExpressionTests.java:42:39:42:59 | getInputStream(...) | user-provided value |
 | XmlInputFactoryTests.java:9:35:9:55 | getInputStream(...) | XmlInputFactoryTests.java:9:35:9:55 | getInputStream(...) | XmlInputFactoryTests.java:9:35:9:55 | getInputStream(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XmlInputFactoryTests.java:9:35:9:55 | getInputStream(...) | user-provided value |
 | XmlInputFactoryTests.java:10:34:10:54 | getInputStream(...) | XmlInputFactoryTests.java:10:34:10:54 | getInputStream(...) | XmlInputFactoryTests.java:10:34:10:54 | getInputStream(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XmlInputFactoryTests.java:10:34:10:54 | getInputStream(...) | user-provided value |
 | XmlInputFactoryTests.java:24:35:24:55 | getInputStream(...) | XmlInputFactoryTests.java:24:35:24:55 | getInputStream(...) | XmlInputFactoryTests.java:24:35:24:55 | getInputStream(...) | XML parsing depends on a $@ without guarding against external entity expansion. | XmlInputFactoryTests.java:24:35:24:55 | getInputStream(...) | user-provided value |