Java: add taint-tracking config for execute to exclude FPs from non-update queries like select

Author: Jami Cogswell

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

@@ -4,9 +4,9 @@ import java
 private import semmle.code.java.frameworks.spring.SpringController
 private import semmle.code.java.frameworks.MyBatis
 private import semmle.code.java.frameworks.Jdbc
-private import semmle.code.java.dataflow.DataFlow
 private import semmle.code.java.dataflow.ExternalFlow
 private import semmle.code.java.dispatch.VirtualDispatch
+private import semmle.code.java.dataflow.TaintTracking
 
 /** A method that is not protected from CSRF by default. */
 abstract class CsrfUnprotectedMethod extends Method { }
@@ -66,24 +66,46 @@ private class PreparedStatementDatabaseUpdateMethod extends DatabaseUpdateMethod
   }
 }
 
-/** A method that updates a SQL database. */
-private class SqlDatabaseUpdateMethod extends DatabaseUpdateMethod {
-  SqlDatabaseUpdateMethod() {
-    // TODO: constrain to only insert/update/delete for `execute%` methods; need to track the sql expression into the execute call.
+/** A method found via the sql-injection models which may update a SQL database. */
+private class SqlInjectionMethod extends DatabaseUpdateMethod {
+  SqlInjectionMethod() {
     exists(DataFlow::Node n | this = n.asExpr().(Argument).getCall().getCallee() |
       sinkNode(n, "sql-injection") and
-      this.getName()
-          .regexpMatch(".*(?i)(delete|insert|update|save|persist|merge|replicate|execute).*")
+      this.getName().regexpMatch(".*(?i)(delete|insert|update|save|persist|merge|replicate).*")
+      or
+      // do not include `executeQuery` since it is typically used with a select statement
+      this.hasName("execute")
+    )
+  }
+}
+
+/**
+ * A taint-tracking configuration for reasoning about SQL queries that update a database.
+ */
+module SqlExecuteConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    exists(StringLiteral sl | source.asExpr() = sl |
+      sl.getValue().regexpMatch("^(?i)(insert|update|delete).*")
+    )
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    exists(Method m | m = sink.asExpr().(Argument).getCall().getCallee() |
+      m instanceof SqlInjectionMethod and
+      m.hasName("execute")
     )
   }
 }
 
+/** Tracks flow from SQL queries that update a database to the argument of an execute method call. */
+module SqlExecuteFlow = TaintTracking::Global<SqlExecuteConfig>;
+
 module CallGraph {
-  newtype TPathNode =
+  newtype TCallPathNode =
     TMethod(Method m) or
     TCall(Call c)
 
-  class PathNode extends TPathNode {
+  class CallPathNode extends TCallPathNode {
     Method asMethod() { this = TMethod(result) }
 
     Call asCall() { this = TCall(result) }
@@ -94,16 +116,16 @@ module CallGraph {
       result = this.asCall().toString()
     }
 
-    private PathNode getACallee() {
+    private CallPathNode getACallee() {
       [viableCallable(this.asCall()), this.asCall().getCallee()] = result.asMethod()
     }
 
-    PathNode getASuccessor() {
+    CallPathNode getASuccessor() {
       this.asMethod() = result.asCall().getEnclosingCallable()
       or
       result = this.getACallee() and
       (
-        exists(PathNode p |
+        exists(CallPathNode p |
           p = this.getACallee() and
           p.asMethod() instanceof DatabaseUpdateMethod
         )
@@ -119,15 +141,25 @@ module CallGraph {
     }
   }
 
-  predicate edges(PathNode pred, PathNode succ) { pred.getASuccessor() = succ }
+  predicate edges(CallPathNode pred, CallPathNode succ) { pred.getASuccessor() = succ }
 }
 
 import CallGraph
 
 /** Holds if `src` is an unprotected request handler that reaches a state-changing `sink`. */
-predicate unprotectedStateChange(PathNode src, PathNode sink, PathNode sinkPred) {
+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
+  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()
 }

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

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

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

@@ -10,9 +10,13 @@
 import java.sql.Connection;
 import java.sql.PreparedStatement;
 import java.sql.SQLException;
+import java.sql.Connection;
+import java.sql.ResultSet;
+import java.sql.Statement;
 
 @Controller
 public class CsrfUnprotectedRequestTypeTest {
+    public static Connection connection;
 
     // Test Spring sources with `PreparedStatement.executeUpdate()` as a default database update method call
 
@@ -129,6 +133,40 @@ public void bad6() { // $ hasCsrfUnprotectedRequestType
         } catch (SQLException e) { }
     }
 
+    @RequestMapping("/")
+    public void badStatementExecuteUpdate() { // $ hasCsrfUnprotectedRequestType
+        try {
+            String item = "item";
+            String price = "price";
+            Statement statement = connection.createStatement();
+            String query = "UPDATE PRODUCT SET PRICE='" + price + "' WHERE ITEM='" + item + "'";
+            int count = statement.executeUpdate(query);
+        } catch (SQLException e) { }
+    }
+
+    @RequestMapping("/")
+    public void badStatementExecute() { // $ hasCsrfUnprotectedRequestType
+        try {
+            String item = "item";
+            String price = "price";
+            Statement statement = connection.createStatement();
+            String query = "UPDATE PRODUCT SET PRICE='" + price + "' WHERE ITEM='" + item + "'";
+            boolean bool = statement.execute(query);
+        } catch (SQLException e) { }
+    }
+
+    // GOOD: select not insert/update/delete
+    @RequestMapping("/")
+    public void goodStatementExecute() {
+        try {
+            String category = "category";
+            Statement statement = connection.createStatement();
+            String query = "SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='"
+                    + category + "' ORDER BY PRICE";
+            boolean bool = statement.execute(query);
+        } catch (SQLException e) { }
+    }
+
     @Autowired
 	private MyBatisService myBatisService;
 

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

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