Skip to content

Commit 29b5f14

Browse files
Add support for auth via xml using the physical path
1 parent e93f318 commit 29b5f14

File tree

1 file changed

+74
-26
lines changed

1 file changed

+74
-26
lines changed

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

Lines changed: 74 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -3,23 +3,40 @@
33
import csharp
44
import semmle.code.csharp.frameworks.microsoft.AspNetCore
55
import semmle.code.csharp.frameworks.system.web.UI
6+
import semmle.code.asp.WebConfig
67

7-
/** Holds if `m` is a method representing an action whose name indicates that it should have some authorization/authentication check. */
8-
predicate needsAuth(Method m) {
9-
(
10-
m = any(MicrosoftAspNetCoreMvcController c).getAnActionMethod()
11-
or
12-
m.getDeclaringType().getBaseClass*() instanceof SystemWebUIPageClass and
13-
m.getAParameter().getType().getName().matches("%EventArgs")
14-
) and
15-
exists(string name |
16-
name =
8+
abstract class ActionMethod extends Method {
9+
string getADescription() {
10+
result =
1711
[
18-
m.getName(), m.getDeclaringType().getBaseClass*().getName(),
19-
m.getDeclaringType().getFile().getRelativePath()
20-
] and
21-
name.toLowerCase().regexpMatch(".*(edit|delete|modify|admin|superuser).*")
22-
)
12+
this.getName(), this.getDeclaringType().getBaseClass*().getName(),
13+
this.getDeclaringType().getFile().getRelativePath()
14+
]
15+
}
16+
17+
predicate needsAuth() {
18+
this.getADescription().toLowerCase().regexpMatch(".*(edit|delete|modify|admin|superuser).*")
19+
}
20+
21+
Callable getAnAuthorizingCallable() { result = this }
22+
}
23+
24+
private class MvcActionMethod extends ActionMethod {
25+
MvcActionMethod() { this = any(MicrosoftAspNetCoreMvcController c).getAnActionMethod() }
26+
}
27+
28+
private class WebFormActionMethod extends ActionMethod {
29+
WebFormActionMethod() {
30+
this.getDeclaringType().getBaseClass*() instanceof SystemWebUIPageClass and
31+
this.getAParameter().getType().getName().matches("%EventArgs")
32+
}
33+
34+
override Callable getAnAuthorizingCallable() {
35+
result = this
36+
or
37+
result.getDeclaringType() = this.getDeclaringType() and
38+
result.getName() = "Page_Load"
39+
}
2340
}
2441

2542
/** An expression that indicates that some authorization/authentication check is being performed. */
@@ -40,21 +57,52 @@ class AuthExpr extends Expr {
4057
}
4158

4259
/** Holds if `m` is a method that should have an auth check, and does indeed have one. */
43-
predicate hasAuth(Method m) {
44-
needsAuth(m) and
45-
exists(Method om, Callable caller, AuthExpr auth |
46-
om = m
60+
predicate hasAuthViaCode(ActionMethod m) {
61+
m.needsAuth() and
62+
exists(Callable caller, AuthExpr auth |
63+
m.getAnAuthorizingCallable().calls*(caller) and
64+
auth.getEnclosingCallable() = caller
65+
)
66+
}
67+
68+
class AuthorizationXmlElement extends XmlElement {
69+
AuthorizationXmlElement() {
70+
this.getParent() instanceof SystemWebXmlElement and
71+
this.getName().toLowerCase() = "authorization"
72+
}
73+
74+
predicate hasDenyElement() { this.getAChild().getName().toLowerCase() = "deny" }
75+
76+
string getPhysicalPath() { result = this.getFile().getParentContainer().getRelativePath() }
77+
78+
string getLocationTagPath() {
79+
exists(LocationXmlElement loc, XmlAttribute path |
80+
loc = this.getParent().(SystemWebXmlElement).getParent() and
81+
path = loc.getAnAttribute() and
82+
path.getName().toLowerCase() = "path" and
83+
result = path.getValue()
84+
)
85+
}
86+
}
87+
88+
/**
89+
* Holds if the given action has an xml `authorization` tag that refers to it.
90+
* TODO: Currently only supports physical paths, however virtual paths defined by `AddRoute` can also be used.
91+
*/
92+
predicate hasAuthViaXml(ActionMethod m) {
93+
exists(AuthorizationXmlElement el, string path, string rest |
94+
path = (el.getPhysicalPath() + "/" + el.getLocationTagPath())
4795
or
48-
om.getDeclaringType() = m.getDeclaringType() and
49-
om.getName() = "Page_Load"
96+
not exists(el.getLocationTagPath()) and
97+
path = el.getPhysicalPath()
5098
|
51-
om.calls*(caller) and
52-
auth.getEnclosingCallable() = caller
99+
el.hasDenyElement() and
100+
m.getDeclaringType().getFile().getRelativePath() = path + rest
53101
)
54102
}
55103

56104
/** Holds if `m` is a method that should have an auth check, but is missing it. */
57-
predicate missingAuth(Method m) {
58-
needsAuth(m) and
59-
not hasAuth(m)
105+
predicate missingAuth(ActionMethod m) {
106+
m.needsAuth() and
107+
not hasAuthViaCode(m)
60108
}

0 commit comments

Comments
 (0)