From fec80973a9425632b51a46617b74628677af0da2 Mon Sep 17 00:00:00 2001
From: Ed Minnix <egregius313@github.com>
Date: Tue, 21 Mar 2023 20:44:47 -0400
Subject: [PATCH] Refactor SpelInjectionQuery

---
 .../code/java/security/SpelInjectionQuery.qll | 37 ++++++++++++++-----
 .../src/Security/CWE/CWE-094/SpelInjection.ql |  6 +--
 .../security/CWE-094/SpelInjectionTest.ql     |  2 +-
 3 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/java/ql/lib/semmle/code/java/security/SpelInjectionQuery.qll b/java/ql/lib/semmle/code/java/security/SpelInjectionQuery.qll
index 0d9cdc853bb..ed4fac7c06f 100644
--- a/java/ql/lib/semmle/code/java/security/SpelInjectionQuery.qll
+++ b/java/ql/lib/semmle/code/java/security/SpelInjectionQuery.qll
@@ -7,10 +7,12 @@ private import semmle.code.java.frameworks.spring.SpringExpression
 private import semmle.code.java.security.SpelInjection
 
 /**
+ * DEPRECATED: Use `SpelInjectionFlow` instead.
+ *
  * A taint-tracking configuration for unsafe user input
  * that is used to construct and evaluate a SpEL expression.
  */
-class SpelInjectionConfig extends TaintTracking::Configuration {
+deprecated class SpelInjectionConfig extends TaintTracking::Configuration {
   SpelInjectionConfig() { this = "SpelInjectionConfig" }
 
   override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
@@ -22,15 +24,30 @@ class SpelInjectionConfig extends TaintTracking::Configuration {
   }
 }
 
+/**
+ * A taint-tracking configuration for unsafe user input
+ * that is used to construct and evaluate a SpEL expression.
+ */
+private module SpelInjectionConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof SpelExpressionEvaluationSink }
+
+  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+    any(SpelExpressionInjectionAdditionalTaintStep c).step(node1, node2)
+  }
+}
+
+/** Tracks flow of unsafe user input that is used to construct and evaluate a SpEL expression. */
+module SpelInjectionFlow = TaintTracking::Make<SpelInjectionConfig>;
+
 /** Default sink for SpEL injection vulnerabilities. */
 private class DefaultSpelExpressionEvaluationSink extends SpelExpressionEvaluationSink {
   DefaultSpelExpressionEvaluationSink() {
     exists(MethodAccess ma |
       ma.getMethod() instanceof ExpressionEvaluationMethod and
       ma.getQualifier() = this.asExpr() and
-      not exists(SafeEvaluationContextFlowConfig config |
-        config.hasFlowTo(DataFlow::exprNode(ma.getArgument(0)))
-      )
+      not SafeEvaluationContextFlow::hasFlowToExpr(ma.getArgument(0))
     )
   }
 }
@@ -38,21 +55,21 @@ private class DefaultSpelExpressionEvaluationSink extends SpelExpressionEvaluati
 /**
  * A configuration for safe evaluation context that may be used in expression evaluation.
  */
-private class SafeEvaluationContextFlowConfig extends DataFlow2::Configuration {
-  SafeEvaluationContextFlowConfig() { this = "SpelInjection::SafeEvaluationContextFlowConfig" }
+private module SafeEvaluationContextFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof SafeContextSource }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof SafeContextSource }
-
-  override predicate isSink(DataFlow::Node sink) {
+  predicate isSink(DataFlow::Node sink) {
     exists(MethodAccess ma |
       ma.getMethod() instanceof ExpressionEvaluationMethod and
       ma.getArgument(0) = sink.asExpr()
     )
   }
 
-  override int fieldFlowBranchLimit() { result = 0 }
+  int fieldFlowBranchLimit() { result = 0 }
 }
 
+private module SafeEvaluationContextFlow = DataFlow::Make<SafeEvaluationContextFlowConfig>;
+
 /**
  * A `ContextSource` that is safe from SpEL injection.
  */
diff --git a/java/ql/src/Security/CWE/CWE-094/SpelInjection.ql b/java/ql/src/Security/CWE/CWE-094/SpelInjection.ql
index d6cc8f33c82..6963dfb22a6 100644
--- a/java/ql/src/Security/CWE/CWE-094/SpelInjection.ql
+++ b/java/ql/src/Security/CWE/CWE-094/SpelInjection.ql
@@ -14,9 +14,9 @@
 import java
 import semmle.code.java.security.SpelInjectionQuery
 import semmle.code.java.dataflow.DataFlow
-import DataFlow::PathGraph
+import SpelInjectionFlow::PathGraph
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, SpelInjectionConfig conf
-where conf.hasFlowPath(source, sink)
+from SpelInjectionFlow::PathNode source, SpelInjectionFlow::PathNode sink
+where SpelInjectionFlow::hasFlowPath(source, sink)
 select sink.getNode(), source, sink, "SpEL expression depends on a $@.", source.getNode(),
   "user-provided value"
diff --git a/java/ql/test/query-tests/security/CWE-094/SpelInjectionTest.ql b/java/ql/test/query-tests/security/CWE-094/SpelInjectionTest.ql
index cf89a1673ba..92bd3b2114e 100644
--- a/java/ql/test/query-tests/security/CWE-094/SpelInjectionTest.ql
+++ b/java/ql/test/query-tests/security/CWE-094/SpelInjectionTest.ql
@@ -11,7 +11,7 @@ class HasSpelInjectionTest extends InlineExpectationsTest {
 
   override predicate hasActualResult(Location location, string element, string tag, string value) {
     tag = "hasSpelInjection" and
-    exists(DataFlow::Node sink, SpelInjectionConfig conf | conf.hasFlowTo(sink) |
+    exists(DataFlow::Node sink | SpelInjectionFlow::hasFlowTo(sink) |
       sink.getLocation() = location and
       element = sink.toString() and
       value = ""