C#: Streamline the implementation for ASP.NET Core tainted members.

Author: michaelnebel

csharp/ql/lib/semmle/code/csharp/security/dataflow/flowsources/Remote.qll

@@ -115,14 +115,33 @@ class AspNetServiceRemoteFlowSource extends AspNetRemoteFlowSource, DataFlow::Pa
   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 {
+private class AspNetRemoteFlowSourceMember extends TaintTracking::TaintedMember,
+  CandidateMembersToTaint
+{
   AspNetRemoteFlowSourceMember() {
     exists(Type t, Type t0 | t = this.getDeclaringType() |
       (t = t0 or t = t0.(ArrayType).getElementType()) and
@@ -133,18 +152,6 @@ private class AspNetRemoteFlowSourceMember extends TaintTracking::TaintedMember
         or
         t0 = any(AspNetServiceRemoteFlowSource source).getType()
       )
-    ) and
-    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())
     )
   }
 }
@@ -253,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()
+      )
+    )
   }
 }
 

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.

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 |