Java: Refactor ArithmeticTainted.ql, TempDirLocalInformationDisclosure.ql

Author: aschackmull

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"