C#: Review fixes

csharp/ql/lib/semmle/code/csharp/dataflow/FlowSummary.qll

@@ -2,7 +2,6 @@
 
 import csharp
 private import dotnet
-private import internal.FlowSummaryImplSpecific as FlowSummaryImplSpecific
 private import internal.FlowSummaryImpl as Impl
 private import internal.DataFlowDispatch as DataFlowDispatch
 
@@ -69,7 +68,7 @@ module SummaryComponent {
   SummaryComponent jump(Callable c) {
     result =
       return(any(DataFlowDispatch::JumpReturnKind jrk |
-          jrk.getTarget().getUnderlyingCallable() = c.getUnboundDeclaration() and
+          jrk.getTarget() = c.getUnboundDeclaration() and
           jrk.getTargetReturnKind() instanceof DataFlowDispatch::NormalReturnKind
         ))
   }
@@ -161,13 +160,6 @@ abstract class SummarizedCallable extends DotNet::Callable {
   predicate isAutoGenerated() { none() }
 }
 
-private class SourceSinkSummarizedCallable extends SummarizedCallable {
-  SourceSinkSummarizedCallable() {
-    FlowSummaryImplSpecific::sourceElement(this, _, _, _) or
-    FlowSummaryImplSpecific::sinkElement(this, _, _, _)
-  }
-}
-
 private class SummarizedCallableAdapter extends Impl::Public::SummarizedCallable {
   private SummarizedCallable sc;
 
@@ -209,8 +201,10 @@ private class RecordConstructorFlow extends SummarizedCallable {
   }
 }
 
-private class SummarizedCallableDefaultClearsContent extends SummarizedCallable {
-  SummarizedCallableDefaultClearsContent() { this instanceof SummarizedCallable or none() }
+private class SummarizedCallableDefaultClearsContent extends Impl::Public::SummarizedCallable {
+  SummarizedCallableDefaultClearsContent() {
+    this instanceof Impl::Public::SummarizedCallable or none()
+  }
 
   // By default, we assume that all stores into arguments are definite
   override predicate clearsContent(ParameterPosition pos, DataFlow::ContentSet content) {

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

@@ -70,17 +70,27 @@ newtype TReturnKind =
       v = def.getSourceVariable().getAssignable()
     )
   } or
-  TJumpReturnKind(DataFlowCallable target, ReturnKind rk) {
-    rk instanceof NormalReturnKind and
+  TJumpReturnKind(Callable target, ReturnKind rk) {
+    target.isUnboundDeclaration() and
     (
-      target.getUnderlyingCallable() instanceof Constructor or
-      not target.getUnderlyingCallable().getReturnType() instanceof VoidType
+      rk instanceof NormalReturnKind and
+      (
+        target instanceof Constructor or
+        not target.getReturnType() instanceof VoidType
+      )
+      or
+      exists(target.getParameter(rk.(OutRefReturnKind).getPosition()))
     )
-    or
-    exists(target.getUnderlyingCallable().getParameter(rk.(OutRefReturnKind).getPosition()))
   }
 
 private module Cached {
+  cached
+  newtype TDataFlowCallable =
+    TDotNetCallable(DotNet::Callable c) {
+      c.isUnboundDeclaration() and not c instanceof FlowSummary::SummarizedCallable
+    } or
+    TSummarizedCallable(FlowSummary::SummarizedCallable c)
+
   cached
   newtype TDataFlowCall =
     TNonDelegateCall(ControlFlow::Nodes::ElementNode cfn, DispatchCall dc) {
@@ -222,13 +232,13 @@ class ImplicitCapturedReturnKind extends ReturnKind, TImplicitCapturedReturnKind
  * one API entry point and out of another.
  */
 class JumpReturnKind extends ReturnKind, TJumpReturnKind {
-  private DataFlowCallable target;
+  private Callable target;
   private ReturnKind rk;
 
   JumpReturnKind() { this = TJumpReturnKind(target, rk) }
 
   /** Gets the target of the jump. */
-  DataFlowCallable getTarget() { result = target }
+  Callable getTarget() { result = target }
 
   /** Gets the return kind of the target. */
   ReturnKind getTargetReturnKind() { result = rk }
@@ -236,15 +246,7 @@ class JumpReturnKind extends ReturnKind, TJumpReturnKind {
   override string toString() { result = "jump to " + target }
 }
 
-/**
- * A type for modeling dataflow callables.
- */
-newtype TDataFlowCallable =
-  TDotNetCallable(DotNet::Callable c) {
-    c.isUnboundDeclaration() and not c instanceof FlowSummary::SummarizedCallable
-  } or
-  TSummarizedCallable(FlowSummary::SummarizedCallable c)
-
+/** A callable used for data flow. */
 class DataFlowCallable extends TDataFlowCallable {
   /** Get the underlying source code callable, if any. */
   DotNet::Callable asCallable() { this = TDotNetCallable(result) }
@@ -252,6 +254,7 @@ class DataFlowCallable extends TDataFlowCallable {
   /** Get the underlying summarized callable, if any. */
   FlowSummary::SummarizedCallable asSummarizedCallable() { this = TSummarizedCallable(result) }
 
+  /** Get the underlying callable. */
   DotNet::Callable getUnderlyingCallable() {
     result = this.asCallable() or result = this.asSummarizedCallable()
   }
@@ -259,7 +262,7 @@ class DataFlowCallable extends TDataFlowCallable {
   /** Gets a textual representation of this dataflow callable. */
   string toString() { result = this.getUnderlyingCallable().toString() }
 
-  /** Get the location of this dataflow callable, if any. */
+  /** Get the location of this dataflow callable. */
   Location getLocation() { result = this.getUnderlyingCallable().getLocation() }
 }
 
@@ -320,13 +323,23 @@ class NonDelegateDataFlowCall extends DataFlowCall, TNonDelegateCall {
   override DataFlowCallable getARuntimeTarget() {
     result.asCallable() = getCallableForDataFlow(dc.getADynamicTarget())
     or
-    exists(Callable c | result.asSummarizedCallable() = c.getUnboundDeclaration() |
-      c = dc.getADynamicTarget()
+    exists(Callable c, boolean static |
+      result.asSummarizedCallable() = c and
+      c = this.getATarget(static)
+    |
+      static = false
       or
-      c = dc.getAStaticTarget() and not c instanceof RuntimeCallable
+      static = true and not c instanceof RuntimeCallable
     )
   }
 
+  /** Gets a static or dynamic target of this call. */
+  Callable getATarget(boolean static) {
+    result = dc.getADynamicTarget().getUnboundDeclaration() and static = false
+    or
+    result = dc.getAStaticTarget().getUnboundDeclaration() and static = true
+  }
+
   override ControlFlow::Nodes::ElementNode getControlFlowNode() { result = cfn }
 
   override DataFlow::ExprNode getNode() { result.getControlFlowNode() = cfn }

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

@@ -694,10 +694,11 @@ private module Cached {
         AssignableDefinitions::ImplicitParameterDefinition
     } or
     TExplicitParameterNode(DotNet::Parameter p) {
-      p = any(DataFlowCallable c).asCallable().getAParameter()
+      p = any(DataFlowCallable dfc).asCallable().getAParameter()
     } or
     TInstanceParameterNode(Callable c) {
-      c.isUnboundDeclaration() and not c.(Modifiable).isStatic()
+      c = any(DataFlowCallable dfc).asCallable() and
+      not c.(Modifiable).isStatic()
     } or
     TYieldReturnNode(ControlFlow::Nodes::ElementNode cfn) {
       any(Callable c).canYieldReturn(cfn.getElement())
@@ -911,7 +912,7 @@ private module ParameterNodes {
     Callable getCallable() { result = callable }
 
     override predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
-      callable = c.getUnderlyingCallable() and pos.isThisParameter()
+      callable = c.asCallable() and pos.isThisParameter()
     }
 
     override DataFlowCallable getEnclosingCallableImpl() {
@@ -1554,9 +1555,9 @@ predicate jumpStep(Node pred, Node succ) {
     flr.hasNonlocalValue()
   )
   or
-  exists(JumpReturnKind jrk, DataFlowCall call |
+  exists(JumpReturnKind jrk, NonDelegateDataFlowCall call |
     FlowSummaryImpl::Private::summaryReturnNode(pred, jrk) and
-    viableCallable(call) = jrk.getTarget() and
+    jrk.getTarget() = call.getATarget(_) and
     succ = getAnOutNode(call, jrk.getTargetReturnKind())
   )
 }

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

@@ -10,7 +10,6 @@ private import FlowSummaryImplSpecific
 private import DataFlowImplSpecific::Private
 private import DataFlowImplSpecific::Public
 private import DataFlowImplCommon
-private import semmle.code.csharp.dataflow.FlowSummary as FlowSummary
 
 /** Provides classes and predicates for defining flow summaries. */
 module Public {
@@ -910,6 +909,33 @@ 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
+        summaryElement(this, inSpec, outSpec, kind, true) and
+        not summaryElement(this, _, _, _, false)
+      }
+
+      override predicate propagatesFlow(
+        SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue
+      ) {
+        exists(AccessPath inSpec, AccessPath outSpec, string kind |
+          this.relevantSummaryElement(inSpec, outSpec, kind) and
+          interpretSpec(inSpec, input) and
+          interpretSpec(outSpec, output)
+        |
+          kind = "value" and preservesValue = true
+          or
+          kind = "taint" and preservesValue = false
+        )
+      }
+
+      override predicate isAutoGenerated() { summaryElement(this, _, _, _, true) }
+    }
+
     /** Holds if component `c` of specification `spec` cannot be parsed. */
     predicate invalidSpecComponent(AccessPath spec, string c) {
       c = spec.getToken(_) and
@@ -1043,7 +1069,7 @@ module Private {
   /** Provides a query predicate for outputting a set of relevant flow summaries. */
   module TestOutput {
     /** A flow summary to include in the `summary/3` query predicate. */
-    abstract class RelevantSummarizedCallable extends FlowSummary::SummarizedCallable {
+    abstract class RelevantSummarizedCallable extends SummarizedCallable {
       /** Gets the string representation of this callable used by `summary/1`. */
       abstract string getCallableCsv();
 

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

@@ -58,9 +58,7 @@ DataFlowType getReturnType(SummarizedCallable c, ReturnKind rk) {
   result = getReturnTypeBase(c.asSummarizedCallable(), rk)
   or
   rk =
-    any(JumpReturnKind jrk |
-      result = getReturnTypeBase(jrk.getTarget().getUnderlyingCallable(), jrk.getTargetReturnKind())
-    )
+    any(JumpReturnKind jrk | result = getReturnTypeBase(jrk.getTarget(), jrk.getTargetReturnKind()))
 }
 
 /**
@@ -100,29 +98,6 @@ private predicate summaryElement0(
 
 private class SummarizedCallableExternal extends FlowSummary::SummarizedCallable {
   SummarizedCallableExternal() { summaryElement0(this, _, _, _, _) }
-
-  private predicate relevantSummaryElement(AccessPath inSpec, AccessPath outSpec, string kind) {
-    summaryElement0(this, inSpec, outSpec, kind, false)
-    or
-    summaryElement0(this, inSpec, outSpec, kind, true) and
-    not summaryElement0(this, _, _, _, false)
-  }
-
-  override predicate propagatesFlow(
-    SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue
-  ) {
-    exists(AccessPath inSpec, AccessPath outSpec, string kind |
-      this.relevantSummaryElement(inSpec, outSpec, kind) and
-      External::interpretSpec(inSpec, input) and
-      External::interpretSpec(outSpec, output)
-    |
-      kind = "value" and preservesValue = true
-      or
-      kind = "taint" and preservesValue = false
-    )
-  }
-
-  override predicate isAutoGenerated() { summaryElement0(this, _, _, _, true) }
 }
 
 /**
@@ -259,7 +234,7 @@ class InterpretNode extends TInterpretNode {
   DataFlowCallable asCallable() { result.getUnderlyingCallable() = this.asElement() }
 
   /** Gets the target of this call, if any. */
-  Callable getCallTarget() { result = viableCallable(this.asCall()).getUnderlyingCallable() }
+  Callable getCallTarget() { result = this.asCall().(NonDelegateDataFlowCall).getATarget(_) }
 
   /** Gets a textual representation of this node. */
   string toString() {

csharp/ql/test/library-tests/dataflow/collections/CollectionFlow.ql

@@ -17,7 +17,7 @@ class Conf extends DataFlow::Configuration {
     )
   }
 
-  override int fieldFlowBranchLimit() { result = 10 }
+  override int fieldFlowBranchLimit() { result = 100 }
 }
 
 from DataFlow::PathNode source, DataFlow::PathNode sink, Conf conf

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

@@ -42,7 +42,9 @@ query predicate summarySetterStep(DataFlow::Node arg, DataFlow::Node out, Conten
   FlowSummaryImpl::Private::Steps::summarySetterStep(arg, c, out)
 }
 
-query predicate clearsContent(SummarizedCallable c, DataFlow::Content k, ParameterPosition pos) {
+query predicate clearsContent(
+  FlowSummaryImpl::Public::SummarizedCallable c, DataFlow::Content k, ParameterPosition pos
+) {
   c.clearsContent(pos, k) and
-  c.fromSource()
+  c.asSummarizedCallable().fromSource()
 }

csharp/ql/test/library-tests/dataflow/library/FlowSummaries.ql

@@ -1,5 +1,5 @@
 import shared.FlowSummaries
 
 private class IncludeAllSummarizedCallable extends IncludeSummarizedCallable {
-  IncludeAllSummarizedCallable() { this instanceof SummarizedCallable }
+  IncludeAllSummarizedCallable() { exists(this) }
 }

csharp/ql/test/library-tests/dataflow/library/FlowSummariesFiltered.ql

@@ -3,7 +3,7 @@ private import semmle.code.csharp.dataflow.internal.DataFlowPrivate::Csv
 private import semmle.code.csharp.dataflow.ExternalFlow
 
 class IncludeFilteredSummarizedCallable extends IncludeSummarizedCallable {
-  IncludeFilteredSummarizedCallable() { this instanceof SummarizedCallable }
+  IncludeFilteredSummarizedCallable() { exists(this) }
 
   /**
    * Holds if flow is propagated between `input` and `output` and
@@ -14,10 +14,11 @@ class IncludeFilteredSummarizedCallable extends IncludeSummarizedCallable {
     SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue
   ) {
     this.propagatesFlow(input, output, preservesValue) and
-    not exists(IncludeSummarizedCallable rsc |
-      isBaseCallableOrPrototype(rsc) and
+    not exists(IncludeSummarizedCallable rsc, SummarizedCallable sc |
+      sc = rsc.asSummarizedCallable() and
+      isBaseCallableOrPrototype(sc) and
       rsc.propagatesFlow(input, output, preservesValue) and
-      this.(UnboundCallable).overridesOrImplementsUnbound(rsc)
+      this.asSummarizedCallable().(UnboundCallable).overridesOrImplementsUnbound(sc)
     )
   }
 }

csharp/ql/test/library-tests/frameworks/EntityFramework/FlowSummaries.ql

@@ -3,7 +3,7 @@ import shared.FlowSummaries
 import semmle.code.csharp.dataflow.ExternalFlow as ExternalFlow
 
 private class IncludeEFSummarizedCallable extends IncludeSummarizedCallable {
-  IncludeEFSummarizedCallable() { this instanceof EFSummarizedCallable }
+  IncludeEFSummarizedCallable() { this.asSummarizedCallable() instanceof EFSummarizedCallable }
 }
 
 query predicate sourceNode(DataFlow::Node node, string kind) {

csharp/ql/test/query-tests/Security Features/CWE-020/ExternalAPIsUsedWithUntrustedData.expected

@@ -1,2 +1,3 @@
 | System.Collections.Specialized.NameValueCollection.get_Item(string) [qualifier] | 1 | 1 |
 | System.Web.HttpRequest.get_QueryString() [qualifier] | 1 | 1 |
+| System.Web.HttpResponse.Write(string) [param 0] | 1 | 1 |

csharp/ql/test/query-tests/Security Features/CWE-020/UntrustedDataToExternalAPI.expected

@@ -1,8 +1,12 @@
 edges
+| UntrustedData.cs:9:20:9:42 | access to property QueryString : NameValueCollection | UntrustedData.cs:13:28:13:31 | access to local variable name |
 nodes
 | UntrustedData.cs:9:20:9:30 | access to property Request | semmle.label | access to property Request |
 | UntrustedData.cs:9:20:9:42 | access to property QueryString | semmle.label | access to property QueryString |
+| UntrustedData.cs:9:20:9:42 | access to property QueryString : NameValueCollection | semmle.label | access to property QueryString : NameValueCollection |
+| UntrustedData.cs:13:28:13:31 | access to local variable name | semmle.label | access to local variable name |
 subpaths
 #select
 | UntrustedData.cs:9:20:9:30 | access to property Request | UntrustedData.cs:9:20:9:30 | access to property Request | UntrustedData.cs:9:20:9:30 | access to property Request | Call to System.Web.HttpRequest.get_QueryString with untrusted data from $@. | UntrustedData.cs:9:20:9:30 | access to property Request | access to property Request |
 | UntrustedData.cs:9:20:9:42 | access to property QueryString | UntrustedData.cs:9:20:9:42 | access to property QueryString | UntrustedData.cs:9:20:9:42 | access to property QueryString | Call to System.Collections.Specialized.NameValueCollection.get_Item with untrusted data from $@. | UntrustedData.cs:9:20:9:42 | access to property QueryString | access to property QueryString |
+| UntrustedData.cs:13:28:13:31 | access to local variable name | UntrustedData.cs:9:20:9:42 | access to property QueryString : NameValueCollection | UntrustedData.cs:13:28:13:31 | access to local variable name | Call to System.Web.HttpResponse.Write with untrusted data from $@. | UntrustedData.cs:9:20:9:42 | access to property QueryString : NameValueCollection | access to property QueryString : NameValueCollection |

csharp/ql/test/shared/FlowSummaries.qll

@@ -4,9 +4,12 @@ private import semmle.code.csharp.dataflow.internal.DataFlowPrivate
 
 abstract class IncludeSummarizedCallable extends RelevantSummarizedCallable {
   IncludeSummarizedCallable() {
-    [this.(Modifiable), this.(Accessor).getDeclaration()].isEffectivelyPublic()
+    this.asSummarizedCallable() =
+      any(Callable c | [c.(Modifiable), c.(Accessor).getDeclaration()].isEffectivelyPublic())
   }
 
   /** Gets a string representing the callable in semi-colon separated format for use in flow summaries. */
-  final override string getCallableCsv() { result = Csv::asPartialModel(this) }
+  final override string getCallableCsv() {
+    result = Csv::asPartialModel(this.asSummarizedCallable())
+  }
 }