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() ) 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. 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..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 +1 @@ -| ProfileController.cs:9:25:9:31 | Delete1 | This action is missing an authorization check. | +| ProfileController.cs:12:25:12: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..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,19 +1,25 @@ using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Authorization; -public class ProfileController : Controller { +public class RequirePermissionAttribute : AuthorizeAttribute { } + +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,35 +28,49 @@ 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(); } + // GOOD: The RequirePermission attribute is used (which extends AuthorizeAttribute). + [RequirePermission] + public ActionResult Delete4(int id) + { + doThings(); + return View(); + } } [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 +}