Merge branch 'main' into static-useInstanceOf

Author: erik-krogh

cpp/ql/src/Security/CWE/CWE-022/TaintedPath.ql

@@ -16,7 +16,7 @@
 
 import cpp
 import semmle.code.cpp.security.FunctionWithWrappers
-import semmle.code.cpp.security.Security
+import semmle.code.cpp.security.FlowSources
 import semmle.code.cpp.ir.IR
 import semmle.code.cpp.ir.dataflow.TaintTracking
 import DataFlow::PathGraph
@@ -47,12 +47,6 @@ class FileFunction extends FunctionWithWrappers {
   override predicate interestingArg(int arg) { arg = 0 }
 }
 
-Expr asSourceExpr(DataFlow::Node node) {
-  result = node.asConvertedExpr()
-  or
-  result = node.asDefiningArgument()
-}
-
 Expr asSinkExpr(DataFlow::Node node) {
   result =
     node.asOperand()
@@ -89,7 +83,7 @@ predicate hasUpperBoundsCheck(Variable var) {
 class TaintedPathConfiguration extends TaintTracking::Configuration {
   TaintedPathConfiguration() { this = "TaintedPathConfiguration" }
 
-  override predicate isSource(DataFlow::Node node) { isUserInput(asSourceExpr(node), _) }
+  override predicate isSource(DataFlow::Node node) { node instanceof FlowSource }
 
   override predicate isSink(DataFlow::Node node) {
     exists(FileFunction fileFunction |
@@ -108,31 +102,16 @@ class TaintedPathConfiguration extends TaintTracking::Configuration {
       hasUpperBoundsCheck(checkedVar)
     )
   }
-
-  predicate hasFilteredFlowPath(DataFlow::PathNode source, DataFlow::PathNode sink) {
-    this.hasFlowPath(source, sink) and
-    // The use of `isUserInput` in `isSink` in combination with `asSourceExpr` causes
-    // duplicate results. Filter these duplicates. The proper solution is to switch to
-    // using `LocalFlowSource` and `RemoteFlowSource`, but this currently only supports
-    // a subset of the cases supported by `isUserInput`.
-    not exists(DataFlow::PathNode source2 |
-      this.hasFlowPath(source2, sink) and
-      asSourceExpr(source.getNode()) = asSourceExpr(source2.getNode())
-    |
-      not exists(source.getNode().asConvertedExpr()) and exists(source2.getNode().asConvertedExpr())
-    )
-  }
 }
 
 from
-  FileFunction fileFunction, Expr taintedArg, Expr taintSource, TaintedPathConfiguration cfg,
-  DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode, string taintCause, string callChain
+  FileFunction fileFunction, Expr taintedArg, FlowSource taintSource, TaintedPathConfiguration cfg,
+  DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode, string callChain
 where
   taintedArg = asSinkExpr(sinkNode.getNode()) and
   fileFunction.outermostWrapperFunctionCall(taintedArg, callChain) and
-  cfg.hasFilteredFlowPath(sourceNode, sinkNode) and
-  taintSource = asSourceExpr(sourceNode.getNode()) and
-  isUserInput(taintSource, taintCause)
+  cfg.hasFlowPath(sourceNode, sinkNode) and
+  taintSource = sourceNode.getNode()
 select taintedArg, sourceNode, sinkNode,
   "This argument to a file access function is derived from $@ and then passed to " + callChain + ".",
-  taintSource, "user input (" + taintCause + ")"
+  taintSource, "user input (" + taintSource.getSourceType() + ")"

cpp/ql/test/query-tests/Security/CWE/CWE-022/SAMATE/TaintedPath/TaintedPath.expected

@@ -5,4 +5,4 @@ nodes
 | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data indirection | semmle.label | data indirection |
 subpaths
 #select
-| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | ... + ... | user input (fgets) |
+| CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:77:23:77:26 | data indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | CWE23_Relative_Path_Traversal__char_console_fopen_11.cpp:55:27:55:38 | fgets output argument | user input (string read by fgets) |

cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/TaintedPath.expected

@@ -2,7 +2,6 @@ edges
 | test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName indirection |
 | test.c:31:22:31:25 | argv | test.c:32:11:32:18 | fileName indirection |
 | test.c:37:17:37:24 | scanf output argument | test.c:38:11:38:18 | fileName indirection |
-| test.c:43:17:43:24 | fileName | test.c:44:11:44:18 | fileName indirection |
 | test.c:43:17:43:24 | scanf output argument | test.c:44:11:44:18 | fileName indirection |
 nodes
 | test.c:9:23:9:26 | argv | semmle.label | argv |
@@ -11,12 +10,11 @@ nodes
 | test.c:32:11:32:18 | fileName indirection | semmle.label | fileName indirection |
 | test.c:37:17:37:24 | scanf output argument | semmle.label | scanf output argument |
 | test.c:38:11:38:18 | fileName indirection | semmle.label | fileName indirection |
-| test.c:43:17:43:24 | fileName | semmle.label | fileName |
 | test.c:43:17:43:24 | scanf output argument | semmle.label | scanf output argument |
 | test.c:44:11:44:18 | fileName indirection | semmle.label | fileName indirection |
 subpaths
 #select
-| test.c:17:11:17:18 | fileName | test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:9:23:9:26 | argv | user input (argv) |
-| test.c:32:11:32:18 | fileName | test.c:31:22:31:25 | argv | test.c:32:11:32:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:31:22:31:25 | argv | user input (argv) |
-| test.c:38:11:38:18 | fileName | test.c:37:17:37:24 | scanf output argument | test.c:38:11:38:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:37:17:37:24 | fileName | user input (scanf) |
-| test.c:44:11:44:18 | fileName | test.c:43:17:43:24 | fileName | test.c:44:11:44:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:43:17:43:24 | fileName | user input (scanf) |
+| test.c:17:11:17:18 | fileName | test.c:9:23:9:26 | argv | test.c:17:11:17:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:9:23:9:26 | argv | user input (a command-line argument) |
+| test.c:32:11:32:18 | fileName | test.c:31:22:31:25 | argv | test.c:32:11:32:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:31:22:31:25 | argv | user input (a command-line argument) |
+| test.c:38:11:38:18 | fileName | test.c:37:17:37:24 | scanf output argument | test.c:38:11:38:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:37:17:37:24 | scanf output argument | user input (value read by scanf) |
+| test.c:44:11:44:18 | fileName | test.c:43:17:43:24 | scanf output argument | test.c:44:11:44:18 | fileName indirection | This argument to a file access function is derived from $@ and then passed to fopen(filename). | test.c:43:17:43:24 | scanf output argument | user input (value read by scanf) |

csharp/ql/lib/ext/generated/dotnet_runtime.model.yml

@@ -10195,7 +10195,7 @@ extensions:
 
   - addsTo:
       pack: codeql/csharp-all
-      extensible: extNegativeSummaryModel
+      extensible: extNeutralModel
     data:
       - ["AssemblyStripper", "AssemblyStripper", "StripAssembly", "(System.String,System.String)", "generated"]
       - ["Generators", "EventSourceGenerator", "Execute", "(Microsoft.CodeAnalysis.GeneratorExecutionContext)", "generated"]

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

@@ -11,9 +11,9 @@
  *   `namespace; type; subtypes; name; signature; ext; input; kind; provenance`
  * - Summaries:
  *   `namespace; type; subtypes; name; signature; ext; input; output; kind; provenance`
- * - Negative Summaries:
+ * - Neutrals:
  *   `namespace; type; name; signature; provenance`
- *   A negative summary is used to indicate that there is no flow via a callable.
+ *   A neutral is used to indicate that there is no flow via a callable.
  *
  * The interpretation of a row is similar to API-graphs with a left-to-right
  * reading.
@@ -132,30 +132,12 @@ private class SummaryModelCsvInternal extends Unit {
   abstract predicate row(string row);
 }
 
-/**
- * DEPRECATED: Define negative summary models as data extensions instead.
- *
- * A unit class for adding additional negative summary model rows.
- *
- * Extend this class to add additional negative summary definitions.
- */
-deprecated class NegativeSummaryModelCsv = NegativeSummaryModelCsvInternal;
-
-private class NegativeSummaryModelCsvInternal extends Unit {
-  /** Holds if `row` specifies a negative summary definition. */
-  abstract predicate row(string row);
-}
-
 private predicate sourceModelInternal(string row) { any(SourceModelCsvInternal s).row(row) }
 
 private predicate summaryModelInternal(string row) { any(SummaryModelCsvInternal s).row(row) }
 
 private predicate sinkModelInternal(string row) { any(SinkModelCsvInternal s).row(row) }
 
-private predicate negativeSummaryModelInternal(string row) {
-  any(NegativeSummaryModelCsvInternal s).row(row)
-}
-
 /**
  * Holds if a source model exists for the given parameters.
  */
@@ -243,25 +225,16 @@ predicate summaryModel(
   extSummaryModel(namespace, type, subtypes, name, signature, ext, input, output, kind, provenance)
 }
 
-/** Holds if a summary model exists indicating there is no flow for the given parameters. */
-extensible predicate extNegativeSummaryModel(
+/** Holds if a model exists indicating there is no flow for the given parameters. */
+extensible predicate extNeutralModel(
   string namespace, string type, string name, string signature, string provenance
 );
 
-/** Holds if a summary model exists indicating there is no flow for the given parameters. */
-predicate negativeSummaryModel(
+/** Holds if a model exists indicating there is no flow for the given parameters. */
+predicate neutralModel(
   string namespace, string type, string name, string signature, string provenance
 ) {
-  exists(string row |
-    negativeSummaryModelInternal(row) and
-    row.splitAt(";", 0) = namespace and
-    row.splitAt(";", 1) = type and
-    row.splitAt(";", 2) = name and
-    row.splitAt(";", 3) = signature and
-    row.splitAt(";", 4) = provenance
-  )
-  or
-  extNegativeSummaryModel(namespace, type, name, signature, provenance)
+  extNeutralModel(namespace, type, name, signature, provenance)
 }
 
 private predicate relevantNamespace(string namespace) {
@@ -393,8 +366,6 @@ module ModelValidation {
       sinkModelInternal(row) and expect = 9 and pred = "sink"
       or
       summaryModelInternal(row) and expect = 10 and pred = "summary"
-      or
-      negativeSummaryModelInternal(row) and expect = 5 and pred = "negative summary"
     |
       exists(int cols |
         cols = 1 + max(int n | exists(row.splitAt(";", n))) and
@@ -418,9 +389,9 @@ module ModelValidation {
       summaryModel(namespace, type, _, name, signature, ext, _, _, _, provenance) and
       pred = "summary"
       or
-      negativeSummaryModel(namespace, type, name, signature, provenance) and
+      neutralModel(namespace, type, name, signature, provenance) and
       ext = "" and
-      pred = "negative summary"
+      pred = "neutral"
     |
       not namespace.regexpMatch("[a-zA-Z0-9_\\.]+") and
       result = "Dubious namespace \"" + namespace + "\" in " + pred + " model."
@@ -461,7 +432,7 @@ private predicate elementSpec(
   or
   summaryModel(namespace, type, subtypes, name, signature, ext, _, _, _, _)
   or
-  negativeSummaryModel(namespace, type, name, signature, _) and ext = "" and subtypes = false
+  neutralModel(namespace, type, name, signature, _) and ext = "" and subtypes = false
 }
 
 private predicate elementSpec(
@@ -595,7 +566,7 @@ private Element interpretElement0(
   )
 }
 
-/** Gets the source/sink/summary/negativesummary element corresponding to the supplied parameters. */
+/** Gets the source/sink/summary/neutral element corresponding to the supplied parameters. */
 Element interpretElement(
   string namespace, string type, boolean subtypes, string name, string signature, string ext
 ) {

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

@@ -2321,8 +2321,8 @@ module Csv {
     )
   }
 
-  /** Computes the first 4 columns for negative CSV rows of `c`. */
-  string asPartialNegativeModel(DotNet::Callable c) {
+  /** Computes the first 4 columns for neutral CSV rows of `c`. */
+  string asPartialNeutralModel(DotNet::Callable c) {
     exists(string namespace, string type, string name, string parameters |
       partialModel(c, namespace, type, name, parameters) and
       result =

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

@@ -246,14 +246,14 @@ module Public {
     predicate isAutoGenerated() { none() }
   }
 
-  /** A callable with a flow summary stating there is no flow via the callable. */
-  class NegativeSummarizedCallable extends SummarizedCallableBase {
-    NegativeSummarizedCallable() { negativeSummaryElement(this, _) }
+  /** A callable where there is no flow via the callable. */
+  class NeutralCallable extends SummarizedCallableBase {
+    NeutralCallable() { neutralElement(this, _) }
 
     /**
-     *  Holds if the negative summary is auto generated.
+     *  Holds if the neutral is auto generated.
      */
-    predicate isAutoGenerated() { negativeSummaryElement(this, true) }
+    predicate isAutoGenerated() { neutralElement(this, true) }
   }
 }
 
@@ -1161,9 +1161,9 @@ module Private {
       string toString() { result = super.toString() }
     }
 
-    /** A flow summary to include in the `negativeSummary/1` query predicate. */
-    abstract class RelevantNegativeSummarizedCallable instanceof NegativeSummarizedCallable {
-      /** Gets the string representation of this callable used by `summary/1`. */
+    /** A model to include in the `neutral/1` query predicate. */
+    abstract class RelevantNeutralCallable instanceof NeutralCallable {
+      /** Gets the string representation of this callable used by `neutral/1`. */
       abstract string getCallableCsv();
 
       string toString() { result = super.toString() }
@@ -1180,13 +1180,13 @@ module Private {
       if c.isAutoGenerated() then result = "generated" else result = "manual"
     }
 
-    private string renderProvenanceNegative(NegativeSummarizedCallable c) {
+    private string renderProvenanceNeutral(NeutralCallable c) {
       if c.isAutoGenerated() then result = "generated" else result = "manual"
     }
 
     /**
      * A query predicate for outputting flow summaries in semi-colon separated format in QL tests.
-     * The syntax is: "namespace;type;overrides;name;signature;ext;inputspec;outputspec;kind;provenance"",
+     * The syntax is: "namespace;type;overrides;name;signature;ext;inputspec;outputspec;kind;provenance",
      * ext is hardcoded to empty.
      */
     query predicate summary(string csv) {
@@ -1205,14 +1205,14 @@ module Private {
     }
 
     /**
-     * Holds if a negative flow summary `csv` exists (semi-colon separated format). Used for testing purposes.
+     * Holds if a neutral model `csv` exists (semi-colon separated format). Used for testing purposes.
      * The syntax is: "namespace;type;name;signature;provenance"",
      */
-    query predicate negativeSummary(string csv) {
-      exists(RelevantNegativeSummarizedCallable c |
+    query predicate neutral(string csv) {
+      exists(RelevantNeutralCallable c |
         csv =
           c.getCallableCsv() // Callable information
-            + renderProvenanceNegative(c) // provenance
+            + renderProvenanceNeutral(c) // provenance
       )
     }
   }

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

@@ -121,12 +121,12 @@ predicate summaryElement(Callable c, string input, string output, string kind, b
 }
 
 /**
- * Holds if a negative flow summary exists for `c`, which means that there is no
- * flow through `c`. The flag `generated` states whether the summary is autogenerated.
+ * Holds if a neutral model exists for `c`, which means that there is no
+ * flow through `c`. The flag `generated` states whether the neutral model is autogenerated.
  */
-predicate negativeSummaryElement(Callable c, boolean generated) {
+predicate neutralElement(Callable c, boolean generated) {
   exists(string namespace, string type, string name, string signature, string provenance |
-    negativeSummaryModel(namespace, type, name, signature, provenance) and
+    neutralModel(namespace, type, name, signature, provenance) and
     generated = isGenerated(provenance) and
     c = interpretElement(namespace, type, false, name, signature, "")
   )

csharp/ql/src/Telemetry/SupportedExternalApis.ql

@@ -15,7 +15,7 @@ private predicate relevant(ExternalApi api) {
   not api.isUninteresting() and
   (
     api.isSupported() or
-    api instanceof FlowSummaryImpl::Public::NegativeSummarizedCallable
+    api instanceof FlowSummaryImpl::Public::NeutralCallable
   )
 }
 

csharp/ql/src/Telemetry/UnsupportedExternalAPIs.ql

@@ -14,7 +14,7 @@ private import ExternalApi
 private predicate relevant(ExternalApi api) {
   not api.isUninteresting() and
   not api.isSupported() and
-  not api instanceof FlowSummaryImpl::Public::NegativeSummarizedCallable
+  not api instanceof FlowSummaryImpl::Public::NeutralCallable
 }
 
 from string info, int usages