diff --git a/csharp/ql/lib/change-notes/2026-08-19-odata-taint-step.md b/csharp/ql/lib/change-notes/2026-08-19-odata-taint-step.md new file mode 100644 index 000000000000..3d0a6cbeb8ae --- /dev/null +++ b/csharp/ql/lib/change-notes/2026-08-19-odata-taint-step.md @@ -0,0 +1,4 @@ +--- +category: feature +--- +* 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` (via `GetInstance`, `Patch`, `Put`, `CopyChangedValues`, and `CopyUnchangedValues`), now taint the members of the target type. diff --git a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll index 238ecab13461..418573cae8cc 100644 --- a/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll +++ b/csharp/ql/lib/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll @@ -9,6 +9,7 @@ private import semmle.code.csharp.dispatch.Dispatch private import semmle.code.csharp.commons.ComparisonTest // import `TaintedMember` definitions from other files to avoid potential reevaluation private import semmle.code.csharp.frameworks.JsonNET +private import semmle.code.csharp.frameworks.OData private import semmle.code.csharp.frameworks.WCF private import semmle.code.csharp.security.dataflow.flowsources.Remote diff --git a/csharp/ql/lib/semmle/code/csharp/frameworks/OData.qll b/csharp/ql/lib/semmle/code/csharp/frameworks/OData.qll new file mode 100644 index 000000000000..80917322f441 --- /dev/null +++ b/csharp/ql/lib/semmle/code/csharp/frameworks/OData.qll @@ -0,0 +1,160 @@ +/** + * Provides taint modeling for `Microsoft.AspNet.OData`/`Microsoft.AspNetCore.OData` + * (and the older `System.Web.Http.OData`) OData action parameter binding. + * + * OData actions receive their untrusted payload in one of two shapes that + * bypass the usual "type used as an action-method parameter" taint modeling: + * + * - `ODataActionParameters`, an untyped `Dictionary` whose + * values are cast, `as`-converted, or type-tested to arbitrary model types + * by the action method body. + * - `Delta`, a change-tracking wrapper for PATCH/PUT requests, whose + * tracked property values are exposed via `GetInstance()` or copied onto an + * existing entity via `Patch`/`Put`/`CopyChangedValues`/`CopyUnchangedValues`. + * + * In both cases the type that ends up holding the client-controlled data has + * no static relationship to the action method's parameter types, so its + * members need to be taint-tracked explicitly. + */ + +import csharp +private import semmle.code.csharp.commons.Collections +private import semmle.code.csharp.dataflow.FlowSteps +private import semmle.code.csharp.dataflow.internal.DataFlowPrivate + +/** The `ODataActionParameters` dictionary type, across OData library versions. */ +class ODataActionParametersClass extends Class { + ODataActionParametersClass() { + this.hasFullyQualifiedName("Microsoft.AspNet.OData", "ODataActionParameters") or + this.hasFullyQualifiedName("Microsoft.AspNetCore.OData.Formatter", "ODataActionParameters") or + this.hasFullyQualifiedName("System.Web.Http.OData", "ODataActionParameters") + } +} + +/** An indexer read on an `ODataActionParameters` dictionary, e.g. `parameters["Foo"]`. */ +class ODataActionParameterRead extends ElementAccess { + ODataActionParameterRead() { this.getQualifier().getType() instanceof ODataActionParametersClass } +} + +/** Holds if `e` may (locally) hold the value of an `ODataActionParameters` entry. */ +private predicate isODataParameterValue(Expr e) { + TaintTracking::localExprTaint(any(ODataActionParameterRead r), e) +} + +/** The generic `Delta` change-tracking class, across OData library versions. */ +class DeltaClass extends UnboundGenericClass { + DeltaClass() { + this.getNumberOfTypeParameters() = 1 and + ( + this.hasFullyQualifiedName("Microsoft.AspNet.OData", "Delta`1") or + this.hasFullyQualifiedName("Microsoft.AspNetCore.OData.Deltas", "Delta`1") + ) + } +} + +/** + * A type that a value read out of `ODataActionParameters` is cast, `as`-converted, + * or type-tested to -- directly, or wrapped in a collection (`List`, + * `IEnumerable`, arrays, ...) -- or a type that is tracked by a `Delta`. + */ +class ODataBoundType extends ValueOrRefType { + ODataBoundType() { + exists(Cast c | isODataParameterValue(c.getExpr()) | + this = c.getTargetType() or + this = c.getTargetType().(CollectionType).getElementType() or + this = c.getTargetType().(ParamsCollectionType).getElementType() + ) + or + exists(IsExpr ie, Type t | + isODataParameterValue(ie.getExpr()) and + t = ie.getPattern().(TypePatternExpr).getCheckedType() + | + this = t or + this = t.(CollectionType).getElementType() or + this = t.(ParamsCollectionType).getElementType() + ) + or + this = any(ConstructedClass c | c.getUnboundGeneric() instanceof DeltaClass).getTypeArgument(0) + } +} + +/** The `Patch`, `Put`, `CopyChangedValues`, and `CopyUnchangedValues` methods on `Delta`. */ +class DeltaMutatingMethod extends Method { + DeltaMutatingMethod() { + this.getDeclaringType() instanceof DeltaClass and + this.hasName(["Patch", "Put", "CopyChangedValues", "CopyUnchangedValues"]) + } +} + +/** The `GetInstance` method on `Delta`. */ +class DeltaGetInstanceMethod extends Method { + DeltaGetInstanceMethod() { + this.getDeclaringType() instanceof DeltaClass and + this.hasName("GetInstance") + } +} + +private class CandidateODataMember extends Member { + CandidateODataMember() { + this.isPublic() and + not this.isStatic() and + ( + this = + any(Property p | + p.isAutoImplemented() and + p.getGetter().isPublic() and + p.getSetter().isPublic() + ) + or + this = any(Field f | f.isPublic()) + ) + } +} + +/** + * Taint members (transitively) on types used in + * 1. Casts, `as`-conversions, or type tests applied to `ODataActionParameters` values. + * 2. The type argument of a `Delta`. + * + * Note that this also impacts uses of such types in other contexts, the same + * trade-off `AspNetRemoteFlowSourceMember` (`Remote.qll`) makes for ASP.NET + * action-method parameters. + */ +private class ODataBoundMember extends TaintTracking::TaintedMember, CandidateODataMember { + ODataBoundMember() { + exists(Type t, Type t0 | t = this.getDeclaringType() | + (t = t0 or t = t0.(CollectionType).getElementType()) and + ( + t0 = any(ODataBoundMember m).getType() + or + t0 instanceof ODataBoundType + ) + ) + } +} + +/** + * A call to `Delta.Patch`/`Put`/`CopyChangedValues`/`CopyUnchangedValues` + * copies the changes tracked by the `Delta` receiver onto its `original` + * entity argument. + */ +private class DeltaMutatingCallTaintStep extends AdditionalTaintStep { + override predicate step(DataFlow::Node node1, DataFlow::Node node2) { + exists(MethodCall mc | + mc.getTarget().getUnboundDeclaration() instanceof DeltaMutatingMethod and + node1.asExpr() = mc.getQualifier() and + node2.(PostUpdateNode).getPreUpdateNode().asExpr() = mc.getArgument(0) + ) + } +} + +/** `Delta.GetInstance()` returns the tracked entity, carrying the same taint as the `Delta` itself. */ +private class DeltaGetInstanceTaintStep extends AdditionalTaintStep { + override predicate step(DataFlow::Node node1, DataFlow::Node node2) { + exists(MethodCall mc | + mc.getTarget().getUnboundDeclaration() instanceof DeltaGetInstanceMethod and + node1.asExpr() = mc.getQualifier() and + node2.asExpr() = mc + ) + } +} diff --git a/csharp/ql/test/library-tests/frameworks/OData/OData.cs b/csharp/ql/test/library-tests/frameworks/OData/OData.cs new file mode 100644 index 000000000000..71a5c2218b68 --- /dev/null +++ b/csharp/ql/test/library-tests/frameworks/OData/OData.cs @@ -0,0 +1,125 @@ +using System.Collections.Generic; + +namespace Microsoft.AspNet.OData +{ + public class ODataActionParameters : Dictionary + { + } + + public class Delta where TStructuralType : class + { + private TStructuralType instance; + + public Delta() { instance = default(TStructuralType); } + + public TStructuralType GetInstance() => instance; + + public void Patch(TStructuralType original) { } + + public void Put(TStructuralType original) { } + + public void CopyChangedValues(TStructuralType original) { } + + public void CopyUnchangedValues(TStructuralType original) { } + } +} + +namespace Test +{ + using Microsoft.AspNet.OData; + using System.Collections.Generic; + + public class EntityMetadata + { + public string Owner { get; set; } + } + + public class BoundEntity + { + public string Name { get; set; } + + public string Content { get; set; } + + public EntityMetadata Metadata { get; set; } + + public List Revisions { get; set; } + } + + public class RelatedItem + { + public string Label { get; set; } + + public string Category { get; set; } + } + + public class Widget + { + public string Name { get; set; } + } + + public class UnrelatedType + { + // Never reached via an ODataActionParameters/Delta cast, so this + // member must stay untainted even though `UnrelatedType` itself is + // used elsewhere in the file. + public string Name { get; set; } + } + + public class SampleController + { + void Sink(object o) { } + + void CastFromDictionary(ODataActionParameters parameters) + { + var entity = (BoundEntity)parameters["Entity"]; + Sink(entity); + Sink(entity.Name); + Sink(entity.Content); + Sink(entity.Metadata.Owner); + foreach (var m in entity.Revisions) + { + Sink(m.Owner); + } + } + + void IsAsFromDictionary(ODataActionParameters parameters) + { + if (parameters["Items"] is IEnumerable items1) + { + foreach (var item in items1) + { + Sink(item.Label); + } + } + + var items2 = parameters["Items"] as IEnumerable; + foreach (var item in items2) + { + Sink(item.Category); + } + } + + void DeltaPatch(Delta delta, Widget original) + { + delta.Patch(original); + Sink(original.Name); + } + + void DeltaGetInstance(Delta delta) + { + var w = delta.GetInstance(); + Sink(w.Name); + } + + void Untainted() + { + var w = new Widget(); + w.Name = "safe"; + Sink(w.Name); + + var u = new UnrelatedType(); + u.Name = "also safe"; + Sink(u.Name); + } + } +} diff --git a/csharp/ql/test/library-tests/frameworks/OData/OData.expected b/csharp/ql/test/library-tests/frameworks/OData/OData.expected new file mode 100644 index 000000000000..a9a2796c0ca5 --- /dev/null +++ b/csharp/ql/test/library-tests/frameworks/OData/OData.expected @@ -0,0 +1,9 @@ +| OData.cs:72:55:72:64 | parameters | OData.cs:75:18:75:23 | access to local variable entity | +| OData.cs:72:55:72:64 | parameters | OData.cs:76:18:76:28 | access to property Name | +| OData.cs:72:55:72:64 | parameters | OData.cs:77:18:77:31 | access to property Content | +| OData.cs:72:55:72:64 | parameters | OData.cs:78:18:78:38 | access to property Owner | +| OData.cs:72:55:72:64 | parameters | OData.cs:81:22:81:28 | access to property Owner | +| OData.cs:85:55:85:64 | parameters | OData.cs:91:26:91:35 | access to property Label | +| OData.cs:85:55:85:64 | parameters | OData.cs:98:22:98:34 | access to property Category | +| OData.cs:102:39:102:43 | delta | OData.cs:105:18:105:30 | access to property Name | +| OData.cs:108:45:108:49 | delta | OData.cs:111:18:111:23 | access to property Name | diff --git a/csharp/ql/test/library-tests/frameworks/OData/OData.ql b/csharp/ql/test/library-tests/frameworks/OData/OData.ql new file mode 100644 index 000000000000..80d0f1d04dec --- /dev/null +++ b/csharp/ql/test/library-tests/frameworks/OData/OData.ql @@ -0,0 +1,21 @@ +import csharp + +module TaintConfig implements DataFlow::ConfigSig { + predicate isSource(DataFlow::Node n) { + exists(Parameter p | p = n.asParameter() | + p.getType().hasFullyQualifiedName("Microsoft.AspNet.OData", "ODataActionParameters") + or + p.getType().getUnboundDeclaration().hasFullyQualifiedName("Microsoft.AspNet.OData", "Delta`1") + ) + } + + predicate isSink(DataFlow::Node sink) { + exists(MethodCall c | c.getArgument(0) = sink.asExpr() and c.getTarget().hasName("Sink")) + } +} + +module Taint = TaintTracking::Global; + +from DataFlow::Node source, DataFlow::Node sink +where Taint::flow(source, sink) +select source, sink