Merge pull request #5889 from japroc/python-clickhouse-driver

Python: Implement module ClickHouseDriver.qll

python/ql/src/experimental/Security/CWE-089/ClickHouseSQLInjection.py

@@ -0,0 +1,28 @@
+from django.conf.urls import url
+from clickhouse_driver import Client
+from clickhouse_driver import connect
+from aioch import Client as aiochClient
+
+class MyClient(Client):
+    def dummy(self):
+        return None
+
+def show_user(request, username):
+
+    # BAD -- Untrusted user input is directly injected into the sql query using async library 'aioch'
+    aclient = aiochClient("localhost")
+    progress = await aclient.execute_with_progress("SELECT * FROM users WHERE username = '%s'" % username)
+
+    # BAD -- Untrusted user input is directly injected into the sql query using native client of library 'clickhouse_driver'
+    Client('localhost').execute("SELECT * FROM users WHERE username = '%s'" % username)
+
+    # GOOD -- query uses prepared statements
+    query = "SELECT * FROM users WHERE username = %(username)s"
+    Client('localhost').execute(query, {"username": username})
+
+    # BAD -- PEP249 interface
+    conn = connect('clickhouse://localhost')
+    cursor = conn.cursor()
+    cursor.execute("SELECT * FROM users WHERE username = '%s'" % username)
+
+urlpatterns = [url(r'^users/(?P<username>[^/]+)$', show_user)]

python/ql/src/experimental/Security/CWE-089/ClickHouseSQLInjection.qhelp

@@ -0,0 +1,59 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+If a database query (such as a SQL or NoSQL query) is built from
+user-provided data without sufficient sanitization, a user
+may be able to run malicious database queries.
+</p>
+</overview>
+
+<recommendation>
+<p>
+Most database connector libraries offer a way of safely
+embedding untrusted data into a query by means of query parameters
+or prepared statements.
+</p>
+</recommendation>
+
+<example>
+<p>
+In the following snippet, a user is fetched from a <code>ClickHouse</code> database
+using five different queries. In the "BAD" cases the query is built directly from user-controlled data.
+In the "GOOD" case the user-controlled data is safely embedded into the query by using query parameters.
+</p>
+
+<p>
+In the first case, the query executed via aioch Client. aioch - is a module
+for asynchronous queries to database.
+</p>
+
+<p>
+In the second and third cases, the connection is established via `Client` class.
+This class implement different method to execute a query.
+</p>
+
+<p>
+In the forth case, good pattern is presented. Query parameters are send through
+second dict-like argument.
+</p>
+
+<p>
+In the fifth case, there is example of PEP249 interface usage.
+</p>
+
+<p>
+In the sixth case, there is custom Class usge which is a subclass of default Client.
+</p>
+
+<sample src="ClickHouseSQLInjection.py" />
+</example>
+
+<references>
+<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/SQL_injection">SQL injection</a>.</li>
+<li>OWASP: <a href="https://cheatsheetseries.owasp.org/cheatsheets/SQL_Injection_Prevention_Cheat_Sheet.html">SQL Injection Prevention Cheat Sheet</a>.</li>
+</references>
+</qhelp>

python/ql/src/experimental/Security/CWE-089/ClickHouseSQLInjection.ql

@@ -0,0 +1,22 @@
+/**
+ * @id py/yandex/clickhouse-sql-injection
+ * @name Clickhouse SQL query built from user-controlled sources
+ * @description Building a SQL query from user-controlled sources is vulnerable to insertion of
+ *              malicious SQL code by the user.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @tags security
+ *       external/cwe/cwe-089
+ *       external/owasp/owasp-a1
+ */
+
+import python
+import experimental.semmle.python.frameworks.ClickHouseDriver
+import semmle.python.security.dataflow.SqlInjection
+import DataFlow::PathGraph
+
+from SQLInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "This SQL query depends on $@.", source.getNode(),
+  "a user-provided value"

python/ql/src/experimental/semmle/python/frameworks/ClickHouseDriver.qll

@@ -0,0 +1,85 @@
+/**
+ * Provides classes modeling security-relevant aspects of `clickhouse-driver` and `aioch` PyPI packages.
+ * See
+ * - https://pypi.org/project/clickhouse-driver/
+ * - https://pypi.org/project/aioch/
+ * - https://clickhouse-driver.readthedocs.io/en/latest/
+ */
+
+private import python
+private import semmle.python.Concepts
+private import semmle.python.ApiGraphs
+private import semmle.python.frameworks.PEP249
+
+/**
+ * Provides models for `clickhouse-driver` and `aioch` PyPI packages.
+ * See
+ * - https://pypi.org/project/clickhouse-driver/
+ * - https://pypi.org/project/aioch/
+ * - https://clickhouse-driver.readthedocs.io/en/latest/
+ */
+module ClickHouseDriver {
+  /** Gets a reference to the `clickhouse_driver` module. */
+  API::Node clickhouse_driver() { result = API::moduleImport("clickhouse_driver") }
+
+  /** Gets a reference to the `aioch` module. This module allows to make async db queries. */
+  API::Node aioch() { result = API::moduleImport("aioch") }
+
+  /**
+   * `clickhouse_driver` implements PEP249,
+   * providing ways to execute SQL statements against a database.
+   */
+  class ClickHouseDriverPEP249 extends PEP249ModuleApiNode {
+    ClickHouseDriverPEP249() { this = clickhouse_driver() }
+  }
+
+  module Client {
+    /** Gets a reference to a Client call. */
+    private DataFlow::Node client_ref() {
+      result = clickhouse_driver().getMember("Client").getASubclass*().getAUse()
+      or
+      result = aioch().getMember("Client").getASubclass*().getAUse()
+    }
+
+    /** A direct instantiation of `clickhouse_driver.Client`. */
+    private class ClientInstantiation extends DataFlow::CallCfgNode {
+      ClientInstantiation() { this.getFunction() = client_ref() }
+    }
+
+    /** Gets a reference to an instance of `clickhouse_driver.Client`. */
+    private DataFlow::LocalSourceNode instance(DataFlow::TypeTracker t) {
+      t.start() and
+      result instanceof ClientInstantiation
+      or
+      exists(DataFlow::TypeTracker t2 | result = instance(t2).track(t2, t))
+    }
+
+    /** Gets a reference to an instance of `clickhouse_driver.Client`. */
+    DataFlow::Node instance() { instance(DataFlow::TypeTracker::end()).flowsTo(result) }
+  }
+
+  /** clickhouse_driver.Client execute methods */
+  private string execute_function() {
+    result in ["execute_with_progress", "execute", "execute_iter"]
+  }
+
+  /** Gets a reference to the `clickhouse_driver.Client.execute` method */
+  private DataFlow::LocalSourceNode clickhouse_execute(DataFlow::TypeTracker t) {
+    t.startInAttr(execute_function()) and
+    result = Client::instance()
+    or
+    exists(DataFlow::TypeTracker t2 | result = clickhouse_execute(t2).track(t2, t))
+  }
+
+  /** Gets a reference to the `clickhouse_driver.Client.execute` method */
+  DataFlow::Node clickhouse_execute() {
+    clickhouse_execute(DataFlow::TypeTracker::end()).flowsTo(result)
+  }
+
+  /** A call to the `clickhouse_driver.Client.execute` method */
+  private class ExecuteCall extends SqlExecution::Range, DataFlow::CallCfgNode {
+    ExecuteCall() { this.getFunction() = clickhouse_execute() }
+
+    override DataFlow::Node getSql() { result.asCfgNode() = node.getArg(0) }
+  }
+}