From f1e71e21ed543e727f403dd7c904e31a4aba1f19 Mon Sep 17 00:00:00 2001 From: Thank You Date: Wed, 14 Apr 2021 22:00:25 -0400 Subject: [PATCH 01/11] Add SqlAlchemy module --- .../semmle/python/frameworks/SqlAlchemy.qll | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll diff --git a/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll b/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll new file mode 100644 index 000000000000..147868a0f901 --- /dev/null +++ b/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll @@ -0,0 +1,70 @@ +/** + * 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 module SqlAlchemy { + private class SqlAlchemySessionInstance extends API::Node { + SqlAlchemySessionInstance() { + this in [ + API::moduleImport("sqlalchemy.orm").getMember("Session").getReturn(), + API::moduleImport("sqlalchemy.orm").getMember("sessionmaker").getReturn().getReturn() + ] + } + + override string toString() { result = "Use of SqlAlchemy Session instance method" } + } + + private class SqlAlchemyEngineInstance extends API::Node { + SqlAlchemyEngineInstance() { + this = API::moduleImport("sqlalchemy").getMember("create_engine").getReturn() + } + + override string toString() { result = "Use of SqlAlchemy Engine instance method" } + } + + private class SqlAlchemyQueryInstance extends API::Node { + SqlAlchemyQueryInstance() { + this instanceof SqlAlchemySessionInstance and + this = this.getMember("query").getReturn() + } + + override string toString() { result = "Use of SqlAlchemy Query instance method" } + } + + private class SqlAlchemyExecuteCall extends DataFlow::CallCfgNode, SqlExecution::Range { + SqlAlchemyExecuteCall() { + exists(SqlAlchemySessionInstance sessionInstance, SqlAlchemyEngineInstance engineInstance | + this = sessionInstance.getMember("execute").getACall() or + this = engineInstance.getMember("connect").getReturn().getMember("execute").getACall() or + this = engineInstance.getMember("begin").getReturn().getMember("execute").getACall() + ) + } + + override DataFlow::Node getSql() { result = this.getArg(0) } + } + + private class SqlAlchemyScalarCall extends DataFlow::CallCfgNode, SqlExecution::Range { + SqlAlchemyScalarCall() { + exists(SqlAlchemySessionInstance sessionInstance | + this = sessionInstance.getMember("scalar").getACall() + ) + } + + override DataFlow::Node getSql() { result = this.getArg(0) } + } + + private class SqlAlchemyQueryCall extends DataFlow::CallCfgNode, SqlExecution::Range { + SqlAlchemyQueryCall() { + exists(SqlAlchemyQueryInstance queryInstance | this = queryInstance.getAMember().getACall()) + } + + override DataFlow::Node getSql() { result = this.getArg(0) } + } +} From a854fb8f8b16a2af5a548465a5e4d551f135a9ba Mon Sep 17 00:00:00 2001 From: thank_you Date: Thu, 15 Apr 2021 15:22:15 -0400 Subject: [PATCH 02/11] Add documentation and refactor code --- .../semmle/python/frameworks/SqlAlchemy.qll | 47 +++++++++++++++---- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll b/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll index 147868a0f901..ca3fd39de5bf 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll @@ -7,9 +7,14 @@ 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 module SqlAlchemy { + /** + * 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 class SqlAlchemySessionInstance extends API::Node { SqlAlchemySessionInstance() { this in [ @@ -18,26 +23,40 @@ private module SqlAlchemy { ] } - override string toString() { result = "Use of SqlAlchemy Session instance method" } + override string toString() { result = "Use of SqlAlchemy Session instantization" } } + /** + * An instantization of a SqlAlchemy Engine object. + * See https://docs.sqlalchemy.org/en/14/core/engines.html#sqlalchemy.create_engine + */ private class SqlAlchemyEngineInstance extends API::Node { SqlAlchemyEngineInstance() { this = API::moduleImport("sqlalchemy").getMember("create_engine").getReturn() } - override string toString() { result = "Use of SqlAlchemy Engine instance method" } + override string toString() { result = "Use of SqlAlchemy create_engine member" } } + /** + * An instantization of a SqlAlchemy Query object. + * See https://docs.sqlalchemy.org/en/14/orm/query.html?highlight=query#sqlalchemy.orm.Query + */ private class SqlAlchemyQueryInstance extends API::Node { SqlAlchemyQueryInstance() { - this instanceof SqlAlchemySessionInstance and - this = this.getMember("query").getReturn() + this = any(SqlAlchemySessionInstance sessionInstance).getMember("query").getReturn() } - override string toString() { result = "Use of SqlAlchemy Query instance method" } + override string toString() { result = "Use of SqlAlchemy Session Query member" } } + /** + * 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() { exists(SqlAlchemySessionInstance sessionInstance, SqlAlchemyEngineInstance engineInstance | @@ -50,19 +69,27 @@ private module SqlAlchemy { 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() { - exists(SqlAlchemySessionInstance sessionInstance | - this = sessionInstance.getMember("scalar").getACall() - ) + this = any(SqlAlchemySessionInstance sessionInstance).getMember("scalar").getACall() or + this = any(SqlAlchemyEngineInstance engineInstance).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() { - exists(SqlAlchemyQueryInstance queryInstance | this = queryInstance.getAMember().getACall()) + this = any(SqlAlchemyQueryInstance queryInstance).getAMember().getACall() } override DataFlow::Node getSql() { result = this.getArg(0) } From c5fbbc055179e4459dd327257fa7c827ed92f5ba Mon Sep 17 00:00:00 2001 From: thank_you Date: Mon, 19 Apr 2021 18:56:00 -0400 Subject: [PATCH 03/11] Refactor SqlAlchemy model - Replaced classes that look for SqlAlchemy instances with predicates - General clean-up of code --- .../semmle/python/frameworks/SqlAlchemy.qll | 57 ++++++++----------- 1 file changed, 23 insertions(+), 34 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll b/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll index ca3fd39de5bf..9baaad4fd6b7 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll @@ -11,43 +11,29 @@ private import semmle.python.Concepts private module SqlAlchemy { /** - * An instantization of a SqlAlchemy Session object. + * 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 class SqlAlchemySessionInstance extends API::Node { - SqlAlchemySessionInstance() { - this in [ - API::moduleImport("sqlalchemy.orm").getMember("Session").getReturn(), - API::moduleImport("sqlalchemy.orm").getMember("sessionmaker").getReturn().getReturn() - ] - } - - override string toString() { result = "Use of SqlAlchemy Session instantization" } + private API::Node getSqlAlchemySessionInstance() { + result = API::moduleImport("sqlalchemy.orm").getMember("Session").getReturn() or + result = API::moduleImport("sqlalchemy.orm").getMember("sessionmaker").getReturn().getReturn() } /** - * An instantization of a SqlAlchemy Engine object. + * Returns an instantization of a SqlAlchemy Engine object. * See https://docs.sqlalchemy.org/en/14/core/engines.html#sqlalchemy.create_engine */ - private class SqlAlchemyEngineInstance extends API::Node { - SqlAlchemyEngineInstance() { - this = API::moduleImport("sqlalchemy").getMember("create_engine").getReturn() - } - - override string toString() { result = "Use of SqlAlchemy create_engine member" } + private API::Node getSqlAlchemyEngineInstance() { + result = API::moduleImport("sqlalchemy").getMember("create_engine").getReturn() } /** - * An instantization of a SqlAlchemy Query object. + * Returns an instantization of a SqlAlchemy Query object. * See https://docs.sqlalchemy.org/en/14/orm/query.html?highlight=query#sqlalchemy.orm.Query */ - private class SqlAlchemyQueryInstance extends API::Node { - SqlAlchemyQueryInstance() { - this = any(SqlAlchemySessionInstance sessionInstance).getMember("query").getReturn() - } - - override string toString() { result = "Use of SqlAlchemy Session Query member" } + private API::Node getSqlAlchemyQueryInstance() { + result = getSqlAlchemySessionInstance().getMember("query").getReturn() } /** @@ -59,11 +45,14 @@ private module SqlAlchemy { */ private class SqlAlchemyExecuteCall extends DataFlow::CallCfgNode, SqlExecution::Range { SqlAlchemyExecuteCall() { - exists(SqlAlchemySessionInstance sessionInstance, SqlAlchemyEngineInstance engineInstance | - this = sessionInstance.getMember("execute").getACall() or - this = engineInstance.getMember("connect").getReturn().getMember("execute").getACall() or - this = engineInstance.getMember("begin").getReturn().getMember("execute").getACall() - ) + // 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) } @@ -76,8 +65,10 @@ private module SqlAlchemy { */ private class SqlAlchemyScalarCall extends DataFlow::CallCfgNode, SqlExecution::Range { SqlAlchemyScalarCall() { - this = any(SqlAlchemySessionInstance sessionInstance).getMember("scalar").getACall() or - this = any(SqlAlchemyEngineInstance engineInstance).getMember("scalar").getACall() + this = + [getSqlAlchemySessionInstance(), getSqlAlchemyEngineInstance()] + .getMember("scalar") + .getACall() } override DataFlow::Node getSql() { result = this.getArg(0) } @@ -88,9 +79,7 @@ private module SqlAlchemy { * 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 = any(SqlAlchemyQueryInstance queryInstance).getAMember().getACall() - } + SqlAlchemyQueryCall() { this = getSqlAlchemyQueryInstance().getAMember().getACall() } override DataFlow::Node getSql() { result = this.getArg(0) } } From 3ace49549a94b34634dcadc8932951c7e944129e Mon Sep 17 00:00:00 2001 From: thank_you Date: Mon, 10 May 2021 16:12:15 -0400 Subject: [PATCH 04/11] Add tests for SqlAlchemy modeling library After researching SqlAlchemy and it's various query methods, I discovered several types of SQL injection possibilities. The SQLExecution.py file contains these examples and can be broken up into two types of injections. Injections requiring the text() taint-step and injections NOT requiring the text() taint step. --- .../frameworks/sqlalchemy/ConceptsTest.ql | 2 + .../frameworks/sqlalchemy/SqlExecution.py | 56 +++++++++++++++++++ 2 files changed, 58 insertions(+) create mode 100644 python/ql/test/experimental/library-tests/frameworks/sqlalchemy/ConceptsTest.ql create mode 100644 python/ql/test/experimental/library-tests/frameworks/sqlalchemy/SqlExecution.py diff --git a/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/ConceptsTest.ql b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/ConceptsTest.ql new file mode 100644 index 000000000000..b557a0bccb69 --- /dev/null +++ b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/ConceptsTest.ql @@ -0,0 +1,2 @@ +import python +import experimental.meta.ConceptsTest diff --git a/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/SqlExecution.py b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/SqlExecution.py new file mode 100644 index 000000000000..9018c2559784 --- /dev/null +++ b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/SqlExecution.py @@ -0,0 +1,56 @@ +import sqlalchemy +from sqlalchemy import Column, Integer, String, ForeignKey, create_engine +from sqlalchemy.ext.declarative import declarative_base +from sqlalchemy.pool import StaticPool +from sqlalchemy.orm import relationship, backref, sessionmaker, joinedload +from sqlalchemy.sql import text + +engine = create_engine( + 'sqlite:///:memory:', + echo=True, + connect_args={"check_same_thread": False}, + poolclass=StaticPool +) + +Base = declarative_base() + +class User(Base): + __tablename__ = 'users' + + id = Column(Integer, primary_key=True) + name = Column(String) + +Base.metadata.create_all(engine) + +Session = sessionmaker(bind=engine) +session = Session() + +ed_user = User(name='ed') +ed_user2 = User(name='george') + +session.add(ed_user) +session.add(ed_user2) + +session.commit() + +# Injection without requiring the text() taint-step +session.query(User).filter_by(name="some sql") # $getSql="some sql" +session.scalar("some sql") # $getSql="some sql" +engine.scalar("some sql") # $getSql="some sql" +session.execute("some sql") # $getSql="some sql" + +with engine.connect() as connection: + connection.execute("some sql") # $getSql="some sql" + +with engine.begin() as connection: + connection.execute("some sql") # $getSql="some sql" + +# Injection requiring the text() taint-step +session.query(User).filter(text("some sql")) # $getSql="some sql" +session.query(User).group_by( User.id ).having(text("some sql")) # $getSql="some sql" +session.query(User).group_by(text("name='some sql'")).first() # $getSql="some sql" +session.query(User).order_by(text("name='some sql'")).first() # $getSql="some sql" + +query = select(User).where(User.name == text("some sql")) # $getSql="some sql" +with engine.connect() as conn: + conn.execute(query) From eac1c5d10982f5c72a57e742dacb4327af0153a6 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 29 Jun 2021 10:58:28 +0200 Subject: [PATCH 05/11] Python: Fix concepts-tests for SQLAlchemy --- .../frameworks/sqlalchemy/ConceptsTest.ql | 1 + .../frameworks/sqlalchemy/SqlExecution.py | 25 ++++++++++--------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/ConceptsTest.ql b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/ConceptsTest.ql index b557a0bccb69..b097eb15648b 100644 --- a/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/ConceptsTest.ql +++ b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/ConceptsTest.ql @@ -1,2 +1,3 @@ import python import experimental.meta.ConceptsTest +import experimental.semmle.python.frameworks.SqlAlchemy diff --git a/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/SqlExecution.py b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/SqlExecution.py index 9018c2559784..1d2e861a8e9f 100644 --- a/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/SqlExecution.py +++ b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/SqlExecution.py @@ -34,23 +34,24 @@ class User(Base): session.commit() # Injection without requiring the text() taint-step -session.query(User).filter_by(name="some sql") # $getSql="some sql" -session.scalar("some sql") # $getSql="some sql" -engine.scalar("some sql") # $getSql="some sql" -session.execute("some sql") # $getSql="some sql" +session.query(User).filter_by(name="some sql") # $ MISSING: getSql="some sql" +session.scalar("some sql") # $ getSql="some sql" +engine.scalar("some sql") # $ getSql="some sql" +session.execute("some sql") # $ getSql="some sql" with engine.connect() as connection: - connection.execute("some sql") # $getSql="some sql" + connection.execute("some sql") # $ getSql="some sql" with engine.begin() as connection: - connection.execute("some sql") # $getSql="some sql" + connection.execute("some sql") # $ getSql="some sql" # Injection requiring the text() taint-step -session.query(User).filter(text("some sql")) # $getSql="some sql" -session.query(User).group_by( User.id ).having(text("some sql")) # $getSql="some sql" -session.query(User).group_by(text("name='some sql'")).first() # $getSql="some sql" -session.query(User).order_by(text("name='some sql'")).first() # $getSql="some sql" +t = text("some sql") +session.query(User).filter(t) # $ getSql=t +session.query(User).group_by(User.id).having(t) # $ getSql=Attribute MISSING: getSql=t +session.query(User).group_by(t).first() # $ getSql=t +session.query(User).order_by(t).first() # $ getSql=t -query = select(User).where(User.name == text("some sql")) # $getSql="some sql" +query = select(User).where(User.name == t) # $ MISSING: getSql=t with engine.connect() as conn: - conn.execute(query) + conn.execute(query) # $ getSql=query From cb112395f84c16bf8e4c1399b4190da744b7ff83 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 29 Jun 2021 10:59:26 +0200 Subject: [PATCH 06/11] Python: Fixup after merging main --- .../library-tests/frameworks/sqlalchemy/ConceptsTest.expected | 0 .../library-tests/frameworks/sqlalchemy/SqlExecution.py | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 python/ql/test/experimental/library-tests/frameworks/sqlalchemy/ConceptsTest.expected diff --git a/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/ConceptsTest.expected b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/ConceptsTest.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/SqlExecution.py b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/SqlExecution.py index 1d2e861a8e9f..7731e80e5340 100644 --- a/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/SqlExecution.py +++ b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/SqlExecution.py @@ -48,7 +48,7 @@ class User(Base): # Injection requiring the text() taint-step t = text("some sql") session.query(User).filter(t) # $ getSql=t -session.query(User).group_by(User.id).having(t) # $ getSql=Attribute MISSING: getSql=t +session.query(User).group_by(User.id).having(t) # $ getSql=User.id MISSING: getSql=t session.query(User).group_by(t).first() # $ getSql=t session.query(User).order_by(t).first() # $ getSql=t From ef48734206153930163d3da7a27f6cec9f3cd80d Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 29 Jun 2021 11:03:40 +0200 Subject: [PATCH 07/11] Python: Add taint-tests for SQLAlchemy --- .../frameworks/sqlalchemy/InlineTaintTest.expected | 3 +++ .../frameworks/sqlalchemy/InlineTaintTest.ql | 2 ++ .../frameworks/sqlalchemy/taint_test.py | 12 ++++++++++++ 3 files changed, 17 insertions(+) create mode 100644 python/ql/test/experimental/library-tests/frameworks/sqlalchemy/InlineTaintTest.expected create mode 100644 python/ql/test/experimental/library-tests/frameworks/sqlalchemy/InlineTaintTest.ql create mode 100644 python/ql/test/experimental/library-tests/frameworks/sqlalchemy/taint_test.py diff --git a/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/InlineTaintTest.expected b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/InlineTaintTest.expected new file mode 100644 index 000000000000..79d760d87f42 --- /dev/null +++ b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/InlineTaintTest.expected @@ -0,0 +1,3 @@ +argumentToEnsureNotTaintedNotMarkedAsSpurious +untaintedArgumentToEnsureTaintedNotMarkedAsMissing +failures diff --git a/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/InlineTaintTest.ql b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/InlineTaintTest.ql new file mode 100644 index 000000000000..2ddb342c62fd --- /dev/null +++ b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/InlineTaintTest.ql @@ -0,0 +1,2 @@ +import experimental.meta.InlineTaintTest +import experimental.semmle.python.frameworks.SqlAlchemy diff --git a/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/taint_test.py b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/taint_test.py new file mode 100644 index 000000000000..cdb3da98044c --- /dev/null +++ b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/taint_test.py @@ -0,0 +1,12 @@ +import sqlalchemy + +def test_taint(): + ts = TAINTED_STRING + + ensure_tainted( + ts, # $ tainted + sqlalchemy.text(ts), # $ MISSING: tainted + sqlalchemy.sql.text(ts),# $ MISSING: tainted + sqlalchemy.sql.expression.text(ts),# $ MISSING: tainted + sqlalchemy.sql.expression.TextClause(ts),# $ MISSING: tainted + ) From a5a7f3e38ac38ef221697e0f541caaffeda0b144 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 29 Jun 2021 11:06:25 +0200 Subject: [PATCH 08/11] Python: Add taint-step for `sqlalchemy.text` --- .../semmle/python/frameworks/SqlAlchemy.qll | 34 +++++++++++++++++++ .../frameworks/sqlalchemy/taint_test.py | 8 ++--- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll b/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll index 9baaad4fd6b7..ea419299897a 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll @@ -83,4 +83,38 @@ private module SqlAlchemy { override DataFlow::Node getSql() { result = this.getArg(0) } } + + /** + * 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 + ) + } + } } diff --git a/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/taint_test.py b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/taint_test.py index cdb3da98044c..91f8987132f5 100644 --- a/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/taint_test.py +++ b/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/taint_test.py @@ -5,8 +5,8 @@ def test_taint(): ensure_tainted( ts, # $ tainted - sqlalchemy.text(ts), # $ MISSING: tainted - sqlalchemy.sql.text(ts),# $ MISSING: tainted - sqlalchemy.sql.expression.text(ts),# $ MISSING: tainted - sqlalchemy.sql.expression.TextClause(ts),# $ MISSING: tainted + sqlalchemy.text(ts), # $ tainted + sqlalchemy.sql.text(ts),# $ tainted + sqlalchemy.sql.expression.text(ts),# $ tainted + sqlalchemy.sql.expression.TextClause(ts),# $ tainted ) From 986f2f43028bd319ad9f6374deb9062560a14798 Mon Sep 17 00:00:00 2001 From: thank_you Date: Tue, 29 Jun 2021 19:39:26 -0400 Subject: [PATCH 09/11] Add SQLEscape module --- .../experimental/semmle/python/Concepts.qll | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/python/ql/src/experimental/semmle/python/Concepts.qll b/python/ql/src/experimental/semmle/python/Concepts.qll index 7641ac1becfb..2fbf5306c760 100644 --- a/python/ql/src/experimental/semmle/python/Concepts.qll +++ b/python/ql/src/experimental/semmle/python/Concepts.qll @@ -146,3 +146,36 @@ class LDAPEscape extends DataFlow::Node { */ DataFlow::Node getAnInput() { result = range.getAnInput() } } + +/** Provides classes for modeling SQL sanitization libraries. */ +module SQLEscape { + /** + * A data-flow node that collects functions that escape SQL statements. + * + * Extend this class to model new APIs. If you want to refine existing API models, + * extend `SQLEscape` instead. + */ + abstract class Range extends DataFlow::Node { + /** + * Gets the argument containing the raw SQL statement. + */ + abstract DataFlow::Node getAnInput(); + } +} + +/** + * A data-flow node that collects functions escaping SQL statements. + * + * Extend this class to refine existing API models. If you want to model new APIs, + * extend `SQLEscape::Range` instead. + */ +class SQLEscape extends DataFlow::Node { + SQLEscape::Range range; + + SQLEscape() { this = range } + + /** + * Gets the argument containing the raw SQL statement. + */ + DataFlow::Node getAnInput() { result = range.getAnInput() } +} From 0be2c6b7657bc1930daf5b3b86545b864dd0e8e2 Mon Sep 17 00:00:00 2001 From: thank_you Date: Tue, 29 Jun 2021 19:39:46 -0400 Subject: [PATCH 10/11] Add SQLEscapySanitizerCall class --- .../semmle/python/frameworks/SqlAlchemy.qll | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll b/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll index ea419299897a..a00d7e781796 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll @@ -8,6 +8,7 @@ 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 { /** @@ -117,4 +118,17 @@ private module SqlAlchemy { ) } } + + /** + * 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) } + } } From 9e01338500b703adf3d9084b0e9d22623ef8643c Mon Sep 17 00:00:00 2001 From: thank_you Date: Sun, 18 Jul 2021 17:13:10 -0400 Subject: [PATCH 11/11] Query only vulnerable methods --- .../semmle/python/frameworks/SqlAlchemy.qll | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll b/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll index a00d7e781796..00d550d9844b 100644 --- a/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll +++ b/python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll @@ -80,11 +80,25 @@ private module SqlAlchemy { * 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().getAMember().getACall() } + 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()` *