Skip to content

Commit a1f2b81

Browse files
hugo-synclaude
hugo-syn
and
claude
committed
C#: add OData action-parameter taint modeling
Adds semmle.code.csharp.frameworks.OData, following the WCF.qll/JsonNET.qll convention: values cast, as-converted, or type-tested out of an untyped ODataActionParameters dictionary, and entities tracked by Delta<T> (via GetInstance/Patch/Put/CopyChangedValues/CopyUnchangedValues), have no static type relationship to the action method's own parameter types, so their members aren't picked up by the existing AspNetRemoteFlowSourceMember modeling. This adds a TaintedMember for those bound types (with the same nested-type/collection recursion as AspNetRemoteFlowSourceMember), plus two AdditionalTaintStep steps for the Delta<T> method calls, which don't fit the member-read shape TaintedMember covers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 01755ec commit a1f2b81

6 files changed

Lines changed: 321 additions & 0 deletions

File tree

‎csharp/ql/lib/change-notes/2026-08-19-odata-taint-step.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: feature
3+
---
4+
* Added taint modeling for OData action parameter binding (`Microsoft.AspNet.OData`/`Microsoft.AspNetCore.OData`). Values cast, `as`-converted, or type-tested out of `ODataActionParameters`, and entities tracked by `Delta<T>` (via `GetInstance`, `Patch`, `Put`, `CopyChangedValues`, and `CopyUnchangedValues`), now taint the members of the target type.

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ private import semmle.code.csharp.dispatch.Dispatch
99
private import semmle.code.csharp.commons.ComparisonTest
1010
// import `TaintedMember` definitions from other files to avoid potential reevaluation
1111
private import semmle.code.csharp.frameworks.JsonNET
12+
private import semmle.code.csharp.frameworks.OData
1213
private import semmle.code.csharp.frameworks.WCF
1314
private import semmle.code.csharp.security.dataflow.flowsources.Remote
1415

‎csharp/ql/lib/semmle/code/csharp/frameworks/OData.qll‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
/**
2+
* Provides taint modeling for `Microsoft.AspNet.OData`/`Microsoft.AspNetCore.OData`
3+
* (and the older `System.Web.Http.OData`) OData action parameter binding.
4+
*
5+
* OData actions receive their untrusted payload in one of two shapes that
6+
* bypass the usual "type used as an action-method parameter" taint modeling:
7+
*
8+
* - `ODataActionParameters`, an untyped `Dictionary<string, object>` whose
9+
* values are cast, `as`-converted, or type-tested to arbitrary model types
10+
* by the action method body.
11+
* - `Delta<T>`, a change-tracking wrapper for PATCH/PUT requests, whose
12+
* tracked property values are exposed via `GetInstance()` or copied onto an
13+
* existing entity via `Patch`/`Put`/`CopyChangedValues`/`CopyUnchangedValues`.
14+
*
15+
* In both cases the type that ends up holding the client-controlled data has
16+
* no static relationship to the action method's parameter types, so its
17+
* members need to be taint-tracked explicitly.
18+
*/
19+
20+
private import csharp
21+
private import semmle.code.csharp.commons.Collections
22+
private import semmle.code.csharp.dataflow.FlowSteps
23+
private import semmle.code.csharp.dataflow.TaintTracking
24+
private import semmle.code.csharp.dataflow.internal.DataFlowPrivate
25+
26+
/** The `ODataActionParameters` dictionary type, across OData library versions. */
27+
private class ODataActionParametersClass extends Class {
28+
ODataActionParametersClass() {
29+
this.hasFullyQualifiedName("Microsoft.AspNet.OData", "ODataActionParameters") or
30+
this.hasFullyQualifiedName("Microsoft.AspNetCore.OData.Formatter", "ODataActionParameters") or
31+
this.hasFullyQualifiedName("System.Web.Http.OData", "ODataActionParameters")
32+
}
33+
}
34+
35+
/** An indexer read on an `ODataActionParameters` dictionary, e.g. `parameters["CabFile"]`. */
36+
private class ODataActionParameterRead extends ElementAccess {
37+
ODataActionParameterRead() { this.getQualifier().getType() instanceof ODataActionParametersClass }
38+
}
39+
40+
/** Holds if `e` may (locally) hold the value of an `ODataActionParameters` entry. */
41+
private predicate isODataParameterValue(Expr e) {
42+
TaintTracking::localExprTaint(any(ODataActionParameterRead r), e)
43+
}
44+
45+
/** The generic `Delta<TStructuralType>` change-tracking class, across OData library versions. */
46+
private class DeltaClass extends UnboundGenericClass {
47+
DeltaClass() {
48+
this.getNumberOfTypeParameters() = 1 and
49+
(
50+
this.hasFullyQualifiedName("Microsoft.AspNet.OData", "Delta`1") or
51+
this.hasFullyQualifiedName("Microsoft.AspNetCore.OData.Deltas", "Delta`1")
52+
)
53+
}
54+
}
55+
56+
/**
57+
* A type that a value read out of `ODataActionParameters` is cast, `as`-converted,
58+
* or type-tested to -- directly, or wrapped in a collection (`List<T>`,
59+
* `IEnumerable<T>`, arrays, ...) -- or a type that is tracked by a `Delta<T>`.
60+
*/
61+
private class ODataBoundType extends ValueOrRefType {
62+
ODataBoundType() {
63+
exists(Cast c | isODataParameterValue(c.getExpr()) |
64+
this = c.getTargetType() or
65+
this = c.getTargetType().(CollectionType).getElementType() or
66+
this = c.getTargetType().(ParamsCollectionType).getElementType()
67+
)
68+
or
69+
exists(IsExpr ie, Type t |
70+
isODataParameterValue(ie.getExpr()) and
71+
t = ie.getPattern().(TypePatternExpr).getCheckedType()
72+
|
73+
this = t or
74+
this = t.(CollectionType).getElementType() or
75+
this = t.(ParamsCollectionType).getElementType()
76+
)
77+
or
78+
this = any(ConstructedClass c | c.getUnboundGeneric() instanceof DeltaClass).getTypeArgument(0)
79+
}
80+
}
81+
82+
private class CandidateODataMember extends Member {
83+
CandidateODataMember() {
84+
this.isPublic() and
85+
not this.isStatic() and
86+
(
87+
this =
88+
any(Property p |
89+
p.isAutoImplemented() and
90+
p.getGetter().isPublic() and
91+
p.getSetter().isPublic()
92+
)
93+
or
94+
this = any(Field f | f.isPublic())
95+
)
96+
}
97+
}
98+
99+
/**
100+
* Taint members (transitively) on types used in
101+
* 1. Casts, `as`-conversions, or type tests applied to `ODataActionParameters` values.
102+
* 2. The type argument of a `Delta<T>`.
103+
*
104+
* Note that this also impacts uses of such types in other contexts, the same
105+
* trade-off `AspNetRemoteFlowSourceMember` (`Remote.qll`) makes for ASP.NET
106+
* action-method parameters.
107+
*/
108+
private class ODataBoundMember extends TaintTracking::TaintedMember, CandidateODataMember {
109+
ODataBoundMember() {
110+
exists(Type t, Type t0 | t = this.getDeclaringType() |
111+
(t = t0 or t = t0.(CollectionType).getElementType()) and
112+
(
113+
t0 = any(ODataBoundMember m).getType()
114+
or
115+
t0 instanceof ODataBoundType
116+
)
117+
)
118+
}
119+
}
120+
121+
/** The `Patch`, `Put`, `CopyChangedValues`, and `CopyUnchangedValues` methods on `Delta<T>`. */
122+
private class DeltaMutatingMethod extends Method {
123+
DeltaMutatingMethod() {
124+
this.getDeclaringType() instanceof DeltaClass and
125+
this.hasName(["Patch", "Put", "CopyChangedValues", "CopyUnchangedValues"])
126+
}
127+
}
128+
129+
/**
130+
* A call to `Delta<T>.Patch`/`Put`/`CopyChangedValues`/`CopyUnchangedValues`
131+
* copies the changes tracked by the `Delta<T>` receiver onto its `original`
132+
* entity argument.
133+
*/
134+
private class DeltaMutatingCallTaintStep extends AdditionalTaintStep {
135+
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
136+
exists(MethodCall mc |
137+
mc.getTarget().getUnboundDeclaration() instanceof DeltaMutatingMethod and
138+
node1.asExpr() = mc.getQualifier() and
139+
node2.(PostUpdateNode).getPreUpdateNode().asExpr() = mc.getArgument(0)
140+
)
141+
}
142+
}
143+
144+
/** The `GetInstance` method on `Delta<T>`. */
145+
private class DeltaGetInstanceMethod extends Method {
146+
DeltaGetInstanceMethod() {
147+
this.getDeclaringType() instanceof DeltaClass and
148+
this.hasName("GetInstance")
149+
}
150+
}
151+
152+
/** `Delta<T>.GetInstance()` returns the tracked entity, carrying the same taint as the `Delta<T>` itself. */
153+
private class DeltaGetInstanceTaintStep extends AdditionalTaintStep {
154+
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
155+
exists(MethodCall mc |
156+
mc.getTarget().getUnboundDeclaration() instanceof DeltaGetInstanceMethod and
157+
node1.asExpr() = mc.getQualifier() and
158+
node2.asExpr() = mc
159+
)
160+
}
161+
}

‎csharp/ql/test/library-tests/frameworks/OData/OData.cs‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
using System.Collections.Generic;
2+
3+
namespace Microsoft.AspNet.OData
4+
{
5+
public class ODataActionParameters : Dictionary<string, object>
6+
{
7+
}
8+
9+
public class Delta<TStructuralType> where TStructuralType : class
10+
{
11+
private TStructuralType instance;
12+
13+
public Delta() { instance = default(TStructuralType); }
14+
15+
public TStructuralType GetInstance() => instance;
16+
17+
public void Patch(TStructuralType original) { }
18+
19+
public void Put(TStructuralType original) { }
20+
21+
public void CopyChangedValues(TStructuralType original) { }
22+
23+
public void CopyUnchangedValues(TStructuralType original) { }
24+
}
25+
}
26+
27+
namespace Test
28+
{
29+
using Microsoft.AspNet.OData;
30+
using System.Collections.Generic;
31+
32+
public class FileMetadata
33+
{
34+
public string Author { get; set; }
35+
}
36+
37+
public class UploadedFile
38+
{
39+
public string FileName { get; set; }
40+
41+
public string FileContent { get; set; }
42+
43+
public FileMetadata Metadata { get; set; }
44+
45+
public List<FileMetadata> History { get; set; }
46+
}
47+
48+
public class SubscriptionRelation
49+
{
50+
public string EventName { get; set; }
51+
52+
public string EventType { get; set; }
53+
}
54+
55+
public class Widget
56+
{
57+
public string Name { get; set; }
58+
}
59+
60+
public class UnrelatedType
61+
{
62+
// Never reached via an ODataActionParameters/Delta<T> cast, so this
63+
// member must stay untainted even though `UnrelatedType` itself is
64+
// used elsewhere in the file.
65+
public string Name { get; set; }
66+
}
67+
68+
public class OrderController
69+
{
70+
void Sink(object o) { }
71+
72+
void CastFromDictionary(ODataActionParameters parameters)
73+
{
74+
var file = (UploadedFile)parameters["CabFile"];
75+
Sink(file);
76+
Sink(file.FileName);
77+
Sink(file.FileContent);
78+
Sink(file.Metadata.Author);
79+
foreach (var m in file.History)
80+
{
81+
Sink(m.Author);
82+
}
83+
}
84+
85+
void IsAsFromDictionary(ODataActionParameters parameters)
86+
{
87+
if (parameters["NewEvents"] is IEnumerable<SubscriptionRelation> relations1)
88+
{
89+
foreach (var item in relations1)
90+
{
91+
Sink(item.EventName);
92+
}
93+
}
94+
95+
var relations2 = parameters["NewEvents"] as IEnumerable<SubscriptionRelation>;
96+
foreach (var item in relations2)
97+
{
98+
Sink(item.EventType);
99+
}
100+
}
101+
102+
void DeltaPatch(Delta<Widget> delta, Widget original)
103+
{
104+
delta.Patch(original);
105+
Sink(original.Name);
106+
}
107+
108+
void DeltaGetInstance(Delta<Widget> delta)
109+
{
110+
var w = delta.GetInstance();
111+
Sink(w.Name);
112+
}
113+
114+
void Untainted()
115+
{
116+
var w = new Widget();
117+
w.Name = "safe";
118+
Sink(w.Name);
119+
120+
var u = new UnrelatedType();
121+
u.Name = "also safe";
122+
Sink(u.Name);
123+
}
124+
}
125+
}

‎csharp/ql/test/library-tests/frameworks/OData/OData.expected‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
| OData.cs:72:55:72:64 | parameters | OData.cs:75:18:75:21 | access to local variable file |
2+
| OData.cs:72:55:72:64 | parameters | OData.cs:76:18:76:30 | access to property FileName |
3+
| OData.cs:72:55:72:64 | parameters | OData.cs:77:18:77:33 | access to property FileContent |
4+
| OData.cs:72:55:72:64 | parameters | OData.cs:78:18:78:37 | access to property Author |
5+
| OData.cs:72:55:72:64 | parameters | OData.cs:81:22:81:29 | access to property Author |
6+
| OData.cs:85:55:85:64 | parameters | OData.cs:91:26:91:39 | access to property EventName |
7+
| OData.cs:85:55:85:64 | parameters | OData.cs:98:22:98:35 | access to property EventType |
8+
| OData.cs:102:39:102:43 | delta | OData.cs:105:18:105:30 | access to property Name |
9+
| OData.cs:108:45:108:49 | delta | OData.cs:111:18:111:23 | access to property Name |

‎csharp/ql/test/library-tests/frameworks/OData/OData.ql‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
import csharp
2+
3+
module TaintConfig implements DataFlow::ConfigSig {
4+
predicate isSource(DataFlow::Node n) {
5+
exists(Parameter p | p = n.asParameter() |
6+
p.getType().hasFullyQualifiedName("Microsoft.AspNet.OData", "ODataActionParameters")
7+
or
8+
p.getType().getUnboundDeclaration().hasFullyQualifiedName("Microsoft.AspNet.OData", "Delta`1")
9+
)
10+
}
11+
12+
predicate isSink(DataFlow::Node sink) {
13+
exists(MethodCall c | c.getArgument(0) = sink.asExpr() and c.getTarget().hasName("Sink"))
14+
}
15+
}
16+
17+
module Taint = TaintTracking::Global<TaintConfig>;
18+
19+
from DataFlow::Node source, DataFlow::Node sink
20+
where Taint::flow(source, sink)
21+
select source, sink

0 commit comments

Comments
 (0)