mirror of
https://github.com/github/codeql.git
synced 2025-12-18 01:33:15 +01:00
Merge pull request #6589 from RasmusWL/promote-sqlalchemy
Python: Promote modeling of SQLAlchemy
This commit is contained in:
yoff
@@ -0,0 +1,53 @@
|
||||
<!DOCTYPE qhelp PUBLIC
|
||||
"-//Semmle//qhelp//EN"
|
||||
"qhelp.dtd">
|
||||
<qhelp>
|
||||
|
||||
<overview>
|
||||
<p>
|
||||
The <code>TextClause</code> class in the <code>SQLAlchemy</code> PyPI package represents
|
||||
a textual SQL string directly. If user-input is added to it without sufficient
|
||||
sanitization, a user may be able to run malicious database queries, since the
|
||||
<code>TextClause</code> is inserted directly into the final SQL.
|
||||
</p>
|
||||
</overview>
|
||||
|
||||
<recommendation>
|
||||
<p>
|
||||
Don't allow user-input to be added to a <code>TextClause</code>, instead construct your
|
||||
full query with constructs from the ORM, or use query parameters for user-input.
|
||||
</p>
|
||||
</recommendation>
|
||||
|
||||
<example>
|
||||
<p>
|
||||
In the following snippet, a user is fetched from the database using three
|
||||
different queries.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
In the first case, the final query string is built by directly including a user-supplied
|
||||
input. The parameter may include quote characters, so this code is vulnerable to a SQL
|
||||
injection attack.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
In the second case, the query is built using ORM models, but part of it is using a
|
||||
<code>TextClause</code> directly including a user-supplied input. Since the
|
||||
<code>TextClause</code> is inserted directly into the final SQL, this code is vulnerable
|
||||
to a SQL injection attack.
|
||||
</p>
|
||||
|
||||
<p>
|
||||
In the third case, the query is built fully using the ORM models, so in the end, the
|
||||
user-supplied input will be passed to the database using query parameters. The
|
||||
database connector library will take care of escaping and inserting quotes as needed.
|
||||
</p>
|
||||
|
||||
<sample src="examples/sqlalchemy_textclause_injection.py" />
|
||||
</example>
|
||||
|
||||
<references>
|
||||
<li><a href="https://docs.sqlalchemy.org/en/14/core/sqlelement.html#sqlalchemy.sql.expression.text.params.text">Official documentation of the text parameter</a>.</li>
|
||||
</references>
|
||||
</qhelp>
|
||||
@@ -0,0 +1,23 @@
|
||||
/**
|
||||
* @name SQLAlchemy TextClause built from user-controlled sources
|
||||
* @description Building a TextClause query from user-controlled sources is vulnerable to insertion of
|
||||
* malicious SQL code by the user.
|
||||
* @kind path-problem
|
||||
* @problem.severity error
|
||||
* @security-severity 8.8
|
||||
* @precision high
|
||||
* @id py/sqlalchemy-textclause-injection
|
||||
* @tags security
|
||||
* external/cwe/cwe-089
|
||||
* external/owasp/owasp-a1
|
||||
*/
|
||||
|
||||
import python
|
||||
import semmle.python.security.dataflow.SQLAlchemyTextClause
|
||||
import DataFlow::PathGraph
|
||||
|
||||
from SQLAlchemyTextClause::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
|
||||
where config.hasFlowPath(source, sink)
|
||||
select sink.getNode(), source, sink,
|
||||
"This SQLAlchemy TextClause depends on $@, which could lead to SQL injection.", source.getNode(),
|
||||
"a user-provided value"
|
||||
@@ -0,0 +1,34 @@
|
||||
from flask import Flask, request
|
||||
import sqlalchemy
|
||||
import sqlalchemy.orm
|
||||
|
||||
app = Flask(__name__)
|
||||
engine = sqlalchemy.create_engine(...)
|
||||
Base = sqlalchemy.orm.declarative_base()
|
||||
|
||||
|
||||
class User(Base):
|
||||
__tablename__ = "users"
|
||||
|
||||
id = sqlalchemy.Column(sqlalchemy.Integer, primary_key=True)
|
||||
username = sqlalchemy.Column(sqlalchemy.String)
|
||||
|
||||
|
||||
@app.route("/users/<username>")
|
||||
def show_user(username):
|
||||
session = sqlalchemy.orm.Session(engine)
|
||||
|
||||
# BAD, normal SQL injection
|
||||
stmt = sqlalchemy.text("SELECT * FROM users WHERE username = '{}'".format(username))
|
||||
results = session.execute(stmt).fetchall()
|
||||
|
||||
# BAD, allows SQL injection
|
||||
username_formatted_for_sql = sqlalchemy.text("'{}'".format(username))
|
||||
stmt = sqlalchemy.select(User).where(User.username == username_formatted_for_sql)
|
||||
results = session.execute(stmt).scalars().all()
|
||||
|
||||
# GOOD, does not allow for SQL injection
|
||||
stmt = sqlalchemy.select(User).where(User.username == username)
|
||||
results = session.execute(stmt).scalars().all()
|
||||
|
||||
...
|
||||
@@ -1,148 +0,0 @@
|
||||
/**
|
||||
* Provides classes modeling security-relevant aspects of the 'SqlAlchemy' package.
|
||||
* See https://pypi.org/project/SQLAlchemy/.
|
||||
*/
|
||||
|
||||
private import python
|
||||
private import semmle.python.dataflow.new.DataFlow
|
||||
private import semmle.python.dataflow.new.TaintTracking
|
||||
private import semmle.python.ApiGraphs
|
||||
private import semmle.python.Concepts
|
||||
private import experimental.semmle.python.Concepts
|
||||
|
||||
private module SqlAlchemy {
|
||||
/**
|
||||
* Returns an instantization of a SqlAlchemy Session object.
|
||||
* See https://docs.sqlalchemy.org/en/14/orm/session_api.html#sqlalchemy.orm.Session and
|
||||
* https://docs.sqlalchemy.org/en/14/orm/session_api.html#sqlalchemy.orm.sessionmaker
|
||||
*/
|
||||
private API::Node getSqlAlchemySessionInstance() {
|
||||
result = API::moduleImport("sqlalchemy.orm").getMember("Session").getReturn() or
|
||||
result = API::moduleImport("sqlalchemy.orm").getMember("sessionmaker").getReturn().getReturn()
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns an instantization of a SqlAlchemy Engine object.
|
||||
* See https://docs.sqlalchemy.org/en/14/core/engines.html#sqlalchemy.create_engine
|
||||
*/
|
||||
private API::Node getSqlAlchemyEngineInstance() {
|
||||
result = API::moduleImport("sqlalchemy").getMember("create_engine").getReturn()
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns an instantization of a SqlAlchemy Query object.
|
||||
* See https://docs.sqlalchemy.org/en/14/orm/query.html?highlight=query#sqlalchemy.orm.Query
|
||||
*/
|
||||
private API::Node getSqlAlchemyQueryInstance() {
|
||||
result = getSqlAlchemySessionInstance().getMember("query").getReturn()
|
||||
}
|
||||
|
||||
/**
|
||||
* A call to `execute` meant to execute an SQL expression
|
||||
* See the following links:
|
||||
* - https://docs.sqlalchemy.org/en/14/core/connections.html?highlight=execute#sqlalchemy.engine.Connection.execute
|
||||
* - https://docs.sqlalchemy.org/en/14/core/connections.html?highlight=execute#sqlalchemy.engine.Engine.execute
|
||||
* - https://docs.sqlalchemy.org/en/14/orm/session_api.html?highlight=execute#sqlalchemy.orm.Session.execute
|
||||
*/
|
||||
private class SqlAlchemyExecuteCall extends DataFlow::CallCfgNode, SqlExecution::Range {
|
||||
SqlAlchemyExecuteCall() {
|
||||
// new way
|
||||
this = getSqlAlchemySessionInstance().getMember("execute").getACall() or
|
||||
this =
|
||||
getSqlAlchemyEngineInstance()
|
||||
.getMember(["connect", "begin"])
|
||||
.getReturn()
|
||||
.getMember("execute")
|
||||
.getACall()
|
||||
}
|
||||
|
||||
override DataFlow::Node getSql() { result = this.getArg(0) }
|
||||
}
|
||||
|
||||
/**
|
||||
* A call to `scalar` meant to execute an SQL expression
|
||||
* See https://docs.sqlalchemy.org/en/14/orm/session_api.html#sqlalchemy.orm.Session.scalar and
|
||||
* https://docs.sqlalchemy.org/en/14/core/connections.html?highlight=scalar#sqlalchemy.engine.Engine.scalar
|
||||
*/
|
||||
private class SqlAlchemyScalarCall extends DataFlow::CallCfgNode, SqlExecution::Range {
|
||||
SqlAlchemyScalarCall() {
|
||||
this =
|
||||
[getSqlAlchemySessionInstance(), getSqlAlchemyEngineInstance()]
|
||||
.getMember("scalar")
|
||||
.getACall()
|
||||
}
|
||||
|
||||
override DataFlow::Node getSql() { result = this.getArg(0) }
|
||||
}
|
||||
|
||||
/**
|
||||
* A call on a Query object
|
||||
* See https://docs.sqlalchemy.org/en/14/orm/query.html?highlight=query#sqlalchemy.orm.Query
|
||||
*/
|
||||
private class SqlAlchemyQueryCall extends DataFlow::CallCfgNode, SqlExecution::Range {
|
||||
SqlAlchemyQueryCall() {
|
||||
this =
|
||||
getSqlAlchemyQueryInstance()
|
||||
.getMember(any(SqlAlchemyVulnerableMethodNames methodName))
|
||||
.getACall()
|
||||
}
|
||||
|
||||
override DataFlow::Node getSql() { result = this.getArg(0) }
|
||||
}
|
||||
|
||||
/**
|
||||
* This class represents a list of methods vulnerable to sql injection.
|
||||
*
|
||||
* See https://github.com/jty-team/codeql/pull/2#issue-611592361
|
||||
*/
|
||||
private class SqlAlchemyVulnerableMethodNames extends string {
|
||||
SqlAlchemyVulnerableMethodNames() { this in ["filter", "filter_by", "group_by", "order_by"] }
|
||||
}
|
||||
|
||||
/**
|
||||
* Additional taint-steps for `sqlalchemy.text()`
|
||||
*
|
||||
* See https://docs.sqlalchemy.org/en/14/core/sqlelement.html#sqlalchemy.sql.expression.text
|
||||
* See https://docs.sqlalchemy.org/en/14/core/sqlelement.html#sqlalchemy.sql.expression.TextClause
|
||||
*/
|
||||
class SqlAlchemyTextAdditionalTaintSteps extends TaintTracking::AdditionalTaintStep {
|
||||
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
|
||||
exists(DataFlow::CallCfgNode call |
|
||||
(
|
||||
call = API::moduleImport("sqlalchemy").getMember("text").getACall()
|
||||
or
|
||||
call = API::moduleImport("sqlalchemy").getMember("sql").getMember("text").getACall()
|
||||
or
|
||||
call =
|
||||
API::moduleImport("sqlalchemy")
|
||||
.getMember("sql")
|
||||
.getMember("expression")
|
||||
.getMember("text")
|
||||
.getACall()
|
||||
or
|
||||
call =
|
||||
API::moduleImport("sqlalchemy")
|
||||
.getMember("sql")
|
||||
.getMember("expression")
|
||||
.getMember("TextClause")
|
||||
.getACall()
|
||||
) and
|
||||
nodeFrom in [call.getArg(0), call.getArgByName("text")] and
|
||||
nodeTo = call
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Gets a reference to `sqlescapy.sqlescape`.
|
||||
*
|
||||
* See https://pypi.org/project/sqlescapy/
|
||||
*/
|
||||
class SQLEscapySanitizerCall extends DataFlow::CallCfgNode, SQLEscape::Range {
|
||||
SQLEscapySanitizerCall() {
|
||||
this = API::moduleImport("sqlescapy").getMember("sqlescape").getACall()
|
||||
}
|
||||
|
||||
override DataFlow::Node getAnInput() { result = this.getArg(0) }
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user