From 5a02b3880e39bd4f31c29308c5577f895b7dd356 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Mon, 25 Oct 2021 13:30:14 +0200 Subject: [PATCH] Python: use `SqlConstruction` in `SqlAlchemy` and `SqlInjection` --- .../lib/semmle/python/frameworks/SqlAlchemy.qll | 6 ++++-- .../dataflow/SqlInjectionCustomizations.qll | 14 +++++++------- .../frameworks/flask_sqlalchemy/SqlExecution.py | 6 +++--- .../frameworks/sqlalchemy/SqlExecution.py | 2 +- .../frameworks/sqlalchemy/new_tests.py | 6 +++--- .../frameworks/sqlalchemy/taint_test.py | 16 ++++++++-------- 6 files changed, 26 insertions(+), 24 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/SqlAlchemy.qll b/python/ql/lib/semmle/python/frameworks/SqlAlchemy.qll index a74d44573f7..6f8bca5aa7e 100644 --- a/python/ql/lib/semmle/python/frameworks/SqlAlchemy.qll +++ b/python/ql/lib/semmle/python/frameworks/SqlAlchemy.qll @@ -313,9 +313,11 @@ module SqlAlchemy { * A construction of a `sqlalchemy.sql.expression.TextClause`, which represents a * textual SQL string directly. */ - abstract class TextClauseConstruction extends DataFlow::CallCfgNode { + abstract class TextClauseConstruction extends SqlConstruction::Range, DataFlow::CallCfgNode { /** Gets the argument that specifies the SQL text. */ - DataFlow::Node getTextArg() { result in [this.getArg(0), this.getArgByName("text")] } + final override DataFlow::Node getSql() { + result in [this.getArg(0), this.getArgByName("text")] + } } /** `TextClause` constructions from the `sqlalchemy` package. */ diff --git a/python/ql/lib/semmle/python/security/dataflow/SqlInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/SqlInjectionCustomizations.qll index c132878951f..756a1f6b773 100644 --- a/python/ql/lib/semmle/python/security/dataflow/SqlInjectionCustomizations.qll +++ b/python/ql/lib/semmle/python/security/dataflow/SqlInjectionCustomizations.qll @@ -42,6 +42,13 @@ module SqlInjection { */ class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { } + /** + * A SQL statement of a SQL construction, considered as a flow sink. + */ + class SqlConstructionAsSink extends Sink { + SqlConstructionAsSink() { this = any(SqlConstruction c).getSql() } + } + /** * A SQL statement of a SQL execution, considered as a flow sink. */ @@ -49,13 +56,6 @@ module SqlInjection { SqlExecutionAsSink() { this = any(SqlExecution e).getSql() } } - /** - * The text argument of a SQLAlchemy TextClause construction, considered as a flow sink. - */ - class TextArgAsSink extends Sink { - TextArgAsSink() { this = any(SqlAlchemy::TextClause::TextClauseConstruction tcc).getTextArg() } - } - /** * A comparison with a constant string, considered as a sanitizer-guard. */ diff --git a/python/ql/test/library-tests/frameworks/flask_sqlalchemy/SqlExecution.py b/python/ql/test/library-tests/frameworks/flask_sqlalchemy/SqlExecution.py index 7560aff2d30..39cd49195d2 100644 --- a/python/ql/test/library-tests/frameworks/flask_sqlalchemy/SqlExecution.py +++ b/python/ql/test/library-tests/frameworks/flask_sqlalchemy/SqlExecution.py @@ -12,7 +12,7 @@ db = SQLAlchemy(app) # - https://github.com/pallets/flask-sqlalchemy/blob/931ec00d1e27f51508e05706eef41cc4419a0b32/src/flask_sqlalchemy/__init__.py#L765 # - https://github.com/pallets/flask-sqlalchemy/blob/931ec00d1e27f51508e05706eef41cc4419a0b32/src/flask_sqlalchemy/__init__.py#L99-L109 -assert str(type(db.text("Foo"))) == "" +assert str(type(db.text("Foo"))) == "" # $ constructedSql="Foo" # also has engine/session instantiated @@ -44,8 +44,8 @@ assert result.fetchall() == [("Foo",)] # text -t = db.text("foo") +t = db.text("foo") # $ constructedSql="foo" assert isinstance(t, sqlalchemy.sql.expression.TextClause) -t = db.text(text="foo") +t = db.text(text="foo") # $ constructedSql="foo" assert isinstance(t, sqlalchemy.sql.expression.TextClause) diff --git a/python/ql/test/library-tests/frameworks/sqlalchemy/SqlExecution.py b/python/ql/test/library-tests/frameworks/sqlalchemy/SqlExecution.py index a7c9af9da32..fe75fc17d5c 100644 --- a/python/ql/test/library-tests/frameworks/sqlalchemy/SqlExecution.py +++ b/python/ql/test/library-tests/frameworks/sqlalchemy/SqlExecution.py @@ -46,7 +46,7 @@ with engine.begin() as connection: connection.execute("some sql") # $ getSql="some sql" # Injection requiring the text() taint-step -t = text("some sql") +t = text("some sql") # $ constructedSql="some sql" session.query(User).filter(t) session.query(User).group_by(User.id).having(t) session.query(User).group_by(t).first() diff --git a/python/ql/test/library-tests/frameworks/sqlalchemy/new_tests.py b/python/ql/test/library-tests/frameworks/sqlalchemy/new_tests.py index 9726e08f6bc..6ccf7b3b47d 100644 --- a/python/ql/test/library-tests/frameworks/sqlalchemy/new_tests.py +++ b/python/ql/test/library-tests/frameworks/sqlalchemy/new_tests.py @@ -6,7 +6,7 @@ import sqlalchemy.orm # either v1.4 or v2.0, such that we cover both. raw_sql = "select 'FOO'" -text_sql = sqlalchemy.text(raw_sql) +text_sql = sqlalchemy.text(raw_sql) # $ constructedSql=raw_sql Base = sqlalchemy.orm.declarative_base() @@ -169,7 +169,7 @@ assert session.query(For14).all()[0].id == 14 # and now we can do the actual querying -text_foo = sqlalchemy.text("'FOO'") +text_foo = sqlalchemy.text("'FOO'") # $ constructedSql="'FOO'" # filter_by is only vulnerable to injection if sqlalchemy.text is used, which is evident # from the logs produced if this file is run @@ -298,7 +298,7 @@ with engine.connect() as conn: assert scalar_result == "FOO" # This is a contrived example - select = sqlalchemy.select(sqlalchemy.text("'BAR'")) + select = sqlalchemy.select(sqlalchemy.text("'BAR'")) # $ constructedSql="'BAR'" result = conn.execute(select) # $ getSql=select assert result.fetchall() == [("BAR",)] diff --git a/python/ql/test/library-tests/frameworks/sqlalchemy/taint_test.py b/python/ql/test/library-tests/frameworks/sqlalchemy/taint_test.py index 9e9764a411f..884d30d3672 100644 --- a/python/ql/test/library-tests/frameworks/sqlalchemy/taint_test.py +++ b/python/ql/test/library-tests/frameworks/sqlalchemy/taint_test.py @@ -8,14 +8,14 @@ def test_taint(): ensure_tainted(ts) # $ tainted - t1 = sqlalchemy.text(ts) - t2 = sqlalchemy.text(text=ts) - t3 = sqlalchemy.sql.text(ts) - t4 = sqlalchemy.sql.text(text=ts) - t5 = sqlalchemy.sql.expression.text(ts) - t6 = sqlalchemy.sql.expression.text(text=ts) - t7 = sqlalchemy.sql.expression.TextClause(ts) - t8 = sqlalchemy.sql.expression.TextClause(text=ts) + t1 = sqlalchemy.text(ts) # $ constructedSql=ts + t2 = sqlalchemy.text(text=ts) # $ constructedSql=ts + t3 = sqlalchemy.sql.text(ts) # $ constructedSql=ts + t4 = sqlalchemy.sql.text(text=ts) # $ constructedSql=ts + t5 = sqlalchemy.sql.expression.text(ts) # $ constructedSql=ts + t6 = sqlalchemy.sql.expression.text(text=ts) # $ constructedSql=ts + t7 = sqlalchemy.sql.expression.TextClause(ts) # $ constructedSql=ts + t8 = sqlalchemy.sql.expression.TextClause(text=ts) # $ constructedSql=ts # Since we flag user-input to a TextClause with its' own query, we don't want to # have a taint-step for it as that would lead to us also giving an alert for normal