Merge pull request #12139 from atorralba/atorralba/java/xxe-local-query

Java: Add local version of the XXE query

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

@@ -0,0 +1,24 @@
+/** Provides classes to reason about XML eXternal Entity (XXE) vulnerabilities. */
+
+import java
+private import semmle.code.java.dataflow.DataFlow
+
+/** A node where insecure XML parsing takes place. */
+abstract class XxeSink extends DataFlow::Node { }
+
+/** A node that acts as a sanitizer in configurations realted to XXE vulnerabilities. */
+abstract class XxeSanitizer extends DataFlow::Node { }
+
+/**
+ * A unit class for adding additional taint steps.
+ *
+ * Extend this class to add additional taint steps that should apply to flows related to
+ * XXE vulnerabilities.
+ */
+class XxeAdditionalTaintStep extends Unit {
+  /**
+   * Holds if the step from `node1` to `node2` should be considered a taint
+   * step for flows related to XXE vulnerabilities.
+   */
+  abstract predicate step(DataFlow::Node n1, DataFlow::Node n2);
+}

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

@@ -0,0 +1,23 @@
+/** Provides taint tracking configurations to be used in local XXE queries. */
+
+import java
+private import semmle.code.java.dataflow.FlowSources
+private import semmle.code.java.dataflow.TaintTracking
+private import semmle.code.java.security.XxeQuery
+
+/**
+ * A taint-tracking configuration for unvalidated local user input that is used in XML external entity expansion.
+ */
+class XxeLocalConfig extends TaintTracking::Configuration {
+  XxeLocalConfig() { this = "XxeLocalConfig" }
+
+  override predicate isSource(DataFlow::Node src) { src instanceof LocalUserInput }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof XxeSink }
+
+  override predicate isSanitizer(DataFlow::Node sanitizer) { sanitizer instanceof XxeSanitizer }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
+    any(XxeAdditionalTaintStep s).step(n1, n2)
+  }
+}

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

@@ -0,0 +1,35 @@
+/** Provides default definitions to be used in XXE queries. */
+
+import java
+private import semmle.code.java.dataflow.TaintTracking2
+private import semmle.code.java.security.XmlParsers
+import semmle.code.java.security.Xxe
+
+/**
+ * The default implementation of a XXE sink.
+ * The argument of a parse call on an insecurely configured XML parser.
+ */
+private class DefaultXxeSink extends XxeSink {
+  DefaultXxeSink() {
+    not exists(SafeSaxSourceFlowConfig safeSource | safeSource.hasFlowTo(this)) and
+    exists(XmlParserCall parse |
+      parse.getSink() = this.asExpr() and
+      not parse.isSafe()
+    )
+  }
+}
+
+/**
+ * A taint-tracking configuration for safe XML readers used to parse XML documents.
+ */
+private class SafeSaxSourceFlowConfig extends TaintTracking2::Configuration {
+  SafeSaxSourceFlowConfig() { this = "SafeSaxSourceFlowConfig" }
+
+  override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SafeSaxSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sink.asExpr() = any(XmlParserCall parse).getSink()
+  }
+
+  override int fieldFlowBranchLimit() { result = 0 }
+}

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

@@ -0,0 +1,23 @@
+/** Provides taint tracking configurations to be used in remote XXE queries. */
+
+import java
+private import semmle.code.java.dataflow.FlowSources
+private import semmle.code.java.dataflow.TaintTracking
+private import semmle.code.java.security.XxeQuery
+
+/**
+ * A taint-tracking configuration for unvalidated remote user input that is used in XML external entity expansion.
+ */
+class XxeConfig extends TaintTracking::Configuration {
+  XxeConfig() { this = "XxeConfig" }
+
+  override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof XxeSink }
+
+  override predicate isSanitizer(DataFlow::Node sanitizer) { sanitizer instanceof XxeSanitizer }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
+    any(XxeAdditionalTaintStep s).step(n1, n2)
+  }
+}

java/ql/src/Security/CWE/CWE-611/XXE.ql

@@ -14,41 +14,10 @@
  */
 
 import java
-import semmle.code.java.security.XmlParsers
-import semmle.code.java.dataflow.FlowSources
-import semmle.code.java.dataflow.TaintTracking2
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.security.XxeRemoteQuery
 import DataFlow::PathGraph
 
-class SafeSaxSourceFlowConfig extends TaintTracking2::Configuration {
-  SafeSaxSourceFlowConfig() { this = "XmlParsers::SafeSAXSourceFlowConfig" }
-
-  override predicate isSource(DataFlow::Node src) { src.asExpr() instanceof SafeSaxSource }
-
-  override predicate isSink(DataFlow::Node sink) {
-    sink.asExpr() = any(XmlParserCall parse).getSink()
-  }
-
-  override int fieldFlowBranchLimit() { result = 0 }
-}
-
-class UnsafeXxeSink extends DataFlow::ExprNode {
-  UnsafeXxeSink() {
-    not exists(SafeSaxSourceFlowConfig safeSource | safeSource.hasFlowTo(this)) and
-    exists(XmlParserCall parse |
-      parse.getSink() = this.getExpr() and
-      not parse.isSafe()
-    )
-  }
-}
-
-class XxeConfig extends TaintTracking::Configuration {
-  XxeConfig() { this = "XXE.ql::XxeConfig" }
-
-  override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
-
-  override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeXxeSink }
-}
-
 from DataFlow::PathNode source, DataFlow::PathNode sink, XxeConfig conf
 where conf.hasFlowPath(source, sink)
 select sink.getNode(), source, sink,

java/ql/src/Security/CWE/CWE-611/XXELocal.qhelp

@@ -0,0 +1,5 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<include src="XXE.qhelp" /></qhelp>

java/ql/src/Security/CWE/CWE-611/XXELocal.ql

@@ -0,0 +1,25 @@
+/**
+ * @name Resolving XML external entity in user-controlled data from local source
+ * @description Parsing user-controlled XML documents and allowing expansion of external entity
+ * references may lead to disclosure of confidential data or denial of service.
+ * @kind path-problem
+ * @problem.severity recommendation
+ * @security-severity 9.1
+ * @precision medium
+ * @id java/xxe-local
+ * @tags security
+ *       external/cwe/cwe-611
+ *       external/cwe/cwe-776
+ *       external/cwe/cwe-827
+ */
+
+import java
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.security.XxeLocalQuery
+import DataFlow::PathGraph
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, XxeLocalConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  "XML parsing depends on a $@ without guarding against external entity expansion.",
+  source.getNode(), "user-provided value"

java/ql/src/change-notes/2023-02-09-xxe-local.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `java/xxe-local`, which is a version of the XXE query that uses local sources (for example, reads from a local file).