C#: Taint members of types in ASP.NET user context.#21612
Open
michaelnebel wants to merge 7 commits intogithub:mainfrom
Open
C#: Taint members of types in ASP.NET user context.#21612michaelnebel wants to merge 7 commits intogithub:mainfrom
michaelnebel wants to merge 7 commits intogithub:mainfrom
Conversation
michaelnebel
force-pushed
the
csharp/legacyasptaintedmember
branch
from
a4648dc to
8721d35
Compare
michaelnebel
force-pushed
the
csharp/legacyasptaintedmember
branch
from
a00f1b5 to
2d4c18e
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Expands the C# CodeQL remote-source modeling for legacy ASP/ASP.NET contexts by treating members (including fields) of certain ASP.NET user-context types as tainted, including transitive member tainting, and updates the associated library tests and stubs.
Changes:
- Add member/field tainting (including transitive tainting) for types used in ASP.NET legacy contexts (e.g., action method parameters and
[WebMethod]parameters). - Streamline tainted-member handling by reusing a shared “candidate member” definition across ASP.NET and ASP.NET Core.
- Extend/adjust library tests (test queries, expected output, and new test cases) and add a stub for
System.Web.Services.WebMethodAttribute.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| csharp/ql/test/resources/stubs/System.Web.cs | Adds a stub for System.Web.Services.WebMethodAttribute to support new test coverage. |
| csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.ql | Updates the test query shape to expose results via query predicates (including tainted members). |
| csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.expected | Updates expected results to include tainted-member outputs and new ASP.NET web service inputs. |
| csharp/ql/test/library-tests/dataflow/flowsources/remote/RemoteFlowSource.cs | Adds ASP.NET web service test cases with [WebMethod] parameters and member accesses. |
| csharp/ql/test/library-tests/dataflow/flowsources/aspremote/aspRemoteFlowSource.expected | Updates expected results to include tainted fields in the ASP legacy scenario. |
| csharp/ql/test/library-tests/dataflow/flowsources/aspremote/AspRemoteFlowSource.cs | Adjusts the test fixture to treat a public field as tainted. |
| csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll | Implements the expanded/streamlined ASP.NET tainted-member modeling (and reuse for ASP.NET Core member candidates). |
| csharp/ql/lib/change-notes/2026-04-01-asp-remote-sources.md | Adds a change note describing the modeling expansion. |
| * 1. Action method parameters. | ||
| * 2. WebMethod parameters. | ||
| * | ||
| * Note, that this also impacts uses of such types in other contexts. |
There was a problem hiding this comment.
In this doc comment, the phrase "Note, that" is grammatically incorrect; consider changing it to "Note that" (without the comma) for clarity.
Suggested change
| * Note, that this also impacts uses of such types in other contexts. | |
| * Note that this also impacts uses of such types in other contexts. |
|
Hello, any update on this PR ? :) |
Contributor
Author
Thanks for reminding. It looks like it might have ended in a review void. |
hvitved
requested changes
Apr 16, 2026
Contributor
hvitved
left a comment
hvitved
left a comment
There was a problem hiding this comment.
LGTM; only two small nits.
| private class AspNetRemoteFlowSourceMember extends TaintTracking::TaintedMember { | ||
| AspNetRemoteFlowSourceMember() { | ||
| exists(Type t, Type t0 | t = this.getDeclaringType() | | ||
| (t = t0 or t = t0.(ArrayType).getElementType()) and |
Contributor
There was a problem hiding this comment.
Could this be any collection type and not just arrays?
| override string getSourceType() { result = "ASP.NET web service input" } | ||
| } | ||
|
|
||
| private class CandidateMembersToTaint extends Member { |
Contributor
There was a problem hiding this comment.
nit: CandidateMemberToTaint instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The background of this PR is this discussion.
In this PR we
It is worth noting that tainting the members (for a given type) used in an ASP context also means that such members will be considered tainted (when the object is tainted) in other contexts. However, to streamline the approach of the legacy ASP with ASP.NET tainted members - this is the approach taken in the PR.
@hvitved : Do you believe this is still acceptable or should we strive for adding access of fields and properties as remote sources instead?