Merge pull request github#12476 from aschackmull/java/refactor-dataflow-queries-2

Java: Refactor more dataflow queries to the new API

Author: aschackmull

java/ql/lib/semmle/code/java/security/regexp/PolynomialReDoSQuery.qll

@@ -32,8 +32,12 @@ private class LengthRestrictedMethod extends Method {
   }
 }
 
-/** A configuration for Polynomial ReDoS queries. */
-class PolynomialRedosConfig extends TaintTracking::Configuration {
+/**
+ * DEPRECATED: Use `PolynomialRedosFlow` instead.
+ *
+ * A configuration for Polynomial ReDoS queries.
+ */
+deprecated class PolynomialRedosConfig extends TaintTracking::Configuration {
   PolynomialRedosConfig() { this = "PolynomialRedosConfig" }
 
   override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
@@ -47,11 +51,34 @@ class PolynomialRedosConfig extends TaintTracking::Configuration {
   }
 }
 
-/** Holds if there is flow from `source` to `sink` that is matched against the regexp term `regexp` that is vulnerable to Polynomial ReDoS. */
-predicate hasPolynomialReDoSResult(
+/**
+ * DEPRECATED: Use `PolynomialRedosFlow` instead.
+ *
+ * Holds if there is flow from `source` to `sink` that is matched against the regexp term `regexp` that is vulnerable to Polynomial ReDoS.
+ */
+deprecated predicate hasPolynomialReDoSResult(
   DataFlow::PathNode source, DataFlow::PathNode sink,
   SuperlinearBackTracking::PolynomialBackTrackingTerm regexp
 ) {
   any(PolynomialRedosConfig config).hasFlowPath(source, sink) and
   regexp.getRootTerm() = sink.getNode().(PolynomialRedosSink).getRegExp()
 }
+
+/** A configuration for Polynomial ReDoS queries. */
+private module PolynomialRedosConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(SuperlinearBackTracking::PolynomialBackTrackingTerm regexp |
+      regexp.getRootTerm() = sink.(PolynomialRedosSink).getRegExp()
+    )
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node.getType() instanceof PrimitiveType or
+    node.getType() instanceof BoxedType or
+    node.asExpr().(MethodAccess).getMethod() instanceof LengthRestrictedMethod
+  }
+}
+
+module PolynomialRedosFlow = TaintTracking::Make<PolynomialRedosConfig>;

java/ql/src/Security/CWE/CWE-190/ArithmeticTainted.ql

@@ -15,35 +15,39 @@
 import java
 import semmle.code.java.dataflow.FlowSources
 import ArithmeticCommon
-import DataFlow::PathGraph
 
-class RemoteUserInputOverflowConfig extends TaintTracking::Configuration {
-  RemoteUserInputOverflowConfig() { this = "ArithmeticTainted.ql:RemoteUserInputOverflowConfig" }
+module RemoteUserInputOverflowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+  predicate isSink(DataFlow::Node sink) { overflowSink(_, sink.asExpr()) }
 
-  override predicate isSink(DataFlow::Node sink) { overflowSink(_, sink.asExpr()) }
+  predicate isBarrier(DataFlow::Node n) { overflowBarrier(n) }
+}
+
+module RemoteUserInputUnderflowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
-  override predicate isSanitizer(DataFlow::Node n) { overflowBarrier(n) }
+  predicate isSink(DataFlow::Node sink) { underflowSink(_, sink.asExpr()) }
+
+  predicate isBarrier(DataFlow::Node n) { underflowBarrier(n) }
 }
 
-class RemoteUserInputUnderflowConfig extends TaintTracking::Configuration {
-  RemoteUserInputUnderflowConfig() { this = "ArithmeticTainted.ql:RemoteUserInputUnderflowConfig" }
+module RemoteUserInputOverflow = TaintTracking::Make<RemoteUserInputOverflowConfig>;
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+module RemoteUserInputUnderflow = TaintTracking::Make<RemoteUserInputUnderflowConfig>;
 
-  override predicate isSink(DataFlow::Node sink) { underflowSink(_, sink.asExpr()) }
+module Flow =
+  DataFlow::MergePathGraph<RemoteUserInputOverflow::PathNode, RemoteUserInputUnderflow::PathNode, RemoteUserInputOverflow::PathGraph, RemoteUserInputUnderflow::PathGraph>;
 
-  override predicate isSanitizer(DataFlow::Node n) { underflowBarrier(n) }
-}
+import Flow::PathGraph
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, ArithExpr exp, string effect
+from Flow::PathNode source, Flow::PathNode sink, ArithExpr exp, string effect
 where
-  any(RemoteUserInputOverflowConfig c).hasFlowPath(source, sink) and
+  RemoteUserInputOverflow::hasFlowPath(source.asPathNode1(), sink.asPathNode1()) and
   overflowSink(exp, sink.getNode().asExpr()) and
   effect = "overflow"
   or
-  any(RemoteUserInputUnderflowConfig c).hasFlowPath(source, sink) and
+  RemoteUserInputUnderflow::hasFlowPath(source.asPathNode2(), sink.asPathNode2()) and
   underflowSink(exp, sink.getNode().asExpr()) and
   effect = "underflow"
 select exp, source, sink,

java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql

@@ -14,8 +14,7 @@
 import java
 import semmle.code.java.os.OSCheck
 import TempDirUtils
-import DataFlow::PathGraph
-import semmle.code.java.dataflow.TaintTracking2
+import semmle.code.java.dataflow.TaintTracking
 
 abstract private class MethodFileSystemFileCreation extends Method {
   MethodFileSystemFileCreation() { this.getDeclaringType() instanceof TypeFile }
@@ -127,19 +126,17 @@ private class IsSpecificWindowsSanitizer extends WindowsOsSanitizer {
  * A taint tracking configuration tracking the access of the system temporary directory
  * flowing to the creation of files or directories.
  */
-private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Configuration {
-  TempDirSystemGetPropertyToCreateConfig() { this = "TempDirSystemGetPropertyToCreateConfig" }
-
-  override predicate isSource(DataFlow::Node source) {
+module TempDirSystemGetPropertyToCreateConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
     source.asExpr() instanceof ExprSystemGetPropertyTempDirTainted
   }
 
-  override predicate isSink(DataFlow::Node sink) {
+  predicate isSink(DataFlow::Node sink) {
     sink instanceof FileCreationSink and
-    not any(TempDirSystemGetPropertyDirectlyToMkdirConfig config).hasFlowTo(sink)
+    not TempDirSystemGetPropertyDirectlyToMkdir::hasFlowTo(sink)
   }
 
-  override predicate isSanitizer(DataFlow::Node sanitizer) {
+  predicate isBarrier(DataFlow::Node sanitizer) {
     exists(FilesSanitizingCreationMethodAccess sanitisingMethodAccess |
       sanitizer.asExpr() = sanitisingMethodAccess.getArgument(0)
     )
@@ -148,6 +145,9 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf
   }
 }
 
+module TempDirSystemGetPropertyToCreate =
+  TaintTracking::Make<TempDirSystemGetPropertyToCreateConfig>;
+
 /**
  * Configuration that tracks calls to to `mkdir` or `mkdirs` that are are directly on the temp directory system property.
  * Examples:
@@ -158,30 +158,29 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf
  * As such, this code pattern is filtered out as an explicit vulnerability in
  * `TempDirSystemGetPropertyToCreateConfig::isSink`.
  */
-private class TempDirSystemGetPropertyDirectlyToMkdirConfig extends TaintTracking2::Configuration {
-  TempDirSystemGetPropertyDirectlyToMkdirConfig() {
-    this = "TempDirSystemGetPropertyDirectlyToMkdirConfig"
-  }
-
-  override predicate isSource(DataFlow::Node node) {
+module TempDirSystemGetPropertyDirectlyToMkdirConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) {
     exists(ExprSystemGetPropertyTempDirTainted propertyGetExpr, DataFlow::Node callSite |
       DataFlow::localFlow(DataFlow::exprNode(propertyGetExpr), callSite)
     |
       isFileConstructorArgument(callSite.asExpr(), node.asExpr(), 1)
     )
   }
 
-  override predicate isSink(DataFlow::Node node) {
+  predicate isSink(DataFlow::Node node) {
     exists(MethodAccess ma | ma.getMethod() instanceof MethodFileDirectoryCreation |
       ma.getQualifier() = node.asExpr()
     )
   }
 
-  override predicate isSanitizer(DataFlow::Node sanitizer) {
+  predicate isBarrier(DataFlow::Node sanitizer) {
     isFileConstructorArgument(sanitizer.asExpr(), _, _)
   }
 }
 
+module TempDirSystemGetPropertyDirectlyToMkdir =
+  TaintTracking::Make<TempDirSystemGetPropertyDirectlyToMkdirConfig>;
+
 //
 // Begin configuration for tracking single-method calls that are vulnerable.
 //
@@ -193,6 +192,8 @@ abstract class MethodAccessInsecureFileCreation extends MethodAccess {
    * Gets the type of entity created (e.g. `file`, `directory`, ...).
    */
   abstract string getFileSystemEntityType();
+
+  DataFlow::Node getNode() { result.asExpr() = this }
 }
 
 /**
@@ -235,39 +236,47 @@ class MethodAccessInsecureGuavaFilesCreateTempFile extends MethodAccessInsecureF
 }
 
 /**
- * A hack: we include use of inherently insecure methods, which don't have any associated
+ * We include use of inherently insecure methods, which don't have any associated
  * flow path, in with results describing a path from reading `java.io.tmpdir` or similar to use
  * in a file creation op.
  *
- * We achieve this by making inherently-insecure method invocations both a source and a sink in
- * this configuration, resulting in a zero-length path which is type-compatible with the actual
- * path-flow results.
+ * We achieve this by making inherently-insecure method invocations into an edge-less graph,
+ * resulting in a zero-length paths.
  */
-class InsecureMethodPseudoConfiguration extends DataFlow::Configuration {
-  InsecureMethodPseudoConfiguration() { this = "InsecureMethodPseudoConfiguration" }
+module InsecureMethodPathGraph implements DataFlow::PathGraphSig<MethodAccessInsecureFileCreation> {
+  predicate edges(MethodAccessInsecureFileCreation n1, MethodAccessInsecureFileCreation n2) {
+    none()
+  }
 
-  override predicate isSource(DataFlow::Node node) {
-    node.asExpr() instanceof MethodAccessInsecureFileCreation
+  predicate nodes(MethodAccessInsecureFileCreation n, string key, string val) {
+    key = "semmle.label" and val = n.toString()
   }
 
-  override predicate isSink(DataFlow::Node node) {
-    node.asExpr() instanceof MethodAccessInsecureFileCreation
+  predicate subpaths(
+    MethodAccessInsecureFileCreation n1, MethodAccessInsecureFileCreation n2,
+    MethodAccessInsecureFileCreation n3, MethodAccessInsecureFileCreation n4
+  ) {
+    none()
   }
 }
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, string message
+module Flow =
+  DataFlow::MergePathGraph<TempDirSystemGetPropertyToCreate::PathNode, MethodAccessInsecureFileCreation, TempDirSystemGetPropertyToCreate::PathGraph, InsecureMethodPathGraph>;
+
+import Flow::PathGraph
+
+from Flow::PathNode source, Flow::PathNode sink, string message
 where
   (
-    any(TempDirSystemGetPropertyToCreateConfig conf).hasFlowPath(source, sink) and
+    TempDirSystemGetPropertyToCreate::hasFlowPath(source.asPathNode1(), sink.asPathNode1()) and
     message =
       "Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users."
     or
-    any(InsecureMethodPseudoConfiguration conf).hasFlowPath(source, sink) and
+    source = sink and
     // Note this message has no "$@" placeholder, so the "system temp directory" template parameter below is not used.
     message =
       "Local information disclosure vulnerability due to use of " +
-        source.getNode().asExpr().(MethodAccessInsecureFileCreation).getFileSystemEntityType() +
-        " readable by other local users."
+        source.asPathNode2().getFileSystemEntityType() + " readable by other local users."
   ) and
   not isPermissionsProtectedTempDirUse(sink.getNode())
 select source.getNode(), source, sink, message, source.getNode(), "system temp directory"

java/ql/src/Security/CWE/CWE-730/PolynomialReDoS.ql

@@ -15,12 +15,14 @@
 
 import java
 import semmle.code.java.security.regexp.PolynomialReDoSQuery
-import DataFlow::PathGraph
+import PolynomialRedosFlow::PathGraph
 
 from
-  DataFlow::PathNode source, DataFlow::PathNode sink,
+  PolynomialRedosFlow::PathNode source, PolynomialRedosFlow::PathNode sink,
   SuperlinearBackTracking::PolynomialBackTrackingTerm regexp
-where hasPolynomialReDoSResult(source, sink, regexp)
+where
+  PolynomialRedosFlow::hasFlowPath(source, sink) and
+  regexp.getRootTerm() = sink.getNode().(PolynomialRedosSink).getRegExp()
 select sink, source, sink,
   "This $@ that depends on a $@ may run slow on strings " + regexp.getPrefixMessage() +
     "with many repetitions of '" + regexp.getPumpString() + "'.", regexp, "regular expression",

java/ql/test/query-tests/security/CWE-730/PolynomialReDoS.ql

@@ -8,10 +8,10 @@ class HasPolyRedos extends InlineExpectationsTest {
 
   override predicate hasActualResult(Location location, string element, string tag, string value) {
     tag = "hasPolyRedos" and
-    exists(DataFlow::PathNode sink |
-      hasPolynomialReDoSResult(_, sink, _) and
-      location = sink.getNode().getLocation() and
-      element = sink.getNode().toString() and
+    exists(DataFlow::Node sink |
+      PolynomialRedosFlow::hasFlowTo(sink) and
+      location = sink.getLocation() and
+      element = sink.toString() and
       value = ""
     )
   }