diff --git a/python/ql/lib/change-notes/2022-05-02-deprecate-sqlconstruction.md b/python/ql/lib/change-notes/2022-05-02-deprecate-sqlconstruction.md deleted file mode 100644 index 4219fd12d9c..00000000000 --- a/python/ql/lib/change-notes/2022-05-02-deprecate-sqlconstruction.md +++ /dev/null @@ -1,4 +0,0 @@ ---- -category: deprecated ---- -The `SqlConstruction` class and module from `Concepts.qll` has been deprecated. Use `SqlExecution` from the same file instead. \ No newline at end of file diff --git a/python/ql/lib/semmle/python/Concepts.qll b/python/ql/lib/semmle/python/Concepts.qll index 6154d24628b..e734105cd2e 100644 --- a/python/ql/lib/semmle/python/Concepts.qll +++ b/python/ql/lib/semmle/python/Concepts.qll @@ -308,19 +308,36 @@ module CodeExecution { } } -/** DEPRECATED: Use `SqlExecution` instead. */ -deprecated class SqlConstruction extends DataFlow::Node instanceof SqlConstruction::Range { +/** + * A data-flow node that constructs an SQL statement. + * + * Often, it is worthy of an alert if an SQL statement is constructed such that + * executing it would be a security risk. + * + * If it is important that the SQL statement is indeed executed, then use `SQLExecution`. + * + * Extend this class to refine existing API models. If you want to model new APIs, + * extend `SqlConstruction::Range` instead. + */ +class SqlConstruction extends DataFlow::Node instanceof SqlConstruction::Range { /** Gets the argument that specifies the SQL statements to be constructed. */ DataFlow::Node getSql() { result = super.getSql() } } -/** - * DEPRECATED: Use `SqlExecution` instead. - * Provides a class for modeling new SQL execution APIs. - */ -deprecated module SqlConstruction { - /** DEPRECATED: Use `SqlExecution::Range` instead. */ - abstract deprecated class Range extends DataFlow::Node { +/** Provides a class for modeling new SQL execution APIs. */ +module SqlConstruction { + /** + * A data-flow node that constructs an SQL statement. + * + * Often, it is worthy of an alert if an SQL statement is constructed such that + * executing it would be a security risk. + * + * If it is important that the SQL statement is indeed executed, then use `SQLExecution`. + * + * Extend this class to model new APIs. If you want to refine existing API models, + * extend `SqlConstruction` instead. + */ + abstract class Range extends DataFlow::Node { /** Gets the argument that specifies the SQL statements to be constructed. */ abstract DataFlow::Node getSql(); } @@ -329,6 +346,9 @@ deprecated module SqlConstruction { /** * A data-flow node that executes SQL statements. * + * If the context of interest is such that merely constructing an SQL statement + * would be valuabe to report, then consider using `SqlConstruction`. + * * Extend this class to refine existing API models. If you want to model new APIs, * extend `SqlExecution::Range` instead. */ @@ -342,6 +362,9 @@ module SqlExecution { /** * A data-flow node that executes SQL statements. * + * If the context of interest is such that merely constructing an SQL statement + * would be valuabe to report, then consider using `SqlConstruction`. + * * Extend this class to model new APIs. If you want to refine existing API models, * extend `SqlExecution` instead. */ diff --git a/python/ql/lib/semmle/python/frameworks/Aiomysql.qll b/python/ql/lib/semmle/python/frameworks/Aiomysql.qll index afff79783c2..112dc58d061 100644 --- a/python/ql/lib/semmle/python/frameworks/Aiomysql.qll +++ b/python/ql/lib/semmle/python/frameworks/Aiomysql.qll @@ -50,7 +50,7 @@ private module Aiomysql { * A query. Calling `execute` on a `Cursor` constructs a query. * See https://aiomysql.readthedocs.io/en/stable/cursors.html#Cursor.execute */ - class CursorExecuteCall extends SqlExecution::Range, API::CallNode { + class CursorExecuteCall extends SqlConstruction::Range, API::CallNode { CursorExecuteCall() { this = cursor().getMember("execute").getACall() } override DataFlow::Node getSql() { result = this.getParameter(0, "operation").getARhs() } @@ -91,7 +91,7 @@ private module Aiomysql { * A query. Calling `execute` on a `SAConnection` constructs a query. * See https://aiomysql.readthedocs.io/en/stable/sa.html#aiomysql.sa.SAConnection.execute */ - class SAConnectionExecuteCall extends SqlExecution::Range, API::CallNode { + class SAConnectionExecuteCall extends SqlConstruction::Range, API::CallNode { SAConnectionExecuteCall() { this = saConnection().getMember("execute").getACall() } override DataFlow::Node getSql() { result = this.getParameter(0, "query").getARhs() } diff --git a/python/ql/lib/semmle/python/frameworks/Aiopg.qll b/python/ql/lib/semmle/python/frameworks/Aiopg.qll index 67ba3c80db0..1a60c433150 100644 --- a/python/ql/lib/semmle/python/frameworks/Aiopg.qll +++ b/python/ql/lib/semmle/python/frameworks/Aiopg.qll @@ -50,7 +50,7 @@ private module Aiopg { * A query. Calling `execute` on a `Cursor` constructs a query. * See https://aiopg.readthedocs.io/en/stable/core.html#aiopg.Cursor.execute */ - class CursorExecuteCall extends SqlExecution::Range, API::CallNode { + class CursorExecuteCall extends SqlConstruction::Range, API::CallNode { CursorExecuteCall() { this = cursor().getMember("execute").getACall() } override DataFlow::Node getSql() { result = this.getParameter(0, "operation").getARhs() } @@ -87,7 +87,7 @@ private module Aiopg { * A query. Calling `execute` on a `SAConnection` constructs a query. * See https://aiopg.readthedocs.io/en/stable/sa.html#aiopg.sa.SAConnection.execute */ - class SAConnectionExecuteCall extends SqlExecution::Range, API::CallNode { + class SAConnectionExecuteCall extends SqlConstruction::Range, API::CallNode { SAConnectionExecuteCall() { this = saConnection().getMember("execute").getACall() } override DataFlow::Node getSql() { result = this.getParameter(0, "query").getARhs() } diff --git a/python/ql/lib/semmle/python/frameworks/Asyncpg.qll b/python/ql/lib/semmle/python/frameworks/Asyncpg.qll index 77678c92b51..2b2b9ed2e1b 100644 --- a/python/ql/lib/semmle/python/frameworks/Asyncpg.qll +++ b/python/ql/lib/semmle/python/frameworks/Asyncpg.qll @@ -56,7 +56,7 @@ private module Asyncpg { * The creation of the `Cursor` executes the query. */ module Cursor { - class CursorConstruction extends SqlExecution::Range, API::CallNode { + class CursorConstruction extends SqlConstruction::Range, API::CallNode { CursorConstruction() { this = ModelOutput::getATypeNode("asyncpg", "Connection").getMember("cursor").getACall() } diff --git a/python/ql/lib/semmle/python/frameworks/SqlAlchemy.qll b/python/ql/lib/semmle/python/frameworks/SqlAlchemy.qll index 8a7e2f176e8..c1712e7e7eb 100644 --- a/python/ql/lib/semmle/python/frameworks/SqlAlchemy.qll +++ b/python/ql/lib/semmle/python/frameworks/SqlAlchemy.qll @@ -323,7 +323,7 @@ module SqlAlchemy { * A construction of a `sqlalchemy.sql.expression.TextClause`, which represents a * textual SQL string directly. */ - abstract class TextClauseConstruction extends SqlExecution::Range, DataFlow::CallCfgNode { + abstract class TextClauseConstruction extends SqlConstruction::Range, DataFlow::CallCfgNode { /** Gets the argument that specifies the SQL text. */ override DataFlow::Node getSql() { result in [this.getArg(0), this.getArgByName("text")] } } diff --git a/python/ql/lib/semmle/python/security/dataflow/SqlInjectionCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/SqlInjectionCustomizations.qll index 9e3e2749a2b..756a1f6b773 100644 --- a/python/ql/lib/semmle/python/security/dataflow/SqlInjectionCustomizations.qll +++ b/python/ql/lib/semmle/python/security/dataflow/SqlInjectionCustomizations.qll @@ -43,10 +43,9 @@ module SqlInjection { class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { } /** - * DEPRECATED: Use `SqlExecutionAsSink` instead. * A SQL statement of a SQL construction, considered as a flow sink. */ - deprecated class SqlConstructionAsSink extends Sink { + class SqlConstructionAsSink extends Sink { SqlConstructionAsSink() { this = any(SqlConstruction c).getSql() } } diff --git a/python/ql/test/experimental/meta/ConceptsTest.qll b/python/ql/test/experimental/meta/ConceptsTest.qll index bdf71059680..8f9435f633f 100644 --- a/python/ql/test/experimental/meta/ConceptsTest.qll +++ b/python/ql/test/experimental/meta/ConceptsTest.qll @@ -128,6 +128,24 @@ class CodeExecutionTest extends InlineExpectationsTest { } } +class SqlConstructionTest extends InlineExpectationsTest { + SqlConstructionTest() { this = "SqlConstructionTest" } + + override string getARelevantTag() { result = "constructedSql" } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + exists(location.getFile().getRelativePath()) and + exists(SqlConstruction e, DataFlow::Node sql | + exists(location.getFile().getRelativePath()) and + sql = e.getSql() and + location = e.getLocation() and + element = sql.toString() and + value = prettyNodeForInlineTest(sql) and + tag = "constructedSql" + ) + } +} + class SqlExecutionTest extends InlineExpectationsTest { SqlExecutionTest() { this = "SqlExecutionTest" } diff --git a/python/ql/test/library-tests/frameworks/aiomysql/test.py b/python/ql/test/library-tests/frameworks/aiomysql/test.py index 5a06b46b9f1..782fa6cb7bf 100644 --- a/python/ql/test/library-tests/frameworks/aiomysql/test.py +++ b/python/ql/test/library-tests/frameworks/aiomysql/test.py @@ -5,24 +5,24 @@ async def test_cursor(): # Create connection directly conn = await aiomysql.connect() cur = await conn.cursor() - await cur.execute("sql") # $ getSql="sql" + await cur.execute("sql") # $ getSql="sql" constructedSql="sql" # Create connection via pool async with aiomysql.create_pool() as pool: # Create Cursor via Connection async with pool.acquire() as conn: async with conn.cursor() as cur: - await cur.execute("sql") # $ getSql="sql" + await cur.execute("sql") # $ getSql="sql" constructedSql="sql" # Create Cursor directly async with pool.cursor() as cur: - await cur.execute("sql") # $ getSql="sql" + await cur.execute("sql") # $ getSql="sql" constructedSql="sql" # variants using as few `async with` as possible pool = await aiomysql.create_pool() conn = await pool.acquire() cur = await conn.cursor() - await cur.execute("sql") # $ getSql="sql" + await cur.execute("sql") # $ getSql="sql" constructedSql="sql" # Test SQLAlchemy integration from aiomysql.sa import create_engine @@ -30,4 +30,4 @@ from aiomysql.sa import create_engine async def test_engine(): engine = await create_engine() conn = await engine.acquire() - await conn.execute("sql") # $ getSql="sql" + await conn.execute("sql") # $ getSql="sql" constructedSql="sql" diff --git a/python/ql/test/library-tests/frameworks/aiopg/test.py b/python/ql/test/library-tests/frameworks/aiopg/test.py index 1d085066688..63bf141f52d 100644 --- a/python/ql/test/library-tests/frameworks/aiopg/test.py +++ b/python/ql/test/library-tests/frameworks/aiopg/test.py @@ -5,24 +5,24 @@ async def test_cursor(): # Create connection directly conn = await aiopg.connect() cur = await conn.cursor() - await cur.execute("sql") # $ getSql="sql" + await cur.execute("sql") # $ getSql="sql" constructedSql="sql" # Create connection via pool async with aiopg.create_pool() as pool: # Create Cursor via Connection async with pool.acquire() as conn: async with conn.cursor() as cur: - await cur.execute("sql") # $ getSql="sql" + await cur.execute("sql") # $ getSql="sql" constructedSql="sql" # Create Cursor directly async with pool.cursor() as cur: - await cur.execute("sql") # $ getSql="sql" + await cur.execute("sql") # $ getSql="sql" constructedSql="sql" # variants using as few `async with` as possible pool = await aiopg.create_pool() conn = await pool.acquire() cur = await conn.cursor() - await cur.execute("sql") # $ getSql="sql" + await cur.execute("sql") # $ getSql="sql" constructedSql="sql" # Test SQLAlchemy integration from aiopg.sa import create_engine @@ -30,4 +30,4 @@ from aiopg.sa import create_engine async def test_engine(): engine = await create_engine() conn = await engine.acquire() - await conn.execute("sql") # $ getSql="sql" + await conn.execute("sql") # $ getSql="sql" constructedSql="sql" diff --git a/python/ql/test/library-tests/frameworks/asyncpg/test.py b/python/ql/test/library-tests/frameworks/asyncpg/test.py index 7e2687e5766..572fc454bed 100644 --- a/python/ql/test/library-tests/frameworks/asyncpg/test.py +++ b/python/ql/test/library-tests/frameworks/asyncpg/test.py @@ -43,20 +43,20 @@ async def test_cursor(): try: async with conn.transaction(): - cursor = await conn.cursor("sql") # $ getSql="sql" + cursor = await conn.cursor("sql") # $ getSql="sql" constructedSql="sql" await cursor.fetch() pstmt = await conn.prepare("psql") # $ getSql="psql" pcursor = await pstmt.cursor() # $ getSql="psql" await pcursor.fetch() - async for record in conn.cursor("sql"): # $ getSql="sql" + async for record in conn.cursor("sql"): # $ getSql="sql" constructedSql="sql" pass async for record in pstmt.cursor(): # $ getSql="psql" pass - cursor_factory = conn.cursor("sql") # $ getSql="sql" + cursor_factory = conn.cursor("sql") # $ constructedSql="sql" cursor = await cursor_factory # $ getSql="sql" pcursor_factory = pstmt.cursor() 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 19a1349ffb6..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"))) == "" # $ getSql="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") # $ getSql="foo" +t = db.text("foo") # $ constructedSql="foo" assert isinstance(t, sqlalchemy.sql.expression.TextClause) -t = db.text(text="foo") # $ getSql="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 7d9c478e27b..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") # $ getSql="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 9cb13d15e05..6b11ee5d070 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) # $ getSql=raw_sql +text_sql = sqlalchemy.text(raw_sql) # $ constructedSql=raw_sql Base = sqlalchemy.orm.declarative_base() @@ -176,7 +176,7 @@ assert session.query(For14).all()[0].id == 14 # and now we can do the actual querying -text_foo = sqlalchemy.text("'FOO'") # $ getSql="'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 @@ -305,7 +305,7 @@ with engine.connect() as conn: assert scalar_result == "FOO" # This is a contrived example - select = sqlalchemy.select(sqlalchemy.text("'BAR'")) # $ getSql="'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 9d346d41979..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) # $ getSql=ts - t2 = sqlalchemy.text(text=ts) # $ getSql=ts - t3 = sqlalchemy.sql.text(ts) # $ getSql=ts - t4 = sqlalchemy.sql.text(text=ts) # $ getSql=ts - t5 = sqlalchemy.sql.expression.text(ts) # $ getSql=ts - t6 = sqlalchemy.sql.expression.text(text=ts) # $ getSql=ts - t7 = sqlalchemy.sql.expression.TextClause(ts) # $ getSql=ts - t8 = sqlalchemy.sql.expression.TextClause(text=ts) # $ getSql=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