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/ext/Microsoft.AspNet.OData.model.yml b/csharp/ql/lib/ext/Microsoft.AspNet.OData.model.yml new file mode 100644 index 000000000000..e27c4ccfe529 --- /dev/null +++ b/csharp/ql/lib/ext/Microsoft.AspNet.OData.model.yml @@ -0,0 +1,23 @@ +extensions: + - addsTo: + pack: codeql/csharp-all + extensible: summaryModel + data: + - ["Microsoft.AspNet.OData", "Delta", True, "GetInstance", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"] + - ["Microsoft.AspNet.OData", "Delta", True, "Patch", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"] + - ["Microsoft.AspNet.OData", "Delta", True, "Patch", "(TStructuralType)", "", "Argument[this]", "ReturnValue", "taint", "manual"] + - ["Microsoft.AspNet.OData", "Delta", True, "Put", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"] + - ["Microsoft.AspNet.OData", "Delta", True, "CopyChangedValues", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"] + - ["Microsoft.AspNet.OData", "Delta", True, "CopyChangedValues", "(TStructuralType)", "", "Argument[this]", "ReturnValue", "taint", "manual"] + - ["Microsoft.AspNet.OData", "Delta", True, "CopyUnchangedValues", "(TStructuralType)", "", "Argument[this]", "Argument[0]", "taint", "manual"] + - ["Microsoft.AspNetCore.OData.Deltas", "Delta", True, "GetInstance", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"] + - ["Microsoft.AspNetCore.OData.Deltas", "Delta", True, "Patch", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"] + - ["Microsoft.AspNetCore.OData.Deltas", "Delta", True, "Patch", "(T)", "", "Argument[this]", "ReturnValue", "taint", "manual"] + - ["Microsoft.AspNetCore.OData.Deltas", "Delta", True, "Put", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"] + - ["Microsoft.AspNetCore.OData.Deltas", "Delta", True, "CopyChangedValues", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"] + - ["Microsoft.AspNetCore.OData.Deltas", "Delta", True, "CopyUnchangedValues", "(T)", "", "Argument[this]", "Argument[0]", "taint", "manual"] + - ["System.Web.Http.OData", "Delta", True, "GetEntity", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"] + - ["System.Web.Http.OData", "Delta", True, "Patch", "(TEntityType)", "", "Argument[this]", "Argument[0]", "taint", "manual"] + - ["System.Web.Http.OData", "Delta", True, "Put", "(TEntityType)", "", "Argument[this]", "Argument[0]", "taint", "manual"] + - ["System.Web.Http.OData", "Delta", True, "CopyChangedValues", "(TEntityType)", "", "Argument[this]", "Argument[0]", "taint", "manual"] + - ["System.Web.Http.OData", "Delta", True, "CopyUnchangedValues", "(TEntityType)", "", "Argument[this]", "Argument[0]", "taint", "manual"] 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..9571268c6b30 --- /dev/null +++ b/csharp/ql/lib/semmle/code/csharp/frameworks/OData.qll @@ -0,0 +1,132 @@ +/** + * 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()` (`GetEntity()` in + * the older `System.Web.Http.OData`) 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.security.dataflow.flowsources.Remote + +/** 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") + } +} + +/** + * Holds if `e` is (or, via local flow -- e.g. an upcast to `IDictionary` + * -- may hold the value of) an `ODataActionParameters` dictionary. + */ +private predicate isODataActionParametersValue(Expr e) { + e.getType() instanceof ODataActionParametersClass + or + DataFlow::localExprFlow(any(Expr e0 | isODataActionParametersValue(e0)), e) +} + +/** + * An indexer read on an `ODataActionParameters` dictionary, e.g. `parameters["Foo"]` + * (including through an upcast to a base dictionary type/interface). + */ +class ODataActionParameterRead extends ElementAccess { + ODataActionParameterRead() { isODataActionParametersValue(this.getQualifier()) } +} + +/** + * A call to `TryGetValue` on an `ODataActionParameters` dictionary copies the value + * of the looked-up entry into the `out` argument. + */ +private class ODataActionParametersTryGetValueTaintStep extends AdditionalTaintStep { + override predicate step(DataFlow::Node node1, DataFlow::Node node2) { + exists(MethodCall mc, AssignableDefinitions::OutRefDefinition def | + mc.getTarget().hasName("TryGetValue") and + isODataActionParametersValue(mc.getQualifier()) and + node1.asExpr() = mc.getQualifier() and + def.getTargetAccess() = mc.getArgumentForName("value") and + node2 = DataFlow::assignableDefinitionNode(def) + ) + } +} + +/** Holds if `e` may (locally) hold the value of an `ODataActionParameters` entry. */ +private predicate isODataParameterValue(Expr e) { + DataFlow::localExprFlow(any(ODataActionParameterRead r), e) +} + +/** The generic ``Delta`1`` 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") or + this.hasFullyQualifiedName("System.Web.Http.OData", "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) + } +} + +/** + * 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, CandidateMemberToTaint { + 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 + ) + ) + } +} diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll index 68c06a1828de..3eac08b1bd93 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll @@ -117,7 +117,8 @@ class AspNetServiceRemoteFlowSource extends AspNetRemoteFlowSource, DataFlow::Pa override string getSourceType() { result = "ASP.NET web service input" } } -private class CandidateMemberToTaint extends Member { +/** A public, non-static, auto-implemented property or field, candidate for taint-tracking. */ +class CandidateMemberToTaint extends Member { CandidateMemberToTaint() { this.isPublic() and not this.isStatic() and 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..bbce96d56fc9 --- /dev/null +++ b/csharp/ql/test/library-tests/frameworks/OData/OData.cs @@ -0,0 +1,139 @@ +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 TryGetValueFromDictionary(ODataActionParameters parameters) + { + if (parameters.TryGetValue("Entity", out var value)) + { + var entity = (BoundEntity)value; + Sink(entity.Name); + } + } + + void UpcastThenIndex(ODataActionParameters parameters) + { + IDictionary dict = parameters; + var entity = (BoundEntity)dict["Entity"]; + Sink(entity.Name); + } + + void DeltaPatch(Delta delta, Widget original) + { + delta.Patch(original); + Sink(original.Name); + } + + void DeltaGetInstance(Delta delta) + { + var w = delta.GetInstance(); + Sink(w.Name); + } + + void DeltaPatchReturnValue(Delta delta, Widget original) + { + var updated = delta.Patch(original); + Sink(updated.Name); + } + + void DeltaCopyChangedValuesReturnValue(Delta delta, Widget original) + { + var updated = delta.CopyChangedValues(original); + Sink(updated.Name); + } + + void LegacyDeltaPatch(System.Web.Http.OData.Delta delta, Widget original) + { + delta.Patch(original); + Sink(original.Name); + } + + void LegacyDeltaGetEntity(System.Web.Http.OData.Delta delta) + { + var w = delta.GetEntity(); + 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..290236df722e --- /dev/null +++ b/csharp/ql/test/library-tests/frameworks/OData/OData.expected @@ -0,0 +1,15 @@ +| OData.cs:46:55:46:64 | parameters | OData.cs:49:18:49:23 | access to local variable entity | +| OData.cs:46:55:46:64 | parameters | OData.cs:50:18:50:28 | access to property Name | +| OData.cs:46:55:46:64 | parameters | OData.cs:51:18:51:31 | access to property Content | +| OData.cs:46:55:46:64 | parameters | OData.cs:52:18:52:38 | access to property Owner | +| OData.cs:46:55:46:64 | parameters | OData.cs:55:22:55:28 | access to property Owner | +| OData.cs:59:55:59:64 | parameters | OData.cs:65:26:65:35 | access to property Label | +| OData.cs:59:55:59:64 | parameters | OData.cs:72:22:72:34 | access to property Category | +| OData.cs:76:62:76:71 | parameters | OData.cs:81:22:81:32 | access to property Name | +| OData.cs:85:52:85:61 | parameters | OData.cs:89:18:89:28 | access to property Name | +| OData.cs:92:39:92:43 | delta | OData.cs:95:18:95:30 | access to property Name | +| OData.cs:98:45:98:49 | delta | OData.cs:101:18:101:23 | access to property Name | +| OData.cs:104:50:104:54 | delta | OData.cs:107:18:107:29 | access to property Name | +| OData.cs:110:62:110:66 | delta | OData.cs:113:18:113:29 | access to property Name | +| OData.cs:116:67:116:71 | delta | OData.cs:119:18:119:30 | access to property Name | +| OData.cs:122:71:122:75 | delta | OData.cs:125:18:125: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..ed70740699e3 --- /dev/null +++ b/csharp/ql/test/library-tests/frameworks/OData/OData.ql @@ -0,0 +1,23 @@ +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") + or + p.getType().getUnboundDeclaration().hasFullyQualifiedName("System.Web.Http.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 diff --git a/csharp/ql/test/library-tests/frameworks/OData/options b/csharp/ql/test/library-tests/frameworks/OData/options new file mode 100644 index 000000000000..e9ba768d29a7 --- /dev/null +++ b/csharp/ql/test/library-tests/frameworks/OData/options @@ -0,0 +1,3 @@ +semmle-extractor-options: /nostdlib /noconfig +semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resources/stubs/Microsoft.AspNet.OData/7.7.5/Microsoft.AspNet.OData.csproj +semmle-extractor-options: ${testdir}/../../../resources/stubs/System.Web.Http.OData.cs diff --git a/csharp/ql/test/resources/stubs/Microsoft.AspNet.OData/7.7.5/Microsoft.AspNet.OData.cs b/csharp/ql/test/resources/stubs/Microsoft.AspNet.OData/7.7.5/Microsoft.AspNet.OData.cs new file mode 100644 index 000000000000..da46281cf2d1 --- /dev/null +++ b/csharp/ql/test/resources/stubs/Microsoft.AspNet.OData/7.7.5/Microsoft.AspNet.OData.cs @@ -0,0 +1,17 @@ +namespace Microsoft.AspNet.OData +{ + public class ODataActionParameters : System.Collections.Generic.Dictionary + { + public ODataActionParameters() => throw null; + } + + public class Delta where TStructuralType : class + { + public Delta() => throw null; + public TStructuralType GetInstance() => throw null; + public TStructuralType Patch(TStructuralType original) => throw null; + public void Put(TStructuralType original) => throw null; + public TStructuralType CopyChangedValues(TStructuralType original) => throw null; + public void CopyUnchangedValues(TStructuralType original) => throw null; + } +} diff --git a/csharp/ql/test/resources/stubs/Microsoft.AspNet.OData/7.7.5/Microsoft.AspNet.OData.csproj b/csharp/ql/test/resources/stubs/Microsoft.AspNet.OData/7.7.5/Microsoft.AspNet.OData.csproj new file mode 100644 index 000000000000..2be6995cd169 --- /dev/null +++ b/csharp/ql/test/resources/stubs/Microsoft.AspNet.OData/7.7.5/Microsoft.AspNet.OData.csproj @@ -0,0 +1,12 @@ + + + net10.0 + true + bin\ + false + + + + + + diff --git a/csharp/ql/test/resources/stubs/System.Web.Http.OData.cs b/csharp/ql/test/resources/stubs/System.Web.Http.OData.cs new file mode 100644 index 000000000000..e4b9775379c7 --- /dev/null +++ b/csharp/ql/test/resources/stubs/System.Web.Http.OData.cs @@ -0,0 +1,15 @@ +namespace System.Web.Http.OData +{ + public class ODataActionParameters : System.Collections.Generic.Dictionary + { + } + + public class Delta where TEntityType : class + { + public TEntityType GetEntity() => throw null; + public void Patch(TEntityType original) { } + public void Put(TEntityType original) { } + public void CopyChangedValues(TEntityType original) { } + public void CopyUnchangedValues(TEntityType original) { } + } +}