Skip to content

Commit bca51a9

Browse files
michaelnebelmichaelnebel
authored
Merge pull request #21612 from michaelnebel/csharp/legacyasptaintedmember
C#: Taint members of types in ASP.NET user context.
2 parents 62f15d0 + 8b93ce2 commit bca51a9

File tree

9 files changed

+175
-23
lines changed

9 files changed

+175
-23
lines changed

‎csharp/ql/lib/change-notes/2026-04-01-asp-remote-sources.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Expanded ASP and ASP.NET remote source modeling to cover additional sources, including fields of tainted parameters as well as properties and fields that become tainted transitively.

‎csharp/ql/lib/semmle/code/csharp/commons/Collections.qll‎

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -54,21 +54,44 @@ private string genericCollectionTypeName() {
5454
]
5555
}
5656

57-
/** A collection type. */
58-
class CollectionType extends RefType {
59-
CollectionType() {
57+
/** A collection type */
58+
abstract private class CollectionTypeImpl extends RefType {
59+
/**
60+
* Gets the element type of this collection, for example `int` in `List<int>`.
61+
*/
62+
abstract Type getElementType();
63+
}
64+
65+
private class GenericCollectionType extends CollectionTypeImpl {
66+
private ConstructedType base;
67+
68+
GenericCollectionType() {
69+
base = this.getABaseType*() and
70+
base.getUnboundGeneric()
71+
.hasFullyQualifiedName(genericCollectionNamespaceName(), genericCollectionTypeName())
72+
}
73+
74+
override Type getElementType() {
75+
result = base.getTypeArgument(0) and base.getNumberOfTypeArguments() = 1
76+
}
77+
}
78+
79+
private class NonGenericCollectionType extends CollectionTypeImpl {
80+
NonGenericCollectionType() {
6081
exists(RefType base | base = this.getABaseType*() |
6182
base.hasFullyQualifiedName(collectionNamespaceName(), collectionTypeName())
62-
or
63-
base.(ConstructedType)
64-
.getUnboundGeneric()
65-
.hasFullyQualifiedName(genericCollectionNamespaceName(), genericCollectionTypeName())
6683
)
67-
or
68-
this instanceof ArrayType
6984
}
85+
86+
override Type getElementType() { none() }
7087
}
7188

89+
private class ArrayCollectionType extends CollectionTypeImpl instanceof ArrayType {
90+
override Type getElementType() { result = ArrayType.super.getElementType() }
91+
}
92+
93+
final class CollectionType = CollectionTypeImpl;
94+
7295
/**
7396
* A collection type that can be used as a `params` parameter type.
7497
*/

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

Lines changed: 57 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
*/
44

55
import csharp
6+
private import semmle.code.csharp.commons.Collections
67
private import semmle.code.csharp.frameworks.system.Net
78
private import semmle.code.csharp.frameworks.system.Web
89
private import semmle.code.csharp.frameworks.system.web.Http
@@ -104,7 +105,7 @@ class WcfRemoteFlowSource extends RemoteFlowSource, DataFlow::ParameterNode {
104105
}
105106

106107
/** A data flow source of remote user input (ASP.NET web service). */
107-
class AspNetServiceRemoteFlowSource extends RemoteFlowSource, DataFlow::ParameterNode {
108+
class AspNetServiceRemoteFlowSource extends AspNetRemoteFlowSource, DataFlow::ParameterNode {
108109
AspNetServiceRemoteFlowSource() {
109110
exists(Method m |
110111
m.getAParameter() = this.getParameter() and
@@ -115,8 +116,50 @@ class AspNetServiceRemoteFlowSource extends RemoteFlowSource, DataFlow::Paramete
115116
override string getSourceType() { result = "ASP.NET web service input" }
116117
}
117118

119+
private class CandidateMemberToTaint extends Member {
120+
CandidateMemberToTaint() {
121+
this.isPublic() and
122+
not this.isStatic() and
123+
(
124+
this =
125+
any(Property p |
126+
p.isAutoImplemented() and
127+
p.getGetter().isPublic() and
128+
p.getSetter().isPublic()
129+
)
130+
or
131+
this = any(Field f | f.isPublic())
132+
)
133+
}
134+
}
135+
136+
/**
137+
* Taint members (transitively) on types used in
138+
* 1. Action method parameters.
139+
* 2. WebMethod parameters.
140+
*
141+
* Note that this also impacts uses of such types in other contexts.
142+
*/
143+
private class AspNetRemoteFlowSourceMember extends TaintTracking::TaintedMember,
144+
CandidateMemberToTaint
145+
{
146+
AspNetRemoteFlowSourceMember() {
147+
exists(Type t, Type t0 | t = this.getDeclaringType() |
148+
(t = t0 or t = t0.(CollectionType).getElementType()) and
149+
(
150+
t0 = any(AspNetRemoteFlowSourceMember m).getType()
151+
or
152+
t0 = any(ActionMethodParameter p).getType()
153+
or
154+
t0 = any(AspNetServiceRemoteFlowSource source).getType()
155+
)
156+
)
157+
}
158+
}
159+
118160
/** A data flow source of remote user input (ASP.NET request message). */
119-
class SystemNetHttpRequestMessageRemoteFlowSource extends RemoteFlowSource, DataFlow::ExprNode {
161+
class SystemNetHttpRequestMessageRemoteFlowSource extends AspNetRemoteFlowSource, DataFlow::ExprNode
162+
{
120163
SystemNetHttpRequestMessageRemoteFlowSource() {
121164
this.getType() instanceof SystemWebHttpRequestMessageClass
122165
}
@@ -166,7 +209,7 @@ class MicrosoftOwinRequestRemoteFlowSource extends RemoteFlowSource, DataFlow::E
166209
}
167210

168211
/** A parameter to an Mvc controller action method, viewed as a source of remote user input. */
169-
class ActionMethodParameter extends RemoteFlowSource, DataFlow::ParameterNode {
212+
class ActionMethodParameter extends AspNetRemoteFlowSource, DataFlow::ParameterNode {
170213
ActionMethodParameter() {
171214
exists(Parameter p |
172215
p = this.getParameter() and
@@ -218,14 +261,18 @@ class AspNetCoreRoutingMethodParameter extends AspNetCoreRemoteFlowSource, DataF
218261
* Flow is defined from any ASP.NET Core remote source object to any of its member
219262
* properties.
220263
*/
221-
private class AspNetCoreRemoteFlowSourceMember extends TaintTracking::TaintedMember, Property {
264+
private class AspNetCoreRemoteFlowSourceMember extends TaintTracking::TaintedMember,
265+
CandidateMemberToTaint
266+
{
222267
AspNetCoreRemoteFlowSourceMember() {
223-
this.getDeclaringType() = any(AspNetCoreRemoteFlowSource source).getType() and
224-
this.isPublic() and
225-
not this.isStatic() and
226-
this.isAutoImplemented() and
227-
this.getGetter().isPublic() and
228-
this.getSetter().isPublic()
268+
exists(Type t, Type t0 | t = this.getDeclaringType() |
269+
(t = t0 or t = t0.(CollectionType).getElementType()) and
270+
(
271+
t0 = any(AspNetCoreRemoteFlowSourceMember m).getType()
272+
or
273+
t0 = any(AspNetCoreRemoteFlowSource m).getType()
274+
)
275+
)
229276
}
230277
}
231278

‎csharp/ql/test/library-tests/dataflow/flowsources/aspremote/AspRemoteFlowSource.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ namespace Testing
88
public class ViewModel
99
{
1010
public string RequestId { get; set; } // Considered tainted.
11-
public object RequestIdField; // Not considered tainted as it is a field.
11+
public object RequestIdField; // Considered tainted.
1212
public string RequestIdOnlyGet { get; } // Not considered tainted as there is no setter.
1313
public string RequestIdPrivateSet { get; private set; } // Not considered tainted as it has a private setter.
1414
public static object RequestIdStatic { get; set; } // Not considered tainted as it is static.

‎csharp/ql/test/library-tests/dataflow/flowsources/aspremote/aspRemoteFlowSource.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
remoteFlowSourceMembers
22
| AspRemoteFlowSource.cs:10:23:10:31 | RequestId |
3+
| AspRemoteFlowSource.cs:11:23:11:36 | RequestIdField |
34
| AspRemoteFlowSource.cs:28:23:28:29 | Tainted |
45
remoteFlowSources
56
| AspRemoteFlowSource.cs:20:42:20:50 | viewModel |

‎csharp/ql/test/library-tests/dataflow/flowsources/remote/RemoteFlowSource.cs‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,3 +54,59 @@ public static async void M3(System.Net.WebSockets.WebSocket webSocket)
5454
}
5555
}
5656
}
57+
58+
namespace AspRemoteFlowSource
59+
{
60+
using System.Web.Services;
61+
using System.Collections.Generic;
62+
63+
public class MySubData
64+
{
65+
public string SubDataProp { get; set; }
66+
}
67+
68+
public class ArrayElementData
69+
{
70+
public string ArrayElementDataProp { get; set; }
71+
}
72+
73+
public class ListElementData
74+
{
75+
public string ListElementDataProp { get; set; }
76+
}
77+
78+
public class MyData
79+
{
80+
public string DataField;
81+
public string DataProp { get; set; }
82+
public MySubData SubData { get; set; }
83+
public ArrayElementData[] Elements { get; set; }
84+
public List<ListElementData> List;
85+
}
86+
87+
public class MyDataElement
88+
{
89+
public string Prop { get; set; }
90+
}
91+
92+
93+
public class MyService
94+
{
95+
[WebMethod]
96+
public void MyMethod(MyData data)
97+
{
98+
Use(data.DataProp);
99+
Use(data.SubData.SubDataProp);
100+
Use(data.Elements[0].ArrayElementDataProp);
101+
Use(data.List[0].ListElementDataProp);
102+
}
103+
104+
[WebMethod]
105+
public void MyMethod2(MyDataElement[] data)
106+
{
107+
Use(data[0].Prop);
108+
}
109+
110+
public static void Use(object o) { }
111+
}
112+
}

‎csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.expected‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,15 @@
1+
remoteFlowSourceMembers
2+
| Controller.cs:6:19:6:25 | Tainted |
3+
| RemoteFlowSource.cs:65:23:65:33 | SubDataProp |
4+
| RemoteFlowSource.cs:70:23:70:42 | ArrayElementDataProp |
5+
| RemoteFlowSource.cs:75:23:75:41 | ListElementDataProp |
6+
| RemoteFlowSource.cs:80:23:80:31 | DataField |
7+
| RemoteFlowSource.cs:81:23:81:30 | DataProp |
8+
| RemoteFlowSource.cs:82:26:82:32 | SubData |
9+
| RemoteFlowSource.cs:83:35:83:42 | Elements |
10+
| RemoteFlowSource.cs:84:38:84:41 | List |
11+
| RemoteFlowSource.cs:89:23:89:26 | Prop |
12+
remoteFlowSources
113
| Controller.cs:11:43:11:52 | sampleData | ASP.NET MVC action method parameter |
214
| Controller.cs:11:62:11:66 | taint | ASP.NET MVC action method parameter |
315
| Controller.cs:16:43:16:52 | sampleData | ASP.NET MVC action method parameter |
@@ -10,3 +22,5 @@
1022
| RemoteFlowSource.cs:45:17:45:23 | access to parameter request | ASP.NET query string |
1123
| RemoteFlowSource.cs:45:17:45:42 | access to property RawUrl | ASP.NET unvalidated request data |
1224
| RemoteFlowSource.cs:52:55:52:61 | [post] access to local variable segment | external |
25+
| RemoteFlowSource.cs:96:37:96:40 | data | ASP.NET web service input |
26+
| RemoteFlowSource.cs:105:47:105:50 | data | ASP.NET web service input |

‎csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.ql‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import semmle.code.csharp.security.dataflow.flowsources.Remote
22

3-
from RemoteFlowSource source
4-
where source.getLocation().getFile().fromSource()
5-
select source, source.getSourceType()
3+
query predicate remoteFlowSourceMembers(TaintTracking::TaintedMember m) { m.fromSource() }
4+
5+
query predicate remoteFlowSources(RemoteFlowSource source, string type) {
6+
source.getLocation().getFile().fromSource() and type = source.getSourceType()
7+
}

‎csharp/ql/test/resources/stubs/System.Web.cs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -454,3 +454,8 @@ public class SimpleTypeResolver : System.Web.Script.Serialization.JavaScriptType
454454
public SimpleTypeResolver() => throw null;
455455
}
456456
}
457+
458+
namespace System.Web.Services
459+
{
460+
public class WebMethodAttribute : Attribute { }
461+
}

0 commit comments

Comments
 (0)