| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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>
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?
Sorry, something went wrong.
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 ? |
Sorry, something went wrong.
I don't agree with this advice. OData can work without inheriting from ODataController by using standard ASP.NET Core Controller or ApiController classes combined with the [EnableQuery] attribute or manual ODataQueryOptions parsing. Therefore, the filter to speed up CodeQL database matches should have all 3 possibilities:
For the latter two, either
|
Sorry, something went wrong.
|
⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged. Click to show differences in coveragecsharpGenerated file changes for csharp
- System,"``System.*``, ``System``",48,12495,59,5
+ System,"``System.*``, ``System``",48,12500,59,5
- Others,"``Amazon.Lambda.APIGatewayEvents``, ``Amazon.Lambda.Core``, ``Dapper``, ``ILCompiler``, ``ILLink.RoslynAnalyzer``, ``ILLink.Shared``, ``ILLink.Tasks``, ``Internal.IL``, ``Internal.Pgo``, ``Internal.TypeSystem``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.AspNetCore.Components``, ``Microsoft.AspNetCore.Http``, ``Microsoft.AspNetCore.Mvc``, ``Microsoft.AspNetCore.WebUtilities``, ``Microsoft.CSharp``, ``Microsoft.Data.SqlClient``, ``Microsoft.Diagnostics.Tools.Pgo``, ``Microsoft.DotNet.Build.Tasks``, ``Microsoft.DotNet.PlatformAbstractions``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.Diagnostics.Metrics``, ``Microsoft.Extensions.FileProviders``, ``Microsoft.Extensions.FileSystemGlobbing``, ``Microsoft.Extensions.Hosting``, ``Microsoft.Extensions.Http``, ``Microsoft.Extensions.Logging``, ``Microsoft.Extensions.Options``, ``Microsoft.Extensions.Primitives``, ``Microsoft.Interop``, ``Microsoft.JSInterop``, ``Microsoft.NET.Build.Tasks``, ``Microsoft.VisualBasic``, ``Microsoft.Win32``, ``Mono.Linker``, ``MySql.Data.MySqlClient``, ``NHibernate``, ``Newtonsoft.Json``, ``SourceGenerators``, ``Windows.Security.Cryptography.Core``",60,2406,162,4
+ Others,"``Amazon.Lambda.APIGatewayEvents``, ``Amazon.Lambda.Core``, ``Dapper``, ``ILCompiler``, ``ILLink.RoslynAnalyzer``, ``ILLink.Shared``, ``ILLink.Tasks``, ``Internal.IL``, ``Internal.Pgo``, ``Internal.TypeSystem``, ``Microsoft.ApplicationBlocks.Data``, ``Microsoft.AspNet.OData``, ``Microsoft.AspNetCore.Components``, ``Microsoft.AspNetCore.Http``, ``Microsoft.AspNetCore.Mvc``, ``Microsoft.AspNetCore.OData.Deltas``, ``Microsoft.AspNetCore.WebUtilities``, ``Microsoft.CSharp``, ``Microsoft.Data.SqlClient``, ``Microsoft.Diagnostics.Tools.Pgo``, ``Microsoft.DotNet.Build.Tasks``, ``Microsoft.DotNet.PlatformAbstractions``, ``Microsoft.EntityFrameworkCore``, ``Microsoft.Extensions.Caching.Distributed``, ``Microsoft.Extensions.Caching.Memory``, ``Microsoft.Extensions.Configuration``, ``Microsoft.Extensions.DependencyInjection``, ``Microsoft.Extensions.DependencyModel``, ``Microsoft.Extensions.Diagnostics.Metrics``, ``Microsoft.Extensions.FileProviders``, ``Microsoft.Extensions.FileSystemGlobbing``, ``Microsoft.Extensions.Hosting``, ``Microsoft.Extensions.Http``, ``Microsoft.Extensions.Logging``, ``Microsoft.Extensions.Options``, ``Microsoft.Extensions.Primitives``, ``Microsoft.Interop``, ``Microsoft.JSInterop``, ``Microsoft.NET.Build.Tasks``, ``Microsoft.VisualBasic``, ``Microsoft.Win32``, ``Mono.Linker``, ``MySql.Data.MySqlClient``, ``NHibernate``, ``Newtonsoft.Json``, ``SourceGenerators``, ``Windows.Security.Cryptography.Core``",60,2416,162,4
- Totals,,108,14908,415,9
+ Totals,,108,14923,415,9
+ Microsoft.AspNet.OData,,,5,,,,,,,,,,,,,,,,,,,5,
+ Microsoft.AspNetCore.OData.Deltas,,,5,,,,,,,,,,,,,,,,,,,5,
- System,59,48,12495,,6,5,12,,,4,1,,31,2,,6,15,17,5,3,,6382,6113
+ System,59,48,12500,,6,5,12,,,4,1,,31,2,,6,15,17,5,3,,6387,6113 |
Sorry, something went wrong.
There was a problem hiding this comment.
I am sorry for the delay in review; Thank you for your diligence @hugo-syn.
Will also start a DCA run (automated testing against a set of repositories)
Sorry, something went wrong.
| @@ -0,0 +1,19 @@ | |||
| // This file contains auto-generated code. | |||
| // Generated from `Microsoft.AspNet.OData, Version=7.7.5.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35`. | |||
There was a problem hiding this comment.
Is this indeed auto-generated? Did you use the make_stubs_nuget.py to generate the file?
If it is not auto-generated, could you then move this file to csharp/ql/test/resources/stubs (and then remove comments about code being auto generated)?
If it is auto generated, then please leave it here (sorry about being a bit pushy about this - otherwise I will be really confused when trying to update all stubs later in the future) and then add the package to the list in make_stubs_all.py.
Sorry, something went wrong.
|
|
||
| /** Holds if `e` may (locally) hold the value of an `ODataActionParameters` entry. */ | ||
| private predicate isODataParameterValue(Expr e) { | ||
| TaintTracking::localExprTaint(any(ODataActionParameterRead r), e) |
There was a problem hiding this comment.
| TaintTracking::localExprTaint(any(ODataActionParameterRead r), e) | |
| DataFlow::localExprFlow(any(ODataActionParameterRead r), e) |
Maybe we should consider using local data flow instead (and not only taint tracking), then it becomes a bit more strict, which types we consider to be ODataBound (and it appears that all test-cases pass). Or do you know of a real world example, where this wouldn't be good enough?
Sorry, something went wrong.
There was a problem hiding this comment.
Adds OData action-parameter and Delta<T> taint tracking to the C# analysis libraries.
Changes:
| File | Description |
|---|---|
| Microsoft.AspNet.OData.csproj | Configures the test stub project. |
| Microsoft.AspNet.OData.cs | Provides generated OData API stubs. |
| OData/options | Loads the OData test stubs. |
| OData/OData.ql | Defines the taint test query. |
| OData/OData.expected | Records expected flows. |
| OData/OData.cs | Exercises dictionary and Delta flows. |
| Remote.qll | Exposes the reusable member candidate class. |
| OData.qll | Implements OData taint modeling. |
| TaintTrackingPrivate.qll | Registers the OData models. |
| Microsoft.AspNet.OData.model.yml | Models GetInstance return flow. |
| 2026-08-19-odata-taint-step.md | Documents the feature. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
| this.hasFullyQualifiedName("Microsoft.AspNet.OData", "Delta`1") or | ||
| this.hasFullyQualifiedName("Microsoft.AspNetCore.OData.Deltas", "Delta`1") |
Co-authored-by: Michael Nebel <michaelnebel@github.com>
Co-authored-by: Michael Nebel <michaelnebel@github.com>
Co-authored-by: Michael Nebel <michaelnebel@github.com>
|
Hey @michaelnebel I've taken into account comments from Copilot and your comments let me know if its better |
Sorry, something went wrong.
|
Also curious about the DCA, do you have a list of projects using OData ? |
Sorry, something went wrong.
There was a problem hiding this comment.
I have added some further comments; I would prefer that we keep TryGetValue out of scope for this PR - it is getting a bit complicated for me to review in one go 😄
From the first DCA run it appears that there are no issues with performance or changes to alerts - this could easily be because none of these projects use ODataParameters (haven't checked).
Do you know of an open source project, where the changes in this PR will lead to changes in alerts? 😄
Sorry, something went wrong.
- Revert Patch/CopyChangedValues to void-only per michaelnebel (defer to maintainer over docs citation despite conflicting reflection evidence). - Move Microsoft.AspNet.OData.cs stub to a flat file, drop its wrapper project. - Drop the TryGetValue AdditionalTaintStep: it only added a taint step and didn't make cast targets recognized as ODataBoundType, so it doesn't fully address the underlying gap; left for a follow-up PR. - Convert OData.ql to a path-problem query for clearer test output. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Hello @michaelnebel , I'm also a bit lost, I've reverted some changes based on your comments I hope it will be better |
Sorry, something went wrong.
Excellent! Thank you! |
Sorry, something went wrong.
I find this discussion about dataflow vs additional taint bit abstract, so I thought I'd chime in again and drive direct business value through actionable insights CodeQL could provide. RaythaCMS has an active vulnerability in their ODATA filtering. I just asked Google Gemini via Google Search AI Mode, "odata cve". Gemini responded with:
I cherry-picked the fourth issue - it turns out Raytha CMS is open source and has done done a release since December 2025, so the June 2026 CVE is currently valid ☠️ . I am guessing the actual vulnerability here is in https://github.com/RaythaHQ/raytha/blob/main/src/Raytha.Infrastructure/JsonQueryEngine/Postgres/ODataFilterToPostgres.cs#L77 |
Sorry, something went wrong.
… update test expected output.
… each other in relation to identifying which members will be tainted).
|
@jzabroski : Thank you for the information! It appears that there are no expressions in RaythaHQ/raytha of type ODataActionParameters (at least not if I do build-mode: none extraction), which means that if an alert is missing - the root cause is different than the modelling being addressed in this PR. If you believe that there is an identified true positive missing, please open an issue. @hugo-syn : Thank you for updating the PR. I checked approximately 1k open source repositories for expressions of type ODataActionParameters and among those I found 3. The repository with the most uses is OData/AspNetCoreOData. I will try and run DCA on this repository to see the effects. |
Sorry, something went wrong.
In this case, RaythaCMS uses ODataQueryOptionParser to essentially bypass what OData does for free if you just build an EFCore model for it. I actually don't know why they are doing this, as it feels a bit too-clever-by-half. I am still new to CodeQL, so I don't actually know how I would automatically write a rule to detect this CVE. My guess is there would be a more general taint rule that traces control flow to find the SQL injection CWE-089 (Improper Neutralization of Special Elements used in an SQL Command) this traces back to. In this particular case, RaythaCMS should probably adopt DynamicODataToSQL rather than roll their own un-parser/sql compiler. I think if there was a taint rule for SqlKata for .FromRaw(), .WhereRaw() and .SelectRaw(), and a taint rule for ODataQueryOptionsParser, it might be possible to catch such attacks. It seems like CodeQL is generally weak at detecting SQL Injection c.f. #16353 (for example, compared to GLM-5.2, which can plainly see the problem the way a human would) What is not clear to me is if by adding explicit OData support, e.g. for ODataQueryOptionsParser, suddenly the CWE-089 would flag via existing rules. From how I understand CodeQL, these rules do not "compose up" across the database. |
Sorry, something went wrong.
|
@jzabroski : The quality/coverage of a query can be different from language to language. Most of the security related queries (for instance the SQL injection) are using the CodeQL data flow library to detect paths from a set of sources to a set of sinks. |
Sorry, something went wrong.
|
Asked Claude Fable 5:
|
Sorry, something went wrong.
Got it. So, while GLM-5.2 could see this, perhaps the right way to approach this is to e-mail Kyle Daigle and Mario Rogriguez and suggest a parsimonious use of Microsoft AI Research resources to test/prove out MAI models for cybersecurity threat modeling. The general sketch of the prompt would be to harden SQL Injection CWE-089 by using package analytics across major ecosystems add add taint support for major data libraries. I predict the total budget for such robustness additions would be <$50,000 based on my personal experiences doing large scale refactorings for <$2,000. |
Sorry, something went wrong.
There was a problem hiding this comment.
Fantastic @hugo-syn ! We really appreciate the extra modelling.
Thank you for pushing through with the review comments - this is also really appreciated! 😄
The DCA run that only targeted the repo which has ODataParameters usages didn't show any regressions.
Approving the PR now, but will trigger one last DCA run with all the projects in our suite before merging!
Sorry, something went wrong.
|
Amazing @michaelnebel thank you for all your comments, it is my first real PR, now I have a better understanding of the architecture for maybe future PR :) |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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