diff --git a/csharp/ql/lib/change-notes/2026-04-01-asp-remote-sources.md b/csharp/ql/lib/change-notes/2026-04-01-asp-remote-sources.md new file mode 100644 index 000000000000..52f3f721e9fa --- /dev/null +++ b/csharp/ql/lib/change-notes/2026-04-01-asp-remote-sources.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Expanded ASP and ASP.NET remote source modeling to cover additional sources, including fields of tainted parameters as well as properties and fields that become tainted transitively. 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 2906fde4e1de..2a74c7844b12 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 @@ -104,7 +104,7 @@ class WcfRemoteFlowSource extends RemoteFlowSource, DataFlow::ParameterNode { } /** A data flow source of remote user input (ASP.NET web service). */ -class AspNetServiceRemoteFlowSource extends RemoteFlowSource, DataFlow::ParameterNode { +class AspNetServiceRemoteFlowSource extends AspNetRemoteFlowSource, DataFlow::ParameterNode { AspNetServiceRemoteFlowSource() { exists(Method m | m.getAParameter() = this.getParameter() and @@ -115,8 +115,50 @@ class AspNetServiceRemoteFlowSource extends RemoteFlowSource, DataFlow::Paramete override string getSourceType() { result = "ASP.NET web service input" } } +private class CandidateMembersToTaint extends Member { + CandidateMembersToTaint() { + this.isPublic() and + not this.isStatic() and + ( + this = + any(Property p | + p.isAutoImplemented() and + p.getGetter().isPublic() and + p.getSetter().isPublic() + ) + or + this = any(Field f | f.isPublic()) + ) + } +} + +/** + * Taint members (transitively) on types used in + * 1. Action method parameters. + * 2. WebMethod parameters. + * + * Note that this also impacts uses of such types in other contexts. + */ +private class AspNetRemoteFlowSourceMember extends TaintTracking::TaintedMember, + CandidateMembersToTaint +{ + AspNetRemoteFlowSourceMember() { + exists(Type t, Type t0 | t = this.getDeclaringType() | + (t = t0 or t = t0.(ArrayType).getElementType()) and + ( + t0 = any(AspNetRemoteFlowSourceMember m).getType() + or + t0 = any(ActionMethodParameter p).getType() + or + t0 = any(AspNetServiceRemoteFlowSource source).getType() + ) + ) + } +} + /** A data flow source of remote user input (ASP.NET request message). */ -class SystemNetHttpRequestMessageRemoteFlowSource extends RemoteFlowSource, DataFlow::ExprNode { +class SystemNetHttpRequestMessageRemoteFlowSource extends AspNetRemoteFlowSource, DataFlow::ExprNode +{ SystemNetHttpRequestMessageRemoteFlowSource() { this.getType() instanceof SystemWebHttpRequestMessageClass } @@ -166,7 +208,7 @@ class MicrosoftOwinRequestRemoteFlowSource extends RemoteFlowSource, DataFlow::E } /** A parameter to an Mvc controller action method, viewed as a source of remote user input. */ -class ActionMethodParameter extends RemoteFlowSource, DataFlow::ParameterNode { +class ActionMethodParameter extends AspNetRemoteFlowSource, DataFlow::ParameterNode { ActionMethodParameter() { exists(Parameter p | p = this.getParameter() and @@ -218,14 +260,18 @@ class AspNetCoreRoutingMethodParameter extends AspNetCoreRemoteFlowSource, DataF * Flow is defined from any ASP.NET Core remote source object to any of its member * properties. */ -private class AspNetCoreRemoteFlowSourceMember extends TaintTracking::TaintedMember, Property { +private class AspNetCoreRemoteFlowSourceMember extends TaintTracking::TaintedMember, + CandidateMembersToTaint +{ AspNetCoreRemoteFlowSourceMember() { - this.getDeclaringType() = any(AspNetCoreRemoteFlowSource source).getType() and - this.isPublic() and - not this.isStatic() and - this.isAutoImplemented() and - this.getGetter().isPublic() and - this.getSetter().isPublic() + exists(Type t, Type t0 | t = this.getDeclaringType() | + (t = t0 or t = t0.(ArrayType).getElementType()) and + ( + t0 = any(AspNetCoreRemoteFlowSourceMember m).getType() + or + t0 = any(AspNetCoreRemoteFlowSource m).getType() + ) + ) } } 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 176f95e4a89d..e554f25f2064 100644 --- a/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/AspRemoteFlowSource.cs +++ b/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/AspRemoteFlowSource.cs @@ -8,7 +8,7 @@ namespace Testing public class ViewModel { public string RequestId { get; set; } // Considered tainted. - public object RequestIdField; // Not considered tainted as it is a field. + public object RequestIdField; // Considered tainted. public string RequestIdOnlyGet { get; } // Not considered tainted as there is no setter. public string RequestIdPrivateSet { get; private set; } // Not considered tainted as it has a private setter. public static object RequestIdStatic { get; set; } // Not considered tainted as it is static. 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 a7442a80839c..d729eb939d28 100644 --- a/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/aspRemoteFlowSource.expected +++ b/csharp/ql/test/library-tests/dataflow/flowsources/aspremote/aspRemoteFlowSource.expected @@ -1,5 +1,6 @@ remoteFlowSourceMembers | AspRemoteFlowSource.cs:10:23:10:31 | RequestId | +| AspRemoteFlowSource.cs:11:23:11:36 | RequestIdField | | AspRemoteFlowSource.cs:28:23:28:29 | Tainted | remoteFlowSources | AspRemoteFlowSource.cs:20:42:20:50 | viewModel | 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 5889183f5257..3c7cbcba04a2 100644 --- a/csharp/ql/test/library-tests/dataflow/flowsources/remote/RemoteFlowSource.cs +++ b/csharp/ql/test/library-tests/dataflow/flowsources/remote/RemoteFlowSource.cs @@ -54,3 +54,51 @@ public static async void M3(System.Net.WebSockets.WebSocket webSocket) } } } + +namespace AspRemoteFlowSource +{ + using System.Web.Services; + + public class MySubData + { + public string SubDataProp { get; set; } + } + + public class MyElementSubData + { + public string ElementSubDataProp { get; set; } + } + + public class MyData + { + public string DataField; + public string DataProp { get; set; } + public MySubData SubData { get; set; } + public MyElementSubData[] Elements { get; set; } + } + + public class MyDataElement + { + public string Prop { get; set; } + } + + + public class MyService + { + [WebMethod] + public void MyMethod(MyData data) + { + Use(data.DataProp); + Use(data.SubData.SubDataProp); + Use(data.Elements[0].ElementSubDataProp); + } + + [WebMethod] + public void MyMethod2(MyDataElement[] data) + { + Use(data[0].Prop); + } + + public static void Use(object o) { } + } +} 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 f5f541d73d53..ef70ca9ad93a 100644 --- a/csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.expected +++ b/csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.expected @@ -1,3 +1,13 @@ +remoteFlowSourceMembers +| Controller.cs:6:19:6:25 | Tainted | +| RemoteFlowSource.cs:64:23:64:33 | SubDataProp | +| RemoteFlowSource.cs:69:23:69:40 | ElementSubDataProp | +| RemoteFlowSource.cs:74:23:74:31 | DataField | +| RemoteFlowSource.cs:75:23:75:30 | DataProp | +| RemoteFlowSource.cs:76:26:76:32 | SubData | +| RemoteFlowSource.cs:77:35:77:42 | Elements | +| RemoteFlowSource.cs:82:23:82:26 | Prop | +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 | | Controller.cs:16:43:16:52 | sampleData | ASP.NET MVC action method parameter | @@ -10,3 +20,5 @@ | 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:89:37:89:40 | data | ASP.NET web service input | +| RemoteFlowSource.cs:97:47:97:50 | data | ASP.NET web service input | diff --git a/csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.ql b/csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.ql index fdea5323d5cb..f6d87eb9ff4f 100644 --- a/csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.ql +++ b/csharp/ql/test/library-tests/dataflow/flowsources/remote/remoteFlowSource.ql @@ -1,5 +1,7 @@ import semmle.code.csharp.security.dataflow.flowsources.Remote -from RemoteFlowSource source -where source.getLocation().getFile().fromSource() -select source, source.getSourceType() +query predicate remoteFlowSourceMembers(TaintTracking::TaintedMember m) { m.fromSource() } + +query predicate remoteFlowSources(RemoteFlowSource source, string type) { + source.getLocation().getFile().fromSource() and type = source.getSourceType() +} diff --git a/csharp/ql/test/resources/stubs/System.Web.cs b/csharp/ql/test/resources/stubs/System.Web.cs index c15b871095ff..23ae0f298cf4 100644 --- a/csharp/ql/test/resources/stubs/System.Web.cs +++ b/csharp/ql/test/resources/stubs/System.Web.cs @@ -454,3 +454,8 @@ public class SimpleTypeResolver : System.Web.Script.Serialization.JavaScriptType public SimpleTypeResolver() => throw null; } } + +namespace System.Web.Services +{ + public class WebMethodAttribute : Attribute { } +}