csharp: odata lib - #22384
Conversation
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>
Match WCF.qll's convention: only the TaintedMember/AdditionalTaintStep wiring classes stay private, everything else that identifies a reusable OData domain concept (ODataActionParametersClass, DeltaClass, ODataBoundType, DeltaMutatingMethod, DeltaGetInstanceMethod) is public. Also renames the test fixtures to generic placeholder names. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
import csharp already publicly imports semmle.code.csharp.dataflow.TaintTracking (and DataFlow), same as WCF.qll/JsonNET.qll rely on implicitly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
michaelnebel
left a comment
michaelnebel
left a comment
There was a problem hiding this comment.
Thank you very much! It is really good, if we can get our modelling extended even further!
I have added some initial comments / questions. Maybe OData parameter like types are only relevant for classes that extend ODataController. Should that somehow be incorporated in the logic?
| 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 | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Perhaps, the QL implementation can be replaced by Models as Data?
Below is the row for one of the GetInstance methods.
extensions:
- addsTo:
pack: codeql/csharp-all
extensible: summaryModel
data:
- ["Microsoft.AspNet.OData", "Delta<TStructuralType>", True, "GetInstance", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
| namespace Microsoft.AspNet.OData | ||
| { | ||
| public class ODataActionParameters : Dictionary<string, object> | ||
| { | ||
| } | ||
|
|
||
| public class Delta<TStructuralType> 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) { } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ideally, we would like to keep stub implementations separate from the test and store them in test/resources/stubs.
This will require an options file for the test; If possible, it is also preferred, if the test relies fully on stubs and not any .dll files.
| 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()) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
This appears to be a copy of the CandidateMemberToTaint. Perhaps, the implementation from Remote.qll can be re-used?
There was a problem hiding this comment.
I've reused this method, switch to public method
| TaintTracking::localExprTaint(any(ODataActionParameterRead r), e) | ||
| } | ||
|
|
||
| /** The generic `Delta<TStructuralType>` change-tracking class, across OData library versions. */ |
There was a problem hiding this comment.
Maybe refer to the unbound declaration with "Delta1" instead of Delta<TStructuralType> as the type parameter is named T for Microsoft.AspNetCore.OData.Deltas.Delta<T>
Per review feedback on github#22384, replace the hand-written DeltaGetInstanceMethod/DeltaGetInstanceTaintStep taint step with a Models-as-Data summaryModel row for both the Microsoft.AspNet.OData and Microsoft.AspNetCore.OData.Deltas variants of Delta<T>.GetInstance().
Per review feedback on github#22384, OData.qll's CandidateODataMember was an exact copy of CandidateMemberToTaint from Remote.qll. Make that class public and import it instead of duplicating it.
Per review feedback on github#22384, keep the ODataActionParameters/Delta<T> stub implementations out of the test .cs file and store them in test/resources/stubs instead, following the pattern used by other frameworks (e.g. JsonNET, Aws). The test now loads the stub project via an options file and relies on no .dll files.
Per review feedback on github#22384, the doc comment named the type parameter TStructuralType, but the AspNetCore variant of Delta<T> names it T. Refer to the unbound generic as \`Delta\`1\`\` instead.
|
Hi @michaelnebel I think I've made changes for all your requests let me know if it's ok I'm not sure to get you question:
Can you give more details / examples ? |
2 participants
Add predicates and classes for the OData library https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnet.odata?view=odata-aspnetcore-7.0&viewFallbackFrom=odata-aspnetcore-8.0