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,15 @@ private module Cached {
     TPrimaryConstructorParameterContent(Parameter p) {
       p.getCallable() instanceof PrimaryConstructor
     } or
-    TCapturedVariableContent(VariableCapture::CapturedVariable v)
+    TCapturedVariableContent(VariableCapture::CapturedVariable v) or
+    TDelegateParameterContent(Parameter p, int i) {
+      i =
+        [0 .. p.getType()
+                .getUnboundDeclaration()
+                .(SystemLinqExpressions::DelegateExtType)
+                .getDelegateType()
+                .getNumberOfParameters() - 1]
+    }
 
   cached
   newtype TContentSet =
@@ -2273,6 +2282,25 @@ private predicate recordProperty(RecordType t, ContentSet c, string name) {
   )
 }
 
+// node2 needs to be repsentation of the callable. In this case the parameter itself.
+// node1 needs to be the argument position in the call.
+// private predicate storeStepDelegateCall(Node node1, ContentSet c, Node node2) {
+//   exists(DelegateCall call, Parameter p, int i |
+//     node1.asExpr() = call.getArgument(i) and
+//     node2.asExpr() = call.getExpr() and
+//     call.getExpr() = p.getAnAccess() and
+//     c.isDelegateParameter(p, i)
+//   )
+// }
+private predicate storeStepDelegateCall(Node node1, ContentSet c, Node node2) {
+  exists(DelegateCall call, Parameter p, int i |
+    node1.asExpr() = call.getArgument(i) and
+    node2.asExpr() = call and
+    call.getExpr() = p.getAnAccess() and
+    c.isDelegateParameter(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,17 @@ private predicate readContentStep(Node node1, Content c, Node node2) {
   VariableCapture::readStep(node1, c, node2)
 }
 
+// TODO: Make comment.
+// Consider putting this into readContentStep.
+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.isDelegateParameter(p, _)
+  )
+}
+
 /**
  * Holds if data can flow from `node1` to `node2` via a read of content `c`.
  */
@@ -2443,6 +2484,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,32 @@ class PropertyContent extends Content, TPropertyContent {
   override Location getLocation() { result = p.getLocation() }
 }
 
+/** A reference to a parameter with delegate type. */
+// TODO: We also need to account for parameter positions.
+class DelegateParameterContent extends Content, TDelegateParameterContent {
+  private Parameter p;
+  private int i;
+
+  DelegateParameterContent() { this = TDelegateParameterContent(p, i) }
+
+  /** Gets the underlying parameter. */
+  Parameter getParameter() { result = p }
+
+  Type getType() {
+    result =
+      p.getType()
+          .(SystemLinqExpressions::DelegateExtType)
+          .getDelegateType()
+          .getParameter(i)
+          .getType()
+  }
+
+  // TODO: Also print the index.
+  override string toString() { result = "delegate parameter " + p.getName() + " position " + i }
+
+  override Location getLocation() { result = p.getLocation() }
+}
+
 /**
  * A reference to a synthetic field corresponding to a
  * primary constructor parameter.
@@ -299,6 +326,10 @@ class ContentSet extends TContentSet {
    */
   predicate isProperty(Property p) { this = TPropertyContentSet(p) }
 
+  predicate isDelegateParameter(Parameter p, int i) {
+    this.isSingleton(TDelegateParameterContent(p, i))
+  }
+
   /** 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

@@ -26,22 +26,23 @@ module ModelGeneratorInput implements ModelGeneratorInputSig<Location, CsharpDat
     Callable getAsExprEnclosingCallable() { result = this.asExpr().getEnclosingCallable() }
   }
 
+  class DelegateCallNode extends CS::DataFlow::Node {
+    DelegateCallNode() { this.asExpr() instanceof CS::DelegateCall }
+  }
+
   /**
    * Holds if any of the parameters of `api` are `System.Func<>`.
    */
-  private predicate isHigherOrder(Callable api) {
-    exists(Type t | t = api.getAParameter().getType().getUnboundDeclaration() |
-      t instanceof SystemLinqExpressions::DelegateExtType
-    )
-  }
+  private predicate isHigherOrder(Callable api) { none() }
 
   private predicate irrelevantAccessor(CS::Accessor a) {
     a.getDeclaration().(CS::Property).isReadWrite()
   }
 
   private predicate isUninterestingForModels(Callable api) {
-    api.getDeclaringType().getNamespace().getFullName() = ""
-    or
+    // TODO Comment this back in.
+    // api.getDeclaringType().getNamespace().getFullName() = ""
+    // or
     api instanceof CS::ConversionOperator
     or
     api instanceof Util::MainMethod
@@ -175,7 +176,8 @@ 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::DelegateParameterContent).getType()
   }
 
   Type getUnderlyingContentType(DataFlow::ContentSet c) {
@@ -338,6 +340,8 @@ module ModelGeneratorInput implements ModelGeneratorInputSig<Location, CsharpDat
       result = "Property[" + name + "]"
     )
     or
+    exists(CS::Parameter p, int i | c.isDelegateParameter(p, i) and result = "Parameter[" + i + "]")
+    or
     result = "SyntheticField[" + getSyntheticName(c) + "]"
     or
     c.isElement() and

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

@@ -61,6 +61,13 @@ signature module ModelGeneratorInputSig<LocationSig Location, InputSig<Location>
     Parameter asParameter();
   }
 
+  class DelegateCallNode extends Lang::Node {
+    /**
+     * Gets the enclosing callable of this node.
+     */
+    Callable getEnclosingCallable();
+  }
+
   /**
    * A class of callables that are potentially relevant for generating summary or
    * neutral models.
@@ -536,7 +543,8 @@ module MakeModelGenerator<
       }
 
       predicate isSink(DataFlow::Node sink) {
-        sink.(ReturnNodeExt).getEnclosingCallable() instanceof DataFlowSummaryTargetApi
+        sink.(ReturnNodeExt).getEnclosingCallable() instanceof DataFlowSummaryTargetApi or
+        sink.(DelegateCallNode).getEnclosingCallable() instanceof DataFlowSummaryTargetApi
       }
 
       predicate isAdditionalFlowStep = isAdditionalContentFlowStep/2;
@@ -652,7 +660,7 @@ module MakeModelGenerator<
     pragma[nomagic]
     private predicate apiContentFlow(
       ContentDataFlowSummaryTargetApi api, DataFlow::ParameterNode p,
-      PropagateContentFlow::AccessPath reads, ReturnNodeExt returnNodeExt,
+      PropagateContentFlow::AccessPath reads, NodeExtended returnNodeExt,
       PropagateContentFlow::AccessPath stores, boolean preservesValue
     ) {
       PropagateContentFlow::flow(p, reads, returnNodeExt, stores, preservesValue) and