Merge pull request #16722 from michaelnebel/csharp/modelgensourcesink

C#/Java: Respect manual neutrals, sources and sinks in model generation.

Author: michaelnebel

csharp/ql/src/utils/modelgenerator/CaptureNeutralModels.ql

@@ -10,6 +10,6 @@ import semmle.code.csharp.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
 import internal.CaptureModels
 import internal.CaptureSummaryFlowQuery
 
-from DataFlowTargetApi api, string noflow
+from DataFlowSummaryTargetApi api, string noflow
 where noflow = captureNoFlow(api)
 select noflow order by noflow

csharp/ql/src/utils/modelgenerator/CaptureSinkModels.ql

@@ -8,6 +8,6 @@
 
 import internal.CaptureModels
 
-from DataFlowTargetApi api, string sink
+from DataFlowSinkTargetApi api, string sink
 where sink = captureSink(api)
 select sink order by sink

csharp/ql/src/utils/modelgenerator/CaptureSourceModels.ql

@@ -8,6 +8,6 @@
 
 import internal.CaptureModels
 
-from DataFlowTargetApi api, string source
+from DataFlowSourceTargetApi api, string source
 where source = captureSource(api)
 select source order by source

csharp/ql/src/utils/modelgenerator/CaptureSummaryModels.ql

@@ -10,6 +10,6 @@ import semmle.code.csharp.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
 import internal.CaptureModels
 import internal.CaptureSummaryFlowQuery
 
-from DataFlowTargetApi api, string flow
+from DataFlowSummaryTargetApi api, string flow
 where flow = captureFlow(api)
 select flow order by flow

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

@@ -29,12 +29,16 @@ private class ReturnNodeExt extends DataFlow::Node {
   }
 }
 
-class DataFlowTargetApi extends TargetApiSpecific {
-  DataFlowTargetApi() { not isUninterestingForDataFlowModels(this) }
+class DataFlowSummaryTargetApi extends SummaryTargetApi {
+  DataFlowSummaryTargetApi() { not isUninterestingForDataFlowModels(this) }
 }
 
+class DataFlowSourceTargetApi = SourceTargetApi;
+
+class DataFlowSinkTargetApi = SinkTargetApi;
+
 private module ModelPrintingInput implements ModelPrintingSig {
-  class Api = DataFlowTargetApi;
+  class Api = TargetApiBase;
 
   string getProvenance() { result = "df-generated" }
 }
@@ -89,7 +93,7 @@ string asInputArgument(DataFlow::Node source) { result = asInputArgumentSpecific
 /**
  * Gets the summary model of `api`, if it follows the `fluent` programming pattern (returns `this`).
  */
-string captureQualifierFlow(TargetApiSpecific api) {
+string captureQualifierFlow(DataFlowSummaryTargetApi api) {
   exists(ReturnNodeExt ret |
     api = returnNodeEnclosingCallable(ret) and
     isOwnInstanceAccessNode(ret)
@@ -150,7 +154,7 @@ module PropagateFlowConfig implements DataFlow::StateConfigSig {
 
   predicate isSource(DataFlow::Node source, FlowState state) {
     source instanceof DataFlow::ParameterNode and
-    source.getEnclosingCallable() instanceof DataFlowTargetApi and
+    source.getEnclosingCallable() instanceof DataFlowSummaryTargetApi and
     state.(TaintRead).getStep() = 0
   }
 
@@ -195,7 +199,7 @@ private module PropagateFlow = TaintTracking::GlobalWithState<PropagateFlowConfi
 /**
  * Gets the summary model(s) of `api`, if there is flow from parameters to return value or parameter.
  */
-string captureThroughFlow(DataFlowTargetApi api) {
+string captureThroughFlow(DataFlowSummaryTargetApi api) {
   exists(DataFlow::ParameterNode p, ReturnNodeExt returnNodeExt, string input, string output |
     PropagateFlow::flow(p, returnNodeExt) and
     returnNodeExt.(DataFlow::Node).getEnclosingCallable() = api and
@@ -222,10 +226,8 @@ module PropagateFromSourceConfig implements DataFlow::ConfigSig {
   }
 
   predicate isSink(DataFlow::Node sink) {
-    exists(DataFlowTargetApi c |
-      sink instanceof ReturnNodeExt and
-      sink.getEnclosingCallable() = c
-    )
+    sink instanceof ReturnNodeExt and
+    sink.getEnclosingCallable() instanceof DataFlowSourceTargetApi
   }
 
   DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSinkCallContext }
@@ -244,7 +246,7 @@ private module PropagateFromSource = TaintTracking::Global<PropagateFromSourceCo
 /**
  * Gets the source model(s) of `api`, if there is flow from an existing known source to the return of `api`.
  */
-string captureSource(DataFlowTargetApi api) {
+string captureSource(DataFlowSourceTargetApi api) {
   exists(DataFlow::Node source, ReturnNodeExt sink, string kind |
     PropagateFromSource::flow(source, sink) and
     ExternalFlow::sourceNode(source, kind) and
@@ -262,7 +264,9 @@ string captureSource(DataFlowTargetApi api) {
  * into an existing known sink (then the API itself becomes a sink).
  */
 module PropagateToSinkConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) { apiSource(source) }
+  predicate isSource(DataFlow::Node source) {
+    apiSource(source) and source.getEnclosingCallable() instanceof DataFlowSinkTargetApi
+  }
 
   predicate isSink(DataFlow::Node sink) {
     exists(string kind | isRelevantSinkKind(kind) and ExternalFlow::sinkNode(sink, kind))
@@ -286,7 +290,7 @@ private module PropagateToSink = TaintTracking::Global<PropagateToSinkConfig>;
 /**
  * Gets the sink model(s) of `api`, if there is flow from a parameter to an existing known sink.
  */
-string captureSink(DataFlowTargetApi api) {
+string captureSink(DataFlowSinkTargetApi api) {
   exists(DataFlow::Node src, DataFlow::Node sink, string kind |
     PropagateToSink::flow(src, sink) and
     ExternalFlow::sinkNode(sink, kind) and

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

@@ -82,11 +82,21 @@ private Callable liftedImpl(Callable api) {
   not exists(getARelevantOverrideeOrImplementee(result))
 }
 
-private predicate hasManualModel(Callable api) {
+private predicate hasManualSummaryModel(Callable api) {
   api = any(FlowSummaryImpl::Public::SummarizedCallable sc | sc.applyManualModel()) or
   api = any(FlowSummaryImpl::Public::NeutralSummaryCallable sc | sc.hasManualModel())
 }
 
+private predicate hasManualSourceModel(Callable api) {
+  api = any(ExternalFlow::SourceCallable sc | sc.hasManualModel()) or
+  api = any(FlowSummaryImpl::Public::NeutralSourceCallable sc | sc.hasManualModel())
+}
+
+private predicate hasManualSinkModel(Callable api) {
+  api = any(ExternalFlow::SinkCallable sc | sc.hasManualModel()) or
+  api = any(FlowSummaryImpl::Public::NeutralSinkCallable sc | sc.hasManualModel())
+}
+
 /**
  * Holds if it is irrelevant to generate models for `api` based on data flow analysis.
  *
@@ -101,20 +111,39 @@ predicate isUninterestingForDataFlowModels(CS::Callable api) { isHigherOrder(api
  */
 predicate isUninterestingForTypeBasedFlowModels(CS::Callable api) { none() }
 
+/**
+ * A class of callables that are potentially relevant for generating summary and
+ * neutral models.
+ */
+class SummaryTargetApi extends TargetApiBase {
+  SummaryTargetApi() { not hasManualSummaryModel(this.lift()) }
+}
+
+/**
+ * A class of callables that are potentially relevant for generating sink models.
+ */
+class SinkTargetApi extends TargetApiBase {
+  SinkTargetApi() { not hasManualSinkModel(this.lift()) }
+}
+
+/**
+ * A class of callables that are potentially relevant for generating source models.
+ */
+class SourceTargetApi extends TargetApiBase {
+  SourceTargetApi() { not hasManualSourceModel(this.lift()) }
+}
+
 /**
  * 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 is the callables (or callables that have a
  * super implementation) that can be called from outside the library itself.
  */
-class TargetApiSpecific extends Callable {
+class TargetApiBase extends Callable {
   private Callable lift;
 
-  TargetApiSpecific() {
-    lift = liftedImpl(this) and
-    not hasManualModel(lift)
-  }
+  TargetApiBase() { lift = liftedImpl(this) }
 
   /**
    * Gets the callable that a model will be lifted to.
@@ -233,24 +262,11 @@ 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
-  exists(Callable enclosing | enclosing = source.getEnclosingCallable() |
-    relevant(enclosing) and
-    not enclosing instanceof ManualNeutralSinkCallable
-  )
+  isRelevantMemberAccess(source) or source instanceof DataFlow::ParameterNode
 }
 
 private predicate uniquelyCalls(DataFlowCallable dc1, DataFlowCallable dc2) {
@@ -269,7 +285,7 @@ private predicate uniquelyCallsPlus(DataFlowCallable dc1, DataFlowCallable dc2)
  * if flow is detected from a node within `source` to a sink within `api`.
  */
 bindingset[sourceEnclosing, api]
-predicate irrelevantSourceSinkApi(Callable sourceEnclosing, TargetApiSpecific api) {
+predicate irrelevantSourceSinkApi(Callable sourceEnclosing, SourceTargetApi api) {
   not exists(DataFlowCallable dc1, DataFlowCallable dc2 | uniquelyCallsPlus(dc1, dc2) or dc1 = dc2 |
     dc1.getUnderlyingCallable() = api and
     dc2.getUnderlyingCallable() = sourceEnclosing
@@ -299,4 +315,4 @@ predicate isRelevantSinkKind(string kind) { any() }
  * Holds if `kind` is a relevant source kind for creating source models.
  */
 bindingset[kind]
-predicate isRelevantSourceKind(string kind) { not kind = "file" }
+predicate isRelevantSourceKind(string kind) { any() }

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

@@ -75,7 +75,7 @@ private import CaptureModels
  * Captured Model:
  * ```Summaries;BasicFlow;false;AssignToArray;(System.Int32,System.Int32[]);Argument[0];Argument[1].Element;taint;df-generated```
  */
-string captureFlow(DataFlowTargetApi api) {
+string captureFlow(DataFlowSummaryTargetApi api) {
   result = captureQualifierFlow(api) or
   result = captureThroughFlow(api)
 }
@@ -86,8 +86,8 @@ string captureFlow(DataFlowTargetApi api) {
  * a summary model that applies to `api` and if it relevant to generate
  * a model for `api`.
  */
-string captureNoFlow(DataFlowTargetApi api) {
-  not exists(DataFlowTargetApi api0 | exists(captureFlow(api0)) and api0.lift() = api.lift()) and
+string captureNoFlow(DataFlowSummaryTargetApi api) {
+  not exists(DataFlowSummaryTargetApi api0 | exists(captureFlow(api0)) and api0.lift() = api.lift()) and
   api.isRelevant() and
   result = Printing::asNeutralSummaryModel(api)
 }

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

@@ -189,7 +189,7 @@ private module Printing = ModelPrinting<ModelPrintingInput>;
  * A class of callables that are relevant generating summaries for based
  * on the Theorems for Free approach.
  */
-class TypeBasedFlowTargetApi extends Specific::TargetApiSpecific {
+class TypeBasedFlowTargetApi extends Specific::SummaryTargetApi {
   TypeBasedFlowTargetApi() { not Specific::isUninterestingForTypeBasedFlowModels(this) }
 
   /**

csharp/ql/test/utils/modelgenerator/dataflow/CaptureSinkModels.ext.yml

@@ -4,3 +4,10 @@ extensions:
       extensible: sinkModel
     data:
       - [ "Sinks", "NewSinks", False, "Sink", "(System.Object)", "", "Argument[0]", "test-sink", "manual"]
+      - [ "Sinks", "NewSinks", False, "ManualSinkAlreadyDefined", "(System.Object)", "", "Argument[0]", "test-sink", "manual"]
+
+  - addsTo:
+      pack: codeql/csharp-all
+      extensible: neutralModel
+    data:
+      - [ "Sinks", "NewSinks", "ManualSinkNeutral", "(System.Object)", "sink", "manual" ]

csharp/ql/test/utils/modelgenerator/dataflow/CaptureSourceModels.ext.yml

@@ -0,0 +1,12 @@
+extensions:
+  - addsTo:
+      pack: codeql/csharp-all
+      extensible: sourceModel
+    data:
+      - ["Sources", "NewSources", False, "ManualSourceAlreadyDefined", "()", "", "ReturnValue", "test-source", "manual"]
+
+  - addsTo:
+      pack: codeql/csharp-all
+      extensible: neutralModel
+    data:
+      - ["Sources", "NewSources", "ManualNeutralSource", "()", "source", "manual"]