Merge pull request github#9270 from michaelnebel/csharp/summarized-callable-fix

C#: Summarized callable

Author: michaelnebel

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

@@ -84,12 +84,16 @@ newtype TReturnKind =
   }
 
 /**
- * Holds if the summary for `c` should be used for dataflow analysis.
+ * A summarized callable where the summary should be used for dataflow analysis.
  */
-predicate useFlowSummary(FlowSummary::SummarizedCallable c) {
-  not c.fromSource()
-  or
-  c.fromSource() and not c.isAutoGenerated()
+class DataFlowSummarizedCallable instanceof FlowSummary::SummarizedCallable {
+  DataFlowSummarizedCallable() {
+    not this.fromSource()
+    or
+    this.fromSource() and not this.isAutoGenerated()
+  }
+
+  string toString() { result = super.toString() }
 }
 
 private module Cached {
@@ -101,8 +105,10 @@ private module Cached {
    */
   cached
   newtype TDataFlowCallable =
-    TDotNetCallable(DotNet::Callable c) { c.isUnboundDeclaration() and not useFlowSummary(c) } or
-    TSummarizedCallable(FlowSummary::SummarizedCallable c) { useFlowSummary(c) }
+    TDotNetCallable(DotNet::Callable c) {
+      c.isUnboundDeclaration() and not c instanceof DataFlowSummarizedCallable
+    } or
+    TSummarizedCallable(DataFlowSummarizedCallable sc)
 
   cached
   newtype TDataFlowCall =

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

@@ -421,7 +421,8 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   or
   exists(Ssa::Definition def |
     LocalFlow::localSsaFlowStepUseUse(def, nodeFrom, nodeTo) and
-    not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom) and
+    not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(nodeFrom,
+      any(DataFlowSummarizedCallable sc)) and
     not LocalFlow::usesInstanceField(def)
   )
   or
@@ -739,15 +740,10 @@ private module Cached {
         )
       )
     } or
-    TSummaryNode(
-      FlowSummaryImpl::Public::SummarizedCallable c,
-      FlowSummaryImpl::Private::SummaryNodeState state
-    ) {
-      useFlowSummary(c) and
+    TSummaryNode(DataFlowSummarizedCallable c, FlowSummaryImpl::Private::SummaryNodeState state) {
       FlowSummaryImpl::Private::summaryNodeRange(c, state)
     } or
-    TSummaryParameterNode(FlowSummaryImpl::Public::SummarizedCallable c, ParameterPosition pos) {
-      useFlowSummary(c) and
+    TSummaryParameterNode(DataFlowSummarizedCallable c, ParameterPosition pos) {
       FlowSummaryImpl::Private::summaryParameterNodeRange(c, pos)
     } or
     TParamsArgumentNode(ControlFlow::Node callCfn) {
@@ -771,7 +767,8 @@ private module Cached {
     or
     // Simple flow through library code is included in the exposed local
     // step relation, even though flow is technically inter-procedural
-    FlowSummaryImpl::Private::Steps::summaryThroughStepValue(nodeFrom, nodeTo)
+    FlowSummaryImpl::Private::Steps::summaryThroughStepValue(nodeFrom, nodeTo,
+      any(DataFlowSummarizedCallable sc))
   }
 
   cached

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

@@ -759,8 +759,8 @@ module Private {
     }
 
     pragma[nomagic]
-    private ParamNode summaryArgParam0(DataFlowCall call, ArgNode arg) {
-      exists(ParameterPosition ppos, SummarizedCallable sc |
+    private ParamNode summaryArgParam0(DataFlowCall call, ArgNode arg, SummarizedCallable sc) {
+      exists(ParameterPosition ppos |
         argumentPositionMatch(call, arg, ppos) and
         viableParam(call, sc, ppos, result)
       )
@@ -774,9 +774,9 @@ module Private {
      * or expects contents.
      */
     pragma[nomagic]
-    predicate prohibitsUseUseFlow(ArgNode arg) {
+    predicate prohibitsUseUseFlow(ArgNode arg, SummarizedCallable sc) {
       exists(ParamNode p, Node mid, ParameterPosition ppos, Node ret |
-        p = summaryArgParam0(_, arg) and
+        p = summaryArgParam0(_, arg, sc) and
         p.isParameterOf(_, ppos) and
         summaryLocalStep(p, mid, true) and
         summaryLocalStep(mid, ret, true) and
@@ -788,9 +788,11 @@ module Private {
     }
 
     bindingset[ret]
-    private ParamNode summaryArgParam(ArgNode arg, ReturnNodeExt ret, OutNodeExt out) {
+    private ParamNode summaryArgParam(
+      ArgNode arg, ReturnNodeExt ret, OutNodeExt out, SummarizedCallable sc
+    ) {
       exists(DataFlowCall call, ReturnKindExt rk |
-        result = summaryArgParam0(call, arg) and
+        result = summaryArgParam0(call, arg, sc) and
         ret.getKind() = pragma[only_bind_into](rk) and
         out = pragma[only_bind_into](rk).getAnOutNode(call)
       )
@@ -803,9 +805,9 @@ module Private {
      * NOTE: This step should not be used in global data-flow/taint-tracking, but may
      * be useful to include in the exposed local data-flow/taint-tracking relations.
      */
-    predicate summaryThroughStepValue(ArgNode arg, Node out) {
+    predicate summaryThroughStepValue(ArgNode arg, Node out, SummarizedCallable sc) {
       exists(ReturnKind rk, ReturnNode ret, DataFlowCall call |
-        summaryLocalStep(summaryArgParam0(call, arg), ret, true) and
+        summaryLocalStep(summaryArgParam0(call, arg, sc), ret, true) and
         ret.getKind() = pragma[only_bind_into](rk) and
         out = getAnOutNode(call, pragma[only_bind_into](rk))
       )
@@ -818,8 +820,8 @@ module Private {
      * NOTE: This step should not be used in global data-flow/taint-tracking, but may
      * be useful to include in the exposed local data-flow/taint-tracking relations.
      */
-    predicate summaryThroughStepTaint(ArgNode arg, Node out) {
-      exists(ReturnNodeExt ret | summaryLocalStep(summaryArgParam(arg, ret, out), ret, false))
+    predicate summaryThroughStepTaint(ArgNode arg, Node out, SummarizedCallable sc) {
+      exists(ReturnNodeExt ret | summaryLocalStep(summaryArgParam(arg, ret, out, sc), ret, false))
     }
 
     /**
@@ -829,9 +831,9 @@ module Private {
      * NOTE: This step should not be used in global data-flow/taint-tracking, but may
      * be useful to include in the exposed local data-flow/taint-tracking relations.
      */
-    predicate summaryGetterStep(ArgNode arg, ContentSet c, Node out) {
+    predicate summaryGetterStep(ArgNode arg, ContentSet c, Node out, SummarizedCallable sc) {
       exists(Node mid, ReturnNodeExt ret |
-        summaryReadStep(summaryArgParam(arg, ret, out), c, mid) and
+        summaryReadStep(summaryArgParam(arg, ret, out, sc), c, mid) and
         summaryLocalStep(mid, ret, _)
       )
     }
@@ -843,9 +845,9 @@ module Private {
      * NOTE: This step should not be used in global data-flow/taint-tracking, but may
      * be useful to include in the exposed local data-flow/taint-tracking relations.
      */
-    predicate summarySetterStep(ArgNode arg, ContentSet c, Node out) {
+    predicate summarySetterStep(ArgNode arg, ContentSet c, Node out, SummarizedCallable sc) {
       exists(Node mid, ReturnNodeExt ret |
-        summaryLocalStep(summaryArgParam(arg, ret, out), mid, _) and
+        summaryLocalStep(summaryArgParam(arg, ret, out, sc), mid, _) and
         summaryStoreStep(mid, c, ret)
       )
     }
@@ -929,14 +931,20 @@ module Private {
     private class SummarizedCallableExternal extends SummarizedCallable {
       SummarizedCallableExternal() { summaryElement(this, _, _, _, _) }
 
-      private predicate relevantSummaryElement(AccessPath inSpec, AccessPath outSpec, string kind) {
-        summaryElement(this, inSpec, outSpec, kind, false)
-        or
+      private predicate relevantSummaryElementGenerated(
+        AccessPath inSpec, AccessPath outSpec, string kind
+      ) {
         summaryElement(this, inSpec, outSpec, kind, true) and
         not summaryElement(this, _, _, _, false) and
         not this.clearsContent(_, _)
       }
 
+      private predicate relevantSummaryElement(AccessPath inSpec, AccessPath outSpec, string kind) {
+        summaryElement(this, inSpec, outSpec, kind, false)
+        or
+        this.relevantSummaryElementGenerated(inSpec, outSpec, kind)
+      }
+
       override predicate propagatesFlow(
         SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue
       ) {
@@ -951,7 +959,7 @@ module Private {
         )
       }
 
-      override predicate isAutoGenerated() { summaryElement(this, _, _, _, true) }
+      override predicate isAutoGenerated() { this.relevantSummaryElementGenerated(_, _, _) }
     }
 
     /** Holds if component `c` of specification `spec` cannot be parsed. */

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

@@ -2,6 +2,7 @@ private import csharp
 private import TaintTrackingPublic
 private import FlowSummaryImpl as FlowSummaryImpl
 private import semmle.code.csharp.Caching
+private import semmle.code.csharp.dataflow.internal.DataFlowDispatch
 private import semmle.code.csharp.dataflow.internal.DataFlowPrivate
 private import semmle.code.csharp.dataflow.internal.ControlFlowReachability
 private import semmle.code.csharp.dispatch.Dispatch
@@ -117,19 +118,22 @@ private module Cached {
     (
       // Simple flow through library code is included in the exposed local
       // step relation, even though flow is technically inter-procedural
-      FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(nodeFrom, nodeTo)
+      FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(nodeFrom, nodeTo,
+        any(DataFlowSummarizedCallable sc))
       or
       // Taint collection by adding a tainted element
       exists(DataFlow::ElementContent c |
         storeStep(nodeFrom, c, nodeTo)
         or
-        FlowSummaryImpl::Private::Steps::summarySetterStep(nodeFrom, c, nodeTo)
+        FlowSummaryImpl::Private::Steps::summarySetterStep(nodeFrom, c, nodeTo,
+          any(DataFlowSummarizedCallable sc))
       )
       or
       exists(DataFlow::Content c |
         readStep(nodeFrom, c, nodeTo)
         or
-        FlowSummaryImpl::Private::Steps::summaryGetterStep(nodeFrom, c, nodeTo)
+        FlowSummaryImpl::Private::Steps::summaryGetterStep(nodeFrom, c, nodeTo,
+          any(DataFlowSummarizedCallable sc))
       |
         // Taint members
         c = any(TaintedMember m).(FieldOrProperty).getContent()

csharp/ql/src/Language Abuse/ForeachCapture.ql

@@ -13,6 +13,7 @@
 
 import csharp
 import semmle.code.csharp.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
+import semmle.code.csharp.dataflow.internal.DataFlowDispatch as DataFlowDispatch
 import semmle.code.csharp.dataflow.internal.DataFlowPrivate as DataFlowPrivate
 import semmle.code.csharp.frameworks.system.Collections
 import semmle.code.csharp.frameworks.system.collections.Generic
@@ -76,7 +77,8 @@ Element getAssignmentTarget(Expr e) {
 Element getCollectionAssignmentTarget(Expr e) {
   // Store into collection via method
   exists(DataFlowPrivate::PostUpdateNode postNode |
-    FlowSummaryImpl::Private::Steps::summarySetterStep(DataFlow::exprNode(e), _, postNode) and
+    FlowSummaryImpl::Private::Steps::summarySetterStep(DataFlow::exprNode(e), _, postNode,
+      any(DataFlowDispatch::DataFlowSummarizedCallable sc)) and
     result.(Variable).getAnAccess() = postNode.getPreUpdateNode().asExpr()
   )
   or

csharp/ql/test/library-tests/dataflow/external-models/steps.ql

@@ -2,6 +2,7 @@ import csharp
 import DataFlow
 import semmle.code.csharp.dataflow.ExternalFlow
 import semmle.code.csharp.dataflow.FlowSummary
+import semmle.code.csharp.dataflow.internal.DataFlowDispatch as DataFlowDispatch
 import semmle.code.csharp.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
 import CsvValidation
 
@@ -43,17 +44,23 @@ private class SummarizedCallableClear extends SummarizedCallable {
 query predicate summaryThroughStep(
   DataFlow::Node node1, DataFlow::Node node2, boolean preservesValue
 ) {
-  FlowSummaryImpl::Private::Steps::summaryThroughStepValue(node1, node2) and preservesValue = true
+  FlowSummaryImpl::Private::Steps::summaryThroughStepValue(node1, node2,
+    any(DataFlowDispatch::DataFlowSummarizedCallable sc)) and
+  preservesValue = true
   or
-  FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(node1, node2) and preservesValue = false
+  FlowSummaryImpl::Private::Steps::summaryThroughStepTaint(node1, node2,
+    any(DataFlowDispatch::DataFlowSummarizedCallable sc)) and
+  preservesValue = false
 }
 
 query predicate summaryGetterStep(DataFlow::Node arg, DataFlow::Node out, Content c) {
-  FlowSummaryImpl::Private::Steps::summaryGetterStep(arg, c, out)
+  FlowSummaryImpl::Private::Steps::summaryGetterStep(arg, c, out,
+    any(DataFlowDispatch::DataFlowSummarizedCallable sc))
 }
 
 query predicate summarySetterStep(DataFlow::Node arg, DataFlow::Node out, Content c) {
-  FlowSummaryImpl::Private::Steps::summarySetterStep(arg, c, out)
+  FlowSummaryImpl::Private::Steps::summarySetterStep(arg, c, out,
+    any(DataFlowDispatch::DataFlowSummarizedCallable sc))
 }
 
 query predicate clearsContent(SummarizedCallable c, DataFlow::Content k, ParameterPosition pos) {

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll

@@ -112,7 +112,7 @@ private module Cached {
     or
     // Simple flow through library code is included in the exposed local
     // step relation, even though flow is technically inter-procedural
-    FlowSummaryImpl::Private::Steps::summaryThroughStepValue(node1, node2)
+    FlowSummaryImpl::Private::Steps::summaryThroughStepValue(node1, node2, _)
   }
 
   /**
@@ -154,7 +154,7 @@ private predicate simpleLocalFlowStep0(Node node1, Node node2) {
   not exists(FieldRead fr |
     hasNonlocalValue(fr) and fr.getField().isStatic() and fr = node1.asExpr()
   ) and
-  not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(node1)
+  not FlowSummaryImpl::Private::Steps::prohibitsUseUseFlow(node1, _)
   or
   ThisFlow::adjacentThisRefs(node1, node2)
   or