hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-04-30 19:26:02 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #5950 from RasmusWL/promote-clickhouse

Browse Source 
Python: Promote ClickHouse SQL models
This commit is contained in:
yoff
2021-06-16 13:38:41 +02:00
committed by GitHub
parent eaa69dfa5d 00af18a622
commit 0ddeb7a8c1
19 changed files with 204 additions and 241 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
python/change-notes/2021-05-25-add-ClickHouse-sql-libs.md Normal file
View File

@@ -0,0 +1,2 @@
lgtm,codescanning
* Added model of SQL execution in `clickhouse-driver` and `aioch` PyPI packages, resulting in additional sinks for the SQL Injection query (`py/sql-injection`). This modeling was originally [submitted as a contribution by @japroc](https://github.com/github/codeql/pull/5889).

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

@@ -1,28 +0,0 @@
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)]

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

@@ -1,59 +0,0 @@
<!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>

22
python/ql/src/experimental/Security/CWE-089/ClickHouseSQLInjection.ql
View File

@@ -1,22 +0,0 @@
/**
* @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"

85
python/ql/src/experimental/semmle/python/frameworks/ClickHouseDriver.qll
View File

@@ -1,85 +0,0 @@
/**
* 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) }
}
}

2
python/ql/src/semmle/python/Frameworks.qll
View File

@@ -4,7 +4,9 @@
// If you add modeling of a new framework/library, remember to add it it to the docs in
// `docs/codeql/support/reusables/frameworks.rst`
private import semmle.python.frameworks.Aioch
private import semmle.python.frameworks.Aiohttp
private import semmle.python.frameworks.ClickhouseDriver
private import semmle.python.frameworks.Cryptodome
private import semmle.python.frameworks.Cryptography
private import semmle.python.frameworks.Dill

52
python/ql/src/semmle/python/frameworks/Aioch.qll Normal file
View File

@@ -0,0 +1,52 @@
/**
* Provides classes modeling security-relevant aspects of the `aioch` PyPI package (an
* async-io version of the `clickhouse-driver` PyPI package).
*
* See https://pypi.org/project/aioch/
*/
private import python
private import semmle.python.Concepts
private import semmle.python.ApiGraphs
private import semmle.python.frameworks.PEP249
private import semmle.python.frameworks.ClickhouseDriver
/**
* INTERNAL: Do not use.
*
* Provides models for `aioch` PyPI package (an async-io version of the
* `clickhouse-driver` PyPI package).
*
* See https://pypi.org/project/aioch/
*/
module Aioch {
/** Provides models for `aioch.Client` class and subclasses. */
module Client {
/** Gets a reference to the `aioch.Client` class or any subclass. */
API::Node subclassRef() {
result = API::moduleImport("aioch").getMember("Client").getASubclass*()
}
/** Gets a reference to an instance of `clickhouse_driver.Client` or any subclass. */
API::Node instance() { result = subclassRef().getReturn() }
}
/**
* A call to any of the the execute methods on a `aioch.Client`, which are just async
* versions of the methods in the `clickhouse-driver` PyPI package.
*
* See
* - https://clickhouse-driver.readthedocs.io/en/latest/api.html#clickhouse_driver.Client.execute
* - https://clickhouse-driver.readthedocs.io/en/latest/api.html#clickhouse_driver.Client.execute_iter
* - https://clickhouse-driver.readthedocs.io/en/latest/api.html#clickhouse_driver.Client.execute_with_progress
*/
class ClientExecuteCall extends SqlExecution::Range, DataFlow::CallCfgNode {
ClientExecuteCall() {
exists(string methodName | methodName = ClickhouseDriver::getExecuteMethodName() |
this = Client::instance().getMember(methodName).getACall()
)
}
override DataFlow::Node getSql() { result in [this.getArg(0), this.getArgByName("query")] }
}
}

65
python/ql/src/semmle/python/frameworks/ClickhouseDriver.qll Normal file
View File

@@ -0,0 +1,65 @@
/**
* Provides classes modeling security-relevant aspects of the `clickhouse-driver` PyPI package.
* See
* - https://pypi.org/project/clickhouse-driver/
* - 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
/**
* INTERNAL: Do not use.
*
* Provides models for `clickhouse-driver` PyPI package (imported as `clickhouse_driver`).
* See
* - https://pypi.org/project/clickhouse-driver/
* - https://clickhouse-driver.readthedocs.io/en/latest/
*/
module ClickhouseDriver {
/**
* `clickhouse_driver` implements PEP249,
* providing ways to execute SQL statements against a database.
*/
class ClickHouseDriverPEP249 extends PEP249ModuleApiNode {
ClickHouseDriverPEP249() { this = API::moduleImport("clickhouse_driver") }
}
/** Provides models for `clickhouse_driver.Client` class and subclasses. */
module Client {
/** Gets a reference to the `clickhouse_driver.Client` class or any subclass. */
API::Node subclassRef() {
exists(API::Node classRef |
// canonical definition
classRef = API::moduleImport("clickhouse_driver").getMember("client").getMember("Client")
or
// commonly used alias
classRef = API::moduleImport("clickhouse_driver").getMember("Client")
|
result = classRef.getASubclass*()
)
}
/** Gets a reference to an instance of `clickhouse_driver.Client` or any subclass. */
API::Node instance() { result = subclassRef().getReturn() }
}
/** `clickhouse_driver.Client` execute method names */
string getExecuteMethodName() { result in ["execute_with_progress", "execute", "execute_iter"] }
/**
* A call to any of the the execute methods on a `clickhouse_driver.Client` method
*
* See
* - https://clickhouse-driver.readthedocs.io/en/latest/api.html#clickhouse_driver.Client.execute
* - https://clickhouse-driver.readthedocs.io/en/latest/api.html#clickhouse_driver.Client.execute_iter
* - https://clickhouse-driver.readthedocs.io/en/latest/api.html#clickhouse_driver.Client.execute_with_progress
*/
class ClientExecuteCall extends SqlExecution::Range, DataFlow::CallCfgNode {
ClientExecuteCall() { this = Client::instance().getMember(getExecuteMethodName()).getACall() }
override DataFlow::Node getSql() { result in [this.getArg(0), this.getArgByName("query")] }
}
}

5
python/ql/test/experimental/semmle/python/frameworks/clickhouse-driver/ClickHouseDriver.expected
View File

@@ -1,5 +0,0 @@
| ClickHouseDriver.py:15:22:15:106 | ControlFlowNode for Attribute() | ClickHouseDriver.py:15:52:15:105 | ControlFlowNode for BinaryExpr |
| ClickHouseDriver.py:18:5:18:87 | ControlFlowNode for Attribute() | ClickHouseDriver.py:18:33:18:86 | ControlFlowNode for BinaryExpr |
| ClickHouseDriver.py:22:5:22:62 | ControlFlowNode for Attribute() | ClickHouseDriver.py:22:33:22:37 | ControlFlowNode for query |
| ClickHouseDriver.py:27:5:27:74 | ControlFlowNode for Attribute() | ClickHouseDriver.py:27:20:27:73 | ControlFlowNode for BinaryExpr |
| ClickHouseDriver.py:30:5:30:89 | ControlFlowNode for Attribute() | ClickHouseDriver.py:30:35:30:88 | ControlFlowNode for BinaryExpr |

32
python/ql/test/experimental/semmle/python/frameworks/clickhouse-driver/ClickHouseDriver.py
View File

@@ -1,32 +0,0 @@
from django.conf.urls import url
from clickhouse_driver import Client
from clickhouse_driver import connect
from aioch import Client as aiochClient
# Dummy Client subclass
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 -- Untrusted user input is directly injected into the sql query using PEP249 interface
conn = connect('clickhouse://localhost')
cursor = conn.cursor()
cursor.execute("SELECT * FROM users WHERE username = '%s'" % username)
# BAD -- Untrusted user input is directly injected into the sql query using MyClient, which is a subclass of Client
MyClient('localhost').execute("SELECT * FROM users WHERE username = '%s'" % username)
urlpatterns = [url(r'^users/(?P<username>[^/]+)$', show_user)]

6
python/ql/test/experimental/semmle/python/frameworks/clickhouse-driver/ClickHouseDriver.ql
View File

@@ -1,6 +0,0 @@
import python
import experimental.semmle.python.frameworks.ClickHouseDriver
import semmle.python.Concepts
from SqlExecution s
select s, s.getSql()

0
python/ql/test/library-tests/frameworks/aioch/ConceptsTest.expected Normal file
View File

2
python/ql/test/library-tests/frameworks/aioch/ConceptsTest.ql Normal file
View File

@@ -0,0 +1,2 @@
import python
import experimental.meta.ConceptsTest

1
python/ql/test/library-tests/frameworks/aioch/options Normal file
View File

@@ -0,0 +1 @@
semmle-extractor-options: --max-import-depth=1 --lang=3

30
python/ql/test/library-tests/frameworks/aioch/sql_test.py Normal file
View File

@@ -0,0 +1,30 @@
import aioch
SQL = "SOME SQL"
async def aioch_test():
client = aioch.Client("localhost")
await client.execute(SQL) # $ getSql=SQL
await client.execute(query=SQL) # $ getSql=SQL
await client.execute_with_progress(SQL) # $ getSql=SQL
await client.execute_with_progress(query=SQL) # $ getSql=SQL
await client.execute_iter(SQL) # $ getSql=SQL
await client.execute_iter(query=SQL) # $ getSql=SQL
# Using custom client (this has been seen done for the blocking version in
# `clickhouse_driver` PyPI package)
class MyClient(aioch.Client):
pass
async def test_custom_client():
client = MyClient("localhost")
await client.execute(SQL) # $ getSql=SQL

0
python/ql/test/library-tests/frameworks/clickhouse_driver/ConceptsTest.expected Normal file
View File

2
python/ql/test/library-tests/frameworks/clickhouse_driver/ConceptsTest.ql Normal file
View File

@@ -0,0 +1,2 @@
import python
import experimental.meta.ConceptsTest

42
python/ql/test/library-tests/frameworks/clickhouse_driver/sql_test.py Normal file
View File

@@ -0,0 +1,42 @@
import clickhouse_driver
SQL = "SOME SQL"
# Normal operation
client = clickhouse_driver.client.Client("localhost")
client.execute(SQL) # $ getSql=SQL
client.execute(query=SQL) # $ getSql=SQL
client.execute_with_progress(SQL) # $ getSql=SQL
client.execute_with_progress(query=SQL) # $ getSql=SQL
client.execute_iter(SQL) # $ getSql=SQL
client.execute_iter(query=SQL) # $ getSql=SQL
# commonly used alias
client = clickhouse_driver.Client("localhost")
client.execute(SQL) # $ getSql=SQL
# Using PEP249 interface
conn = clickhouse_driver.connect('clickhouse://localhost')
cursor = conn.cursor()
cursor.execute(SQL) # $ getSql=SQL
# Using custom client
#
# examples from real world code
# https://github.com/Altinity/clickhouse-mysql-data-reader/blob/3b1b7088751b05e5bbf45890c5949b58208c2343/clickhouse_mysql/dbclient/chclient.py#L10
# https://github.com/Felixoid/clickhouse-plantuml/blob/d8b2ba7d164a836770ec21f5e4035dfb04c41d9c/clickhouse_plantuml/client.py#L9
class MyClient(clickhouse_driver.Client):
pass
MyClient("localhost").execute(SQL) # $ getSql=SQL