Skip to content

Commit 40a7223

Browse files
Implement xss flow step for absolute filepath case
1 parent d056706 commit 40a7223

File tree

2 files changed

+89
-0
lines changed

2 files changed

+89
-0
lines changed
Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
import csharp
2+
private import codeql.util.Unit
3+
private import semmle.code.csharp.frameworks.microsoft.AspNetCore
4+
5+
/** An additional flow step for cross-site scripting (XSS) vulnerabilities */
6+
abstract class XssAdditionalFlowStep extends Unit {
7+
abstract predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2);
8+
}
9+
10+
/** A call to the `View` method */
11+
private class ViewCall extends MethodCall {
12+
ViewCall() { this.getTarget().hasQualifiedName("Microsoft.AspNetCore.Mvc", "Controller", "View") }
13+
14+
/** Gets the `name` argument to this call, if any. */
15+
string getNameArgument() {
16+
exists(StringLiteral lit, int i | i in [0 .. 1] |
17+
this.getTarget().getParameter(i).getType() instanceof StringType and
18+
DataFlow::localExprFlow(lit, this.getArgument(i)) and
19+
result = lit.getValue()
20+
)
21+
}
22+
23+
/** Gets the `model` argument to this call, if any. */
24+
Expr getModelArgument() {
25+
exists(int i | i in [0 .. 1] |
26+
this.getTarget().getParameter(i).getType() instanceof ObjectType and
27+
result = this.getArgument(i)
28+
)
29+
}
30+
31+
/** Gets the MVC action method that this call is made from, if any. */
32+
Method getActionMethod() {
33+
result = this.getEnclosingCallable() and
34+
result = this.getController().getAnActionMethod()
35+
}
36+
37+
/** Gets the MVC controller that this call is made from, if any. */
38+
MicrosoftAspNetCoreMvcController getController() {
39+
result = this.getEnclosingCallable().getDeclaringType()
40+
}
41+
42+
/** Gets the name of the MVC controller that this call is made from, if any. */
43+
string getControllerName() { result + "Controller" = this.getController().getName() }
44+
}
45+
46+
/** A compiler-generated Razor page. */
47+
private class RazorPage extends Class {
48+
AssemblyAttribute attr;
49+
50+
RazorPage() {
51+
attr.getFile() = this.getFile() and
52+
attr.getType()
53+
.hasQualifiedName("Microsoft.AspNetCore.Razor.Hosting", "RazorCompiledItemAttribute")
54+
}
55+
56+
/**
57+
* Gets the filepath of the source file that this class was generated from,
58+
* relative to the application root.
59+
*/
60+
string getSourceFilepath() { result = attr.getArgument(2).(StringLiteral).getValue() }
61+
}
62+
63+
private class ViewCallFlowStep extends XssAdditionalFlowStep {
64+
override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
65+
exists(ViewCall vc, RazorPage rp, PropertyAccess modelProp |
66+
viewCallRefersToPage(vc, rp) and
67+
node1.asExpr() = vc.getModelArgument() and
68+
node2.asExpr() = modelProp and
69+
modelProp.getTarget().hasName("Model") and
70+
modelProp.getEnclosingCallable().getDeclaringType() = rp
71+
)
72+
}
73+
}
74+
75+
private predicate viewCallRefersToPage(ViewCall vc, RazorPage rp) {
76+
viewCallRefersToPageAbsolute(vc, rp)
77+
}
78+
79+
private predicate viewCallRefersToPageAbsolute(ViewCall vc, RazorPage rp) {
80+
["/", "~/", ""] + vc.getNameArgument() = rp.getSourceFilepath()
81+
}

csharp/ql/lib/semmle/code/csharp/security/dataflow/XSSQuery.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
import csharp
77
private import XSSSinks
8+
private import XSSFlowSteps
89
private import semmle.code.csharp.security.Sanitizers
910
private import semmle.code.csharp.security.dataflow.flowsources.Remote
1011
private import semmle.code.csharp.dataflow.DataFlow2
@@ -166,6 +167,13 @@ module XssTrackingConfig implements DataFlow::ConfigSig {
166167
*/
167168
predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
168169

170+
/**
171+
* Holds if there is an additional dataflow step from `node1` to `node2`.
172+
*/
173+
predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
174+
any(XssAdditionalFlowStep x).isAdditionalFlowStep(node1, node2)
175+
}
176+
169177
/**
170178
* Holds if data flow through `node` is prohibited. This completely removes
171179
* `node` from the data flow graph.

0 commit comments

Comments
 (0)