Skip to content

Commit a510a7b

Browse files
joefarebrotherjoefarebrother
committed
Add insecure direct object reference definitions and factor out those from missing access control
1 parent 6c7833f commit a510a7b

File tree

3 files changed

+136
-90
lines changed

3 files changed

+136
-90
lines changed

‎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")
24+
.toLowerCase() and
25+
str.regexpMatch(".*(edit|delete|modify|change).*") and
26+
not str.regexpMatch(".*(on_?change|changed).*")
27+
)
28+
}
29+
30+
/** Holds if this method may be intended to be restricted to admin users */
31+
predicate isAdmin() {
32+
this.getADescription()
33+
// separate camelCase words
34+
.regexpReplaceAll("([a-z])([A-Z])", "$1_$2")
35+
.toLowerCase()
36+
.regexpMatch(".*(admin|superuser).*")
37+
}
38+
39+
/** Holds if this method may need an authorization check. */
40+
predicate needsAuth() { this.isEdit() or this.isAdmin() }
41+
42+
/** Gets a callable for which if it contains an auth check, this method should be considered authenticated. */
43+
Callable getAnAuthorizingCallable() { result = this }
44+
45+
/**
46+
* Gets a possible url route that could refer to this action,
47+
* which would be covered by `<location>` configurations specifying a prefix of it.
48+
*/
49+
string getARoute() { result = this.getDeclaringType().getFile().getRelativePath() }
50+
}
51+
52+
/** An action method in the MVC framework. */
53+
private class MvcActionMethod extends ActionMethod {
54+
MvcActionMethod() { this = any(MicrosoftAspNetCoreMvcController c).getAnActionMethod() }
55+
}
56+
57+
/** An action method on a subclass of `System.Web.UI.Page`. */
58+
private class WebFormActionMethod extends ActionMethod {
59+
WebFormActionMethod() {
60+
this.getDeclaringType().getBaseClass+() instanceof SystemWebUIPageClass and
61+
this.getAParameter().getType().getName().matches("%EventArgs")
62+
}
63+
64+
override Callable getAnAuthorizingCallable() {
65+
result = super.getAnAuthorizingCallable()
66+
or
67+
result.getDeclaringType() = this.getDeclaringType() and
68+
result.getName() = "Page_Load"
69+
}
70+
71+
override string getARoute() {
72+
exists(string physicalRoute | physicalRoute = super.getARoute() |
73+
result = physicalRoute
74+
or
75+
exists(string absolutePhysical |
76+
virtualRouteMapping(result, absolutePhysical) and
77+
physicalRouteMatches(absolutePhysical, physicalRoute)
78+
)
79+
)
80+
}
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/InsecureDirectObjectRefrerenceQuery.qll

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
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+
private predicate needsChecks(ActionMethod m) { m.isEdit() and not m.isAdmin() }
8+
9+
private predicate hasIdParameter(ActionMethod m) {
10+
exists(RemoteFlowSource src | src.getEnclosingCallable() = m |
11+
src.asParameter().getName().toLowerCase().matches("%id")
12+
or
13+
exists(StringLiteral idStr |
14+
idStr.getValue().toLowerCase().matches("%id") and
15+
idStr.getParent*() = src.asExpr()
16+
)
17+
)
18+
}
19+
20+
private predicate checksUser(ActionMethod m) {
21+
exists(Callable c | c.getName().toLowerCase().matches("%user%") | m.calls*(c))
22+
}
23+
24+
predicate hasInsecureDirectObjectReference(ActionMethod m) {
25+
needsChecks(m) and
26+
hasIdParameter(m) and
27+
not checksUser(m)
28+
}

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

Lines changed: 1 addition & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -4,96 +4,7 @@ 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-
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-
}
7+
import ActionMethods
978

989
/** An expression that indicates that some authorization/authentication check is being performed. */
9910
class AuthExpr extends Expr {

0 commit comments

Comments
 (0)