Merge pull request github#12751 from egregius313/egregius313/dataflow-refactor-cleanup

egregius313 · web-flow · commit 38826c98f1de · 2023-04-14T10:35:11.000-04:00
Java: Finish dataflow refactor
diff --git a/java/ql/lib/change-notes/2023-04-05-deprecated-sensitive-result-receiver-predicate.md b/java/ql/lib/change-notes/2023-04-05-deprecated-sensitive-result-receiver-predicate.md
@@ -0,0 +1,4 @@
+---
+category: deprecated
+---
+* The `sensitiveResultReceiver` predicate in `SensitiveResultReceiverQuery.qll` has been deprecated and replaced with `isSensitiveResultReceiver` in order to use the new dataflow API.
diff --git a/java/ql/lib/semmle/code/java/security/HardcodedCredentialsSourceCallQuery.qll b/java/ql/lib/semmle/code/java/security/HardcodedCredentialsSourceCallQuery.qll
@@ -8,10 +8,12 @@ import semmle.code.java.dataflow.DataFlow2
 import HardcodedCredentials
 
 /**
+ * DEPRECATED: Use `HardcodedCredentialSourceCallFlow` instead.
+ *
  * A data-flow configuration that tracks hardcoded expressions flowing to a parameter whose name suggests
  * it may be a credential, excluding those which flow on to other such insecure usage sites.
  */
-class HardcodedCredentialSourceCallConfiguration extends DataFlow::Configuration {
+deprecated class HardcodedCredentialSourceCallConfiguration extends DataFlow::Configuration {
   HardcodedCredentialSourceCallConfiguration() {
     this = "HardcodedCredentialSourceCallConfiguration"
   }
@@ -22,10 +24,28 @@ class HardcodedCredentialSourceCallConfiguration extends DataFlow::Configuration
 }
 
 /**
+ * A data-flow configuration that tracks hardcoded expressions flowing to a parameter whose name suggests
+ * it may be a credential, excluding those which flow on to other such insecure usage sites.
+ */
+module HardcodedCredentialSourceCallConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node n) { n.asExpr() instanceof HardcodedExpr }
+
+  predicate isSink(DataFlow::Node n) { n.asExpr() instanceof FinalCredentialsSourceSink }
+}
+
+/**
+ * Tracks hardcoded expressions flowing to a parameter whose name suggests
+ * it may be a credential, excluding those which flow on to other such insecure usage sites.
+ */
+module HardcodedCredentialSourceCallFlow = DataFlow::Global<HardcodedCredentialSourceCallConfig>;
+
+/**
+ * DEPRECATED: Use `HardcodedCredentialParameterSourceCallFlow` instead.
+ *
  * A data-flow configuration that tracks flow from an argument whose corresponding parameter name suggests
  * a credential, to an argument to a sensitive call.
  */
-class HardcodedCredentialSourceCallConfiguration2 extends DataFlow2::Configuration {
+deprecated class HardcodedCredentialSourceCallConfiguration2 extends DataFlow2::Configuration {
   HardcodedCredentialSourceCallConfiguration2() {
     this = "HardcodedCredentialSourceCallConfiguration2"
   }
@@ -35,17 +55,33 @@ class HardcodedCredentialSourceCallConfiguration2 extends DataFlow2::Configurati
   override predicate isSink(DataFlow::Node n) { n.asExpr() instanceof CredentialsSink }
 }
 
+/**
+ * A data-flow configuration that tracks flow from an argument whose corresponding parameter name suggests
+ * a credential, to an argument to a sensitive call.
+ */
+module HardcodedCredentialParameterSourceCallConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node n) { n.asExpr() instanceof CredentialsSourceSink }
+
+  predicate isSink(DataFlow::Node n) { n.asExpr() instanceof CredentialsSink }
+}
+
+/**
+ * Tracks flow from an argument whose corresponding parameter name suggests
+ * a credential, to an argument to a sensitive call.
+ */
+module HardcodedCredentialParameterSourceCallFlow =
+  DataFlow::Global<HardcodedCredentialParameterSourceCallConfig>;
+
 /**
  * An argument to a call, where the parameter name corresponding
  * to the argument indicates that it may contain credentials, and
  * where this expression does not flow on to another `CredentialsSink`.
  */
 class FinalCredentialsSourceSink extends CredentialsSourceSink {
   FinalCredentialsSourceSink() {
-    not exists(HardcodedCredentialSourceCallConfiguration2 conf, CredentialsSink other |
-      this != other
-    |
-      conf.hasFlow(DataFlow::exprNode(this), DataFlow::exprNode(other))
+    not exists(CredentialsSink other | this != other |
+      HardcodedCredentialParameterSourceCallFlow::flow(DataFlow::exprNode(this),
+        DataFlow::exprNode(other))
     )
   }
 }
diff --git a/java/ql/lib/semmle/code/java/security/SensitiveResultReceiverQuery.qll b/java/ql/lib/semmle/code/java/security/SensitiveResultReceiverQuery.qll
@@ -1,7 +1,7 @@
 /** Definitions for the sensitive result receiver query. */
 
 import java
-import semmle.code.java.dataflow.TaintTracking2
+import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.security.SensitiveActions
 
@@ -17,21 +17,21 @@ private class ResultReceiverSendCall extends MethodAccess {
   Expr getSentData() { result = this.getArgument(1) }
 }
 
-private class UntrustedResultReceiverConf extends TaintTracking2::Configuration {
-  UntrustedResultReceiverConf() { this = "UntrustedResultReceiverConf" }
+private module UntrustedResultReceiverConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
 
-  override predicate isSource(DataFlow::Node node) { node instanceof RemoteFlowSource }
-
-  override predicate isSink(DataFlow::Node node) {
+  predicate isSink(DataFlow::Node node) {
     node.asExpr() = any(ResultReceiverSendCall c).getReceiver()
   }
 }
 
+private module UntrustedResultReceiverFlow = TaintTracking::Global<UntrustedResultReceiverConfig>;
+
 private predicate untrustedResultReceiverSend(DataFlow::Node src, ResultReceiverSendCall call) {
-  any(UntrustedResultReceiverConf c).hasFlow(src, DataFlow::exprNode(call.getReceiver()))
+  UntrustedResultReceiverFlow::flow(src, DataFlow::exprNode(call.getReceiver()))
 }
 
-private class SensitiveResultReceiverConf extends TaintTracking::Configuration {
+deprecated private class SensitiveResultReceiverConf extends TaintTracking::Configuration {
   SensitiveResultReceiverConf() { this = "SensitiveResultReceiverConf" }
 
   override predicate isSource(DataFlow::Node node) { node.asExpr() instanceof SensitiveExpr }
@@ -50,12 +50,46 @@ private class SensitiveResultReceiverConf extends TaintTracking::Configuration {
   }
 }
 
-/** Holds if there is a path from sensitive data at `src` to a result receiver at `sink`, and the receiver was obtained from an untrusted source `recSrc`. */
-predicate sensitiveResultReceiver(
+private module SensitiveResultReceiverConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) { node.asExpr() instanceof SensitiveExpr }
+
+  predicate isSink(DataFlow::Node node) {
+    exists(ResultReceiverSendCall call |
+      untrustedResultReceiverSend(_, call) and
+      node.asExpr() = call.getSentData()
+    )
+  }
+
+  predicate allowImplicitRead(DataFlow::Node n, DataFlow::ContentSet c) { isSink(n) and exists(c) }
+}
+
+/** Taint tracking flow for sensitive expressions flowing to untrusted result receivers. */
+module SensitiveResultReceiverFlow = TaintTracking::Global<SensitiveResultReceiverConfig>;
+
+/**
+ * DEPRECATED: Use `isSensitiveResultReceiver` instead.
+ *
+ * Holds if there is a path from sensitive data at `src` to a result receiver at `sink`, and the receiver was obtained from an untrusted source `recSrc`.
+ */
+deprecated predicate sensitiveResultReceiver(
   DataFlow::PathNode src, DataFlow::PathNode sink, DataFlow::Node recSrc
 ) {
-  exists(ResultReceiverSendCall call, SensitiveResultReceiverConf conf |
-    conf.hasFlowPath(src, sink) and
+  exists(ResultReceiverSendCall call |
+    any(SensitiveResultReceiverConf c).hasFlowPath(src, sink) and
+    sink.getNode().asExpr() = call.getSentData() and
+    untrustedResultReceiverSend(recSrc, call)
+  )
+}
+
+/**
+ * Holds if there is a path from sensitive data at `src` to a result receiver at `sink`, and the receiver was obtained from an untrusted source `recSrc`.
+ */
+predicate isSensitiveResultReceiver(
+  SensitiveResultReceiverFlow::PathNode src, SensitiveResultReceiverFlow::PathNode sink,
+  DataFlow::Node recSrc
+) {
+  exists(ResultReceiverSendCall call |
+    SensitiveResultReceiverFlow::flowPath(src, sink) and
     sink.getNode().asExpr() = call.getSentData() and
     untrustedResultReceiverSend(recSrc, call)
   )
diff --git a/java/ql/src/Security/CWE/CWE-798/HardcodedCredentialsSourceCall.ql b/java/ql/src/Security/CWE/CWE-798/HardcodedCredentialsSourceCall.ql
@@ -12,11 +12,11 @@
 
 import java
 import semmle.code.java.security.HardcodedCredentialsSourceCallQuery
-import DataFlow::PathGraph
+import HardcodedCredentialSourceCallFlow::PathGraph
 
 from
-  DataFlow::PathNode source, DataFlow::PathNode sink,
-  HardcodedCredentialSourceCallConfiguration conf
-where conf.hasFlowPath(source, sink)
+  HardcodedCredentialSourceCallFlow::PathNode source,
+  HardcodedCredentialSourceCallFlow::PathNode sink
+where HardcodedCredentialSourceCallFlow::flowPath(source, sink)
 select source.getNode(), source, sink, "Hard-coded value flows to $@.", sink.getNode(),
   "sensitive call"
diff --git a/java/ql/src/Security/CWE/CWE-927/SensitiveResultReceiver.ql b/java/ql/src/Security/CWE/CWE-927/SensitiveResultReceiver.ql
@@ -13,9 +13,11 @@
 
 import java
 import semmle.code.java.security.SensitiveResultReceiverQuery
-import DataFlow::PathGraph
+import SensitiveResultReceiverFlow::PathGraph
 
-from DataFlow::PathNode src, DataFlow::PathNode sink, DataFlow::Node recSrc
-where sensitiveResultReceiver(src, sink, recSrc)
+from
+  SensitiveResultReceiverFlow::PathNode src, SensitiveResultReceiverFlow::PathNode sink,
+  DataFlow::Node recSrc
+where isSensitiveResultReceiver(src, sink, recSrc)
 select sink, src, sink, "This $@ is sent to a ResultReceiver obtained from $@.", src,
   "sensitive information", recSrc, "this untrusted source"
diff --git a/java/ql/test/library-tests/frameworks/ratpack/flow.ql b/java/ql/test/library-tests/frameworks/ratpack/flow.ql
@@ -3,28 +3,28 @@ import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.dataflow.FlowSources
 import TestUtilities.InlineExpectationsTest
 
-class Conf extends TaintTracking::Configuration {
-  Conf() { this = "qltest:frameworks:ratpack" }
-
-  override predicate isSource(DataFlow::Node n) {
+module Config implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node n) {
     n.asExpr().(MethodAccess).getMethod().hasName("taint")
     or
     n instanceof RemoteFlowSource
   }
 
-  override predicate isSink(DataFlow::Node n) {
+  predicate isSink(DataFlow::Node n) {
     exists(MethodAccess ma | ma.getMethod().hasName("sink") | n.asExpr() = ma.getAnArgument())
   }
 }
 
+module Flow = TaintTracking::Global<Config>;
+
 class HasFlowTest extends InlineExpectationsTest {
   HasFlowTest() { this = "HasFlowTest" }
 
   override string getARelevantTag() { result = "hasTaintFlow" }
 
   override predicate hasActualResult(Location location, string element, string tag, string value) {
     tag = "hasTaintFlow" and
-    exists(DataFlow::Node sink, Conf conf | conf.hasFlowTo(sink) |
+    exists(DataFlow::Node sink | Flow::flowTo(sink) |
       sink.getLocation() = location and
       element = sink.toString() and
       value = ""
diff --git a/java/ql/test/query-tests/security/CWE-089/semmle/examples/taintedString.ql b/java/ql/test/query-tests/security/CWE-089/semmle/examples/taintedString.ql
@@ -1,16 +1,16 @@
 import semmle.code.java.dataflow.FlowSources
 
-class Conf extends TaintTracking::Configuration {
-  Conf() { this = "qltest:cwe-089:taintedString" }
+module Config implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof UserInput }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof UserInput }
-
-  override predicate isSink(DataFlow::Node sink) { any() }
+  predicate isSink(DataFlow::Node sink) { any() }
 }
 
-from Conf conf, Expr tainted, Method method
+module Flow = TaintTracking::Global<Config>;
+
+from Expr tainted, Method method
 where
-  conf.hasFlowToExpr(tainted) and
+  Flow::flowToExpr(tainted) and
   tainted.getEnclosingCallable() = method and
   tainted.getFile().getStem() = ["Test", "Validation"]
 select method, tainted.getLocation().getStartLine() - method.getLocation().getStartLine(), tainted
diff --git a/java/ql/test/query-tests/security/CWE-798/semmle/tests/HardcodedCredentialsSourceCall.ql b/java/ql/test/query-tests/security/CWE-798/semmle/tests/HardcodedCredentialsSourceCall.ql
@@ -9,9 +9,7 @@ class HardcodedCredentialsSourceCallTest extends InlineExpectationsTest {
 
   override predicate hasActualResult(Location location, string element, string tag, string value) {
     tag = "HardcodedCredentialsSourceCall" and
-    exists(DataFlow::Node sink, HardcodedCredentialSourceCallConfiguration conf |
-      conf.hasFlow(_, sink)
-    |
+    exists(DataFlow::Node sink | HardcodedCredentialSourceCallFlow::flowTo(sink) |
       sink.getLocation() = location and
       element = sink.toString() and
       value = ""
diff --git a/java/ql/test/query-tests/security/CWE-927/SensitiveResultReceiver.ql b/java/ql/test/query-tests/security/CWE-927/SensitiveResultReceiver.ql
@@ -14,8 +14,8 @@ class ResultReceiverTest extends InlineExpectationsTest {
   override string getARelevantTag() { result = "hasSensitiveResultReceiver" }
 
   override predicate hasActualResult(Location loc, string element, string tag, string value) {
-    exists(DataFlow::PathNode sink |
-      sensitiveResultReceiver(_, sink, _) and
+    exists(SensitiveResultReceiverFlow::PathNode sink |
+      isSensitiveResultReceiver(_, sink, _) and
       element = sink.toString() and
       loc = sink.getNode().getLocation() and
       tag = "hasSensitiveResultReceiver" and

-Original file line number
+Diff line change
@@ @@ -0,0 +1,4 @@ @@
 +---
 +category: deprecated
 +---
 +* The `sensitiveResultReceiver` predicate in `SensitiveResultReceiverQuery.qll` has been deprecated and replaced with `isSensitiveResultReceiver` in order to use the new dataflow API.