Java: add name-based heuristic

Author: Jami Cogswell

java/ql/lib/semmle/code/java/security/CsrfUnprotectedRequestTypeQuery.qll

@@ -49,6 +49,15 @@ private class StaplerCsrfUnprotectedMethod extends CsrfUnprotectedMethod instanc
   }
 }
 
+/** A method that appears to change application state based on its name. */
+private class NameStateChangeMethod extends Method {
+  NameStateChangeMethod() {
+    this.getName()
+        .regexpMatch(".*(?i)(post|put|patch|delete|remove|create|add|update|edit|publish|unpublish|fill|move|transfer|log(out|in)|access|connect|register|submit|den(y|ied)).*") and
+    not this.getName().regexpMatch("^(get|show|view|list|query|find).*")
+  }
+}
+
 /** A method that updates a database. */
 abstract class DatabaseUpdateMethod extends Method { }
 
@@ -162,20 +171,46 @@ module CallGraph {
 
 import CallGraph
 
-/** Holds if `src` is an unprotected request handler that reaches a state-changing `sink`. */
-predicate unprotectedStateChange(CallPathNode src, CallPathNode sink, CallPathNode sinkPred) {
-  src.asMethod() instanceof CsrfUnprotectedMethod and
-  sink.asMethod() instanceof DatabaseUpdateMethod and
-  sinkPred.getASuccessor() = sink and
-  src.getASuccessor+() = sinkPred and
-  if
-    sink.asMethod() instanceof SqlInjectionMethod and
-    sink.asMethod().hasName("execute")
-  then
-    exists(SqlExecuteFlow::PathNode executeSrc, SqlExecuteFlow::PathNode executeSink |
-      SqlExecuteFlow::flowPath(executeSrc, executeSink)
-    |
-      sinkPred.asCall() = executeSink.getNode().asExpr().(Argument).getCall()
-    )
-  else any()
+/**
+ * Holds if `src` is an unprotected request handler that reaches a
+ * `sink` that updates a database.
+ */
+predicate unprotectedDatabaseUpdate(CallPathNode sourceMethod, CallPathNode sinkMethodCall) {
+  sourceMethod.asMethod() instanceof CsrfUnprotectedMethod and
+  exists(CallPathNode sinkMethod |
+    sinkMethod.asMethod() instanceof DatabaseUpdateMethod and
+    sinkMethodCall.getASuccessor() = sinkMethod and
+    sourceMethod.getASuccessor+() = sinkMethodCall and
+    if
+      sinkMethod.asMethod() instanceof SqlInjectionMethod and
+      sinkMethod.asMethod().hasName("execute")
+    then
+      exists(SqlExecuteFlow::PathNode executeSrc, SqlExecuteFlow::PathNode executeSink |
+        SqlExecuteFlow::flowPath(executeSrc, executeSink)
+      |
+        sinkMethodCall.asCall() = executeSink.getNode().asExpr().(Argument).getCall()
+      )
+    else any()
+  )
+}
+
+/**
+ * Holds if `src` is an unprotected request handler that appears to
+ * change application state based on its name.
+ */
+private predicate unprotectedHeuristicStateChange(CallPathNode sourceMethod, CallPathNode sinkMethod) {
+  sourceMethod.asMethod() instanceof CsrfUnprotectedMethod and
+  sinkMethod.asMethod() instanceof NameStateChangeMethod and
+  sinkMethod = sourceMethod and
+  // exclude any alerts that update a database
+  not unprotectedDatabaseUpdate(sourceMethod, _)
+}
+
+/**
+ * Holds if `src` is an unprotected request handler that may
+ * change an application's state.
+ */
+predicate unprotectedStateChange(CallPathNode source, CallPathNode sink) {
+  unprotectedDatabaseUpdate(source, sink) or
+  unprotectedHeuristicStateChange(source, sink)
 }

java/ql/src/Security/CWE/CWE-352/CsrfUnprotectedRequestType.ql

@@ -17,8 +17,8 @@ import semmle.code.java.security.CsrfUnprotectedRequestTypeQuery
 
 query predicate edges(CallPathNode pred, CallPathNode succ) { CallGraph::edges(pred, succ) }
 
-from CallPathNode source, CallPathNode reachable, CallPathNode callsReachable
-where unprotectedStateChange(source, reachable, callsReachable)
-select source.asMethod(), source, callsReachable,
+from CallPathNode source, CallPathNode sink
+where unprotectedStateChange(source, sink)
+select source.asMethod(), source, sink,
   "Potential CSRF vulnerability due to using an HTTP request type which is not default-protected from CSRF for an apparent $@.",
-  callsReachable, "state-changing action"
+  sink, "state-changing action"

java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.java

@@ -211,4 +211,10 @@ public void bad9(@RequestParam String user) { // $ hasCsrfUnprotectedRequestType
 	public void bad10(@RequestParam String user) { // $ hasCsrfUnprotectedRequestType
 		myBatisService.bad10(user);
 	}
+
+    // BAD: method name implies a state-change
+    @GetMapping(value = "delete")
+	public String delete(@RequestParam String user) { // $ hasCsrfUnprotectedRequestType
+		return "delete";
+	}
 }

java/ql/test/query-tests/security/CWE-352/CsrfUnprotectedRequestTypeTest.ql

@@ -7,9 +7,7 @@ module CsrfUnprotectedRequestTypeTest implements TestSig {
 
   predicate hasActualResult(Location location, string element, string tag, string value) {
     tag = "hasCsrfUnprotectedRequestType" and
-    exists(CallPathNode src, CallPathNode sink, CallPathNode sinkPred |
-      unprotectedStateChange(src, sink, sinkPred)
-    |
+    exists(CallPathNode src, CallPathNode sink | unprotectedStateChange(src, sink) |
       src.getLocation() = location and
       element = src.toString() and
       value = ""