Skip to content

Commit 7c230d6

Browse files
joefarebrotherjoefarebrother
authored
Merge pull request #13882 from joefarebrother/csharp-insecure-direct-object-ref
C#: Add query for Insecure Direct Object Reference
2 parents 4183fbe + d7c1be4 commit 7c230d6

19 files changed

Lines changed: 469 additions & 91 deletions

‎csharp/ql/lib/semmle/code/csharp/security/auth/ActionMethods.qll‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
/** Common definitions for queries checking for access control measures on action methods. */
2+
3+
import csharp
4+
import semmle.code.csharp.frameworks.microsoft.AspNetCore
5+
import semmle.code.csharp.frameworks.system.web.UI
6+
7+
/** A method representing an action for a web endpoint. */
8+
abstract class ActionMethod extends Method {
9+
/**
10+
* Gets a string that can indicate what this method does to determine if it should have an auth check;
11+
* such as its method name, class name, or file path.
12+
*/
13+
string getADescription() {
14+
result = [this.getName(), this.getDeclaringType().getBaseClass*().getName(), this.getARoute()]
15+
}
16+
17+
/** Holds if this method may represent a stateful action such as editing or deleting */
18+
predicate isEdit() {
19+
exists(string str |
20+
str =
21+
this.getADescription()
22+
// separate camelCase words
23+
.regexpReplaceAll("([a-z])([A-Z])", "$1_$2") and
24+
str.regexpMatch("(?i).*(edit|delete|modify|change).*") and
25+
not str.regexpMatch("(?i).*(on_?change|changed).*")
26+
)
27+
}
28+
29+
/** Holds if this method may be intended to be restricted to admin users */
30+
predicate isAdmin() {
31+
this.getADescription()
32+
// separate camelCase words
33+
.regexpReplaceAll("([a-z])([A-Z])", "$1_$2")
34+
.regexpMatch("(?i).*(admin|superuser).*")
35+
}
36+
37+
/** Gets a callable for which if it contains an auth check, this method should be considered authenticated. */
38+
Callable getAnAuthorizingCallable() { result = this }
39+
40+
/**
41+
* Gets a possible url route that could refer to this action,
42+
* which would be covered by `<location>` configurations specifying a prefix of it.
43+
*/
44+
string getARoute() { result = this.getDeclaringType().getFile().getRelativePath() }
45+
}
46+
47+
/** An action method in the MVC framework. */
48+
private class MvcActionMethod extends ActionMethod {
49+
MvcActionMethod() { this = any(MicrosoftAspNetCoreMvcController c).getAnActionMethod() }
50+
}
51+
52+
/** An action method on a subclass of `System.Web.UI.Page`. */
53+
private class WebFormActionMethod extends ActionMethod {
54+
WebFormActionMethod() {
55+
this.getDeclaringType().getBaseClass+() instanceof SystemWebUIPageClass and
56+
this.getAParameter().getType().getName().matches("%EventArgs")
57+
}
58+
59+
override Callable getAnAuthorizingCallable() {
60+
result = super.getAnAuthorizingCallable()
61+
or
62+
pageLoad(result, this.getDeclaringType())
63+
}
64+
65+
override string getARoute() {
66+
exists(string physicalRoute | physicalRoute = super.getARoute() |
67+
result = physicalRoute
68+
or
69+
exists(string absolutePhysical |
70+
virtualRouteMapping(result, absolutePhysical) and
71+
physicalRouteMatches(absolutePhysical, physicalRoute)
72+
)
73+
)
74+
}
75+
}
76+
77+
pragma[nomagic]
78+
private predicate pageLoad(Callable c, Type decl) {
79+
c.getName() = "Page_Load" and
80+
decl = c.getDeclaringType()
81+
}
82+
83+
/**
84+
* Holds if `virtualRoute` is a URL path
85+
* that can map to the corresponding `physicalRoute` filepath
86+
* through a call to `MapPageRoute`
87+
*/
88+
private predicate virtualRouteMapping(string virtualRoute, string physicalRoute) {
89+
exists(MethodCall mapPageRouteCall, StringLiteral virtualLit, StringLiteral physicalLit |
90+
mapPageRouteCall
91+
.getTarget()
92+
.hasQualifiedName("System.Web.Routing", "RouteCollection", "MapPageRoute") and
93+
virtualLit = mapPageRouteCall.getArgument(1) and
94+
physicalLit = mapPageRouteCall.getArgument(2) and
95+
virtualLit.getValue() = virtualRoute and
96+
physicalLit.getValue() = physicalRoute
97+
)
98+
}
99+
100+
/** Holds if the filepath `route` can refer to `actual` after expanding a '~". */
101+
bindingset[route, actual]
102+
private predicate physicalRouteMatches(string route, string actual) {
103+
route = actual
104+
or
105+
route.charAt(0) = "~" and
106+
exists(string dir | actual = dir + route.suffix(1) + ".cs")
107+
}

‎csharp/ql/lib/semmle/code/csharp/security/auth/InsecureDirectObjectReferenceQuery.qll‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
/** Definitions for the Insecure Direct Object Reference query */
2+
3+
import csharp
4+
import semmle.code.csharp.dataflow.flowsources.Remote
5+
import ActionMethods
6+
7+
/**
8+
* Holds if `m` is a method that may require checks
9+
* against the current user to modify a particular resource.
10+
*/
11+
// We exclude admin methods as it may be expected that an admin user should be able to modify any resource.
12+
// Other queries check that there are authorization checks in place for admin methods.
13+
private predicate needsChecks(ActionMethod m) { m.isEdit() and not m.isAdmin() }
14+
15+
/**
16+
* Holds if `m` has a parameter or access a remote flow source
17+
* that may indicate that it's used as the ID for some resource
18+
*/
19+
private predicate hasIdParameter(ActionMethod m) {
20+
exists(RemoteFlowSource src | src.getEnclosingCallable() = m |
21+
src.asParameter().getName().toLowerCase().matches(["%id", "%idx"])
22+
or
23+
// handle cases like `Request.QueryString["Id"]`
24+
exists(StringLiteral idStr, IndexerCall idx |
25+
idStr.getValue().toLowerCase().matches(["%id", "%idx"]) and
26+
TaintTracking::localTaint(src, DataFlow::exprNode(idx.getQualifier())) and
27+
DataFlow::localExprFlow(idStr, idx.getArgument(0))
28+
)
29+
)
30+
}
31+
32+
private predicate authorizingCallable(Callable c) {
33+
exists(string name | name = c.getName().toLowerCase() |
34+
name.matches(["%user%", "%session%"]) and
35+
not name.matches("%get%by%") // methods like `getUserById` or `getXByUsername` aren't likely to be referring to the current user
36+
)
37+
}
38+
39+
/** Holds if `m` at some point in its call graph may make some kind of check against the current user. */
40+
private predicate checksUser(ActionMethod m) {
41+
exists(Callable c |
42+
authorizingCallable(c) and
43+
callsPlus(m, c)
44+
)
45+
}
46+
47+
private predicate calls(Callable c1, Callable c2) { c1.calls(c2) }
48+
49+
private predicate callsPlus(Callable c1, Callable c2) = fastTC(calls/2)(c1, c2)
50+
51+
/** Holds if `m`, its containing class, or a parent class has an attribute that extends `AuthorizeAttribute` */
52+
private predicate hasAuthorizeAttribute(ActionMethod m) {
53+
exists(Attribute attr |
54+
attr.getType()
55+
.getABaseType*()
56+
.hasQualifiedName([
57+
"Microsoft.AspNetCore.Authorization", "System.Web.Mvc", "System.Web.Http"
58+
], "AuthorizeAttribute")
59+
|
60+
attr = m.getOverridee*().getAnAttribute() or
61+
attr = m.getDeclaringType().getABaseType*().getAnAttribute()
62+
)
63+
}
64+
65+
/** Holds if `m`, its containing class, or a parent class has an attribute that extends `AllowAnonymousAttribute` */
66+
private predicate hasAllowAnonymousAttribute(ActionMethod m) {
67+
exists(Attribute attr |
68+
attr.getType()
69+
.getABaseType*()
70+
.hasQualifiedName([
71+
"Microsoft.AspNetCore.Authorization", "System.Web.Mvc", "System.Web.Http"
72+
], "AllowAnonymousAttribute")
73+
|
74+
attr = m.getOverridee*().getAnAttribute() or
75+
attr = m.getDeclaringType().getABaseType*().getAnAttribute()
76+
)
77+
}
78+
79+
/** Holds if `m` is authorized via an `Authorize` attribute */
80+
private predicate isAuthorizedViaAttribute(ActionMethod m) {
81+
hasAuthorizeAttribute(m) and
82+
not hasAllowAnonymousAttribute(m)
83+
}
84+
85+
/**
86+
* Holds if `m` is a method that modifies a particular resource based on
87+
* an ID provided by user input, but does not check anything based on the current user
88+
* to determine if they should modify this resource.
89+
*/
90+
predicate hasInsecureDirectObjectReference(ActionMethod m) {
91+
needsChecks(m) and
92+
hasIdParameter(m) and
93+
not checksUser(m) and
94+
not isAuthorizedViaAttribute(m) and
95+
exists(m.getBody().getAChildStmt())
96+
}

‎csharp/ql/lib/semmle/code/csharp/security/auth/MissingFunctionLevelAccessControlQuery.qll‎

Lines changed: 5 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -4,96 +4,10 @@ import csharp
44
import semmle.code.csharp.frameworks.microsoft.AspNetCore
55
import semmle.code.csharp.frameworks.system.web.UI
66
import semmle.code.asp.WebConfig
7+
import ActionMethods
78

8-
/** A method representing an action for a web endpoint. */
9-
abstract class ActionMethod extends Method {
10-
/**
11-
* Gets a string that can indicate what this method does to determine if it should have an auth check;
12-
* such as its method name, class name, or file path.
13-
*/
14-
string getADescription() {
15-
result =
16-
[
17-
this.getName(), this.getDeclaringType().getBaseClass*().getName(),
18-
this.getDeclaringType().getFile().getRelativePath()
19-
]
20-
}
21-
22-
/** Holds if this method may need an authorization check. */
23-
predicate needsAuth() {
24-
this.getADescription()
25-
.regexpReplaceAll("([a-z])([A-Z])", "$1_$2")
26-
// separate camelCase words
27-
.toLowerCase()
28-
.regexpMatch(".*(edit|delete|modify|admin|superuser).*")
29-
}
30-
31-
/** Gets a callable for which if it contains an auth check, this method should be considered authenticated. */
32-
Callable getAnAuthorizingCallable() { result = this }
33-
34-
/**
35-
* Gets a possible url route that could refer to this action,
36-
* which would be covered by `<location>` configurations specifying a prefix of it.
37-
*/
38-
string getARoute() { result = this.getDeclaringType().getFile().getRelativePath() }
39-
}
40-
41-
/** An action method in the MVC framework. */
42-
private class MvcActionMethod extends ActionMethod {
43-
MvcActionMethod() { this = any(MicrosoftAspNetCoreMvcController c).getAnActionMethod() }
44-
}
45-
46-
/** An action method on a subclass of `System.Web.UI.Page`. */
47-
private class WebFormActionMethod extends ActionMethod {
48-
WebFormActionMethod() {
49-
this.getDeclaringType().getBaseClass+() instanceof SystemWebUIPageClass and
50-
this.getAParameter().getType().getName().matches("%EventArgs")
51-
}
52-
53-
override Callable getAnAuthorizingCallable() {
54-
result = super.getAnAuthorizingCallable()
55-
or
56-
result.getDeclaringType() = this.getDeclaringType() and
57-
result.getName() = "Page_Load"
58-
}
59-
60-
override string getARoute() {
61-
exists(string physicalRoute | physicalRoute = super.getARoute() |
62-
result = physicalRoute
63-
or
64-
exists(string absolutePhysical |
65-
virtualRouteMapping(result, absolutePhysical) and
66-
physicalRouteMatches(absolutePhysical, physicalRoute)
67-
)
68-
)
69-
}
70-
}
71-
72-
/**
73-
* Holds if `virtualRoute` is a URL path
74-
* that can map to the corresponding `physicalRoute` filepath
75-
* through a call to `MapPageRoute`
76-
*/
77-
private predicate virtualRouteMapping(string virtualRoute, string physicalRoute) {
78-
exists(MethodCall mapPageRouteCall, StringLiteral virtualLit, StringLiteral physicalLit |
79-
mapPageRouteCall
80-
.getTarget()
81-
.hasQualifiedName("System.Web.Routing", "RouteCollection", "MapPageRoute") and
82-
virtualLit = mapPageRouteCall.getArgument(1) and
83-
physicalLit = mapPageRouteCall.getArgument(2) and
84-
virtualLit.getValue() = virtualRoute and
85-
physicalLit.getValue() = physicalRoute
86-
)
87-
}
88-
89-
/** Holds if the filepath `route` can refer to `actual` after expanding a '~". */
90-
bindingset[route, actual]
91-
private predicate physicalRouteMatches(string route, string actual) {
92-
route = actual
93-
or
94-
route.charAt(0) = "~" and
95-
exists(string dir | actual = dir + route.suffix(1) + ".cs")
96-
}
9+
/** Holds if the method `m` may need an authorization check. */
10+
predicate needsAuth(ActionMethod m) { m.isEdit() or m.isAdmin() }
9711

9812
/** An expression that indicates that some authorization/authentication check is being performed. */
9913
class AuthExpr extends Expr {
@@ -114,7 +28,7 @@ class AuthExpr extends Expr {
11428

11529
/** Holds if `m` is a method that should have an auth check, and does indeed have one. */
11630
predicate hasAuthViaCode(ActionMethod m) {
117-
m.needsAuth() and
31+
needsAuth(m) and
11832
exists(Callable caller, AuthExpr auth |
11933
m.getAnAuthorizingCallable().calls*(caller) and
12034
auth.getEnclosingCallable() = caller
@@ -175,7 +89,7 @@ predicate hasAuthViaAttribute(ActionMethod m) {
17589

17690
/** Holds if `m` is a method that should have an auth check, but is missing it. */
17791
predicate missingAuth(ActionMethod m) {
178-
m.needsAuth() and
92+
needsAuth(m) and
17993
not hasAuthViaCode(m) and
18094
not hasAuthViaXml(m) and
18195
not hasAuthViaAttribute(m) and

‎csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.qhelp‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>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.</p>
7+
8+
</overview>
9+
<recommendation>
10+
<p>
11+
Ensure that the current user is authorized to access the resource of the provided ID.
12+
</p>
13+
14+
</recommendation>
15+
<example>
16+
<p>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.
17+
The "GOOD" case includes a check that the current user matches the author of the comment, preventing unauthorized access.</p>
18+
<sample src="WebFormsExample.cs" />
19+
<p>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.</p>
20+
<sample src="MVCExample.cs" />
21+
22+
</example>
23+
<references>
24+
25+
<li>OWASP: <a href="https://wiki.owasp.org/index.php/Top_10_2013-A4-Insecure_Direct_Object_References">Insecure Direct Object Refrences</a>.</li>
26+
<li>OWASP: <a href="https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/05-Authorization_Testing/04-Testing_for_Insecure_Direct_Object_References">Testing for Insecure Direct Object References</a>.</li>
27+
<li>Microsoft Learn: <a href="https://learn.microsoft.com/en-us/aspnet/core/security/authorization/resourcebased?view=aspnetcore-7.0">Resource-based authorization in ASP.NET Core</a>.</li>
28+
29+
</references>
30+
</qhelp>

‎csharp/ql/src/Security Features/CWE-639/InsecureDirectObjectReference.ql‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* @name Insecure Direct Object Reference
3+
* @description Using user input to control which object is modified without
4+
* proper authorization checks allows an attacker to modify arbitrary objects.
5+
* @kind problem
6+
* @problem.severity warning
7+
* @security-severity 7.5
8+
* @precision medium
9+
* @id cs/web/insecure-direct-object-reference
10+
* @tags security
11+
* external/cwe-639
12+
*/
13+
14+
import csharp
15+
import semmle.code.csharp.security.auth.InsecureDirectObjectReferenceQuery
16+
17+
from ActionMethod m
18+
where hasInsecureDirectObjectReference(m)
19+
select m,
20+
"This method may be missing authorization checks for which users can access the resource of the provided ID."

0 commit comments

Comments
 (0)