Merge pull request github#14143 from alexet/global-from-unreachble

CPP: Handle globals flowing into "UnreacheachedInstruction"

Author: MathiasVP

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll

@@ -447,9 +447,16 @@ class GlobalUse extends UseImpl, TGlobalUse {
   IRFunction getIRFunction() { result = f }
 
   final override predicate hasIndexInBlock(IRBlock block, int index) {
-    exists(ExitFunctionInstruction exit |
-      exit = f.getExitFunctionInstruction() and
-      block.getInstruction(index) = exit
+    // Similar to the `FinalParameterUse` case, we want to generate flow out of
+    // globals at any exit so that we can flow out of non-returning functions.
+    // Obviously this isn't correct as we can't actually flow but the global flow
+    // requires this if we want to flow into children.
+    exists(Instruction return |
+      return instanceof ReturnInstruction or
+      return instanceof UnreachedInstruction
+    |
+      block.getInstruction(index) = return and
+      return.getEnclosingIRFunction() = f
     )
   }
 

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

@@ -1,12 +1,21 @@
 edges
 | test.c:14:27:14:30 | argv indirection | test.c:21:18:21:23 | query1 indirection |
+| test.c:14:27:14:30 | argv indirection | test.c:35:16:35:23 | userName indirection |
+| test.c:35:16:35:23 | userName indirection | test.c:40:25:40:32 | username indirection |
+| test.c:38:7:38:20 | globalUsername indirection | test.c:51:18:51:23 | query1 indirection |
+| test.c:40:25:40:32 | username indirection | test.c:38:7:38:20 | globalUsername indirection |
 | test.cpp:39:27:39:30 | argv indirection | test.cpp:43:27:43:33 | access to array indirection |
 nodes
 | test.c:14:27:14:30 | argv indirection | semmle.label | argv indirection |
 | test.c:21:18:21:23 | query1 indirection | semmle.label | query1 indirection |
+| test.c:35:16:35:23 | userName indirection | semmle.label | userName indirection |
+| test.c:38:7:38:20 | globalUsername indirection | semmle.label | globalUsername indirection |
+| test.c:40:25:40:32 | username indirection | semmle.label | username indirection |
+| test.c:51:18:51:23 | query1 indirection | semmle.label | query1 indirection |
 | test.cpp:39:27:39:30 | argv indirection | semmle.label | argv indirection |
 | test.cpp:43:27:43:33 | access to array indirection | semmle.label | access to array indirection |
 subpaths
 #select
 | test.c:21:18:21:23 | query1 | test.c:14:27:14:30 | argv indirection | test.c:21:18:21:23 | query1 indirection | This argument to a SQL query function is derived from $@ and then passed to mysql_query(sqlArg). | test.c:14:27:14:30 | argv indirection | user input (a command-line argument) |
+| test.c:51:18:51:23 | query1 | test.c:14:27:14:30 | argv indirection | test.c:51:18:51:23 | query1 indirection | This argument to a SQL query function is derived from $@ and then passed to mysql_query(sqlArg). | test.c:14:27:14:30 | argv indirection | user input (a command-line argument) |
 | test.cpp:43:27:43:33 | access to array | test.cpp:39:27:39:30 | argv indirection | test.cpp:43:27:43:33 | access to array indirection | 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 indirection | user input (a command-line argument) |

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

@@ -8,7 +8,7 @@ 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);
-
+void exit(int i);
 ///// Test code /////
 
 int main(int argc, char** argv) {
@@ -31,4 +31,22 @@ int main(int argc, char** argv) {
   char query3[1000] = {0};
   snprintf(query3, 1000, "SELECT UID FROM USERS where number = \"%i\"", userNumber);
   mysql_query(0, query3); // GOOD
+
+  nonReturning(userName);
+}
+
+char* globalUsername;
+
+void nonReturning(char* username) {
+  globalUsername = username;
+  badFunc();
+  // This function does not return, so we used to lose the global flow here.
+  exit(0);
+}
+
+void badFunc() {
+  char *userName = globalUsername;
+  char query1[1000] = {0};
+  snprintf(query1, 1000, "SELECT UID FROM USERS where name = \"%s\"", userName);
+  mysql_query(0, query1); // BAD
 }