Merge pull request #16484 from michaelnebel/csharp/superimplmodelgen

C#: Lift models.

Author: michaelnebel

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

@@ -94,6 +94,8 @@ private import FlowSummaryImpl::Public
 private import FlowSummaryImpl::Private
 private import FlowSummaryImpl::Private::External
 private import semmle.code.csharp.commons.QualifiedName
+private import semmle.code.csharp.dispatch.OverridableCallable
+private import semmle.code.csharp.frameworks.System
 private import codeql.mad.ModelValidation as SharedModelVal
 
 private predicate relevantNamespace(string namespace) {
@@ -442,20 +444,16 @@ predicate sourceNode(Node node, string kind) { sourceNode(node, kind, _) }
  */
 predicate sinkNode(Node node, string kind) { sinkNode(node, kind, _) }
 
-/** Holds if the summary should apply for all overrides of `c`. */
-predicate isBaseCallableOrPrototype(UnboundCallable c) {
-  c.getDeclaringType() instanceof Interface
-  or
-  exists(Modifiable m | m = [c.(Modifiable), c.(Accessor).getDeclaration()] |
-    m.isAbstract()
-    or
-    c.getDeclaringType().(Modifiable).isAbstract() and m.(Virtualizable).isVirtual()
+private predicate isOverridableCallable(OverridableCallable c) {
+  not exists(Type t, Callable base | c.getOverridee+() = base and t = base.getDeclaringType() |
+    t instanceof SystemObjectClass or
+    t instanceof SystemValueTypeClass
   )
 }
 
 /** Gets a string representing whether the summary should apply for all overrides of `c`. */
 private string getCallableOverride(UnboundCallable c) {
-  if isBaseCallableOrPrototype(c) then result = "true" else result = "false"
+  if isOverridableCallable(c) then result = "true" else result = "false"
 }
 
 private module QualifiedNameInput implements QualifiedNameInputSig {

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

@@ -8,6 +8,7 @@ private import semmle.code.csharp.commons.Collections as Collections
 private import semmle.code.csharp.dataflow.internal.DataFlowDispatch
 private import semmle.code.csharp.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
 private import semmle.code.csharp.frameworks.system.linq.Expressions
+private import semmle.code.csharp.frameworks.System
 import semmle.code.csharp.dataflow.internal.ExternalFlow as ExternalFlow
 import semmle.code.csharp.dataflow.internal.DataFlowImplCommon as DataFlowImplCommon
 import semmle.code.csharp.dataflow.internal.DataFlowPrivate as DataFlowPrivate
@@ -19,10 +20,12 @@ module TaintTracking = CS::TaintTracking;
 
 class Type = CS::Type;
 
+class Callable = CS::Callable;
+
 /**
  * Holds if any of the parameters of `api` are `System.Func<>`.
  */
-private predicate isHigherOrder(CS::Callable api) {
+private predicate isHigherOrder(Callable api) {
   exists(Type t | t = api.getAParameter().getType().getUnboundDeclaration() |
     t instanceof SystemLinqExpressions::DelegateExtType
   )
@@ -32,23 +35,56 @@ private predicate irrelevantAccessor(CS::Accessor a) {
   a.getDeclaration().(CS::Property).isReadWrite()
 }
 
-/**
- * Holds if it is relevant to generate models for `api`.
- */
-private predicate isRelevantForModels(CS::Callable api) {
-  [api.(CS::Modifiable), api.(CS::Accessor).getDeclaration()].isEffectivelyPublic() and
-  api.getDeclaringType().getNamespace().getFullName() != "" and
-  not api instanceof CS::ConversionOperator and
-  not api instanceof Util::MainMethod and
-  not api instanceof CS::Destructor and
-  not api instanceof CS::AnonymousFunctionExpr and
-  not api.(CS::Constructor).isParameterless() and
-  // Disregard all APIs that have a manual model.
-  not api = any(FlowSummaryImpl::Public::SummarizedCallable sc | sc.applyManualModel()) and
-  not api = any(FlowSummaryImpl::Public::NeutralSummaryCallable sc | sc.hasManualModel()) and
+private predicate isUninterestingForModels(Callable api) {
+  api.getDeclaringType().getNamespace().getFullName() = ""
+  or
+  api instanceof CS::ConversionOperator
+  or
+  api instanceof Util::MainMethod
+  or
+  api instanceof CS::Destructor
+  or
+  api instanceof CS::AnonymousFunctionExpr
+  or
+  api.(CS::Constructor).isParameterless()
+  or
+  exists(Type decl | decl = api.getDeclaringType() |
+    decl instanceof SystemObjectClass or
+    decl instanceof SystemValueTypeClass
+  )
+  or
   // Disregard properties that have both a get and a set accessor,
   // which implicitly means auto implemented properties.
-  not irrelevantAccessor(api)
+  irrelevantAccessor(api)
+}
+
+private predicate relevant(Callable api) {
+  [api.(CS::Modifiable), api.(CS::Accessor).getDeclaration()].isEffectivelyPublic() and
+  api.fromSource() and
+  api.isUnboundDeclaration() and
+  not isUninterestingForModels(api)
+}
+
+private Callable getARelevantOverrideeOrImplementee(Overridable m) {
+  m.overridesOrImplements(result) and relevant(result)
+}
+
+/**
+ * Gets the super implementation of `api` if it is relevant.
+ * If such a super implementation does not exist, returns `api` if it is relevant.
+ */
+private Callable liftedImpl(Callable api) {
+  (
+    result = getARelevantOverrideeOrImplementee(api)
+    or
+    result = api and relevant(api)
+  ) and
+  not exists(getARelevantOverrideeOrImplementee(result))
+}
+
+private predicate hasManualModel(Callable api) {
+  api = any(FlowSummaryImpl::Public::SummarizedCallable sc | sc.applyManualModel()) or
+  api = any(FlowSummaryImpl::Public::NeutralSummaryCallable sc | sc.hasManualModel())
 }
 
 /**
@@ -66,23 +102,37 @@ predicate isUninterestingForDataFlowModels(CS::Callable api) { isHigherOrder(api
 predicate isUninterestingForTypeBasedFlowModels(CS::Callable api) { none() }
 
 /**
- * A class of callables that are relevant generating summary, source and sinks models for.
+ * A class of callables that are potentially relevant for generating summary, source, sink
+ * and neutral models.
  *
- * In the Standard library and 3rd party libraries it the callables that can be called
- * from outside the library itself.
+ * In the Standard library and 3rd party libraries it is the callables (or callables that have a
+ * super implementation) that can be called from outside the library itself.
  */
-class TargetApiSpecific extends CS::Callable {
+class TargetApiSpecific extends Callable {
+  private Callable lift;
+
   TargetApiSpecific() {
-    this.fromSource() and
-    this.isUnboundDeclaration() and
-    isRelevantForModels(this)
+    lift = liftedImpl(this) and
+    not hasManualModel(lift)
   }
+
+  /**
+   * Gets the callable that a model will be lifted to.
+   *
+   * The lifted callable is relevant in terms of model
+   * generation (this is ensured by `liftedImpl`).
+   */
+  Callable lift() { result = lift }
+
+  /**
+   * Holds if `this` is relevant in terms of model generation.
+   */
+  predicate isRelevant() { relevant(this) }
 }
 
-predicate asPartialModel = ExternalFlow::asPartialModel/1;
+string asPartialModel(TargetApiSpecific api) { result = ExternalFlow::asPartialModel(api.lift()) }
 
-/** Computes the first 4 columns for neutral CSV rows of `c`. */
-predicate asPartialNeutralModel = ExternalFlow::getSignature/1;
+string asPartialNeutralModel(TargetApiSpecific api) { result = ExternalFlow::getSignature(api) }
 
 /**
  * Holds if `t` is a type that is generally used for bulk data in collection types.
@@ -151,7 +201,7 @@ string paramReturnNodeAsOutput(CS::Callable c, ParameterPosition pos) {
 /**
  * Gets the enclosing callable of `ret`.
  */
-CS::Callable returnNodeEnclosingCallable(DataFlow::Node ret) {
+Callable returnNodeEnclosingCallable(DataFlow::Node ret) {
   result = DataFlowImplCommon::getNodeEnclosingCallable(ret).asCallable()
 }
 
@@ -176,12 +226,24 @@ private predicate isRelevantMemberAccess(DataFlow::Node node) {
 
 predicate sinkModelSanitizer(DataFlow::Node node) { none() }
 
+private class ManualNeutralSinkCallable extends Callable {
+  ManualNeutralSinkCallable() {
+    this =
+      any(FlowSummaryImpl::Public::NeutralCallable nc |
+        nc.hasManualModel() and nc.getKind() = "sink"
+      )
+  }
+}
+
 /**
  * Holds if `source` is an api entrypoint relevant for creating sink models.
  */
 predicate apiSource(DataFlow::Node source) {
   (isRelevantMemberAccess(source) or source instanceof DataFlow::ParameterNode) and
-  isRelevantForModels(source.getEnclosingCallable())
+  exists(Callable enclosing | enclosing = source.getEnclosingCallable() |
+    relevant(enclosing) and
+    not enclosing instanceof ManualNeutralSinkCallable
+  )
 }
 
 /**

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

@@ -81,10 +81,13 @@ string captureFlow(DataFlowTargetApi api) {
 }
 
 /**
- * Gets the neutral model for `api`, if any.
- * A neutral model is generated, if there does not exist summary model.
+ * Gets the neutral summary model for `api`, if any.
+ * A neutral summary model is generated, if we are not generating
+ * a summary model that applies to `api` and if it relevant to generate
+ * a model for `api`.
  */
 string captureNoFlow(DataFlowTargetApi api) {
-  not exists(captureFlow(api)) and
+  not exists(DataFlowTargetApi api0 | exists(captureFlow(api0)) and api0.lift() = api.lift()) and
+  api.isRelevant() and
   result = ModelPrinting::asNeutralSummaryModel(api)
 }

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

@@ -1,6 +1,17 @@
 import shared.FlowSummaries
 private import semmle.code.csharp.dataflow.internal.ExternalFlow
 
+/** Holds if `c` is a base callable or prototype. */
+private predicate isBaseCallableOrPrototype(UnboundCallable c) {
+  c.getDeclaringType() instanceof Interface
+  or
+  exists(Modifiable m | m = [c.(Modifiable), c.(Accessor).getDeclaration()] |
+    m.isAbstract()
+    or
+    c.getDeclaringType().(Modifiable).isAbstract() and m.(Virtualizable).isVirtual()
+  )
+}
+
 class IncludeFilteredSummarizedCallable extends IncludeSummarizedCallable {
   /**
    * Holds if flow is propagated between `input` and `output` and

csharp/ql/test/utils/modelgenerator/dataflow/Summaries.cs

@@ -245,7 +245,7 @@ public string ReturnParam1(string input0, string input1)
 
 public class DerivedClass2Flow : BaseClassFlow
 {
-    // summary=Models;DerivedClass2Flow;false;ReturnParam;(System.Object);;Argument[0];ReturnValue;taint;df-generated
+    // summary=Models;BaseClassFlow;true;ReturnParam;(System.Object);;Argument[0];ReturnValue;taint;df-generated
     public override object ReturnParam(object input)
     {
         return input;
@@ -490,3 +490,76 @@ public ParameterlessConstructor()
         IsInitialized = true;
     }
 }
+
+public class Inheritance
+{
+    public abstract class BasePublic
+    {
+        public abstract string Id(string x);
+    }
+
+    public class AImplBasePublic : BasePublic
+    {
+        // summary=Models;Inheritance+BasePublic;true;Id;(System.String);;Argument[0];ReturnValue;taint;df-generated
+        public override string Id(string x)
+        {
+            return x;
+        }
+    }
+
+    public interface IPublic1
+    {
+        string Id(string x);
+    }
+
+    public interface IPublic2
+    {
+        string Id(string x);
+    }
+
+    public abstract class B : IPublic1
+    {
+        public abstract string Id(string x);
+    }
+
+    private abstract class C : IPublic2
+    {
+        public abstract string Id(string x);
+    }
+
+    public class BImpl : B
+    {
+        // summary=Models;Inheritance+IPublic1;true;Id;(System.String);;Argument[0];ReturnValue;taint;df-generated
+        public override string Id(string x)
+        {
+            return x;
+        }
+    }
+
+    private class CImpl : C
+    {
+        // summary=Models;Inheritance+IPublic2;true;Id;(System.String);;Argument[0];ReturnValue;taint;df-generated
+        public override string Id(string x)
+        {
+            return x;
+        }
+    }
+
+    public interface IPublic3
+    {
+        string Prop { get; }
+    }
+
+    public abstract class D : IPublic3
+    {
+        public abstract string Prop { get; }
+    }
+
+    public class DImpl : D
+    {
+        private string tainted;
+
+        // summary=Models;Inheritance+IPublic3;true;get_Prop;();;Argument[this];ReturnValue;taint;df-generated
+        public override string Prop { get { return tainted; } }
+    }
+}