diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/ActionMethods.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/ActionMethods.qll new file mode 100644 index 000000000000..81ba6c70a06b --- /dev/null +++ b/csharp/ql/lib/semmle/code/csharp/security/auth/ActionMethods.qll @@ -0,0 +1,107 @@ +/** Common definitions for queries checking for access control measures on action methods. */ + +import csharp +import semmle.code.csharp.frameworks.microsoft.AspNetCore +import semmle.code.csharp.frameworks.system.web.UI + +/** A method representing an action for a web endpoint. */ +abstract class ActionMethod extends Method { + /** + * Gets a string that can indicate what this method does to determine if it should have an auth check; + * such as its method name, class name, or file path. + */ + string getADescription() { + result = [this.getName(), this.getDeclaringType().getBaseClass*().getName(), this.getARoute()] + } + + /** Holds if this method may represent a stateful action such as editing or deleting */ + predicate isEdit() { + exists(string str | + str = + this.getADescription() + // separate camelCase words + .regexpReplaceAll("([a-z])([A-Z])", "$1_$2") and + str.regexpMatch("(?i).*(edit|delete|modify|change).*") and + not str.regexpMatch("(?i).*(on_?change|changed).*") + ) + } + + /** Holds if this method may be intended to be restricted to admin users */ + predicate isAdmin() { + this.getADescription() + // separate camelCase words + .regexpReplaceAll("([a-z])([A-Z])", "$1_$2") + .regexpMatch("(?i).*(admin|superuser).*") + } + + /** Gets a callable for which if it contains an auth check, this method should be considered authenticated. */ + Callable getAnAuthorizingCallable() { result = this } + + /** + * Gets a possible url route that could refer to this action, + * which would be covered by `` configurations specifying a prefix of it. + */ + string getARoute() { result = this.getDeclaringType().getFile().getRelativePath() } +} + +/** An action method in the MVC framework. */ +private class MvcActionMethod extends ActionMethod { + MvcActionMethod() { this = any(MicrosoftAspNetCoreMvcController c).getAnActionMethod() } +} + +/** An action method on a subclass of `System.Web.UI.Page`. */ +private class WebFormActionMethod extends ActionMethod { + WebFormActionMethod() { + this.getDeclaringType().getBaseClass+() instanceof SystemWebUIPageClass and + this.getAParameter().getType().getName().matches("%EventArgs") + } + + override Callable getAnAuthorizingCallable() { + result = super.getAnAuthorizingCallable() + or + pageLoad(result, this.getDeclaringType()) + } + + override string getARoute() { + exists(string physicalRoute | physicalRoute = super.getARoute() | + result = physicalRoute + or + exists(string absolutePhysical | + virtualRouteMapping(result, absolutePhysical) and + physicalRouteMatches(absolutePhysical, physicalRoute) + ) + ) + } +} + +pragma[nomagic] +private predicate pageLoad(Callable c, Type decl) { + c.getName() = "Page_Load" and + decl = c.getDeclaringType() +} + +/** + * Holds if `virtualRoute` is a URL path + * that can map to the corresponding `physicalRoute` filepath + * through a call to `MapPageRoute` + */ +private predicate virtualRouteMapping(string virtualRoute, string physicalRoute) { + exists(MethodCall mapPageRouteCall, StringLiteral virtualLit, StringLiteral physicalLit | + mapPageRouteCall + .getTarget() + .hasQualifiedName("System.Web.Routing", "RouteCollection", "MapPageRoute") and + virtualLit = mapPageRouteCall.getArgument(1) and + physicalLit = mapPageRouteCall.getArgument(2) and + virtualLit.getValue() = virtualRoute and + physicalLit.getValue() = physicalRoute + ) +} + +/** Holds if the filepath `route` can refer to `actual` after expanding a '~". */ +bindingset[route, actual] +private predicate physicalRouteMatches(string route, string actual) { + route = actual + or + route.charAt(0) = "~" and + exists(string dir | actual = dir + route.suffix(1) + ".cs") +} diff --git a/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll new file mode 100644 index 000000000000..6325c4ff3b37 --- /dev/null +++ b/csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll @@ -0,0 +1,96 @@ +/** Definitions for the Insecure Direct Object Reference query */ + +import csharp +import semmle.code.csharp.dataflow.flowsources.Remote +import ActionMethods + +/** + * Holds if `m` is a method that may require checks + * against the current user to modify a particular resource. + */ +// We exclude admin methods as it may be expected that an admin user should be able to modify any resource. +// Other queries check that there are authorization checks in place for admin methods. +private predicate needsChecks(ActionMethod m) { m.isEdit() and not m.isAdmin() } + +/** + * Holds if `m` has a parameter or access a remote flow source + * that may indicate that it's used as the ID for some resource + */ +private predicate hasIdParameter(ActionMethod m) { + exists(RemoteFlowSource src | src.getEnclosingCallable() = m | + src.asParameter().getName().toLowerCase().matches(["%id", "%idx"]) + or + // handle cases like `Request.QueryString["Id"]` + exists(StringLiteral idStr, IndexerCall idx | + idStr.getValue().toLowerCase().matches(["%id", "%idx"]) and + TaintTracking::localTaint(src, DataFlow::exprNode(idx.getQualifier())) and + DataFlow::localExprFlow(idStr, idx.getArgument(0)) + ) + ) +} + +private predicate authorizingCallable(Callable c) { + exists(string name | name = c.getName().toLowerCase() | + name.matches(["%user%", "%session%"]) and + not name.matches("%get%by%") // methods like `getUserById` or `getXByUsername` aren't likely to be referring to the current user + ) +} + +/** Holds if `m` at some point in its call graph may make some kind of check against the current user. */ +private predicate checksUser(ActionMethod m) { + exists(Callable c | + authorizingCallable(c) and + callsPlus(m, c) + ) +} + +private predicate calls(Callable c1, Callable c2) { c1.calls(c2) } + +private predicate callsPlus(Callable c1, Callable c2) = fastTC(calls/2)(c1, c2) + +/** Holds if `m`, its containing class, or a parent class has an attribute that extends `AuthorizeAttribute` */ +private predicate hasAuthorizeAttribute(ActionMethod m) { + exists(Attribute attr | + attr.getType() + .getABaseType*() + .hasQualifiedName([ + "Microsoft.AspNetCore.Authorization", "System.Web.Mvc", "System.Web.Http" + ], "AuthorizeAttribute") + | + attr = m.getOverridee*().getAnAttribute() or + attr = m.getDeclaringType().getABaseType*().getAnAttribute() + ) +} + +/** Holds if `m`, its containing class, or a parent class has an attribute that extends `AllowAnonymousAttribute` */ +private predicate hasAllowAnonymousAttribute(ActionMethod m) { + exists(Attribute attr | + attr.getType() + .getABaseType*() + .hasQualifiedName([ + "Microsoft.AspNetCore.Authorization", "System.Web.Mvc", "System.Web.Http" + ], "AllowAnonymousAttribute") + | + attr = m.getOverridee*().getAnAttribute() or + attr = m.getDeclaringType().getABaseType*().getAnAttribute() + ) +} + +/** Holds if `m` is authorized via an `Authorize` attribute */ +private predicate isAuthorizedViaAttribute(ActionMethod m) { + hasAuthorizeAttribute(m) and + not hasAllowAnonymousAttribute(m) +} + +/** + * Holds if `m` is a method that modifies a particular resource based on + * an ID provided by user input, but does not check anything based on the current user + * to determine if they should modify this resource. + */ +predicate hasInsecureDirectObjectReference(ActionMethod m) { + needsChecks(m) and + hasIdParameter(m) and + not checksUser(m) and + not isAuthorizedViaAttribute(m) and + exists(m.getBody().getAChildStmt()) +} 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 7623cb6b9f75..79a39f093ee1 100644 --- a/csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll +++ b/csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll @@ -4,96 +4,10 @@ import csharp import semmle.code.csharp.frameworks.microsoft.AspNetCore import semmle.code.csharp.frameworks.system.web.UI import semmle.code.asp.WebConfig +import ActionMethods -/** A method representing an action for a web endpoint. */ -abstract class ActionMethod extends Method { - /** - * Gets a string that can indicate what this method does to determine if it should have an auth check; - * such as its method name, class name, or file path. - */ - string getADescription() { - result = - [ - this.getName(), this.getDeclaringType().getBaseClass*().getName(), - this.getDeclaringType().getFile().getRelativePath() - ] - } - - /** Holds if this method may need an authorization check. */ - predicate needsAuth() { - this.getADescription() - .regexpReplaceAll("([a-z])([A-Z])", "$1_$2") - // separate camelCase words - .toLowerCase() - .regexpMatch(".*(edit|delete|modify|admin|superuser).*") - } - - /** Gets a callable for which if it contains an auth check, this method should be considered authenticated. */ - Callable getAnAuthorizingCallable() { result = this } - - /** - * Gets a possible url route that could refer to this action, - * which would be covered by `` configurations specifying a prefix of it. - */ - string getARoute() { result = this.getDeclaringType().getFile().getRelativePath() } -} - -/** An action method in the MVC framework. */ -private class MvcActionMethod extends ActionMethod { - MvcActionMethod() { this = any(MicrosoftAspNetCoreMvcController c).getAnActionMethod() } -} - -/** An action method on a subclass of `System.Web.UI.Page`. */ -private class WebFormActionMethod extends ActionMethod { - WebFormActionMethod() { - this.getDeclaringType().getBaseClass+() instanceof SystemWebUIPageClass and - this.getAParameter().getType().getName().matches("%EventArgs") - } - - override Callable getAnAuthorizingCallable() { - result = super.getAnAuthorizingCallable() - or - result.getDeclaringType() = this.getDeclaringType() and - result.getName() = "Page_Load" - } - - override string getARoute() { - exists(string physicalRoute | physicalRoute = super.getARoute() | - result = physicalRoute - or - exists(string absolutePhysical | - virtualRouteMapping(result, absolutePhysical) and - physicalRouteMatches(absolutePhysical, physicalRoute) - ) - ) - } -} - -/** - * Holds if `virtualRoute` is a URL path - * that can map to the corresponding `physicalRoute` filepath - * through a call to `MapPageRoute` - */ -private predicate virtualRouteMapping(string virtualRoute, string physicalRoute) { - exists(MethodCall mapPageRouteCall, StringLiteral virtualLit, StringLiteral physicalLit | - mapPageRouteCall - .getTarget() - .hasQualifiedName("System.Web.Routing", "RouteCollection", "MapPageRoute") and - virtualLit = mapPageRouteCall.getArgument(1) and - physicalLit = mapPageRouteCall.getArgument(2) and - virtualLit.getValue() = virtualRoute and - physicalLit.getValue() = physicalRoute - ) -} - -/** Holds if the filepath `route` can refer to `actual` after expanding a '~". */ -bindingset[route, actual] -private predicate physicalRouteMatches(string route, string actual) { - route = actual - or - route.charAt(0) = "~" and - exists(string dir | actual = dir + route.suffix(1) + ".cs") -} +/** Holds if the method `m` may need an authorization check. */ +predicate needsAuth(ActionMethod m) { m.isEdit() or m.isAdmin() } /** An expression that indicates that some authorization/authentication check is being performed. */ class AuthExpr extends Expr { @@ -114,7 +28,7 @@ class AuthExpr extends Expr { /** Holds if `m` is a method that should have an auth check, and does indeed have one. */ predicate hasAuthViaCode(ActionMethod m) { - m.needsAuth() and + needsAuth(m) and exists(Callable caller, AuthExpr auth | m.getAnAuthorizingCallable().calls*(caller) and auth.getEnclosingCallable() = caller @@ -175,7 +89,7 @@ predicate hasAuthViaAttribute(ActionMethod m) { /** Holds if `m` is a method that should have an auth check, but is missing it. */ predicate missingAuth(ActionMethod m) { - m.needsAuth() and + needsAuth(m) and not hasAuthViaCode(m) and not hasAuthViaXml(m) and not hasAuthViaAttribute(m) and diff --git a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp new file mode 100644 index 000000000000..aa0af4a69f5e --- /dev/null +++ b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp @@ -0,0 +1,30 @@ + + + +

Resources like comments or user profiles can be accessed and modified through an action method. To target a certain resource, the action method accepts an ID parameter pointing to that specific resource. If the methods do not check that the current user is authorized to access the specified resource, an attacker can access a resource by guessing or otherwise determining the linked ID parameter.

+ +
+ +

+Ensure that the current user is authorized to access the resource of the provided ID. +

+ +
+ +

In the following example, in the "BAD" case, there is no authorization check, so any user can edit any comment for which they guess or determine the ID parameter. +The "GOOD" case includes a check that the current user matches the author of the comment, preventing unauthorized access.

+ +

The following example shows a similar scenario for the ASP.NET Core framework. As above, the "BAD" case provides an example with no authorization check, and the first "GOOD" case provides an example with a check that the current user authored the specified comment. Additionally, in the second "GOOD" case, the `Authorize` attribute is used to restrict the method to administrators, who are expected to be able to access arbitrary resources.

+ + +
+ + +
  • OWASP: Insecure Direct Object Refrences.
  • +
  • OWASP: Testing for Insecure Direct Object References.
  • +
  • Microsoft Learn: Resource-based authorization in ASP.NET Core.
  • + +
    +
    \ No newline at end of file diff --git a/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql new file mode 100644 index 000000000000..06129ec88ec7 --- /dev/null +++ b/csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql @@ -0,0 +1,20 @@ +/** + * @name Insecure Direct Object Reference + * @description Using user input to control which object is modified without + * proper authorization checks allows an attacker to modify arbitrary objects. + * @kind problem + * @problem.severity warning + * @security-severity 7.5 + * @precision medium + * @id cs/web/insecure-direct-object-reference + * @tags security + * external/cwe-639 + */ + +import csharp +import semmle.code.csharp.security.auth.InsecureDirectObjectReferenceQuery + +from ActionMethod m +where hasInsecureDirectObjectReference(m) +select m, + "This method may be missing authorization checks for which users can access the resource of the provided ID." diff --git a/csharp/ql/src/Security Features/CWE-639/MVCExample.cs b/csharp/ql/src/Security Features/CWE-639/MVCExample.cs new file mode 100644 index 000000000000..4137bccb395e --- /dev/null +++ b/csharp/ql/src/Security Features/CWE-639/MVCExample.cs @@ -0,0 +1,45 @@ +public class CommentController : Controller { + private readonly IAuthorizationService _authorizationService; + private readonly IDocumentRepository _commentRepository; + + public CommentController(IAuthorizationService authorizationService, + ICommentRepository commentRepository) + { + _authorizationService = authorizationService; + _commentRepository = commentRepository; + } + + // BAD: Any user can access this. + public async Task Edit1(int commentId, string text) { + Comment comment = _commentRepository.Find(commentId); + + comment.Text = text; + + return View(); + } + + // GOOD: An authorization check is made. + public async Task Edit2(int commentId, string text) { + Comment comment = _commentRepository.Find(commentId); + + var authResult = await _authorizationService.AuthorizeAsync(User, Comment, "EditPolicy"); + + if (authResult.Succeeded) { + comment.Text = text; + return View(); + } + else { + return ForbidResult(); + } + } + + // GOOD: Only users with the `admin` role can access this method. + [Authorize(Roles="admin")] + public async Task Edit3(int commentId, string text) { + Comment comment = _commentRepository.Find(commentId); + + comment.Text = text; + + return View(); + } +} \ No newline at end of file diff --git a/csharp/ql/src/Security Features/CWE-639/WebFormsExample.cs b/csharp/ql/src/Security Features/CWE-639/WebFormsExample.cs new file mode 100644 index 000000000000..48f870db451a --- /dev/null +++ b/csharp/ql/src/Security Features/CWE-639/WebFormsExample.cs @@ -0,0 +1,15 @@ + // BAD - Any user can access this method. + protected void btn1_Click(object sender, EventArgs e) { + string commentId = Request.QueryString["Id"]; + Comment comment = getCommentById(commentId); + comment.Body = inputCommentBody.Text; + } + + // GOOD - The user ID is verified. + protected void btn2_Click(object sender, EventArgs e) { + string commentId = Request.QueryString["Id"]; + Comment comment = getCommentById(commentId); + if (comment.AuthorName == User.Identity.Name){ + comment.Body = inputCommentBody.Text; + } + } \ No newline at end of file diff --git a/csharp/ql/src/change-notes/2023-08-21-insecure-direct-object-reference.md b/csharp/ql/src/change-notes/2023-08-21-insecure-direct-object-reference.md new file mode 100644 index 000000000000..edbb11347390 --- /dev/null +++ b/csharp/ql/src/change-notes/2023-08-21-insecure-direct-object-reference.md @@ -0,0 +1,4 @@ +--- +category: newQuery +--- +* Added a new query, `cs/web/insecure-direct-object-reference`, to find instances of missing authorization checks for resources selected by an ID parameter. \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/CommentController.cs b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/CommentController.cs new file mode 100644 index 000000000000..891e8374c1cc --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/CommentController.cs @@ -0,0 +1,37 @@ +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Authorization; + +public class CommentController : Controller { + // BAD: Any user can access this. + public ActionResult Edit1(int commentId, string text) { + editComment(commentId, text); + return View(); + } + + // GOOD: The user's authorization is checked. + public ActionResult Edit2(int commentId, string text) { + if (canEditComment(commentId, User.Identity.Name)){ + editComment(commentId, text); + } + return View(); + } + + // GOOD: The Authorize attribute is used + [Authorize] + public ActionResult Edit3(int commentId, string text) { + editComment(commentId, text); + return View(); + } + + // BAD: The AllowAnonymous attribute overrides the Authorize attribute + [Authorize] + [AllowAnonymous] + public ActionResult Edit4(int commentId, string text) { + editComment(commentId, text); + return View(); + } + + void editComment(int commentId, string text) { } + + bool canEditComment(int commentId, string userName) { return false; } +} \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected new file mode 100644 index 000000000000..061b87dc6afe --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected @@ -0,0 +1,6 @@ +| CommentController.cs:6:25:6:29 | Edit1 | This method may be missing authorization checks for which users can access the resource of the provided ID. | +| CommentController.cs:29:25:29:29 | Edit4 | This method may be missing authorization checks for which users can access the resource of the provided ID. | +| MiscTestControllers.cs:26:33:26:40 | EditAnon | This method may be missing authorization checks for which users can access the resource of the provided ID. | +| MiscTestControllers.cs:34:34:34:41 | EditAnon | This method may be missing authorization checks for which users can access the resource of the provided ID. | +| MiscTestControllers.cs:45:25:45:29 | Edit4 | This method may be missing authorization checks for which users can access the resource of the provided ID. | +| ProfileController.cs:14:25:14:29 | Edit2 | This method may be missing authorization checks for which users can access the resource of the provided ID. | diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.qlref b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.qlref new file mode 100644 index 000000000000..4756d5a76a4f --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.qlref @@ -0,0 +1 @@ +Security Features/CWE-639/InsecureDirectObjectReference.ql \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/MiscTestControllers.cs b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/MiscTestControllers.cs new file mode 100644 index 000000000000..3966d418a931 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/MiscTestControllers.cs @@ -0,0 +1,46 @@ +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Authorization; + +public class BaseController : Controller { + // GOOD + [Authorize] + public virtual ActionResult Edit1(int id) { return View(); } +} + +class MyAuthorizeAttribute : AuthorizeAttribute { } +class MyAllowAnonymousAttribute : AllowAnonymousAttribute { } + +public class AController : BaseController { + // GOOD - Authorize is inherited from overridden method + public override ActionResult Edit1(int id) { return View(); } + + // GOOD - A subclass of Authorize is used + [MyAuthorize] + public ActionResult Edit2(int id) { return View(); } +} + +[Authorize] +public class BaseAuthController : Controller { + // BAD - A subclass of AllowAnonymous is used + [MyAllowAnonymous] + public virtual ActionResult EditAnon(int id) { return View(); } +} + +public class BController : BaseAuthController { + // GOOD - Authorize is inherited from parent class + public ActionResult Edit3(int id) { return View(); } + + // BAD - MyAllowAnonymous is inherited from overridden method + public override ActionResult EditAnon(int id) { return View(); } +} + +[AllowAnonymous] +public class BaseAnonController : Controller { + +} + +public class CController : BaseAnonController { + // BAD - AllowAnonymous is inherited from base class and overrides Authorize + [Authorize] + public ActionResult Edit4(int id) { return View(); } +} \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/ProfileController.cs b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/ProfileController.cs new file mode 100644 index 000000000000..a41c32db6411 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/ProfileController.cs @@ -0,0 +1,20 @@ +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Authorization; + +[Authorize] +public class ProfileController : Controller { + // GOOD: The Authorize attribute of the class restricts access to this method. + public ActionResult Edit1(int profileId, string text) { + editProfileName(profileId, text); + return View(); + } + + // BAD: The AllowAnonymous attribute overrides the Authorize attribute on the class. + [AllowAnonymous] + public ActionResult Edit2(int profileId, string text) { + editProfileName(profileId, text); + return View(); + } + + void editProfileName(int profileId, string text) { } +} \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/options b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/options new file mode 100644 index 000000000000..c69b0d035a69 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/options @@ -0,0 +1,3 @@ +semmle-extractor-options: /nostdlib /noconfig +semmle-extractor-options: --load-sources-from-project:${testdir}/../../../../resources/stubs/_frameworks/Microsoft.NETCore.App/Microsoft.NETCore.App.csproj +semmle-extractor-options: --load-sources-from-project:${testdir}/../../../../resources/stubs/_frameworks/Microsoft.AspNetCore.App/Microsoft.AspNetCore.App.csproj \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/EditComment.aspx.cs b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/EditComment.aspx.cs new file mode 100644 index 000000000000..974869a0af4b --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/EditComment.aspx.cs @@ -0,0 +1,28 @@ +using System; +using System.Web.UI; + +class EditComment : System.Web.UI.Page { + + // BAD - Any user can access this method. + protected void btn1_Click(object sender, EventArgs e) { + string commentId = Request.QueryString["Id"]; + Comment comment = getCommentById(commentId); + comment.Text = "xyz"; + } + + // GOOD - The user ID is verified. + protected void btn2_Click(object sender, EventArgs e) { + string commentId = Request.QueryString["Id"]; + Comment comment = getCommentById(commentId); + if (comment.AuthorName == User.Identity.Name){ + comment.Text = "xyz"; + } + } + + class Comment { + public string Text { get; set; } + public string AuthorName { get; } + } + + Comment getCommentById(string id) { return null; } +} \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.expected b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.expected new file mode 100644 index 000000000000..8cb9e542f311 --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.expected @@ -0,0 +1 @@ +| EditComment.aspx.cs:7:20:7:29 | btn1_Click | This method may be missing authorization checks for which users can access the resource of the provided ID. | diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.qlref b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.qlref new file mode 100644 index 000000000000..4756d5a76a4f --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.qlref @@ -0,0 +1 @@ +Security Features/CWE-639/InsecureDirectObjectReference.ql \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/options b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/options new file mode 100644 index 000000000000..a5d7077ef37a --- /dev/null +++ b/csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/options @@ -0,0 +1,3 @@ +semmle-extractor-options: /nostdlib /noconfig +semmle-extractor-options: --load-sources-from-project:${testdir}/../../../../resources/stubs/_frameworks/Microsoft.NETCore.App/Microsoft.NETCore.App.csproj +semmle-extractor-options: ${testdir}/../../../../resources/stubs/System.Web.cs \ No newline at end of file diff --git a/csharp/ql/test/resources/stubs/System.Web.cs b/csharp/ql/test/resources/stubs/System.Web.cs index c3c8a0a753fa..0f055fcc123b 100644 --- a/csharp/ql/test/resources/stubs/System.Web.cs +++ b/csharp/ql/test/resources/stubs/System.Web.cs @@ -82,6 +82,7 @@ public class Control public class Page { public System.Security.Principal.IPrincipal User { get; } + public System.Web.HttpRequest Request { get; } } interface IPostBackDataHandler