Merge pull request github#12870 from michaelnebel/csharp/refactordataflow6

C#: Re-factor data flow and taint tracking configurations to use the new API.

Author: michaelnebel

csharp/ql/src/API Abuse/FormatInvalid.ql

@@ -12,38 +12,36 @@
 
 import csharp
 import semmle.code.csharp.frameworks.Format
-import DataFlow::PathGraph
+import FormatInvalid::PathGraph
 
-private class FormatConfiguration extends DataFlow::Configuration {
-  FormatConfiguration() { this = "format" }
+module FormatInvalidConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node n) { n.asExpr() instanceof StringLiteral }
 
-  override predicate isSource(DataFlow::Node n) { n.asExpr() instanceof StringLiteral }
-
-  override predicate isSink(DataFlow::Node n) {
-    exists(FormatCall c | n.asExpr() = c.getFormatExpr())
-  }
+  predicate isSink(DataFlow::Node n) { exists(FormatCall c | n.asExpr() = c.getFormatExpr()) }
 }
 
+module FormatInvalid = DataFlow::Global<FormatInvalidConfig>;
+
 private predicate invalidFormatString(
-  InvalidFormatString src, DataFlow::PathNode source, DataFlow::PathNode sink, string msg,
+  InvalidFormatString src, FormatInvalid::PathNode source, FormatInvalid::PathNode sink, string msg,
   FormatCall call, string callString
 ) {
   source.getNode().asExpr() = src and
   sink.getNode().asExpr() = call.getFormatExpr() and
-  any(FormatConfiguration conf).hasFlowPath(source, sink) and
+  FormatInvalid::flowPath(source, sink) and
   call.hasInsertions() and
   msg = "Invalid format string used in $@ formatting call." and
   callString = "this"
 }
 
 private predicate unusedArgument(
-  FormatCall call, DataFlow::PathNode source, DataFlow::PathNode sink, string msg,
+  FormatCall call, FormatInvalid::PathNode source, FormatInvalid::PathNode sink, string msg,
   ValidFormatString src, string srcString, Expr unusedExpr, string unusedString
 ) {
   exists(int unused |
     source.getNode().asExpr() = src and
     sink.getNode().asExpr() = call.getFormatExpr() and
-    any(FormatConfiguration conf).hasFlowPath(source, sink) and
+    FormatInvalid::flowPath(source, sink) and
     unused = call.getASuppliedArgument() and
     not unused = src.getAnInsert() and
     not src.getValue() = "" and
@@ -55,13 +53,13 @@ private predicate unusedArgument(
 }
 
 private predicate missingArgument(
-  FormatCall call, DataFlow::PathNode source, DataFlow::PathNode sink, string msg,
+  FormatCall call, FormatInvalid::PathNode source, FormatInvalid::PathNode sink, string msg,
   ValidFormatString src, string srcString
 ) {
   exists(int used, int supplied |
     source.getNode().asExpr() = src and
     sink.getNode().asExpr() = call.getFormatExpr() and
-    any(FormatConfiguration conf).hasFlowPath(source, sink) and
+    FormatInvalid::flowPath(source, sink) and
     used = src.getAnInsert() and
     supplied = call.getSuppliedArguments() and
     used >= supplied and
@@ -71,8 +69,8 @@ private predicate missingArgument(
 }
 
 from
-  Element alert, DataFlow::PathNode source, DataFlow::PathNode sink, string msg, Element extra1,
-  string extra1String, Element extra2, string extra2String
+  Element alert, FormatInvalid::PathNode source, FormatInvalid::PathNode sink, string msg,
+  Element extra1, string extra1String, Element extra2, string extra2String
 where
   invalidFormatString(alert, source, sink, msg, extra1, extra1String) and
   extra2 = extra1 and

csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.ql

@@ -24,10 +24,8 @@ private class ReturnNode extends DataFlow::ExprNode {
   }
 }
 
-private class Conf extends DataFlow::Configuration {
-  Conf() { this = "NoDisposeCallOnLocalIDisposable" }
-
-  override predicate isSource(DataFlow::Node node) {
+module DisposeCallOnLocalIDisposableConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) {
     node.asExpr() =
       any(LocalScopeDisposableCreation disposable |
         // Only care about library types - user types often have spurious IDisposable declarations
@@ -37,7 +35,7 @@ private class Conf extends DataFlow::Configuration {
       )
   }
 
-  override predicate isSink(DataFlow::Node node) {
+  predicate isSink(DataFlow::Node node) {
     // Things that return may be disposed elsewhere
     node instanceof ReturnNode
     or
@@ -80,23 +78,27 @@ private class Conf extends DataFlow::Configuration {
     )
   }
 
-  override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     node2.asExpr() =
       any(LocalScopeDisposableCreation other | other.getAnArgument() = node1.asExpr())
   }
 
-  override predicate isBarrierOut(DataFlow::Node node) {
-    this.isSink(node) and
+  predicate isBarrierOut(DataFlow::Node node) {
+    isSink(node) and
     not node instanceof ReturnNode
   }
 }
 
+module DisposeCallOnLocalIDisposable = DataFlow::Global<DisposeCallOnLocalIDisposableConfig>;
+
 /** Holds if `disposable` may not be disposed. */
 predicate mayNotBeDisposed(LocalScopeDisposableCreation disposable) {
-  exists(Conf conf, DataFlow::ExprNode e |
+  exists(DataFlow::ExprNode e |
     e.getExpr() = disposable and
-    conf.isSource(e) and
-    not exists(DataFlow::Node sink | conf.hasFlow(DataFlow::exprNode(disposable), sink) |
+    DisposeCallOnLocalIDisposableConfig::isSource(e) and
+    not exists(DataFlow::Node sink |
+      DisposeCallOnLocalIDisposable::flow(DataFlow::exprNode(disposable), sink)
+    |
       sink instanceof ReturnNode
       implies
       sink.getEnclosingCallable() = disposable.getEnclosingCallable()

csharp/ql/src/Security Features/InsecureRandomness.ql

@@ -14,7 +14,7 @@
 
 import csharp
 import semmle.code.csharp.frameworks.Test
-import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph
+import Random::InsecureRandomness::PathGraph
 
 module Random {
   import semmle.code.csharp.security.dataflow.flowsources.Remote
@@ -38,21 +38,24 @@ module Random {
   /**
    * A taint-tracking configuration for insecure randomness in security sensitive context.
    */
-  class TaintTrackingConfiguration extends TaintTracking::Configuration {
-    TaintTrackingConfiguration() { this = "RandomDataFlowConfiguration" }
+  module InsecureRandomnessConfig implements DataFlow::ConfigSig {
+    predicate isSource(DataFlow::Node source) { source instanceof Source }
 
-    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+    predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
-    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+    predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
 
-    override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
-
-    override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+    predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
       // succ = array_or_indexer[pred] - use of random numbers in an index
       succ.asExpr().(ElementAccess).getAnIndex() = pred.asExpr()
     }
   }
 
+  /**
+   * A taint-tracking module for insecure randomness in security sensitive context.
+   */
+  module InsecureRandomness = TaintTracking::Global<InsecureRandomnessConfig>;
+
   /** A source of cryptographically insecure random numbers. */
   class RandomSource extends Source {
     RandomSource() {
@@ -112,10 +115,8 @@ module Random {
   }
 }
 
-from
-  Random::TaintTrackingConfiguration randomTracking, DataFlow::PathNode source,
-  DataFlow::PathNode sink
-where randomTracking.hasFlowPath(source, sink)
+from Random::InsecureRandomness::PathNode source, Random::InsecureRandomness::PathNode sink
+where Random::InsecureRandomness::flowPath(source, sink)
 select sink.getNode(), source, sink,
   "This uses a cryptographically insecure random number generated at $@ in a security context.",
   source.getNode(), source.getNode().toString()

csharp/ql/src/experimental/CWE-918/RequestForgery.ql

@@ -12,9 +12,9 @@
 
 import csharp
 import RequestForgery::RequestForgery
-import semmle.code.csharp.dataflow.DataFlow::DataFlow::PathGraph
+import RequestForgeryFlow::PathGraph
 
-from RequestForgeryConfiguration c, DataFlow::PathNode source, DataFlow::PathNode sink
-where c.hasFlowPath(source, sink)
+from RequestForgeryFlow::PathNode source, RequestForgeryFlow::PathNode sink
+where RequestForgeryFlow::flowPath(source, sink)
 select sink.getNode(), source, sink, "The URL of this request depends on a $@.", source.getNode(),
   "user-provided value"

csharp/ql/src/experimental/CWE-918/RequestForgery.qll

@@ -24,9 +24,11 @@ module RequestForgery {
   abstract private class Barrier extends DataFlow::Node { }
 
   /**
+   * DEPRECATED: Use `RequestForgeryFlow` instead.
+   *
    * A data flow configuration for detecting server side request forgery vulnerabilities.
    */
-  class RequestForgeryConfiguration extends DataFlow::Configuration {
+  deprecated class RequestForgeryConfiguration extends DataFlow::Configuration {
     RequestForgeryConfiguration() { this = "Server Side Request forgery" }
 
     override predicate isSource(DataFlow::Node source) { source instanceof Source }
@@ -54,6 +56,40 @@ module RequestForgery {
     override predicate isBarrier(DataFlow::Node node) { node instanceof Barrier }
   }
 
+  /**
+   * A data flow configuration for detecting server side request forgery vulnerabilities.
+   */
+  private module RequestForgeryFlowConfig implements DataFlow::ConfigSig {
+    predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+    predicate isAdditionalFlowStep(DataFlow::Node prev, DataFlow::Node succ) {
+      interpolatedStringFlowStep(prev, succ)
+      or
+      stringReplaceStep(prev, succ)
+      or
+      uriCreationStep(prev, succ)
+      or
+      formatConvertStep(prev, succ)
+      or
+      toStringStep(prev, succ)
+      or
+      stringConcatStep(prev, succ)
+      or
+      stringFormatStep(prev, succ)
+      or
+      pathCombineStep(prev, succ)
+    }
+
+    predicate isBarrier(DataFlow::Node node) { node instanceof Barrier }
+  }
+
+  /**
+   * A data flow module for detecting server side request forgery vulnerabilities.
+   */
+  module RequestForgeryFlow = DataFlow::Global<RequestForgeryFlowConfig>;
+
   /**
    * A remote data flow source taken as a source
    * for Server Side Request Forgery(SSRF) Vulnerabilities.

csharp/ql/src/experimental/Security Features/CWE-1004/CookieWithoutHttpOnly.ql

@@ -36,7 +36,7 @@ where
             iResponse.getAppendMethod() = mc.getTarget() and
             isCookieWithSensitiveName(mc.getArgument(0)) and
             // there is no callback `OnAppendCookie` that sets `HttpOnly` to true
-            not exists(OnAppendCookieHttpOnlyTrackingConfig config | config.hasFlowTo(_)) and
+            not OnAppendCookieHttpOnlyTracking::flowTo(_) and
             // Passed as third argument to `IResponseCookies.Append`
             exists(DataFlow::Node creation, DataFlow::Node append |
               CookieOptionsTracking::flow(creation, append) and
@@ -67,7 +67,7 @@ where
         // default is not configured or is not set to `Always`
         not getAValueForCookiePolicyProp("HttpOnly").getValue() = "1" and
         // there is no callback `OnAppendCookie` that sets `HttpOnly` to true
-        not exists(OnAppendCookieHttpOnlyTrackingConfig config | config.hasFlowTo(_)) and
+        not OnAppendCookieHttpOnlyTracking::flowTo(_) and
         iResponse.getAppendMethod() = mc.getTarget() and
         isCookieWithSensitiveName(mc.getArgument(0)) and
         (

csharp/ql/src/experimental/Security Features/CWE-614/CookieWithoutSecure.ql

@@ -30,7 +30,7 @@ where
         getAValueForCookiePolicyProp("Secure").getValue() = "1"
       ) and
       // there is no callback `OnAppendCookie` that sets `Secure` to true
-      not exists(OnAppendCookieSecureTrackingConfig config | config.hasFlowTo(_)) and
+      not OnAppendCookieSecureTracking::flowTo(_) and
       (
         // `Secure` property in `CookieOptions` passed to IResponseCookies.Append(...) wasn't set
         exists(ObjectCreation oc |
@@ -80,7 +80,7 @@ where
           or
           oc.getType() instanceof MicrosoftAspNetCoreHttpCookieOptions and
           // there is no callback `OnAppendCookie` that sets `Secure` to true
-          not exists(OnAppendCookieSecureTrackingConfig config | config.hasFlowTo(_)) and
+          not OnAppendCookieSecureTracking::flowTo(_) and
           // the cookie option is passed to `Append`
           exists(DataFlow::Node creation |
             CookieOptionsTracking::flow(creation, _) and

csharp/ql/src/experimental/Security Features/JsonWebTokenHandler/JsonWebTokenHandlerLib.qll

@@ -102,21 +102,6 @@ private class TokenValidationResultIsValidCall extends PropertyRead {
   }
 }
 
-/**
- * Dataflow from the output of `Microsoft.IdentityModel.JsonWebTokens.JsonWebTokenHandler.ValidateToken` call to access the `IsValid` or `Exception` property
- */
-private class FlowsToTokenValidationResultIsValidCall extends DataFlow::Configuration {
-  FlowsToTokenValidationResultIsValidCall() { this = "FlowsToTokenValidationResultIsValidCall" }
-
-  override predicate isSource(DataFlow::Node source) {
-    source.asExpr() instanceof JsonWebTokenHandlerValidateTokenCall
-  }
-
-  override predicate isSink(DataFlow::Node sink) {
-    exists(TokenValidationResultIsValidCall call | sink.asExpr() = call.getQualifier())
-  }
-}
-
 /**
  * A security-sensitive property for `Microsoft.IdentityModel.Tokens.TokenValidationParameters`
  */