C#: Allow implicit collection reads in sink nodes. #20089
Conversation
michaelnebel
changed the title
michaelnebel
changed the title
michaelnebel
force-pushed
the
csharp/allowsinkimplicitread
branch
from
0956459 to
ca5a0dc
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR enables implicit collection reads in C# taint tracking sink nodes to improve flow coverage and reduce false negatives. The change allows taint tracking to recognize when tainted elements within a collection can flow to sinks that receive the entire collection.
- Adds implicit collection reads for argument nodes at sinks and flow steps
- Updates default taint tracking configuration to handle collection element flows
- Enhances external model flow detection for collection-based operations
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| TaintTrackingPrivate.qll | Implements the core logic for implicit collection reads at argument nodes |
| HashWithoutSalt.ql | Adds collection interface support for CopyTo method detection |
| ExternalFlow.expected | Updates test expectations with new flow detection results |
| ExternalFlow.cs | Updates test comment to reflect new flow behavior |
| CollectionTaintTracking.* | Adds new test cases for collection taint tracking scenarios |
| change-notes/* | Documents the analysis improvement |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
…d additional flow steps) for default taint tracking configurations.
…y accident before as we didn't allow implicit element reads at sinks.
| node instanceof ArgumentNode and | ||
| Collections::isCollectionType(node.getType()) and |
There was a problem hiding this comment.
Actually, I would have thought that this was simply exists(node).
There was a problem hiding this comment.
I added these extra criteria to narrow it as much as possible (primarily interested in adding the implicit reads on sink method calls - note that the predicate is also used to allow implicit reads on inputs to additional taint steps). Will try with your suggestion and see what DCA says.
michaelnebel
force-pushed
the
csharp/allowsinkimplicitread
branch
from
ca5a0dc to
b42c366
Compare
| private import semmle.code.csharp.dataflow.internal.ControlFlowReachability | ||
| private import semmle.code.csharp.dispatch.Dispatch | ||
| private import semmle.code.csharp.commons.ComparisonTest | ||
| private import semmle.code.csharp.commons.Collections as Collections |
There was a problem hiding this comment.
This import is now redundant.
3 participants
And additional flow steps (for default taint tracking configurations).
DCA shows a couple of extra alerts, where the added step looks sounds. The alerts show up in various cases where the elements of a collection are tainted and the sink receives the entire collection as input.