Merge pull request github#4915 from geoffw0/sqltaint

C++: Fix FPs in cpp/sql-injection

Author: MathiasVP

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

@@ -27,6 +27,10 @@ class Configuration extends TaintTrackingConfiguration {
   override predicate isSink(Element tainted) {
     exists(SQLLikeFunction runSql | runSql.outermostWrapperFunctionCall(tainted, _))
   }
+
+  override predicate isBarrier(Expr e) {
+    super.isBarrier(e) or e.getUnspecifiedType() instanceof IntegralType
+  }
 }
 
 from

cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -545,6 +545,9 @@ module TaintedWithPath {
     /** Override this to specify which elements are sinks in this configuration. */
     abstract predicate isSink(Element e);
 
+    /** Override this to specify which expressions are barriers in this configuration. */
+    predicate isBarrier(Expr e) { nodeIsBarrier(getNodeForExpr(e)) }
+
     /**
      * Override this predicate to `any()` to allow taint to flow through global
      * variables.
@@ -578,7 +581,9 @@ module TaintedWithPath {
       )
     }
 
-    override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }
+    override predicate isBarrier(DataFlow::Node node) {
+      exists(TaintTrackingConfiguration cfg, Expr e | cfg.isBarrier(e) and node = getNodeForExpr(e))
+    }
 
     override predicate isBarrierIn(DataFlow::Node node) { nodeIsBarrierIn(node) }
   }

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

@@ -0,0 +1,13 @@
+edges
+| test.c:15:20:15:23 | argv | test.c:21:18:21:23 | (const char *)... |
+| test.c:15:20:15:23 | argv | test.c:21:18:21:23 | (const char *)... |
+| test.c:15:20:15:23 | argv | test.c:21:18:21:23 | query1 |
+| test.c:15:20:15:23 | argv | test.c:21:18:21:23 | query1 |
+nodes
+| test.c:15:20:15:23 | argv | semmle.label | argv |
+| test.c:15:20:15:23 | argv | semmle.label | argv |
+| test.c:21:18:21:23 | (const char *)... | semmle.label | (const char *)... |
+| test.c:21:18:21:23 | (const char *)... | semmle.label | (const char *)... |
+| test.c:21:18:21:23 | query1 | semmle.label | query1 |
+#select
+| test.c:21:18:21:23 | query1 | test.c:15:20:15:23 | 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:15:20:15:23 | argv | user input (argv) |

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

@@ -0,0 +1 @@
+Security/CWE/CWE-089/SqlTainted.ql

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

@@ -0,0 +1,34 @@
+// Semmle test case for rule SprintfToSqlQuery.ql (Uncontrolled sprintf for SQL query)
+// Associated with CWE-089: SQL injection. http://cwe.mitre.org/data/definitions/89.html
+
+///// Library routines /////
+
+typedef unsigned long size_t;
+int snprintf(char *s, size_t n, const char *format, ...);
+void sanitizeString(char *stringOut, size_t len, const char *strIn);
+int mysql_query(int arg1, const char *sqlArg);
+int atoi(const char *nptr);
+
+///// Test code /////
+
+int main(int argc, char** argv) {
+  char *userName = argv[2];
+  int userNumber = atoi(argv[3]);
+
+  // a string from the user is injected directly into an SQL query.
+  char query1[1000] = {0};
+  snprintf(query1, 1000, "SELECT UID FROM USERS where name = \"%s\"", userName);
+  mysql_query(0, query1); // BAD
+  
+  // the user string is encoded by a library routine.
+  char userNameSanitized[1000] = {0};
+  sanitizeString(userNameSanitized, 1000, userName); 
+  char query2[1000] = {0};
+  snprintf(query2, 1000, "SELECT UID FROM USERS where name = \"%s\"", userNameSanitized);
+  mysql_query(0, query2); // GOOD
+
+  // an integer from the user is injected into an SQL query.
+  char query3[1000] = {0};
+  snprintf(query3, 1000, "SELECT UID FROM USERS where number = \"%i\"", userNumber);
+  mysql_query(0, query3); // GOOD
+}