C#: Add read and store steps for delegate calls.

Author: michaelnebel

csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll

@@ -13,6 +13,7 @@ private import semmle.code.csharp.Unification
 private import semmle.code.csharp.controlflow.Guards
 private import semmle.code.csharp.dispatch.Dispatch
 private import semmle.code.csharp.frameworks.EntityFramework
+private import semmle.code.csharp.frameworks.system.linq.Expressions
 private import semmle.code.csharp.frameworks.NHibernate
 private import semmle.code.csharp.frameworks.Razor
 private import semmle.code.csharp.frameworks.system.Collections
@@ -1146,7 +1147,18 @@ private module Cached {
     TPrimaryConstructorParameterContent(Parameter p) {
       p.getCallable() instanceof PrimaryConstructor
     } or
-    TCapturedVariableContent(VariableCapture::CapturedVariable v)
+    TCapturedVariableContent(VariableCapture::CapturedVariable v) or
+    TDelegateCallArgumentContent(Parameter p, int i) {
+      i =
+        [0 .. p.getType()
+                .getUnboundDeclaration()
+                .(SystemLinqExpressions::DelegateExtType)
+                .getDelegateType()
+                .getNumberOfParameters() - 1]
+    } or
+    TDelegateCallResultContent(Parameter p) {
+      p.getType().getUnboundDeclaration() instanceof SystemLinqExpressions::DelegateExtType
+    }
 
   cached
   newtype TContentSet =
@@ -2273,6 +2285,22 @@ private predicate recordProperty(RecordType t, ContentSet c, string name) {
   )
 }
 
+/**
+ * Holds if data can flow from `node1` to `node2` via an assignment to
+ * the content set `c` of a delegate call.
+ *
+ * If there is a delegate call f(x), then we store "x" on "f"
+ * using a delegate parameter content set.
+ */
+private predicate storeStepDelegateCall(Node node1, ContentSet c, Node node2) {
+  exists(DelegateCall call, Parameter p, int i |
+    node1.asExpr() = call.getArgument(i) and
+    node2.(PostUpdateNode).getPreUpdateNode().asExpr() = call.getExpr() and
+    call.getExpr() = p.getAnAccess() and
+    c.isDelegateCallArgument(p, i)
+  )
+}
+
 /**
  * Holds if data can flow from `node1` to `node2` via an assignment to
  * content `c`.
@@ -2305,6 +2333,8 @@ predicate storeStep(Node node1, ContentSet c, Node node2) {
   or
   FlowSummaryImpl::Private::Steps::summaryStoreStep(node1.(FlowSummaryNode).getSummaryNode(), c,
     node2.(FlowSummaryNode).getSummaryNode())
+  or
+  storeStepDelegateCall(node1, c, node2)
 }
 
 private class ReadStepConfiguration extends ControlFlowReachabilityConfiguration {
@@ -2425,6 +2455,22 @@ private predicate readContentStep(Node node1, Content c, Node node2) {
   VariableCapture::readStep(node1, c, node2)
 }
 
+/**
+ * Holds if data can flow from `node1` to `node2` via an assignment to
+ * the content set `c` of a delegate call.
+ *
+ * If there is a delegate call f(x), then we read the result of the delegate
+ * call.
+ */
+private predicate readStepDelegateCall(Node node1, ContentSet c, Node node2) {
+  exists(DelegateCall call, Parameter p |
+    node1.asExpr() = call.getExpr() and
+    node2.asExpr() = call and
+    call.getExpr() = p.getAnAccess() and
+    c.isDelegateCallResult(p)
+  )
+}
+
 /**
  * Holds if data can flow from `node1` to `node2` via a read of content `c`.
  */
@@ -2443,6 +2489,8 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
   or
   FlowSummaryImpl::Private::Steps::summaryReadStep(node1.(FlowSummaryNode).getSummaryNode(), c,
     node2.(FlowSummaryNode).getSummaryNode())
+  or
+  readStepDelegateCall(node1, c, node2)
 }
 
 private predicate clearsCont(Node n, Content c) {

csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowPublic.qll

@@ -3,6 +3,7 @@ private import DataFlowDispatch
 private import DataFlowPrivate
 private import semmle.code.csharp.controlflow.Guards
 private import semmle.code.csharp.Unification
+private import semmle.code.csharp.frameworks.system.linq.Expressions
 
 /**
  * An element, viewed as a node in a data flow graph. Either an expression
@@ -238,6 +239,61 @@ class PropertyContent extends Content, TPropertyContent {
   override Location getLocation() { result = p.getLocation() }
 }
 
+/**
+ * A reference to the index of an argument of a delegate call
+ * (where the delegate is a parameter)
+ */
+class DelegateCallArgumentContent extends Content, TDelegateCallArgumentContent {
+  private Parameter p;
+  private int i;
+
+  DelegateCallArgumentContent() { this = TDelegateCallArgumentContent(p, i) }
+
+  /** Gets the underlying parameter. */
+  Parameter getParameter() { result = p }
+
+  /**
+   * Gets the type of the `i`th parameter of the underlying parameter `p`
+   * (which is of delegate type).
+   */
+  Type getType() {
+    result =
+      p.getType()
+          .(SystemLinqExpressions::DelegateExtType)
+          .getDelegateType()
+          .getParameter(i)
+          .getType()
+  }
+
+  override string toString() { result = "delegate parameter " + p.getName() + " at position " + i }
+
+  override Location getLocation() { result = p.getLocation() }
+}
+
+/**
+ * A reference to the result of a delegate call
+ * (where the delegate is a parameter)
+ */
+class DelegateCallResultContent extends Content, TDelegateCallResultContent {
+  private Parameter p;
+
+  DelegateCallResultContent() { this = TDelegateCallResultContent(p) }
+
+  /** Gets the underlying parameter. */
+  Parameter getParameter() { result = p }
+
+  /**
+   * Gets the return type of the underlying parameter `p` (which is of delegate type).
+   */
+  Type getType() {
+    result = p.getType().(SystemLinqExpressions::DelegateExtType).getDelegateType().getReturnType()
+  }
+
+  override string toString() { result = "delegate parameter " + p.getName() + " result" }
+
+  override Location getLocation() { result = p.getLocation() }
+}
+
 /**
  * A reference to a synthetic field corresponding to a
  * primary constructor parameter.
@@ -299,6 +355,20 @@ class ContentSet extends TContentSet {
    */
   predicate isProperty(Property p) { this = TPropertyContentSet(p) }
 
+  /**
+   * Holds if this content set represents the `i`th argument of
+   * the parameter `p` of delegate type in a delegate call.
+   */
+  predicate isDelegateCallArgument(Parameter p, int i) {
+    this.isSingleton(TDelegateCallArgumentContent(p, i))
+  }
+
+  /**
+   * Holds if this content set represents the result of a delegate call
+   * of parameter `p` (which is of delegate type).
+   */
+  predicate isDelegateCallResult(Parameter p) { this.isSingleton(TDelegateCallResultContent(p)) }
+
   /** Holds if this content set represents the field `f`. */
   predicate isField(Field f) { this.isSingleton(TFieldContent(f)) }
 

csharp/ql/src/utils/modelgenerator/internal/CaptureModels.qll

@@ -101,7 +101,9 @@ module ModelGeneratorInput implements ModelGeneratorInputSig<Location, CsharpDat
     api = any(FlowSummaryImpl::Public::NeutralSinkCallable sc | sc.hasManualModel())
   }
 
-  predicate isUninterestingForDataFlowModels(Callable api) { isHigherOrder(api) }
+  predicate isUninterestingForDataFlowModels(Callable api) { none() }
+
+  predicate isUninterestingForHeuristicDataFlowModels(Callable api) { isHigherOrder(api) }
 
   class SourceOrSinkTargetApi extends Callable {
     SourceOrSinkTargetApi() { relevant(this) }
@@ -175,7 +177,9 @@ module ModelGeneratorInput implements ModelGeneratorInputSig<Location, CsharpDat
    */
   private CS::Type getUnderlyingContType(DataFlow::Content c) {
     result = c.(DataFlow::FieldContent).getField().getType() or
-    result = c.(DataFlow::SyntheticFieldContent).getField().getType()
+    result = c.(DataFlow::SyntheticFieldContent).getField().getType() or
+    result = c.(DataFlow::DelegateCallArgumentContent).getType() or
+    result = c.(DataFlow::DelegateCallResultContent).getType()
   }
 
   Type getUnderlyingContentType(DataFlow::ContentSet c) {
@@ -342,6 +346,10 @@ module ModelGeneratorInput implements ModelGeneratorInputSig<Location, CsharpDat
     or
     c.isElement() and
     result = "Element"
+    or
+    exists(int i | c.isDelegateCallArgument(_, i) and result = "Parameter[" + i + "]")
+    or
+    c.isDelegateCallResult(_) and result = "ReturnValue"
   }
 
   predicate partialModel = ExternalFlow::partialModel/6;

shared/mad/codeql/mad/modelgenerator/internal/ModelGeneratorImpl.qll

@@ -223,6 +223,15 @@ signature module ModelGeneratorInputSig<LocationSig Location, InputSig<Location>
    */
   predicate isUninterestingForDataFlowModels(Callable api);
 
+  /**
+   * Holds if it is irrelevant to generate models for `api` based on the heuristic
+   * (non-content) flow analysis.
+   *
+   * This serves as an extra filter for the `relevant`
+   * and `isUninterestingForDataFlowModels` predicates.
+   */
+  predicate isUninterestingForHeuristicDataFlowModels(Callable api);
+
   /**
    * Holds if `namespace`, `type`, `extensible`, `name` and `parameters` are string representations
    * for the corresponding MaD columns for `api`.
@@ -300,7 +309,7 @@ module MakeModelGenerator<
     }
   }
 
-  string getOutput(ReturnNodeExt node) {
+  private string getOutput(ReturnNodeExt node) {
     result = PrintReturnNodeExt<paramReturnNodeAsOutput/2>::getOutput(node)
   }
 
@@ -432,7 +441,11 @@ module MakeModelGenerator<
 
     predicate isSource(DataFlow::Node source, FlowState state) {
       source instanceof DataFlow::ParameterNode and
-      source.(NodeExtended).getEnclosingCallable() instanceof DataFlowSummaryTargetApi and
+      exists(Callable c |
+        c = source.(NodeExtended).getEnclosingCallable() and
+        c instanceof DataFlowSummaryTargetApi and
+        not isUninterestingForHeuristicDataFlowModels(c)
+      ) and
       state.(TaintRead).getStep() = 0
     }