Skip to content

Commit 9e3707f

Browse files
michaelnebelmichaelnebel
authored
Merge pull request #22384 from hugo-syn/hugo-syn/csharp-odata-tainted-member
csharp: odata lib
2 parents fa8a287 + 8519b91 commit 9e3707f

11 files changed

Lines changed: 420 additions & 1 deletion

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/ext/Microsoft.AspNet.OData.model.yml‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
extensions:
2+
- addsTo:
3+
pack: codeql/csharp-all
4+
extensible: summaryModel
5+
data:
6+
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "GetInstance", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
7+
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "Patch", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
8+
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "Put", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
9+
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "CopyChangedValues", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
10+
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "CopyUnchangedValues", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
11+
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "GetInstance", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
12+
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "Patch", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
13+
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "Put", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
14+
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "CopyChangedValues", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
15+
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "CopyUnchangedValues", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
16+
- ["System.Web.Http.OData", "Delta<TEntityType>", True, "GetEntity", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
17+
- ["System.Web.Http.OData", "Delta<TEntityType>", True, "Patch", "(TEntityType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
18+
- ["System.Web.Http.OData", "Delta<TEntityType>", True, "Put", "(TEntityType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
19+
- ["System.Web.Http.OData", "Delta<TEntityType>", True, "CopyChangedValues", "(TEntityType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
20+
- ["System.Web.Http.OData", "Delta<TEntityType>", True, "CopyUnchangedValues", "(TEntityType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]

‎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: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
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()` (`GetEntity()` in
13+
* the older `System.Web.Http.OData`) or copied onto an existing entity via
14+
* `Patch`/`Put`/`CopyChangedValues`/`CopyUnchangedValues`.
15+
*
16+
* In both cases the type that ends up holding the client-controlled data has
17+
* no static relationship to the action method's parameter types, so its
18+
* members need to be taint-tracked explicitly.
19+
*/
20+
21+
import csharp
22+
private import semmle.code.csharp.commons.Collections
23+
private import semmle.code.csharp.security.dataflow.flowsources.Remote
24+
25+
/** The `ODataActionParameters` dictionary type, across OData library versions. */
26+
class ODataActionParametersClass extends Class {
27+
ODataActionParametersClass() {
28+
this.hasFullyQualifiedName("Microsoft.AspNet.OData", "ODataActionParameters") or
29+
this.hasFullyQualifiedName("Microsoft.AspNetCore.OData.Formatter", "ODataActionParameters") or
30+
this.hasFullyQualifiedName("System.Web.Http.OData", "ODataActionParameters")
31+
}
32+
}
33+
34+
/**
35+
* Holds if `e` is (or, via local flow -- e.g. an upcast to `IDictionary<string, object>`
36+
* -- may hold the value of) an `ODataActionParameters` dictionary.
37+
*/
38+
private predicate isODataActionParametersValue(Expr e) {
39+
exists(ParameterAccess e0 | e0.getType() instanceof ODataActionParametersClass |
40+
e0 = e or DataFlow::localExprFlow(e0, e)
41+
)
42+
}
43+
44+
/**
45+
* An indexer read on an `ODataActionParameters` dictionary, e.g. `parameters["Foo"]`
46+
* (including through an upcast to a base dictionary type/interface).
47+
*/
48+
class ODataActionParameterRead extends ElementAccess {
49+
ODataActionParameterRead() { isODataActionParametersValue(this.getQualifier()) }
50+
}
51+
52+
/** Holds if `e` may (locally) hold the value of an `ODataActionParameters` entry. */
53+
private predicate isODataParameterValue(Expr e) {
54+
DataFlow::localExprFlow(any(ODataActionParameterRead r), e)
55+
}
56+
57+
/** The generic ``Delta`1`` change-tracking class, across OData library versions. */
58+
class DeltaClass extends UnboundGenericClass {
59+
DeltaClass() {
60+
this.getNumberOfTypeParameters() = 1 and
61+
(
62+
this.hasFullyQualifiedName("Microsoft.AspNet.OData", "Delta`1") or
63+
this.hasFullyQualifiedName("Microsoft.AspNetCore.OData.Deltas", "Delta`1") or
64+
this.hasFullyQualifiedName("System.Web.Http.OData", "Delta`1")
65+
)
66+
}
67+
}
68+
69+
/**
70+
* A type that a value read out of `ODataActionParameters` is cast, `as`-converted,
71+
* or type-tested to -- directly, or wrapped in a collection (`List<T>`,
72+
* `IEnumerable<T>`, arrays, ...) -- or a type that is tracked by a `Delta<T>`.
73+
*/
74+
class ODataBoundType extends ValueOrRefType {
75+
ODataBoundType() {
76+
exists(Cast c | isODataParameterValue(c.getExpr()) |
77+
this = c.getTargetType() or
78+
this = c.getTargetType().(CollectionType).getElementType() or
79+
this = c.getTargetType().(ParamsCollectionType).getElementType()
80+
)
81+
or
82+
exists(IsExpr ie, Type t |
83+
isODataParameterValue(ie.getExpr()) and
84+
t = ie.getPattern().(TypePatternExpr).getCheckedType()
85+
|
86+
this = t or
87+
this = t.(CollectionType).getElementType() or
88+
this = t.(ParamsCollectionType).getElementType()
89+
)
90+
or
91+
this = any(ConstructedClass c | c.getUnboundGeneric() instanceof DeltaClass).getTypeArgument(0)
92+
}
93+
}
94+
95+
/**
96+
* Taint members (transitively) on types used in
97+
* 1. Casts, `as`-conversions, or type tests applied to `ODataActionParameters` values.
98+
* 2. The type argument of a `Delta<T>`.
99+
*
100+
* Note that this also impacts uses of such types in other contexts, the same
101+
* trade-off `AspNetRemoteFlowSourceMember` (`Remote.qll`) makes for ASP.NET
102+
* action-method parameters.
103+
*/
104+
private class ODataBoundMember extends TaintTracking::TaintedMember, CandidateMemberToTaint {
105+
ODataBoundMember() {
106+
exists(Type t, Type t0 | t = this.getDeclaringType() |
107+
(t = t0 or t = t0.(CollectionType).getElementType()) and
108+
(
109+
t0 = any(ODataBoundMember m).getType()
110+
or
111+
t0 instanceof ODataBoundType
112+
)
113+
)
114+
}
115+
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,8 @@ class AspNetServiceRemoteFlowSource extends AspNetRemoteFlowSource, DataFlow::Pa
117117
override string getSourceType() { result = "ASP.NET web service input" }
118118
}
119119

120-
private class CandidateMemberToTaint extends Member {
120+
/** A public, non-static, auto-implemented property or field, candidate for taint-tracking. */
121+
class CandidateMemberToTaint extends Member {
121122
CandidateMemberToTaint() {
122123
this.isPublic() and
123124
not this.isStatic() and

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

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

0 commit comments

Comments
 (0)