Skip to content

C#: Taint more members of ASP.NET remote flow source types - #23043

Open
hugo-syn wants to merge 1 commit into
github:mainfrom
hugo-syn:csharp/taint-plain-data-members
Open

hugo-syn wants to merge 1 commit into
github:mainfrom
hugo-syn:csharp/taint-plain-data-members

Conversation

@hugo-syn

@hugo-syn hugo-syn commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

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 CandidateMemberToTaint and the two transitive classes (AspNetRemoteFlowSourceMember, AspNetCoreRemoteFlowSourceMember):

  • Backing-field properties: a property is accepted if its getter returns an expression reading a field and its setter assigns an expression using value to that field, for example lazy initialization or a null check. Properties that compute a value are still excluded, which keeps the isAutoImplemented guard's intent.
  • Get-only collections: a plain-data property of collection type is accepted without a setter, since binders such as XmlSerializer and Json.NET call the getter and Add to the returned collection. Get-only non-collections stay untainted.
  • Base types: members declared on base types of a bound type are now covered (shared helper isMemberOfBoundType).
  • Constants: const fields are excluded. The extractor does not mark them static, so the transitive rule otherwise taints short.MaxValue and similar.

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.

isPlainDataProperty is 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:

public class Model
{
    private string f;

    // Tainted: the setter assigns `value` directly and the getter returns the field.
    public string Simple { get { return f; } set { f = value; } }

    // Not tainted: `value` reaches `f` only through a local variable.
    public string ViaLocal
    {
        get { var r = f; return r; }       // the returned expression is `r`, not a read of `f`
        set { var t = value; f = t; }      // the assigned expression is `t`, not `value`
    }

    // Not tainted: same problem with a branch.
    public string Conditional
    {
        get { string r; if (f != null) r = f; else r = ""; return r; }
        set { var t = value; if (t.Length > 0) f = t; }
    }

    // Tainted by a syntactic check, but only because `value` appears in the call.
    public string Wrapped { get { return f; } set { f = Normalize(value); } }
}

Local data flow (DataFlow::localFlow) would handle ViaLocal and Conditional, because it follows locals and branches. I confirmed it also handles f ?? "" and casts, which the syntactic check misses on the getter side. It would miss Wrapped, because it follows value-preserving steps only, so Trim() and Normalize() end the flow. TaintTracking::localTaint would follow those. However, TaintedMember feeds 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.

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>
@hugo-syn
hugo-syn requested a review from a team as a code owner November 6, 2026 16:51
Copilot AI auto review requested due to automatic review settings November 6, 2026 16:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants