Merge pull request github#5842 from japroc/cpp-pqxx-sqli-sink

C++: SqlPqxxTainted query searches for sql injections via pqxx connector to postgres

Author: MathiasVP

cpp/ql/src/experimental/Security/CWE/CWE-089/SqlPqxxTainted.cpp

@@ -0,0 +1,28 @@
+#include <iostream>
+#include <stdexcept>
+#include <pqxx/pqxx>
+
+int main(int argc, char ** argv) {
+
+    if (argc != 2) { 
+        throw std::runtime_error("Give me a string!");
+    }
+    
+    pqxx::connection c;
+    pqxx::work w(c);
+
+    // BAD
+    char *userName = argv[1];
+    char query1[1000] = {0};
+    sprintf(query1, "SELECT UID FROM USERS where name = \"%s\"", userName);
+    pqxx::row r = w.exec1(query1);
+    w.commit();
+    std::cout << r[0].as<int>() << std::endl;
+
+    // GOOD
+    pqxx::result r2 = w.exec("SELECT " + w.quote(argv[1]));
+    w.commit();
+    std::cout << r2[0][0].c_str() << std::endl;
+
+    return 0;
+}

cpp/ql/src/experimental/Security/CWE/CWE-089/SqlPqxxTainted.qhelp

@@ -0,0 +1,31 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>The code passes user input as part of a SQL query without escaping special elements. 
+It generates a SQL query to Postgres using <code>sprintf</code>,
+with the user-supplied data directly passed as an argument
+to <code>sprintf</code>. This leaves the code vulnerable to attack by SQL Injection.</p>
+
+</overview>
+<recommendation>
+
+<p>Use a library routine to escape characters in the user-supplied
+string before converting it to SQL. Use <code>esc</code> and <code>quote</code> pqxx library functions.</p>
+
+</recommendation>
+<example>
+<sample src="SqlPqxxTainted.cpp" />
+
+</example>
+<references>
+
+<li>MSDN Library: <a href="https://docs.microsoft.com/en-us/sql/relational-databases/security/sql-injection">SQL Injection</a>.</li>
+
+
+<!--  LocalWords:  SQL CWE
+ -->
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-089/SqlPqxxTainted.ql

@@ -0,0 +1,113 @@
+/**
+ * @name Uncontrolled data in SQL query to Postgres
+ * @description Including user-supplied data in a SQL query to Postgres
+ *              without neutralizing special elements can make code
+ *              vulnerable to SQL Injection.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id cpp/sql-injection-via-pqxx
+ * @tags security
+ *       external/cwe/cwe-089
+ */
+
+import cpp
+import semmle.code.cpp.security.Security
+import semmle.code.cpp.dataflow.TaintTracking
+import DataFlow::PathGraph
+
+predicate pqxxTransationClassNames(string className, string namespace) {
+  namespace = "pqxx" and
+  className in [
+      "dbtransaction", "nontransaction", "basic_robusttransaction", "robusttransaction",
+      "subtransaction", "transaction", "basic_transaction", "transaction_base", "work"
+    ]
+}
+
+predicate pqxxConnectionClassNames(string className, string namespace) {
+  namespace = "pqxx" and
+  className in ["connection_base", "basic_connection", "connection"]
+}
+
+predicate pqxxTransactionSqlArgument(string function, int arg) {
+  function = "exec" and arg = 0
+  or
+  function = "exec0" and arg = 0
+  or
+  function = "exec1" and arg = 0
+  or
+  function = "exec_n" and arg = 1
+  or
+  function = "exec_params" and arg = 0
+  or
+  function = "exec_params0" and arg = 0
+  or
+  function = "exec_params1" and arg = 0
+  or
+  function = "exec_params_n" and arg = 1
+  or
+  function = "query_value" and arg = 0
+  or
+  function = "stream" and arg = 0
+}
+
+predicate pqxxConnectionSqlArgument(string function, int arg) { function = "prepare" and arg = 1 }
+
+Expr getPqxxSqlArgument() {
+  exists(FunctionCall fc, Expr e, int argIndex, UserType t |
+    // examples: 'work' for 'work.exec(...)'; '->' for 'tx->exec()'.
+    e = fc.getQualifier() and
+    // to find ConnectionHandle/TransationHandle and similar classes which override '->' operator behavior
+    // and return pointer to a connection/transation object
+    e.getType().refersTo(t) and
+    // transaction exec and connection prepare variations
+    (
+      pqxxTransationClassNames(t.getName(), t.getNamespace().getName()) and
+      pqxxTransactionSqlArgument(fc.getTarget().getName(), argIndex)
+      or
+      pqxxConnectionClassNames(t.getName(), t.getNamespace().getName()) and
+      pqxxConnectionSqlArgument(fc.getTarget().getName(), argIndex)
+    ) and
+    result = fc.getArgument(argIndex)
+  )
+}
+
+predicate pqxxEscapeArgument(string function, int arg) {
+  arg = 0 and
+  function in ["esc", "esc_raw", "quote", "quote_raw", "quote_name", "quote_table", "esc_like"]
+}
+
+predicate isEscapedPqxxArgument(Expr argExpr) {
+  exists(FunctionCall fc, Expr e, int argIndex, UserType t |
+    // examples: 'work' for 'work.exec(...)'; '->' for 'tx->exec()'.
+    e = fc.getQualifier() and
+    // to find ConnectionHandle/TransationHandle and similar classes which override '->' operator behavior
+    // and return pointer to a connection/transation object
+    e.getType().refersTo(t) and
+    // transaction and connection escape functions
+    (
+      pqxxTransationClassNames(t.getName(), t.getNamespace().getName()) or
+      pqxxConnectionClassNames(t.getName(), t.getNamespace().getName())
+    ) and
+    pqxxEscapeArgument(fc.getTarget().getName(), argIndex) and
+    // is escaped arg == argExpr
+    argExpr = fc.getArgument(argIndex)
+  )
+}
+
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "SqlPqxxTainted" }
+
+  override predicate isSource(DataFlow::Node source) { isUserInput(source.asExpr(), _) }
+
+  override predicate isSink(DataFlow::Node sink) { sink.asExpr() = getPqxxSqlArgument() }
+
+  override predicate isSanitizer(DataFlow::Node node) { isEscapedPqxxArgument(node.asExpr()) }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, Configuration config, string taintCause
+where
+  config.hasFlowPath(source, sink) and
+  isUserInput(source.getNode().asExpr(), taintCause)
+select sink, source, sink, "This argument to a SQL query function is derived from $@", source,
+  "user input (" + taintCause + ")"