diff --git a/csharp/ql/lib/change-notes/2026-11-06-taint-plain-data-members.md b/csharp/ql/lib/change-notes/2026-11-06-taint-plain-data-members.md new file mode 100644 index 000000000000..17f806bc70e5 --- /dev/null +++ b/csharp/ql/lib/change-notes/2026-11-06-taint-plain-data-members.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Tainted members of ASP.NET and ASP.NET Core remote flow source types now also include public properties whose accessors wrap a backing field, public get-only properties of collection type, and members inherited from base types. Constants are no longer tainted. diff --git a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll index 2a04d9c24c3d..85565d16154e 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll @@ -117,17 +117,54 @@ class AspNetServiceRemoteFlowSource extends AspNetRemoteFlowSource, DataFlow::Pa override string getSourceType() { result = "ASP.NET web service input" } } -/** A public, non-static, auto-implemented property or field, candidate for taint-tracking. */ +/** + * Holds if `p` is auto-implemented, or its getter returns an expression reading a field + * and its setter (if any) assigns an expression using `value` to that field. + * Properties that compute a value do not qualify. + */ +private predicate isPlainDataProperty(Property p) { + p.isAutoImplemented() + or + p.isAutoImplementedReadOnly() + or + exists(Field f | f.getDeclaringType() = p.getDeclaringType() | + exists(FieldRead fr, Expr ret | + fr.getTarget() = f and + fr.getParent*() = ret and + ( + ret = any(ReturnStmt r | r.getEnclosingCallable() = p.getGetter()).getExpr() + or + ret = p.getGetter().getExpressionBody() + ) + ) and + ( + not exists(p.getSetter()) + or + exists(AssignExpr ae, ParameterAccess v | + ae.getLValue().(FieldWrite).getTarget() = f and + ae.getEnclosingCallable() = p.getSetter() and + v.getTarget().hasName("value") and + v.getParent*() = ae.getRValue() + ) + ) + ) +} + +/** + * A public, non-static field, or a public plain-data property that is either settable or + * of collection type. Binders such as `XmlSerializer` populate a get-only collection in place. + */ class CandidateMemberToTaint extends Member { CandidateMemberToTaint() { this.isPublic() and not this.isStatic() and + not this.(Field).isConst() and ( this = any(Property p | - p.isAutoImplemented() and + isPlainDataProperty(p) and p.getGetter().isPublic() and - p.getSetter().isPublic() + (p.getSetter().isPublic() or p.getType() instanceof CollectionType) ) or this = any(Field f | f.isPublic()) @@ -135,6 +172,16 @@ class CandidateMemberToTaint extends Member { } } +/** + * Holds if `m` is declared on `t0`, on a base type of `t0`, or on (a base type of) + * the element type of `t0` when `t0` is a collection. + */ +private predicate isMemberOfBoundType(Member m, Type t0) { + exists(Type t | t = t0 or t = t0.(CollectionType).getElementType() | + m.getDeclaringType() = t.(ValueOrRefType).getABaseType*() + ) +} + /** * Taint members (transitively) on types used in * 1. Action method parameters. @@ -146,8 +193,8 @@ private class AspNetRemoteFlowSourceMember extends TaintTracking::TaintedMember, CandidateMemberToTaint { AspNetRemoteFlowSourceMember() { - exists(Type t, Type t0 | t = this.getDeclaringType() | - (t = t0 or t = t0.(CollectionType).getElementType()) and + exists(Type t0 | + isMemberOfBoundType(this, t0) and ( t0 = any(AspNetRemoteFlowSourceMember m).getType() or @@ -267,8 +314,8 @@ private class AspNetCoreRemoteFlowSourceMember extends TaintTracking::TaintedMem CandidateMemberToTaint { AspNetCoreRemoteFlowSourceMember() { - exists(Type t, Type t0 | t = this.getDeclaringType() | - (t = t0 or t = t0.(CollectionType).getElementType()) and + exists(Type t0 | + isMemberOfBoundType(this, t0) and ( t0 = any(AspNetCoreRemoteFlowSourceMember m).getType() or diff --git a/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/AspRemoteFlowSource.cs b/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/AspRemoteFlowSource.cs index d7b085ef61fd..c8842a43b227 100644 --- a/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/AspRemoteFlowSource.cs +++ b/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/AspRemoteFlowSource.cs @@ -2,6 +2,7 @@ using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.DependencyInjection; using System; +using System.Collections.Generic; namespace Testing { @@ -16,8 +17,53 @@ public class ViewModel private string RequestIdPrivate { get; set; } // Not considered tainted as it is private. } + public class BaseModel + { + public string BaseProp { get; set; } // Considered tainted, as it is inherited by `DerivedModel`. + } + + public class DerivedModel : BaseModel { } + + public class PlainDataModel + { + private string backing; + private List items = new List(); + + public string Backed // Considered tainted. Accessors wrap a backing field. + { + get { if (backing == null) backing = ""; return backing; } + set { backing = value ?? throw new ArgumentNullException(); } + } + + public List ItemsOnlyGet => items; // Considered tainted. Get-only collection. + public List ItemsAutoOnlyGet { get; } = new List(); // Considered tainted. Get-only collection. + public string BackedOnlyGet => backing; // Not considered tainted as there is no setter. + public List ItemsCopy => new List(items); // Not considered tainted as the getter computes a value. + public string Computed // Not considered tainted as the getter computes a value. + { + get { return backing + "x"; } + set { backing = value; } + } + public const string Constant = "x"; // Not considered tainted as it is a constant. + public short Number { get; set; } // Considered tainted. `short.MaxValue` is not. + } + + public class FlowProps + { + private string f; + public string Discard { get { return ""; } set { } } // Not considered tainted. Ignores the value. + public string Coalesce { get { return f ?? ""; } set { f = value; } } // Considered tainted. + public string Cast { get { return f; } set { f = (string)(object)value; } } // Considered tainted. + public string Ignore { get { return f; } set { f = "x"; } } // Not considered tainted. Setter ignores the value. + } + public class TestController : Controller { + public object MyAction(PlainDataModel plain, DerivedModel derived, FlowProps fp) + { + throw null; + } + public object MyAction(ViewModel viewModel) { throw null; diff --git a/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/aspRemoteFlowSource.expected b/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/aspRemoteFlowSource.expected index 6ba2741857b5..3aa471a7f992 100644 --- a/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/aspRemoteFlowSource.expected +++ b/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/aspRemoteFlowSource.expected @@ -1,27 +1,39 @@ remoteFlowSourceMembers -| AspRemoteFlowSource.cs:11:23:11:31 | RequestId | -| AspRemoteFlowSource.cs:12:23:12:36 | RequestIdField | -| AspRemoteFlowSource.cs:50:23:50:29 | Tainted | +| AspRemoteFlowSource.cs:12:23:12:31 | RequestId | +| AspRemoteFlowSource.cs:13:23:13:36 | RequestIdField | +| AspRemoteFlowSource.cs:22:23:22:30 | BaseProp | +| AspRemoteFlowSource.cs:32:23:32:28 | Backed | +| AspRemoteFlowSource.cs:38:29:38:40 | ItemsOnlyGet | +| AspRemoteFlowSource.cs:39:29:39:44 | ItemsAutoOnlyGet | +| AspRemoteFlowSource.cs:41:29:41:37 | ItemsCopy | +| AspRemoteFlowSource.cs:42:23:42:30 | Computed | +| AspRemoteFlowSource.cs:48:22:48:27 | Number | +| AspRemoteFlowSource.cs:55:23:55:30 | Coalesce | +| AspRemoteFlowSource.cs:56:23:56:26 | Cast | +| AspRemoteFlowSource.cs:96:23:96:29 | Tainted | remoteFlowSources -| AspRemoteFlowSource.cs:21:42:21:50 | viewModel | -| AspRemoteFlowSource.cs:27:20:27:27 | ordinary | -| AspRemoteFlowSource.cs:28:32:28:40 | fromQuery | -| AspRemoteFlowSource.cs:29:31:29:38 | fromBody | -| AspRemoteFlowSource.cs:30:32:30:40 | fromRoute | -| AspRemoteFlowSource.cs:31:33:31:42 | fromHeader | -| AspRemoteFlowSource.cs:57:42:57:46 | param | -| AspRemoteFlowSource.cs:65:58:65:63 | newUrl | -| AspRemoteFlowSource.cs:66:61:66:65 | myApi | -| AspRemoteFlowSource.cs:66:75:66:79 | myUrl | -| AspRemoteFlowSource.cs:68:46:68:56 | lambdaParam | -| AspRemoteFlowSource.cs:74:65:74:76 | mapPostParam | -| AspRemoteFlowSource.cs:75:63:75:73 | mapPutParam | -| AspRemoteFlowSource.cs:76:69:76:82 | mapDeleteParam | -| AspRemoteFlowSource.cs:78:41:78:44 | item | -| AspRemoteFlowSource.cs:93:34:93:35 | id | -| AspRemoteFlowSource.cs:95:35:95:41 | command | -| AspRemoteFlowSource.cs:95:48:95:52 | count | -| AspRemoteFlowSource.cs:97:40:97:43 | data | -| AspRemoteFlowSource.cs:99:34:99:38 | value | -| AspRemoteFlowSource.cs:101:37:101:42 | itemId | -| AspRemoteFlowSource.cs:114:35:114:46 | derivedParam | +| AspRemoteFlowSource.cs:62:47:62:51 | plain | +| AspRemoteFlowSource.cs:62:67:62:73 | derived | +| AspRemoteFlowSource.cs:62:86:62:87 | fp | +| AspRemoteFlowSource.cs:67:42:67:50 | viewModel | +| AspRemoteFlowSource.cs:73:20:73:27 | ordinary | +| AspRemoteFlowSource.cs:74:32:74:40 | fromQuery | +| AspRemoteFlowSource.cs:75:31:75:38 | fromBody | +| AspRemoteFlowSource.cs:76:32:76:40 | fromRoute | +| AspRemoteFlowSource.cs:77:33:77:42 | fromHeader | +| AspRemoteFlowSource.cs:103:42:103:46 | param | +| AspRemoteFlowSource.cs:111:58:111:63 | newUrl | +| AspRemoteFlowSource.cs:112:61:112:65 | myApi | +| AspRemoteFlowSource.cs:112:75:112:79 | myUrl | +| AspRemoteFlowSource.cs:114:46:114:56 | lambdaParam | +| AspRemoteFlowSource.cs:120:65:120:76 | mapPostParam | +| AspRemoteFlowSource.cs:121:63:121:73 | mapPutParam | +| AspRemoteFlowSource.cs:122:69:122:82 | mapDeleteParam | +| AspRemoteFlowSource.cs:124:41:124:44 | item | +| AspRemoteFlowSource.cs:139:34:139:35 | id | +| AspRemoteFlowSource.cs:141:35:141:41 | command | +| AspRemoteFlowSource.cs:141:48:141:52 | count | +| AspRemoteFlowSource.cs:143:40:143:43 | data | +| AspRemoteFlowSource.cs:145:34:145:38 | value | +| AspRemoteFlowSource.cs:147:37:147:42 | itemId | +| AspRemoteFlowSource.cs:160:35:160:46 | derivedParam | diff --git a/csharp/ql/test/library-tests/dataflow/flowsources/remote/RemoteFlowSource.cs b/csharp/ql/test/library-tests/dataflow/flowsources/remote/RemoteFlowSource.cs index d54a359aca0c..85b92d44bb67 100644 --- a/csharp/ql/test/library-tests/dataflow/flowsources/remote/RemoteFlowSource.cs +++ b/csharp/ql/test/library-tests/dataflow/flowsources/remote/RemoteFlowSource.cs @@ -90,8 +90,50 @@ public class MyDataElement } + public class BaseData + { + public string BaseProp { get; set; } + } + + public class DerivedData : BaseData { } + + public class PlainData + { + private string backing; + private List items = new List(); + + public string Backed + { + get { if (backing == null) backing = ""; return backing; } + set { backing = value ?? throw new ArgumentNullException(); } + } + + public List ItemsOnlyGet => items; + public string BackedOnlyGet => backing; + public List ItemsCopy => new List(items); + public string Computed + { + get { return backing + "x"; } + set { backing = value; } + } + public const string Constant = "x"; + public short Number { get; set; } + } + + public class FlowProps + { + private string f; + public string Discard { get { return ""; } set { } } // Not considered tainted. Ignores the value. + public string Coalesce { get { return f ?? ""; } set { f = value; } } // Considered tainted. + public string Cast { get { return f; } set { f = (string)(object)value; } } // Considered tainted. + public string Ignore { get { return f; } set { f = "x"; } } // Not considered tainted. Setter ignores the value. + } + public class MyService { + [WebMethod] + public void MyMethod3(PlainData plain, DerivedData derived, FlowProps fp) { } + [WebMethod] public void MyMethod(MyData data) { diff --git a/csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.expected b/csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.expected index 242080e6bda2..f5c08f5dbc55 100644 --- a/csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.expected +++ b/csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.expected @@ -9,6 +9,14 @@ remoteFlowSourceMembers | RemoteFlowSource.cs:83:35:83:42 | Elements | | RemoteFlowSource.cs:84:38:84:41 | List | | RemoteFlowSource.cs:89:23:89:26 | Prop | +| RemoteFlowSource.cs:95:23:95:30 | BaseProp | +| RemoteFlowSource.cs:105:23:105:28 | Backed | +| RemoteFlowSource.cs:111:29:111:40 | ItemsOnlyGet | +| RemoteFlowSource.cs:113:29:113:37 | ItemsCopy | +| RemoteFlowSource.cs:114:23:114:30 | Computed | +| RemoteFlowSource.cs:120:22:120:27 | Number | +| RemoteFlowSource.cs:127:23:127:30 | Coalesce | +| RemoteFlowSource.cs:128:23:128:26 | Cast | remoteFlowSources | Controller.cs:11:43:11:52 | sampleData | ASP.NET MVC action method parameter | | Controller.cs:11:62:11:66 | taint | ASP.NET MVC action method parameter | @@ -22,5 +30,8 @@ remoteFlowSources | RemoteFlowSource.cs:45:17:45:23 | access to parameter request | ASP.NET query string | | RemoteFlowSource.cs:45:17:45:42 | access to property RawUrl | ASP.NET unvalidated request data | | RemoteFlowSource.cs:52:55:52:61 | [post] access to local variable segment | external | -| RemoteFlowSource.cs:96:37:96:40 | data | ASP.NET web service input | -| RemoteFlowSource.cs:105:47:105:50 | data | ASP.NET web service input | +| RemoteFlowSource.cs:135:41:135:45 | plain | ASP.NET web service input | +| RemoteFlowSource.cs:135:60:135:66 | derived | ASP.NET web service input | +| RemoteFlowSource.cs:135:79:135:80 | fp | ASP.NET web service input | +| RemoteFlowSource.cs:138:37:138:40 | data | ASP.NET web service input | +| RemoteFlowSource.cs:147:47:147:50 | data | ASP.NET web service input |