Repository navigation
Conversation
Also taint properties whose accessors wrap a backing field, get-only properties of collection type, and members inherited from base types. Skip constants. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The source modeling incorrectly taints copied/computed properties and non-bindable collections while missing request-data flow through properties backed by inherited fields.
3 open issues
What changed in this PR
This PR broadens CodeQL’s C# ASP.NET remote-source modeling so taint can propagate through more binder-populated model members.
Changes:
- Expands property eligibility to backing-field properties and get-only collections.
- Adds inherited-member traversal and excludes constant fields.
- Extends legacy ASP.NET and ASP.NET Core test fixtures, expected results, and release documentation.
| File | Description |
|---|---|
csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.expected |
Updates expected legacy source and member results. |
csharp/ql/test/library-tests/dataflow/flowsources/remote/RemoteFlowSource.cs |
Adds legacy web-service model cases. |
csharp/ql/test/library-tests/dataflow/flowsources/aspremote/aspRemoteFlowSource.expected |
Updates expected ASP.NET Core results. |
csharp/ql/test/library-tests/dataflow/flowsources/aspremote/AspRemoteFlowSource.cs |
Adds ASP.NET Core model cases and behavior annotations. |
csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll |
Expands candidate-member rules and shares base-type traversal. |
csharp/ql/lib/change-notes/2026-11-06-taint-plain-data-members.md |
Documents the analysis changes. |
Review effort: Max (auto)
| or | ||
| p.isAutoImplementedReadOnly() | ||
| or | ||
| exists(Field f | f.getDeclaringType() = p.getDeclaringType() | |
| exists(Field f | f.getDeclaringType() = p.getDeclaringType() | | ||
| exists(FieldRead fr, Expr ret | | ||
| fr.getTarget() = f and | ||
| fr.getParent*() = ret and |
| isPlainDataProperty(p) and | ||
| p.getGetter().isPublic() and | ||
| p.getSetter().isPublic() | ||
| (p.getSetter().isPublic() or p.getType() instanceof CollectionType) |
This branch has not been deployed
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.

Hello @michaelnebel
Follow-up to #21567 / #21612. Members of types bound from
[WebMethod]or action-method parameters are only tainted when they are public auto-implemented properties or public fields. Real binder-populated types often don't fit that, so taint stops at the first such member and flow is lost. From my security researcher point of view I was not fully satisfied.This PR relaxes
CandidateMemberToTaintand the two transitive classes (AspNetRemoteFlowSourceMember,AspNetCoreRemoteFlowSourceMember):Addto the returned collection. Get-only non-collections stay untainted.isMemberOfBoundType).As noted in the discussion, tainting is type-wide, so these members are tainted wherever the type is used. The change stays narrow for that reason.
Tests extend the existing aspremote and remote flow-source tests. A change note is included.
I'm still not 100% satisfied because for more complex operations in Getter/Setter this would not work. The only way to do it properly would be with TaintTracking I think.
isPlainDataPropertyis purely syntactic: the getter must return an expression that reads a field, and the setter must assign an expression using value to that field. It does not follow accessors with several statements:Local data flow (
DataFlow::localFlow) would handleViaLocaland Conditional, because it follows locals and branches. I confirmed it also handlesf ?? ""and casts, which the syntactic check misses on the getter side. It would missWrapped, because it follows value-preserving steps only, soTrim()andNormalize()end the flow.TaintTracking::localTaintwould follow those. However,TaintedMemberfeeds the taint-step relation, so I expect it to be circular and I haven't tried it.I would like feedback on whether the extra precision is worth a data-flow dependency inside a candidate-member predicate.