From 8d571672e9256ce021f556bebe167f32b7ef4043 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 14 Apr 2025 13:54:43 +0200 Subject: [PATCH 1/5] C#: Convert cs/missing-access-control to inline expectations test. --- .../MVCTests/MissingAccessControl.expected | 2 +- .../MVCTests/MissingAccessControl.qlref | 5 ++- .../MVCTests/ProfileController.cs | 35 ++++++++++++------- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.expected b/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.expected index 87fc29167beb..b0cd401adf00 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.expected +++ b/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.expected @@ -1 +1 @@ -| ProfileController.cs:9:25:9:31 | Delete1 | This action is missing an authorization check. | +| ProfileController.cs:10:25:10:31 | Delete1 | This action is missing an authorization check. | diff --git a/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.qlref b/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.qlref index a4173778d9fa..304adda34284 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.qlref +++ b/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.qlref @@ -1 +1,4 @@ -Security Features/CWE-285/MissingAccessControl.ql +query: Security Features/CWE-285/MissingAccessControl.ql +postprocess: + - utils/test/PrettyPrintModels.ql + - utils/test/InlineExpectationsTestQuery.ql diff --git a/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/ProfileController.cs b/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/ProfileController.cs index 8fb88eacd3ef..071dd9e9839a 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/ProfileController.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/ProfileController.cs @@ -1,19 +1,23 @@ using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Authorization; -public class ProfileController : Controller { +public class ProfileController : Controller +{ private void doThings() { } private bool isAuthorized() { return false; } // BAD: This is a Delete method, but no auth is specified. - public ActionResult Delete1(int id) { + public ActionResult Delete1(int id) // $ Alert + { doThings(); return View(); } // GOOD: isAuthorized is checked. - public ActionResult Delete2(int id) { - if (!isAuthorized()) { + public ActionResult Delete2(int id) + { + if (!isAuthorized()) + { return null; } doThings(); @@ -22,7 +26,8 @@ public ActionResult Delete2(int id) { // GOOD: The Authorize attribute is used. [Authorize] - public ActionResult Delete3(int id) { + public ActionResult Delete3(int id) + { doThings(); return View(); } @@ -30,27 +35,33 @@ public ActionResult Delete3(int id) { } [Authorize] -public class AuthBaseController : Controller { +public class AuthBaseController : Controller +{ protected void doThings() { } } -public class SubController : AuthBaseController { +public class SubController : AuthBaseController +{ // GOOD: The Authorize attribute is used on the base class. - public ActionResult Delete4(int id) { + public ActionResult Delete4(int id) + { doThings(); return View(); } } [Authorize] -public class AuthBaseGenericController : Controller { +public class AuthBaseGenericController : Controller +{ protected void doThings() { } } -public class SubGenericController : AuthBaseGenericController { +public class SubGenericController : AuthBaseGenericController +{ // GOOD: The Authorize attribute is used on the base class. - public ActionResult Delete5(int id) { + public ActionResult Delete5(int id) + { doThings(); return View(); } -} \ No newline at end of file +} From 2e7e276806a82a12ed0d2310cc7fbd138490a3c5 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 14 Apr 2025 14:18:30 +0200 Subject: [PATCH 2/5] C#: Add test case for authorization attribute that extends Authorize. --- .../MVCTests/MissingAccessControl.expected | 6 +++++- .../MissingAccessControl/MVCTests/ProfileController.cs | 9 +++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.expected b/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.expected index b0cd401adf00..539a53c21937 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.expected +++ b/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.expected @@ -1 +1,5 @@ -| ProfileController.cs:10:25:10:31 | Delete1 | This action is missing an authorization check. | +#select +| ProfileController.cs:12:25:12:31 | Delete1 | This action is missing an authorization check. | +| ProfileController.cs:39:25:39:31 | Delete4 | This action is missing an authorization check. | +testFailures +| ProfileController.cs:39:25:39:31 | This action is missing an authorization check. | Unexpected result: Alert | diff --git a/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/ProfileController.cs b/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/ProfileController.cs index 071dd9e9839a..9c20313b84b7 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/ProfileController.cs +++ b/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/ProfileController.cs @@ -1,6 +1,8 @@ using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Authorization; +public class RequirePermissionAttribute : AuthorizeAttribute { } + public class ProfileController : Controller { private void doThings() { } @@ -32,6 +34,13 @@ public ActionResult Delete3(int id) return View(); } + // GOOD: The RequirePermission attribute is used (which extends AuthorizeAttribute). + [RequirePermission] + public ActionResult Delete4(int id) + { + doThings(); + return View(); + } } [Authorize] From c15d1ab3bdd92f57a4a0901508dcd312fef21216 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 14 Apr 2025 14:25:31 +0200 Subject: [PATCH 3/5] C#: Consider an attribute to be authorization like, if it extends an attribute that has an authorization like name. --- .../security/auth/MissingFunctionLevelAccessControlQuery.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll index f43d42b87142..0e3074065f46 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll @@ -81,7 +81,7 @@ predicate hasAuthViaXml(ActionMethod m) { /** Holds if the given action has an attribute that indications authorization. */ predicate hasAuthViaAttribute(ActionMethod m) { - exists(Attribute attr | attr.getType().getName().toLowerCase().matches("%auth%") | + exists(Attribute attr | attr.getType().getABaseType*().getName().toLowerCase().matches("%auth%") | attr = m.getOverridee*().getAnAttribute() or attr = getAnUnboundBaseType*(m.getDeclaringType()).getAnAttribute() ) From f11aec35921acb2443b6dbe80bc8a067a96f6ad7 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Mon, 14 Apr 2025 14:26:51 +0200 Subject: [PATCH 4/5] C#: Update test expected output. --- .../MVCTests/MissingAccessControl.expected | 4 ---- 1 file changed, 4 deletions(-) diff --git a/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.expected b/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.expected index 539a53c21937..513fab4804a9 100644 --- a/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.expected +++ b/csharp/ql/test/query-tests/Security Features/CWE-285/MissingAccessControl/MVCTests/MissingAccessControl.expected @@ -1,5 +1 @@ -#select | ProfileController.cs:12:25:12:31 | Delete1 | This action is missing an authorization check. | -| ProfileController.cs:39:25:39:31 | Delete4 | This action is missing an authorization check. | -testFailures -| ProfileController.cs:39:25:39:31 | This action is missing an authorization check. | Unexpected result: Alert | From 0b10d34cae0ca325a93e5c90b25186d4779754a4 Mon Sep 17 00:00:00 2001 From: Michael Nebel Date: Tue, 15 Apr 2025 10:53:40 +0200 Subject: [PATCH 5/5] C#: Add change note. --- .../2025-04-15-missing-function-level-access-control.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 csharp/ql/src/change-notes/2025-04-15-missing-function-level-access-control.md diff --git a/csharp/ql/src/change-notes/2025-04-15-missing-function-level-access-control.md b/csharp/ql/src/change-notes/2025-04-15-missing-function-level-access-control.md new file mode 100644 index 000000000000..dbea56dbeb84 --- /dev/null +++ b/csharp/ql/src/change-notes/2025-04-15-missing-function-level-access-control.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Improved detection of authorization checks in the `cs/web/missing-function-level-access-control` query. The query now recognizes authorization attributes inherited from base classes and interfaces.