Update alert message and make user checks more precise

Author: joefarebrother

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

@@ -22,6 +22,7 @@ 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 |
       idStr.getValue().toLowerCase().matches(["%id", "%idx"]) and
       getParentExpr*(src.asExpr()) = getParentExpr*(idStr)
@@ -31,8 +32,10 @@ private predicate hasIdParameter(ActionMethod m) {
 
 /** 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(Property p | p.getName().toLowerCase().matches(["%user%", "%session%"]) |
-    m.calls*(p.getGetter())
+  exists(Callable c, string name | name = c.getName().toLowerCase() |
+    name.matches(["%user%", "%session%"]) and
+    not name.matches("%get%by%") and // methods like `getUserById` or `getXByUsername` aren't likely to be referring to the current user
+    m.calls*(c)
   )
 }
 

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

@@ -17,4 +17,4 @@ import semmle.code.csharp.security.auth.InsecureDirectObjectReferenceQuery
 from ActionMethod m
 where hasInsecureDirectObjectReference(m)
 select m,
-  "This method may not verify which users should be able to access resources of the provided ID."
+  "This method may be missing authorization checks for which users can access the resource of the provided ID."

csharp/ql/test/query-tests/Security Features/CWE-639/MVCTests/InsecureDirectObjectReference.expected

@@ -1 +1 @@
-| CommentController.cs:6:25:6:29 | Edit1 | This method may not verify which users should be able to access resources of the provided ID. |
+| 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. |

csharp/ql/test/query-tests/Security Features/CWE-639/WebFormsTests/InsecureDirectObjectReference.expected

@@ -1 +1 @@
-| EditComment.aspx.cs:7:20:7:29 | btn1_Click | This method may not verify which users should be able to access resources of the provided ID. |
+| 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. |