C++: Fixup 'cpp/sql-injection' so that it doesn't require the sink to be an (indirect)expr.

Author: MathiasVP

cpp/ql/src/Security/CWE/CWE-089/SqlTainted.ql

@@ -33,15 +33,20 @@ Expr asSinkExpr(DataFlow::Node node) {
   result = node.asExpr()
 }
 
+private predicate isSink(DataFlow::Node sink, string extraText) {
+  exists(SqlLikeFunction runSql, string callChain |
+    runSql.outermostWrapperFunctionCall(asSinkExpr(sink), callChain) and
+    extraText = " and then passed to " + callChain
+  )
+  or
+  // sink defined using models-as-data
+  sinkNode(sink, "sql-injection") and extraText = ""
+}
+
 module SqlTaintedConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node node) { node instanceof FlowSource }
 
-  predicate isSink(DataFlow::Node node) {
-    exists(SqlLikeFunction runSql | runSql.outermostWrapperFunctionCall(asSinkExpr(node), _))
-    or
-    // sink defined using models-as-data
-    sinkNode(node, "sql-injection")
-  }
+  predicate isSink(DataFlow::Node node) { isSink(node, _) }
 
   predicate isBarrier(DataFlow::Node node) {
     node.asExpr().getUnspecifiedType() instanceof IntegralType
@@ -68,21 +73,12 @@ module SqlTaintedConfig implements DataFlow::ConfigSig {
 module SqlTainted = TaintTracking::Global<SqlTaintedConfig>;
 
 from
-  Expr taintedArg, FlowSource taintSource, SqlTainted::PathNode sourceNode,
-  SqlTainted::PathNode sinkNode, string extraText
+  FlowSource taintSource, SqlTainted::PathNode sourceNode, SqlTainted::PathNode sinkNode,
+  string extraText
 where
-  (
-    exists(SqlLikeFunction runSql, string callChain |
-      runSql.outermostWrapperFunctionCall(taintedArg, callChain) and
-      extraText = " and then passed to " + callChain
-    )
-    or
-    sinkNode(sinkNode.getNode(), "sql-injection") and
-    extraText = ""
-  ) and
   SqlTainted::flowPath(sourceNode, sinkNode) and
-  taintedArg = asSinkExpr(sinkNode.getNode()) and
+  isSink(sinkNode.getNode(), extraText) and
   taintSource = sourceNode.getNode()
-select taintedArg, sourceNode, sinkNode,
+select sinkNode.getNode(), sourceNode, sinkNode,
   "This argument to a SQL query function is derived from $@" + extraText + ".", taintSource,
   "user input (" + taintSource.getSourceType() + ")"

cpp/ql/test/query-tests/Security/CWE/CWE-089/SqlTainted/SqlTainted.expected

@@ -1,11 +1,11 @@
 #select
-| test.c:21:18:21:23 | query1 | test.c:14:27:14:30 | **argv | test.c:21:18:21:23 | *query1 | This argument to a SQL query function is derived from $@ and then passed to mysql_query(sqlArg). | test.c:14:27:14:30 | **argv | user input (a command-line argument) |
-| test.c:51:18:51:23 | query1 | test.c:14:27:14:30 | **argv | test.c:51:18:51:23 | *query1 | This argument to a SQL query function is derived from $@ and then passed to mysql_query(sqlArg). | test.c:14:27:14:30 | **argv | user input (a command-line argument) |
-| test.c:76:17:76:25 | userInput | test.c:75:8:75:16 | gets output argument | test.c:76:17:76:25 | *userInput | This argument to a SQL query function is derived from $@ and then passed to SQLPrepare(StatementText). | test.c:75:8:75:16 | gets output argument | user input (string read by gets) |
-| test.c:77:20:77:28 | userInput | test.c:75:8:75:16 | gets output argument | test.c:77:20:77:28 | *userInput | This argument to a SQL query function is derived from $@ and then passed to SQLExecDirect(StatementText). | test.c:75:8:75:16 | gets output argument | user input (string read by gets) |
-| test.c:106:24:106:29 | query1 | test.c:101:8:101:16 | gets output argument | test.c:106:24:106:29 | *query1 | This argument to a SQL query function is derived from $@. | test.c:101:8:101:16 | gets output argument | user input (string read by gets) |
-| test.c:107:28:107:33 | query1 | test.c:101:8:101:16 | gets output argument | test.c:107:28:107:33 | *query1 | This argument to a SQL query function is derived from $@. | test.c:101:8:101:16 | gets output argument | user input (string read by gets) |
-| test.cpp:43:27:43:33 | access to array | test.cpp:39:27:39:30 | **argv | test.cpp:43:27:43:33 | *access to array | This argument to a SQL query function is derived from $@ and then passed to pqxx::work::exec1((unnamed parameter 0)). | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
+| test.c:21:18:21:23 | *query1 | test.c:14:27:14:30 | **argv | test.c:21:18:21:23 | *query1 | This argument to a SQL query function is derived from $@ and then passed to mysql_query(sqlArg). | test.c:14:27:14:30 | **argv | user input (a command-line argument) |
+| test.c:51:18:51:23 | *query1 | test.c:14:27:14:30 | **argv | test.c:51:18:51:23 | *query1 | This argument to a SQL query function is derived from $@ and then passed to mysql_query(sqlArg). | test.c:14:27:14:30 | **argv | user input (a command-line argument) |
+| test.c:76:17:76:25 | *userInput | test.c:75:8:75:16 | gets output argument | test.c:76:17:76:25 | *userInput | This argument to a SQL query function is derived from $@ and then passed to SQLPrepare(StatementText). | test.c:75:8:75:16 | gets output argument | user input (string read by gets) |
+| test.c:77:20:77:28 | *userInput | test.c:75:8:75:16 | gets output argument | test.c:77:20:77:28 | *userInput | This argument to a SQL query function is derived from $@ and then passed to SQLExecDirect(StatementText). | test.c:75:8:75:16 | gets output argument | user input (string read by gets) |
+| test.c:106:24:106:29 | query1 | test.c:101:8:101:16 | gets output argument | test.c:106:24:106:29 | query1 | This argument to a SQL query function is derived from $@. | test.c:101:8:101:16 | gets output argument | user input (string read by gets) |
+| test.c:107:28:107:33 | query1 | test.c:101:8:101:16 | gets output argument | test.c:107:28:107:33 | query1 | This argument to a SQL query function is derived from $@. | test.c:101:8:101:16 | gets output argument | user input (string read by gets) |
+| test.cpp:43:27:43:33 | *access to array | test.cpp:39:27:39:30 | **argv | test.cpp:43:27:43:33 | *access to array | This argument to a SQL query function is derived from $@ and then passed to pqxx::work::exec1((unnamed parameter 0)). | test.cpp:39:27:39:30 | **argv | user input (a command-line argument) |
 edges
 | test.c:14:27:14:30 | **argv | test.c:15:20:15:26 | *access to array | provenance |  |
 | test.c:15:20:15:26 | *access to array | test.c:21:18:21:23 | *query1 | provenance | TaintFunction |
@@ -17,8 +17,8 @@ edges
 | test.c:48:20:48:33 | *globalUsername | test.c:51:18:51:23 | *query1 | provenance | TaintFunction |
 | test.c:75:8:75:16 | gets output argument | test.c:76:17:76:25 | *userInput | provenance |  |
 | test.c:75:8:75:16 | gets output argument | test.c:77:20:77:28 | *userInput | provenance |  |
-| test.c:101:8:101:16 | gets output argument | test.c:106:24:106:29 | *query1 | provenance | TaintFunction Sink:MaD:2 |
-| test.c:101:8:101:16 | gets output argument | test.c:107:28:107:33 | *query1 | provenance | TaintFunction Sink:MaD:1 |
+| test.c:101:8:101:16 | gets output argument | test.c:106:24:106:29 | query1 | provenance | TaintFunction Sink:MaD:2 |
+| test.c:101:8:101:16 | gets output argument | test.c:107:28:107:33 | query1 | provenance | TaintFunction Sink:MaD:1 |
 | test.cpp:39:27:39:30 | **argv | test.cpp:43:27:43:33 | *access to array | provenance |  |
 models
 | 1 | Sink: ; ; false; OCIStmtPrepare2; ; ; Argument[*3]; sql-injection; manual |
@@ -37,8 +37,8 @@ nodes
 | test.c:76:17:76:25 | *userInput | semmle.label | *userInput |
 | test.c:77:20:77:28 | *userInput | semmle.label | *userInput |
 | test.c:101:8:101:16 | gets output argument | semmle.label | gets output argument |
-| test.c:106:24:106:29 | *query1 | semmle.label | *query1 |
-| test.c:107:28:107:33 | *query1 | semmle.label | *query1 |
+| test.c:106:24:106:29 | query1 | semmle.label | query1 |
+| test.c:107:28:107:33 | query1 | semmle.label | query1 |
 | test.cpp:39:27:39:30 | **argv | semmle.label | **argv |
 | test.cpp:43:27:43:33 | *access to array | semmle.label | *access to array |
 subpaths