Skip to content

Commit 93baa40

Browse files
author
hugo-syn
committed
fix: add fix from review
1 parent de9eae2 commit 93baa40

8 files changed

Lines changed: 113 additions & 13 deletions

File tree

‎csharp/ql/lib/ext/Microsoft.AspNet.OData.model.yml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,19 @@ extensions:
55
data:
66
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "GetInstance", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
77
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "Patch", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
8+
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "Patch", "(TStructuralType)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
89
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "Put", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
910
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "CopyChangedValues", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
11+
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "CopyChangedValues", "(TStructuralType)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
1012
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "CopyUnchangedValues", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
1113
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "GetInstance", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
1214
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "Patch", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
15+
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "Patch", "(T)", "", "Argument[this]", "ReturnValue", "taint", "manual"]
1316
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "Put", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
1417
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "CopyChangedValues", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
1518
- ["Microsoft.AspNetCore.OData.Deltas", "Delta<T>", True, "CopyUnchangedValues", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
19+
- ["System.Web.Http.OData", "Delta<TEntityType>", True, "GetEntity", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
20+
- ["System.Web.Http.OData", "Delta<TEntityType>", True, "Patch", "(TEntityType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
21+
- ["System.Web.Http.OData", "Delta<TEntityType>", True, "Put", "(TEntityType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
22+
- ["System.Web.Http.OData", "Delta<TEntityType>", True, "CopyChangedValues", "(TEntityType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]
23+
- ["System.Web.Http.OData", "Delta<TEntityType>", True, "CopyUnchangedValues", "(TEntityType)", "", "Argument[this]", "Argument[0]", "taint", "manual"]

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

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,9 @@
99
* values are cast, `as`-converted, or type-tested to arbitrary model types
1010
* by the action method body.
1111
* - `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`.
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`.
1415
*
1516
* In both cases the type that ends up holding the client-controlled data has
1617
* no static relationship to the action method's parameter types, so its
@@ -20,7 +21,6 @@
2021
import csharp
2122
private import semmle.code.csharp.commons.Collections
2223
private import semmle.code.csharp.dataflow.FlowSteps
23-
private import semmle.code.csharp.dataflow.internal.DataFlowPrivate
2424
private import semmle.code.csharp.security.dataflow.flowsources.Remote
2525

2626
/** The `ODataActionParameters` dictionary type, across OData library versions. */
@@ -32,14 +32,43 @@ class ODataActionParametersClass extends Class {
3232
}
3333
}
3434

35-
/** An indexer read on an `ODataActionParameters` dictionary, e.g. `parameters["Foo"]`. */
35+
/**
36+
* Holds if `e` is (or, via local flow -- e.g. an upcast to `IDictionary<string, object>`
37+
* -- may hold the value of) an `ODataActionParameters` dictionary.
38+
*/
39+
private predicate isODataActionParametersValue(Expr e) {
40+
e.getType() instanceof ODataActionParametersClass
41+
or
42+
DataFlow::localExprFlow(any(Expr e0 | isODataActionParametersValue(e0)), e)
43+
}
44+
45+
/**
46+
* An indexer read on an `ODataActionParameters` dictionary, e.g. `parameters["Foo"]`
47+
* (including through an upcast to a base dictionary type/interface).
48+
*/
3649
class ODataActionParameterRead extends ElementAccess {
37-
ODataActionParameterRead() { this.getQualifier().getType() instanceof ODataActionParametersClass }
50+
ODataActionParameterRead() { isODataActionParametersValue(this.getQualifier()) }
51+
}
52+
53+
/**
54+
* A call to `TryGetValue` on an `ODataActionParameters` dictionary copies the value
55+
* of the looked-up entry into the `out` argument.
56+
*/
57+
private class ODataActionParametersTryGetValueTaintStep extends AdditionalTaintStep {
58+
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
59+
exists(MethodCall mc, AssignableDefinitions::OutRefDefinition def |
60+
mc.getTarget().hasName("TryGetValue") and
61+
isODataActionParametersValue(mc.getQualifier()) and
62+
node1.asExpr() = mc.getQualifier() and
63+
def.getTargetAccess() = mc.getArgumentForName("value") and
64+
node2 = DataFlow::assignableDefinitionNode(def)
65+
)
66+
}
3867
}
3968

4069
/** Holds if `e` may (locally) hold the value of an `ODataActionParameters` entry. */
4170
private predicate isODataParameterValue(Expr e) {
42-
TaintTracking::localExprTaint(any(ODataActionParameterRead r), e)
71+
DataFlow::localExprFlow(any(ODataActionParameterRead r), e)
4372
}
4473

4574
/** The generic ``Delta`1`` change-tracking class, across OData library versions. */
@@ -48,7 +77,8 @@ class DeltaClass extends UnboundGenericClass {
4877
this.getNumberOfTypeParameters() = 1 and
4978
(
5079
this.hasFullyQualifiedName("Microsoft.AspNet.OData", "Delta`1") or
51-
this.hasFullyQualifiedName("Microsoft.AspNetCore.OData.Deltas", "Delta`1")
80+
this.hasFullyQualifiedName("Microsoft.AspNetCore.OData.Deltas", "Delta`1") or
81+
this.hasFullyQualifiedName("System.Web.Http.OData", "Delta`1")
5282
)
5383
}
5484
}

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,22 @@ void IsAsFromDictionary(ODataActionParameters parameters)
7373
}
7474
}
7575

76+
void TryGetValueFromDictionary(ODataActionParameters parameters)
77+
{
78+
if (parameters.TryGetValue("Entity", out var value))
79+
{
80+
var entity = (BoundEntity)value;
81+
Sink(entity.Name);
82+
}
83+
}
84+
85+
void UpcastThenIndex(ODataActionParameters parameters)
86+
{
87+
IDictionary<string, object> dict = parameters;
88+
var entity = (BoundEntity)dict["Entity"];
89+
Sink(entity.Name);
90+
}
91+
7692
void DeltaPatch(Delta<Widget> delta, Widget original)
7793
{
7894
delta.Patch(original);
@@ -85,6 +101,30 @@ void DeltaGetInstance(Delta<Widget> delta)
85101
Sink(w.Name);
86102
}
87103

104+
void DeltaPatchReturnValue(Delta<Widget> delta, Widget original)
105+
{
106+
var updated = delta.Patch(original);
107+
Sink(updated.Name);
108+
}
109+
110+
void DeltaCopyChangedValuesReturnValue(Delta<Widget> delta, Widget original)
111+
{
112+
var updated = delta.CopyChangedValues(original);
113+
Sink(updated.Name);
114+
}
115+
116+
void LegacyDeltaPatch(System.Web.Http.OData.Delta<Widget> delta, Widget original)
117+
{
118+
delta.Patch(original);
119+
Sink(original.Name);
120+
}
121+
122+
void LegacyDeltaGetEntity(System.Web.Http.OData.Delta<Widget> delta)
123+
{
124+
var w = delta.GetEntity();
125+
Sink(w.Name);
126+
}
127+
88128
void Untainted()
89129
{
90130
var w = new Widget();

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,5 +5,11 @@
55
| OData.cs:46:55:46:64 | parameters | OData.cs:55:22:55:28 | access to property Owner |
66
| OData.cs:59:55:59:64 | parameters | OData.cs:65:26:65:35 | access to property Label |
77
| OData.cs:59:55:59:64 | parameters | OData.cs:72:22:72:34 | access to property Category |
8-
| OData.cs:76:39:76:43 | delta | OData.cs:79:18:79:30 | access to property Name |
9-
| OData.cs:82:45:82:49 | delta | OData.cs:85:18:85:23 | access to property Name |
8+
| OData.cs:76:62:76:71 | parameters | OData.cs:81:22:81:32 | access to property Name |
9+
| OData.cs:85:52:85:61 | parameters | OData.cs:89:18:89:28 | access to property Name |
10+
| OData.cs:92:39:92:43 | delta | OData.cs:95:18:95:30 | access to property Name |
11+
| OData.cs:98:45:98:49 | delta | OData.cs:101:18:101:23 | access to property Name |
12+
| OData.cs:104:50:104:54 | delta | OData.cs:107:18:107:29 | access to property Name |
13+
| OData.cs:110:62:110:66 | delta | OData.cs:113:18:113:29 | access to property Name |
14+
| OData.cs:116:67:116:71 | delta | OData.cs:119:18:119:30 | access to property Name |
15+
| OData.cs:122:71:122:75 | delta | OData.cs:125:18:125:23 | access to property Name |

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ module TaintConfig implements DataFlow::ConfigSig {
66
p.getType().hasFullyQualifiedName("Microsoft.AspNet.OData", "ODataActionParameters")
77
or
88
p.getType().getUnboundDeclaration().hasFullyQualifiedName("Microsoft.AspNet.OData", "Delta`1")
9+
or
10+
p.getType().getUnboundDeclaration().hasFullyQualifiedName("System.Web.Http.OData", "Delta`1")
911
)
1012
}
1113

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,3 @@
11
semmle-extractor-options: /nostdlib /noconfig
22
semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resources/stubs/Microsoft.AspNet.OData/7.7.5/Microsoft.AspNet.OData.csproj
3+
semmle-extractor-options: ${testdir}/../../../resources/stubs/System.Web.Http.OData.cs

‎csharp/ql/test/resources/stubs/Microsoft.AspNet.OData/7.7.5/Microsoft.AspNet.OData.cs‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
// This file contains auto-generated code.
2-
// Generated from `Microsoft.AspNet.OData, Version=7.7.5.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35`.
31
namespace Microsoft.AspNet.OData
42
{
53
public class ODataActionParameters : System.Collections.Generic.Dictionary<string, object>
@@ -11,9 +9,9 @@ public class Delta<TStructuralType> where TStructuralType : class
119
{
1210
public Delta() => throw null;
1311
public TStructuralType GetInstance() => throw null;
14-
public void Patch(TStructuralType original) => throw null;
12+
public TStructuralType Patch(TStructuralType original) => throw null;
1513
public void Put(TStructuralType original) => throw null;
16-
public void CopyChangedValues(TStructuralType original) => throw null;
14+
public TStructuralType CopyChangedValues(TStructuralType original) => throw null;
1715
public void CopyUnchangedValues(TStructuralType original) => throw null;
1816
}
1917
}

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
namespace System.Web.Http.OData
2+
{
3+
public class ODataActionParameters : System.Collections.Generic.Dictionary<string, object>
4+
{
5+
}
6+
7+
public class Delta<TEntityType> where TEntityType : class
8+
{
9+
public TEntityType GetEntity() => throw null;
10+
public void Patch(TEntityType original) { }
11+
public void Put(TEntityType original) { }
12+
public void CopyChangedValues(TEntityType original) { }
13+
public void CopyUnchangedValues(TEntityType original) { }
14+
}
15+
}

0 commit comments

Comments
 (0)