Minor changes from review suggestions to shared logic between this and missing access control

Use case insensitive regex, factor out page load to improve possible bad joins make needsAuth not a member predicate
This commit is contained in:
Joe Farebrother
2023-08-25 11:05:00 +01:00
parent a022893f0f
commit 0a27da08d6
2 changed files with 16 additions and 13 deletions

View File

@@ -20,10 +20,9 @@ abstract class ActionMethod extends Method {
str =
this.getADescription()
// separate camelCase words
.regexpReplaceAll("([a-z])([A-Z])", "$1_$2")
.toLowerCase() and
str.regexpMatch(".*(edit|delete|modify|change).*") and
not str.regexpMatch(".*(on_?change|changed).*")
.regexpReplaceAll("([a-z])([A-Z])", "$1_$2") and
str.regexpMatch("(?i).*(edit|delete|modify|change).*") and
not str.regexpMatch("(?i).*(on_?change|changed).*")
)
}
@@ -32,13 +31,9 @@ abstract class ActionMethod extends Method {
this.getADescription()
// separate camelCase words
.regexpReplaceAll("([a-z])([A-Z])", "$1_$2")
.toLowerCase()
.regexpMatch(".*(admin|superuser).*")
.regexpMatch("(?i).*(admin|superuser).*")
}
/** Holds if this method may need an authorization check. */
predicate needsAuth() { this.isEdit() or this.isAdmin() }
/** Gets a callable for which if it contains an auth check, this method should be considered authenticated. */
Callable getAnAuthorizingCallable() { result = this }
@@ -64,8 +59,7 @@ private class WebFormActionMethod extends ActionMethod {
override Callable getAnAuthorizingCallable() {
result = super.getAnAuthorizingCallable()
or
result.getDeclaringType() = this.getDeclaringType() and
result.getName() = "Page_Load"
pageLoad(result, this.getDeclaringType())
}
override string getARoute() {
@@ -80,6 +74,12 @@ private class WebFormActionMethod extends ActionMethod {
}
}
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

View File

@@ -6,6 +6,9 @@ import semmle.code.csharp.frameworks.system.web.UI
import semmle.code.asp.WebConfig
import ActionMethods
/** 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 {
AuthExpr() {
@@ -25,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
@@ -86,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