Make the additional flow steps generally applicible to all queries

This commit is contained in:
Joe Farebrother
2023-10-26 14:05:20 +01:00
parent 0ed7b3c3ad
commit 047f8e485a
5 changed files with 24 additions and 23 deletions

View File

@@ -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)
}
}

View File

@@ -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;

View File

@@ -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

View File

@@ -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.

View File

@@ -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`.