Make the additional flow steps generally applicible to all queries

joefarebrother · joefarebrother · commit 047f8e485ae7 · 2023-11-23T10:56:43.000Z
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll
@@ -9,9 +9,10 @@ private import semmle.code.csharp.dispatch.Dispatch
 private import semmle.code.csharp.commons.ComparisonTest
 private import cil
 private import dotnet
-// import `TaintedMember` definitions from other files to avoid potential reevaluation
+// import `TaintedMember` and `AdditionalTaintStep` definitions from other files to avoid potential reevaluation
 private import semmle.code.csharp.frameworks.JsonNET
 private import semmle.code.csharp.frameworks.WCF
+private import semmle.code.csharp.frameworks.Razor
 private import semmle.code.csharp.security.dataflow.flowsources.Remote
 
 /**
@@ -160,6 +161,8 @@ private module Cached {
       nodeTo.(FlowSummaryNode).getSummaryNode(), false)
     or
     nodeTo = nodeFrom.(DataFlow::NonLocalJumpNode).getAJumpSuccessor(false)
+    or
+    any(AdditionalTaintStep step).step(nodeFrom, nodeTo)
   }
 }
 
diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPublic.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPublic.qll
@@ -1,5 +1,6 @@
 private import csharp
 private import TaintTrackingPrivate
+private import codeql.util.Unit
 
 /**
  * Holds if taint propagates from `source` to `sink` in zero or more local
@@ -20,4 +21,18 @@ predicate localExprTaint(Expr e1, Expr e2) {
 /** A member (property or field) that is tainted if its containing object is tainted. */
 abstract class TaintedMember extends AssignableMember { }
 
+/**
+ * A unit class for adding additional taint steps.
+ *
+ * Extend this class to add additional taint steps that should apply to all
+ * taint configurations.
+ */
+class AdditionalTaintStep extends Unit {
+  /**
+   * Holds if the step from `node1` to `node2` should be considered a taint
+   * step for all configurations.
+   */
+  abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
+}
+
 predicate localTaintStep = localTaintStepImpl/2;
diff --git a/csharp/ql/lib/semmle/code/csharp/frameworks/Razor.qll b/csharp/ql/lib/semmle/code/csharp/frameworks/Razor.qll
@@ -1,19 +1,10 @@
-/** Definitions for additional flow steps for cross-site scripting (XSS) vulnerabilities. */
+/** Provides definitions and flow steps related to Razor pages. */
 
 import csharp
 private import codeql.util.Unit
 private import codeql.util.FilePath
 private import semmle.code.csharp.frameworks.microsoft.AspNetCore
 
-/**
- * A unit class for providing additional flow steps for cross-site scripting (XSS) vulnerabilities.
- * Extend to provide additional flow steps.
- */
-class XssAdditionalFlowStep extends Unit {
-  /** Holds if there is an additional dataflow step from `node1` to `node2`. */
-  abstract predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2);
-}
-
 /** A call to the `View` method */
 private class ViewCall extends MethodCall {
   ViewCall() { this.getTarget().hasQualifiedName("Microsoft.AspNetCore.Mvc", "Controller", "View") }
@@ -74,7 +65,7 @@ private class ViewCall extends MethodCall {
 }
 
 /** A compiler-generated Razor page. */
-private class RazorPage extends Class {
+class RazorPage extends Class {
   AssemblyAttribute attr;
 
   RazorPage() {
@@ -90,8 +81,8 @@ private class RazorPage extends Class {
   string getSourceFilepath() { result = attr.getArgument(2).(StringLiteral).getValue() }
 }
 
-private class ViewCallFlowStep extends XssAdditionalFlowStep {
-  override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
+private class ViewCallFlowStep extends TaintTracking::AdditionalTaintStep {
+  override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
     exists(ViewCall vc, RazorPage rp, PropertyAccess modelProp |
       viewCallRefersToPage(vc, rp) and
       node1.asExpr() = vc.getModelArgument() and
diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/XSSQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/XSSQuery.qll
@@ -5,7 +5,6 @@
 
 import csharp
 private import XSSSinks
-private import XSSFlowSteps
 private import semmle.code.csharp.security.Sanitizers
 private import semmle.code.csharp.security.dataflow.flowsources.Remote
 private import semmle.code.csharp.dataflow.DataFlow2
@@ -167,13 +166,6 @@ module XssTrackingConfig implements DataFlow::ConfigSig {
    */
   predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
-  /**
-   * Holds if there is an additional dataflow step from `node1` to `node2`.
-   */
-  predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-    any(XssAdditionalFlowStep x).isAdditionalFlowStep(node1, node2)
-  }
-
   /**
    * Holds if data flow through `node` is prohibited. This completely removes
    * `node` from the data flow graph.
diff --git a/csharp/ql/src/change-notes/2023-10-24-xss-flow-steps.md b/csharp/ql/src/change-notes/2023-10-24-xss-flow-steps.md
@@ -1,4 +1,4 @@
 ---
 category: minorAnalysis
 ---
-Additional flow steps have been added to the `cs/web/xss` query to track flow from a `View` call in an MVC controller to the corresponding Razor page.
+Additional flow steps  to track flow from a `View` call in an MVC controller to the corresponding Razor page, which may result in additional results for queries such as `cs/web/xss`.
