Merge pull request github#12427 from aschackmull/java/refactor-dataflow-queries-1

Java: Refactor some dataflow queries to the new API

Author: aschackmull

Diff for: java/ql/lib/semmle/code/java/security/RequestForgeryConfig.qll

@@ -8,9 +8,11 @@ import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.security.RequestForgery
 
 /**
+ * DEPRECATED: Use `RequestForgeryConfiguration` module instead.
+ *
  * A taint-tracking configuration characterising request-forgery risks.
  */
-class RequestForgeryConfiguration extends TaintTracking::Configuration {
+deprecated class RequestForgeryConfiguration extends TaintTracking::Configuration {
   RequestForgeryConfiguration() { this = "Server-Side Request Forgery" }
 
   override predicate isSource(DataFlow::Node source) {
@@ -29,3 +31,26 @@ class RequestForgeryConfiguration extends TaintTracking::Configuration {
 
   override predicate isSanitizer(DataFlow::Node node) { node instanceof RequestForgerySanitizer }
 }
+
+/**
+ * A taint-tracking configuration characterising request-forgery risks.
+ */
+private module RequestForgeryConfiguration implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source instanceof RemoteFlowSource and
+    // Exclude results of remote HTTP requests: fetching something else based on that result
+    // is no worse than following a redirect returned by the remote server, and typically
+    // we're requesting a resource via https which we trust to only send us to safe URLs.
+    not source.asExpr().(MethodAccess).getCallee() instanceof UrlConnectionGetInputStreamMethod
+  }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof RequestForgerySink }
+
+  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+    any(RequestForgeryAdditionalTaintStep r).propagatesTaint(pred, succ)
+  }
+
+  predicate isBarrier(DataFlow::Node node) { node instanceof RequestForgerySanitizer }
+}
+
+module RequestForgeryFlow = TaintTracking::Make<RequestForgeryConfiguration>;

Diff for: java/ql/lib/semmle/code/java/security/SensitiveLoggingQuery.qll

@@ -25,8 +25,12 @@ private class TypeType extends RefType {
   }
 }
 
-/** A data-flow configuration for identifying potentially-sensitive data flowing to a log output. */
-class SensitiveLoggerConfiguration extends TaintTracking::Configuration {
+/**
+ * DEPRECATED: Use `SensitiveLoggerConfiguration` module instead.
+ *
+ * A data-flow configuration for identifying potentially-sensitive data flowing to a log output.
+ */
+deprecated class SensitiveLoggerConfiguration extends TaintTracking::Configuration {
   SensitiveLoggerConfiguration() { this = "SensitiveLoggerConfiguration" }
 
   override predicate isSource(DataFlow::Node source) { source.asExpr() instanceof CredentialExpr }
@@ -43,3 +47,22 @@ class SensitiveLoggerConfiguration extends TaintTracking::Configuration {
 
   override predicate isSanitizerIn(Node node) { this.isSource(node) }
 }
+
+/** A data-flow configuration for identifying potentially-sensitive data flowing to a log output. */
+private module SensitiveLoggerConfiguration implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source.asExpr() instanceof CredentialExpr }
+
+  predicate isSink(DataFlow::Node sink) { sinkNode(sink, "logging") }
+
+  predicate isBarrier(DataFlow::Node sanitizer) {
+    sanitizer.asExpr() instanceof LiveLiteral or
+    sanitizer.getType() instanceof PrimitiveType or
+    sanitizer.getType() instanceof BoxedType or
+    sanitizer.getType() instanceof NumberType or
+    sanitizer.getType() instanceof TypeType
+  }
+
+  predicate isBarrierIn(Node node) { isSource(node) }
+}
+
+module SensitiveLoggerFlow = TaintTracking::Make<SensitiveLoggerConfiguration>;

Diff for: java/ql/lib/semmle/code/java/security/XSS.qll

@@ -7,7 +7,7 @@ import semmle.code.java.frameworks.spring.SpringController
 import semmle.code.java.frameworks.spring.SpringHttp
 import semmle.code.java.frameworks.javaee.jsf.JSFRenderer
 import semmle.code.java.dataflow.DataFlow
-import semmle.code.java.dataflow.TaintTracking2
+import semmle.code.java.dataflow.TaintTracking
 private import semmle.code.java.dataflow.ExternalFlow
 
 /** A sink that represent a method that outputs data without applying contextual output encoding. */
@@ -41,9 +41,9 @@ private class DefaultXssSink extends XssSink {
   DefaultXssSink() {
     sinkNode(this, "xss")
     or
-    exists(XssVulnerableWriterSourceToWritingMethodFlowConfig writer, MethodAccess ma |
+    exists(MethodAccess ma |
       ma.getMethod() instanceof WritingMethod and
-      writer.hasFlowToExpr(ma.getQualifier()) and
+      XssVulnerableWriterSourceToWritingMethodFlow::hasFlowToExpr(ma.getQualifier()) and
       this.asExpr() = ma.getArgument(_)
     )
   }
@@ -60,23 +60,19 @@ private class DefaultXssSanitizer extends XssSanitizer {
 }
 
 /** A configuration that tracks data from a servlet writer to an output method. */
-private class XssVulnerableWriterSourceToWritingMethodFlowConfig extends TaintTracking2::Configuration
-{
-  XssVulnerableWriterSourceToWritingMethodFlowConfig() {
-    this = "XSS::XssVulnerableWriterSourceToWritingMethodFlowConfig"
-  }
+private module XssVulnerableWriterSourceToWritingMethodFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node src) { src.asExpr() instanceof XssVulnerableWriterSource }
 
-  override predicate isSource(DataFlow::Node src) {
-    src.asExpr() instanceof XssVulnerableWriterSource
-  }
-
-  override predicate isSink(DataFlow::Node sink) {
+  predicate isSink(DataFlow::Node sink) {
     exists(MethodAccess ma |
       sink.asExpr() = ma.getQualifier() and ma.getMethod() instanceof WritingMethod
     )
   }
 }
 
+private module XssVulnerableWriterSourceToWritingMethodFlow =
+  TaintTracking::Make<XssVulnerableWriterSourceToWritingMethodFlowConfig>;
+
 /** A method that can be used to output data to an output stream or writer. */
 private class WritingMethod extends Method {
   WritingMethod() {

Diff for: java/ql/src/Security/CWE/CWE-022/TaintedPath.ql

@@ -18,32 +18,33 @@ import semmle.code.java.dataflow.FlowSources
 private import semmle.code.java.dataflow.ExternalFlow
 import semmle.code.java.security.PathCreation
 import semmle.code.java.security.PathSanitizer
-import DataFlow::PathGraph
 import TaintedPathCommon
 
-class TaintedPathConfig extends TaintTracking::Configuration {
-  TaintedPathConfig() { this = "TaintedPathConfig" }
+module TaintedPathConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
-
-  override predicate isSink(DataFlow::Node sink) {
+  predicate isSink(DataFlow::Node sink) {
     sink.asExpr() = any(PathCreation p).getAnInput()
     or
     sinkNode(sink, ["create-file", "read-file"])
   }
 
-  override predicate isSanitizer(DataFlow::Node sanitizer) {
+  predicate isBarrier(DataFlow::Node sanitizer) {
     sanitizer.getType() instanceof BoxedType or
     sanitizer.getType() instanceof PrimitiveType or
     sanitizer.getType() instanceof NumberType or
     sanitizer instanceof PathInjectionSanitizer
   }
 
-  override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
+  predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
     any(TaintedPathAdditionalTaintStep s).step(n1, n2)
   }
 }
 
+module TaintedPath = TaintTracking::Make<TaintedPathConfig>;
+
+import TaintedPath::PathGraph
+
 /**
  * Gets the data-flow node at which to report a path ending at `sink`.
  *
@@ -52,13 +53,13 @@ class TaintedPathConfig extends TaintTracking::Configuration {
  * continue to report there; otherwise we report directly at `sink`.
  */
 DataFlow::Node getReportingNode(DataFlow::Node sink) {
-  any(TaintedPathConfig c).hasFlowTo(sink) and
+  TaintedPath::hasFlowTo(sink) and
   if exists(PathCreation pc | pc.getAnInput() = sink.asExpr())
   then result.asExpr() = any(PathCreation pc | pc.getAnInput() = sink.asExpr())
   else result = sink
 }
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, TaintedPathConfig conf
-where conf.hasFlowPath(source, sink)
+from TaintedPath::PathNode source, TaintedPath::PathNode sink
+where TaintedPath::hasFlowPath(source, sink)
 select getReportingNode(sink.getNode()), source, sink, "This path depends on a $@.",
   source.getNode(), "user-provided value"

Diff for: java/ql/src/Security/CWE/CWE-079/XSS.ql

@@ -14,25 +14,26 @@
 import java
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.security.XSS
-import DataFlow::PathGraph
 
-class XssConfig extends TaintTracking::Configuration {
-  XssConfig() { this = "XSSConfig" }
+module XssConfig 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) { sink instanceof XssSink }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof XssSink }
+  predicate isBarrier(DataFlow::Node node) { node instanceof XssSanitizer }
 
-  override predicate isSanitizer(DataFlow::Node node) { node instanceof XssSanitizer }
+  predicate isBarrierOut(DataFlow::Node node) { node instanceof XssSinkBarrier }
 
-  override predicate isSanitizerOut(DataFlow::Node node) { node instanceof XssSinkBarrier }
-
-  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     any(XssAdditionalTaintStep s).step(node1, node2)
   }
 }
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, XssConfig conf
-where conf.hasFlowPath(source, sink)
+module XssFlow = TaintTracking::Make<XssConfig>;
+
+import XssFlow::PathGraph
+
+from XssFlow::PathNode source, XssFlow::PathNode sink
+where XssFlow::hasFlowPath(source, sink)
 select sink.getNode(), source, sink, "Cross-site scripting vulnerability due to a $@.",
   source.getNode(), "user-provided value"

Diff for: java/ql/src/Security/CWE/CWE-113/ResponseSplitting.ql

@@ -14,19 +14,16 @@
 import java
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.security.ResponseSplitting
-import DataFlow::PathGraph
 
-class ResponseSplittingConfig extends TaintTracking::Configuration {
-  ResponseSplittingConfig() { this = "ResponseSplittingConfig" }
-
-  override predicate isSource(DataFlow::Node source) {
+module ResponseSplittingConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
     source instanceof RemoteFlowSource and
     not source instanceof SafeHeaderSplittingSource
   }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof HeaderSplittingSink }
+  predicate isSink(DataFlow::Node sink) { sink instanceof HeaderSplittingSink }
 
-  override predicate isSanitizer(DataFlow::Node node) {
+  predicate isBarrier(DataFlow::Node node) {
     node.getType() instanceof PrimitiveType
     or
     node.getType() instanceof BoxedType
@@ -45,8 +42,12 @@ class ResponseSplittingConfig extends TaintTracking::Configuration {
   }
 }
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, ResponseSplittingConfig conf
-where conf.hasFlowPath(source, sink)
+module ResponseSplitting = TaintTracking::Make<ResponseSplittingConfig>;
+
+import ResponseSplitting::PathGraph
+
+from ResponseSplitting::PathNode source, ResponseSplitting::PathNode sink
+where ResponseSplitting::hasFlowPath(source, sink)
 select sink.getNode(), source, sink,
   "This header depends on a $@, which may cause a response-splitting vulnerability.",
   source.getNode(), "user-provided value"

Diff for: java/ql/src/Security/CWE/CWE-113/ResponseSplittingLocal.ql

@@ -14,23 +14,24 @@
 import java
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.security.ResponseSplitting
-import DataFlow::PathGraph
 
-class ResponseSplittingLocalConfig extends TaintTracking::Configuration {
-  ResponseSplittingLocalConfig() { this = "ResponseSplittingLocalConfig" }
+module ResponseSplittingLocalConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof LocalUserInput }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof LocalUserInput }
+  predicate isSink(DataFlow::Node sink) { sink instanceof HeaderSplittingSink }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof HeaderSplittingSink }
-
-  override predicate isSanitizer(DataFlow::Node node) {
+  predicate isBarrier(DataFlow::Node node) {
     node.getType() instanceof PrimitiveType or
     node.getType() instanceof BoxedType
   }
 }
 
-from DataFlow::PathNode source, DataFlow::PathNode sink, ResponseSplittingLocalConfig conf
-where conf.hasFlowPath(source, sink)
+module ResponseSplitting = TaintTracking::Make<ResponseSplittingLocalConfig>;
+
+import ResponseSplitting::PathGraph
+
+from ResponseSplitting::PathNode source, ResponseSplitting::PathNode sink
+where ResponseSplitting::hasFlowPath(source, sink)
 select sink.getNode(), source, sink,
   "This header depends on a $@, which may cause a response-splitting vulnerability.",
   source.getNode(), "user-provided value"

Diff for: java/ql/src/Security/CWE/CWE-209/StackTraceExposure.ql

@@ -31,33 +31,27 @@ class PrintStackTraceMethod extends Method {
   }
 }
 
-class ServletWriterSourceToPrintStackTraceMethodFlowConfig extends TaintTracking::Configuration {
-  ServletWriterSourceToPrintStackTraceMethodFlowConfig() {
-    this = "StackTraceExposure::ServletWriterSourceToPrintStackTraceMethodFlowConfig"
-  }
-
-  override predicate isSource(DataFlow::Node src) {
-    src.asExpr() instanceof XssVulnerableWriterSource
-  }
+module ServletWriterSourceToPrintStackTraceMethodFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node src) { src.asExpr() instanceof XssVulnerableWriterSource }
 
-  override predicate isSink(DataFlow::Node sink) {
+  predicate isSink(DataFlow::Node sink) {
     exists(MethodAccess ma |
       sink.asExpr() = ma.getAnArgument() and ma.getMethod() instanceof PrintStackTraceMethod
     )
   }
 }
 
+module ServletWriterSourceToPrintStackTraceMethodFlow =
+  TaintTracking::Make<ServletWriterSourceToPrintStackTraceMethodFlowConfig>;
+
 /**
  * A call that uses `Throwable.printStackTrace()` on a stream that is connected
  * to external output.
  */
 predicate printsStackToWriter(MethodAccess call) {
-  exists(
-    ServletWriterSourceToPrintStackTraceMethodFlowConfig writerSource,
-    PrintStackTraceMethod printStackTrace
-  |
+  exists(PrintStackTraceMethod printStackTrace |
     call.getMethod() = printStackTrace and
-    writerSource.hasFlowToExpr(call.getAnArgument())
+    ServletWriterSourceToPrintStackTraceMethodFlow::hasFlowToExpr(call.getAnArgument())
   )
 }
 
@@ -86,16 +80,15 @@ predicate stackTraceExpr(Expr exception, MethodAccess stackTraceString) {
   )
 }
 
-class StackTraceStringToHttpResponseSinkFlowConfig extends TaintTracking::Configuration {
-  StackTraceStringToHttpResponseSinkFlowConfig() {
-    this = "StackTraceExposure::StackTraceStringToHttpResponseSinkFlowConfig"
-  }
-
-  override predicate isSource(DataFlow::Node src) { stackTraceExpr(_, src.asExpr()) }
+module StackTraceStringToHttpResponseSinkFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node src) { stackTraceExpr(_, src.asExpr()) }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof InformationLeakSink }
+  predicate isSink(DataFlow::Node sink) { sink instanceof InformationLeakSink }
 }
 
+module StackTraceStringToHttpResponseSinkFlow =
+  TaintTracking::Make<StackTraceStringToHttpResponseSinkFlowConfig>;
+
 /**
  * A write of stack trace data to an external stream.
  */
@@ -109,9 +102,10 @@ predicate printsStackExternally(MethodAccess call, Expr stackTrace) {
  * A stringified stack trace flows to an external sink.
  */
 predicate stringifiedStackFlowsExternally(DataFlow::Node externalExpr, Expr stackTrace) {
-  exists(MethodAccess stackTraceString, StackTraceStringToHttpResponseSinkFlowConfig conf |
+  exists(MethodAccess stackTraceString |
     stackTraceExpr(stackTrace, stackTraceString) and
-    conf.hasFlow(DataFlow::exprNode(stackTraceString), externalExpr)
+    StackTraceStringToHttpResponseSinkFlow::hasFlow(DataFlow::exprNode(stackTraceString),
+      externalExpr)
   )
 }