Skip to content

Commit 7433e8e

Browse files
tamasvajktamasvajk
committed
WIP: handle binding on InputText 
1 parent d3a8688 commit 7433e8e

File tree

6 files changed

+166
-29
lines changed

6 files changed

+166
-29
lines changed

‎csharp/ql/integration-tests/all-platforms/blazor/XSS.expected

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,13 @@
11
edges
2+
| BlazorTest/Components/Pages/TestPage.razor:27:29:27:39 | access to property InputValue1 : String | BlazorTest/Components/Pages/TestPage.razor:27:29:27:39 | access to property InputValue1 : String | provenance | |
3+
| BlazorTest/Components/Pages/TestPage.razor:27:29:27:39 | access to property InputValue1 : String | BlazorTest/Components/Pages/TestPage.razor:29:53:29:63 | access to property InputValue1 | provenance | Sink:MaD:142 |
24
nodes
35
| BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | semmle.label | access to property UrlParam |
46
| BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | semmle.label | access to property QueryParam |
7+
| BlazorTest/Components/Pages/TestPage.razor:27:29:27:39 | access to property InputValue1 : String | semmle.label | access to property InputValue1 : String |
8+
| BlazorTest/Components/Pages/TestPage.razor:29:53:29:63 | access to property InputValue1 | semmle.label | access to property InputValue1 |
59
subpaths
610
#select
711
| BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | $@ flows to here and is written to HTML or JavaScript. | BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | User-provided value |
812
| BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | $@ flows to here and is written to HTML or JavaScript. | BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | User-provided value |
13+
| BlazorTest/Components/Pages/TestPage.razor:29:53:29:63 | access to property InputValue1 | BlazorTest/Components/Pages/TestPage.razor:27:29:27:39 | access to property InputValue1 : String | BlazorTest/Components/Pages/TestPage.razor:29:53:29:63 | access to property InputValue1 | $@ flows to here and is written to HTML or JavaScript. | BlazorTest/Components/Pages/TestPage.razor:27:29:27:39 | access to property InputValue1 : String | User-provided value |

‎csharp/ql/integration-tests/all-platforms/blazor/remoteFlowSource.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,3 +2,4 @@
22
| BlazorTest/Components/Pages/TestPage.razor:11:48:11:55 | access to property UrlParam | ASP.NET Core component route parameter |
33
| BlazorTest/Components/Pages/TestPage.razor:19:38:19:47 | access to property QueryParam | ASP.NET Core component query string |
44
| BlazorTest/Components/Pages/TestPage.razor:20:60:20:69 | access to property QueryParam | ASP.NET Core component query string |
5+
| BlazorTest/Components/Pages/TestPage.razor:27:29:27:39 | access to property InputValue1 | ASP.NET Core `InputBase<>.Value`-bound property read |

‎csharp/ql/lib/ext/Microsoft.AspNetCore.Components.model.yml

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,17 @@
11
extensions:
2+
- addsTo:
3+
pack: codeql/csharp-all
4+
extensible: sourceModel
5+
data: []
6+
27
- addsTo:
38
pack: codeql/csharp-all
49
extensible: sinkModel
510
data:
611
- ["Microsoft.AspNetCore.Components", "MarkupString", False, "MarkupString", "(System.String)", "", "Argument[0]", "html-injection", "manual"]
712
- ["Microsoft.AspNetCore.Components", "MarkupString", False, "op_Explicit", "(System.String)", "", "Argument[0]", "html-injection", "manual"]
13+
14+
- addsTo:
15+
pack: codeql/csharp-all
16+
extensible: summaryModel
17+
data: []

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ private import semmle.code.csharp.dispatch.Dispatch
1515
private import semmle.code.csharp.frameworks.EntityFramework
1616
private import semmle.code.csharp.frameworks.NHibernate
1717
private import semmle.code.csharp.frameworks.Razor
18+
private import semmle.code.csharp.frameworks.microsoft.Blazor
1819
private import semmle.code.csharp.frameworks.system.Collections
1920
private import semmle.code.csharp.frameworks.system.threading.Tasks
2021
private import semmle.code.csharp.internal.Location

‎csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/Blazor.qll

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,4 +85,151 @@ class Component extends Class {
8585
result.getAnAttribute().getType() instanceof ParameterAttributeClass
8686
)
8787
}
88+
89+
private predicate hasSubComponent(
90+
MethodCall openCall, int openCallIndex, MethodCall closeCall, int closeCallIndex,
91+
ValueOrRefType componentType, Callable enclosing
92+
) {
93+
openCall.getEnclosingCallable+() = this.getAMethod("BuildRenderTree") and
94+
openCall.getEnclosingCallable() = enclosing and
95+
// TODO: there's another overload of OpenComponent
96+
openCall = this.getRenderTreeBuilderCall(openCallIndex, "OpenComponent`1", enclosing) and
97+
openCall.getTarget().(ConstructedGeneric).getTypeArgument(0) = componentType and
98+
closeCall = this.getCloseComponentCall(closeCallIndex, enclosing) and
99+
closeCallIndex > openCallIndex and
100+
not exists(int k, MethodCall mc |
101+
k in [openCallIndex + 1 .. closeCallIndex - 1] and
102+
mc = this.getCloseComponentCall(k, enclosing)
103+
)
104+
}
105+
106+
predicate hasPropertySetOnSubComponent(
107+
MethodCall addCall, ValueOrRefType componentType, Property p, Expr value
108+
) {
109+
exists(
110+
int i, int j, int k, MethodCall openCall, Callable enclosing, Expr cast, Expr typeCheckCall
111+
|
112+
this.hasSubComponent(openCall, i, _, j, componentType, enclosing) and
113+
p = componentType.getABaseType*().getAProperty() and
114+
// The below doesn't work for InputText:
115+
// p.getAnAttribute().getType() instanceof ParameterAttributeClass and
116+
k in [i + 1 .. j - 1] and
117+
addCall = this.getRenderTreeBuilderCall(k, "AddComponentParameter", enclosing) and
118+
p.getName() = addCall.getArgument(1).getValue() and
119+
cast = addCall.getArgument(2) and
120+
(
121+
if cast.(CastExpr).getTargetType().hasFullyQualifiedName("System", "Object")
122+
then typeCheckCall = cast.(CastExpr).getExpr()
123+
else typeCheckCall = cast
124+
) and
125+
(
126+
if
127+
typeCheckCall
128+
.(MethodCall)
129+
.getTarget()
130+
.getUnboundDeclaration()
131+
.hasFullyQualifiedName("Microsoft.AspNetCore.Components.CompilerServices.RuntimeHelpers",
132+
"TypeCheck`1")
133+
then value = typeCheckCall.(MethodCall).getArgument(0)
134+
else value = typeCheckCall
135+
)
136+
)
137+
}
138+
139+
private MethodCall getRenderTreeBuilderCall(int index, string name, Callable enclosing) {
140+
result.getEnclosingCallable() = enclosing and
141+
result.getParent().getIndex() = index and
142+
result
143+
.getTarget()
144+
.getUnboundDeclaration()
145+
.hasFullyQualifiedName("Microsoft.AspNetCore.Components.Rendering.RenderTreeBuilder", name)
146+
}
147+
148+
private MethodCall getCloseComponentCall(int index, Callable enclosing) {
149+
result = this.getRenderTreeBuilderCall(index, "CloseComponent", enclosing)
150+
}
151+
}
152+
153+
private AssignableMemberAccess getMemberAccessAssignedToInputValueProperty(
154+
Component c, AssignableMember member
155+
) {
156+
exists(ValueOrRefType componentType, Property componentProperty |
157+
c.hasPropertySetOnSubComponent(_, componentType, componentProperty, result) and
158+
componentType
159+
.getBaseClass+()
160+
.getUnboundDeclaration()
161+
.hasFullyQualifiedName("Microsoft.AspNetCore.Components.Forms", "InputBase`1") and
162+
componentProperty.hasName("Value") and
163+
result.getTarget() = member and
164+
member.getDeclaringType() = c and
165+
result.hasThisQualifier()
166+
// TODO: add support for Prop1.Prop2.Prop3 chains
167+
)
168+
}
169+
170+
module Sources {
171+
private import semmle.code.csharp.security.dataflow.flowsources.Remote
172+
173+
/** A data flow source of remote user input (ASP.NET Core component query string). */
174+
private class AspNetComponentQueryStringRemoteFlowSource extends AspNetRemoteFlowSource,
175+
DataFlow::ExprNode
176+
{
177+
AspNetComponentQueryStringRemoteFlowSource() {
178+
exists(Property p |
179+
p = any(Component c).getACascadingParameterProperty() and
180+
p.getAnAttribute().getType() instanceof SupplyParameterFromQueryAttributeClass and
181+
this.getExpr() = p.getGetter().getACall()
182+
)
183+
}
184+
185+
override string getSourceType() { result = "ASP.NET Core component query string" }
186+
}
187+
188+
/** A data flow source of remote user input (ASP.NET Core component route parameter). */
189+
private class AspNetComponentRouteParameterRemoteFlowSource extends AspNetRemoteFlowSource,
190+
DataFlow::ExprNode
191+
{
192+
AspNetComponentRouteParameterRemoteFlowSource() {
193+
exists(Property p |
194+
p = any(Component c).getARouteParameterProperty() and
195+
this.getExpr() = p.getGetter().getACall()
196+
)
197+
}
198+
199+
override string getSourceType() { result = "ASP.NET Core component route parameter" }
200+
}
201+
202+
private class AspNetComponentInputBaseValueFlowSource extends AspNetRemoteFlowSource,
203+
DataFlow::ExprNode
204+
{
205+
AspNetComponentInputBaseValueFlowSource() {
206+
this.getExpr() = getMemberAccessAssignedToInputValueProperty(_, _)
207+
}
208+
209+
override string getSourceType() {
210+
result = "ASP.NET Core `InputBase<>.Value`-bound property read"
211+
}
212+
}
213+
}
214+
215+
// TODO: this is only true
216+
// - if there's no custom `ValueChanged` handler defined on the `InputText` component, such as `<InputText Value="@InputValue1" ValueChanged="HandleChange" />` or
217+
// - if `@bind-Value` is used on the component. In case of `<InputText Value="@InputValue1" />`, there's only one way binding.
218+
private class InputBaseValuePropertyJumpNode extends DataFlow::NonLocalJumpNode {
219+
Component c;
220+
AssignableMember member;
221+
222+
InputBaseValuePropertyJumpNode() {
223+
this.asExpr() = getMemberAccessAssignedToInputValueProperty(c, member)
224+
}
225+
226+
override DataFlow::Node getAJumpSuccessor(boolean preservesValue) {
227+
preservesValue = true and
228+
exists(MemberAccess access |
229+
result.asExpr() = access and
230+
access.getTarget() = member and
231+
access.hasThisQualifier() and
232+
access instanceof AssignableRead
233+
)
234+
}
88235
}

‎csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll

Lines changed: 2 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ private import semmle.code.csharp.frameworks.system.web.ui.WebControls
1212
private import semmle.code.csharp.frameworks.WCF
1313
private import semmle.code.csharp.frameworks.microsoft.Owin
1414
private import semmle.code.csharp.frameworks.microsoft.AspNetCore
15-
private import semmle.code.csharp.frameworks.microsoft.Blazor
1615
private import semmle.code.csharp.dataflow.internal.ExternalFlow
1716
private import semmle.code.csharp.security.dataflow.flowsources.FlowSources
1817

@@ -27,7 +26,8 @@ abstract class RemoteFlowSource extends SourceNode {
2726
* A module for importing frameworks that defines remote flow sources.
2827
*/
2928
private module RemoteFlowSources {
30-
private import semmle.code.csharp.frameworks.ServiceStack
29+
private import semmle.code.csharp.frameworks.ServiceStack as ServiceStack
30+
private import semmle.code.csharp.frameworks.microsoft.Blazor as Blazor
3131
}
3232

3333
/** A data flow source of remote user input (ASP.NET). */
@@ -76,33 +76,6 @@ class AspNetQueryStringRemoteFlowSource extends AspNetRemoteFlowSource, DataFlow
7676
override string getSourceType() { result = "ASP.NET query string" }
7777
}
7878

79-
/** A data flow source of remote user input (ASP.NET Core component query string). */
80-
class AspNetComponentQueryStringRemoteFlowSource extends AspNetRemoteFlowSource, DataFlow::ExprNode {
81-
AspNetComponentQueryStringRemoteFlowSource() {
82-
exists(Property p |
83-
p = any(Component c).getACascadingParameterProperty() and
84-
p.getAnAttribute().getType() instanceof SupplyParameterFromQueryAttributeClass and
85-
this.getExpr() = p.getGetter().getACall()
86-
)
87-
}
88-
89-
override string getSourceType() { result = "ASP.NET Core component query string" }
90-
}
91-
92-
/** A data flow source of remote user input (ASP.NET Core component route parameter). */
93-
class AspNetComponentRouteParameterRemoteFlowSource extends AspNetRemoteFlowSource,
94-
DataFlow::ExprNode
95-
{
96-
AspNetComponentRouteParameterRemoteFlowSource() {
97-
exists(Property p |
98-
p = any(Component c).getARouteParameterProperty() and
99-
this.getExpr() = p.getGetter().getACall()
100-
)
101-
}
102-
103-
override string getSourceType() { result = "ASP.NET Core component route parameter" }
104-
}
105-
10679
/** A data flow source of remote user input (ASP.NET unvalidated request data). */
10780
class AspNetUnvalidatedQueryStringRemoteFlowSource extends AspNetRemoteFlowSource,
10881
DataFlow::ExprNode

0 commit comments

Comments
 (0)