Merge pull request #17273 from michaelnebel/csharp/sqlinject

C#: ASP.NET Controller is allowed to be abstract.

Author: michaelnebel

csharp/ql/lib/change-notes/2024-08-21-abstract-asp-controller.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Parameters of public methods in abstract controller-like classes are now considered remote flow sources.

csharp/ql/lib/semmle/code/csharp/frameworks/microsoft/AspNetCore.qll

@@ -205,7 +205,6 @@ class MicrosoftAspNetCoreMvcController extends Class {
       )
     ) and
     this.isPublic() and
-    (not this.isAbstract() or this instanceof MicrosoftAspNetCoreMvcControllerBaseClass) and
     not this instanceof Generic and
     (
       this.getABaseType*() instanceof MicrosoftAspNetCoreMvcControllerBaseClass

csharp/ql/test/library-tests/dataflow/flowsources/aspremote/AspRemoteFlowSource.cs

@@ -58,4 +58,9 @@ public void M1(string[] args)
             app.Run();
         }
     }
-}
+
+    public abstract class AbstractTestController : Controller
+    {
+        public void MyActionMethod(string param) { }
+    }
+}

csharp/ql/test/library-tests/dataflow/flowsources/aspremote/aspRemoteFlowSource.expected

@@ -12,3 +12,4 @@ remoteFlowSources
 | AspRemoteFlowSource.cs:53:63:53:73 | mapPutParam |
 | AspRemoteFlowSource.cs:54:69:54:82 | mapDeleteParam |
 | AspRemoteFlowSource.cs:56:41:56:44 | item |
+| AspRemoteFlowSource.cs:64:43:64:47 | param |

csharp/ql/test/library-tests/frameworks/microsoft/AspNetCore.cs

@@ -56,17 +56,17 @@ public string Index()
     }
 }
 
-// is not public
-internal class NotHomeController : Controller
+// is abstract
+public abstract class HomeController6 : Controller
 {
     public string Index()
     {
         return "This is Home Controller";
     }
 }
 
-// is abstract
-public abstract class NotHomeController2 : Controller
+// is not public
+internal class NotHomeController : Controller
 {
     public string Index()
     {
@@ -75,7 +75,7 @@ public string Index()
 }
 
 // contains generic parameters
-public class NotHomeController3<T> : Controller
+public class NotHomeController2<T> : Controller
 {
     public string Index()
     {
@@ -85,7 +85,7 @@ public string Index()
 
 // has [NonController] attribute
 [NonController]
-public class NotHomeController4 : Controller
+public class NotHomeController3 : Controller
 {
     public string Index()
     {
@@ -94,10 +94,10 @@ public string Index()
 }
 
 // derived from a class that has [NonController] attribute
-public class NotController : NotHomeController4
+public class NotController : NotHomeController3
 {
     public string Index()
     {
         return "This is Home Controller";
     }
-}
+}

csharp/ql/test/library-tests/frameworks/microsoft/AspNetCore.expected

@@ -4,3 +4,4 @@
 | AspNetCore.cs:32:14:32:28 | HomeController3 |
 | AspNetCore.cs:42:14:42:28 | HomeController4 |
 | AspNetCore.cs:51:14:51:28 | HomeController5 |
+| AspNetCore.cs:60:23:60:37 | HomeController6 |

csharp/ql/test/query-tests/Security Features/CWE-089/SqlInjection.cs

@@ -10,7 +10,12 @@ namespace Test
     using System.Data;
     using System.Data.Entity;
     using System.Data.SqlClient;
+    using System.Diagnostics.CodeAnalysis;
+    using System.Threading;
+    using System.Threading.Tasks;
     using System.Web.UI.WebControls;
+    using Microsoft.AspNetCore.Http;
+    using Microsoft.AspNetCore.Mvc;
 
     public class EntityFrameworkContext : DbContext
     {
@@ -110,4 +115,28 @@ public void GetDataSetByCategory()
 
         System.Windows.Forms.TextBox box1;
     }
+
+    public abstract class MyController : Controller
+    {
+        [HttpPost("{userId:string}")]
+        public async Task<IActionResult> GetUserById([FromRoute] string userId, CancellationToken cancellationToken)
+        {
+            // This is a vulnerable method due to SQL injection
+            string query = "SELECT * FROM Users WHERE UserId = '" + userId + "'";
+
+            using (SqlConnection connection = new SqlConnection("YourConnectionString"))
+            {
+                SqlCommand command = new SqlCommand(query, connection);
+                connection.Open();
+
+                SqlDataReader reader = command.ExecuteReader();
+                while (reader.Read())
+                {
+                    Console.WriteLine(String.Format("{0}, {1}", reader["UserId"], reader["Username"]));
+                }
+            }
+
+            return Ok();
+        }
+    }
 }

csharp/ql/test/query-tests/Security Features/CWE-089/SqlInjection.expected

@@ -3,3 +3,4 @@ semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resour
 semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resources/stubs/System.Data.SqlClient/4.8.5/System.Data.SqlClient.csproj
 semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resources/stubs/System.Data.SQLite/1.0.118/System.Data.SQLite.csproj
 semmle-extractor-options: ${testdir}/../../../resources/stubs/System.Windows.cs
+semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resources/stubs/_frameworks/Microsoft.AspNetCore.App/Microsoft.AspNetCore.App.csproj