Python: Add SqlAlchemy model #5680

mrthankyou · 2021-04-15T02:09:33Z

This PR is meant to add a module for dealing with SqlAlchemy in Python CodeQL queries. For context, this will be used in an upcoming CodeQL security query so I decided to go ahead and submit this PR to get the ball rolling.

mrthankyou · 2021-04-15T19:25:15Z

python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll

+      this = API::moduleImport("sqlalchemy").getMember("create_engine").getReturn()
+    }
+
+    override string toString() { result = "Use of SqlAlchemy create_engine member" }


I'm not 100% the best wording practices here so if this needs to be changed let me know!

Usually we don't build classes for this, but instead use a predicate, such as

/** Gets an instance of ... */ getSqlAlchemyEngineInstance() { result = API::moduleImport("sqlalchemy").getMember("create_engine").getReturn() }

and I think doing so in this case would also make things a bit more clean.

python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll

mrthankyou · 2021-04-19T14:52:24Z

Small note: I have another model I'm currently building and is related to the security query that will be using the model in this PR. I've decided to wait for this to be merged in before I'll submit the other query. This way, if there are any glaring issues in this PR, I can apply them to upcoming PR as well.

python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll

- Replaced classes that look for SqlAlchemy instances with predicates - General clean-up of code

mrthankyou · 2021-04-19T22:57:52Z

This is ready for re-review.

PS: Are there additional requirements such as tests for supplementary code like this to a query? I know tests matter for the actual queries and I thought I saw tests written for other models but I haven't seen any guidance on models.

mrthankyou · 2021-04-27T21:13:39Z

Friendly ping about this PR 😄

RasmusWL · 2021-04-28T09:30:28Z

Are there additional requirements such as tests for supplementary code like this to a query? I know tests matter for the actual queries and I thought I saw tests written for other models but I haven't seen any guidance on models.

Missed this response, but yes, please add tests demonstrating behavior that is covered by the library modeling. Please also include test examples for any interesting behavior that you can do with the library that you currently don't model (that is, if there is some SQL executions that you know can happen with the SQL Alchemy library, that it just hasn't been possible to model yet).

The way I would want you to do this is by creating a new folder python/ql/test/experimental/library-tests/frameworks/sqlalchemy/, and adding a copy of our standadized ConceptsTest.ql file. Then add your examples as .py files, and add annotations inside the Python file -- this uses a special comment syntax, so everything after a $ in a comment means it's an inline annotation.

For an example, you can see the tests we did for SQL execution in django: https://github.com/github/codeql/blob/c8a6e837b57513df5c8ccd081e193be3443987d1/python/ql/test/library-tests/frameworks/django/SqlExecution.py

and this is the ConceptsTest.ql file that you jsut need to have a copy of: https://github.com/github/codeql/blob/c8a6e837b57513df5c8ccd081e193be3443987d1/python/ql/test/library-tests/frameworks/django/ConceptsTest.ql

RasmusWL · 2021-04-28T10:17:44Z

I think there might be a misconception about the SqlExecution concept. We want to capture calls that execute SQL provided in textual form, and not just any call that might execute some SQL under the hood. I think the ORM layers sits on a knife edge on this, but we're only interested in the calls where you can provide actual SQL (as a string) to be executed, and not so much in calls that build uses the ORM to interact with a database.

One of the examples I found was (rewritten slightly)

with engine.connect() as conn:
    t = text("select * from table")
    result = conn.execution_options(stream_results=True).execute(t)

As I see it, the current modeling would capture that the t argument could represent SQL (correct). As I see it, there is currently no taint-step from the argument to text and the resulting object, so such a use-case would never be able to be found by our SQL injection query 😞

I'm not so sure about the SqlAlchemyQueryCall, are there any methods on sqlalchemy.orm.Query that accepts SQL to be executed?

Looking forward to seeing your tests, probably they will clear most of this up 👍

mrthankyou · 2021-05-10T19:51:25Z

As I see it, there is currently no taint-step from the argument to text and the resulting object, so such a use-case would never be able to be found by our SQL injection query 😞

I'm a little lost here so I'm going to ask some questions and hopefully this will get cleared up on my end:

When I eventually submit my SqlAlchemy query that relies on this PR, I will have accounted for this taint-step in the query I'm writing. However, as it stands the current SQL execution query can't account for this additional taint-step and will fail.

Is the expectation here that if this PR is merged in it should automatically work with the current queries?

I'm not so sure about the SqlAlchemyQueryCall, are there any methods on sqlalchemy.orm.Query that accepts SQL to be executed?

There are some cases where the taint-step text() is not required. I've attached a link here (see Vulnerable Sinks section) that documents the testing I did on SqlAlchemy.

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.

RasmusWL

We've talked this over in private a bit. For the record, I've created a few small changes in mrthankyou#1, and with that merged, I think this PR is OK to go, and I'm happy to do the last bit of polishing to get this promoted out of experiemntal.

Minor updates to SQL alchemy PR

mrthankyou · 2021-06-29T22:23:40Z

python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll

+   */
+  private class SqlAlchemyExecuteCall extends DataFlow::CallCfgNode, SqlExecution::Range {
+    SqlAlchemyExecuteCall() {
+      // new way


I wrote this and I have no idea what I was referencing. This model library points to SqlAlchemy 1.X so I don't think I'm referencing 2.0.

mrthankyou · 2021-06-29T22:25:01Z

python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll

+private import semmle.python.ApiGraphs
+private import semmle.python.Concepts
+
+private module SqlAlchemy {


Is there any desire on your side Rasmus to version this model? SqlAlchemy came out with 2.0 and I don't know how compatible this model library is for 2.0.

2.0 has still not been released it seems (https://pypi.org/project/SQLAlchemy/#history), so I think it's OK. Thanks for letting me know about this though 👍

RasmusWL

I think this PR is ok to go now 👍

I assume the MISSING keyword represents a scenario where the test is skippable?

It means that the modeling was not able to find the result we know should be there. (so the expected result is MISSING, if the modeling provides a result where we know there shouldn't be any, we mark that as SPURIOUS)

So if you want to, feel free to fix up those instances (like filter_by and having).

RasmusWL · 2021-06-30T18:39:09Z

python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll

+private import semmle.python.ApiGraphs
+private import semmle.python.Concepts
+
+private module SqlAlchemy {


2.0 has still not been released it seems (https://pypi.org/project/SQLAlchemy/#history), so I think it's OK. Thanks for letting me know about this though 👍

mrthankyou · 2021-07-19T12:34:28Z

Hi,

I have submitted the changes but am still working on getting the tests updated. Unfortunately I'm encountering errors while attempting to codeql test run and am having to debug it.

Please note that I'm slammed right now with work and moving. I have no free time to work on this at least for the next week. If I have some free time today I will try to tackle it. I'm sorry for the delay on my end. This is unfortunately out of my control and will be tied down to at least next Wednesday.

RasmusWL · 2021-07-19T12:37:55Z

All the tests do pass though, so I don't think there's anything more for you to do on the tests 😉

mrthankyou · 2021-07-19T13:01:13Z

Yep, you're correct Rasmus. Testing results below:

codeql test run . --search-path /Users/hacker/Programming/hacking/research/codeql/my-codeql/python/ql

Executing 2 tests in 1 directories.
Extracting test database in /Users/hacker/Programming/hacking/research/codeql/my-codeql/python/ql/test/experimental/library-tests/frameworks/sqlalchemy.
Compiling queries in /Users/hacker/Programming/hacking/research/codeql/my-codeql/python/ql/test/experimental/library-tests/frameworks/sqlalchemy.
Executing tests in /Users/hacker/Programming/hacking/research/codeql/my-codeql/python/ql/test/experimental/library-tests/frameworks/sqlalchemy.
[1/2 eval 10.4s] PASSED /Users/hacker/Programming/hacking/research/codeql/my-codeql/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/ConceptsTest.ql
[2/2 eval 1.4s] PASSED /Users/hacker/Programming/hacking/research/codeql/my-codeql/python/ql/test/experimental/library-tests/frameworks/sqlalchemy/InlineTaintTest.ql
All 2 tests passed.

Add SqlAlchemy module

f1e71e2

github-actions bot added the Python label Apr 15, 2021

mrthankyou changed the title ~~Python: Add SqlAlchemy module~~ Python: Add SqlAlchemy model Apr 15, 2021

Add documentation and refactor code

a854fb8

mrthankyou marked this pull request as ready for review April 15, 2021 19:24

mrthankyou requested a review from a team as a code owner April 15, 2021 19:24

mrthankyou commented Apr 15, 2021

View reviewed changes

python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll Show resolved Hide resolved

RasmusWL reviewed Apr 19, 2021

View reviewed changes

python/ql/src/experimental/semmle/python/frameworks/SqlAlchemy.qll Outdated Show resolved Hide resolved

RasmusWL mentioned this pull request Apr 19, 2021

Python: CWE-943 - Add NoSQL injection query #5612

Merged

Refactor SqlAlchemy model

c5fbbc0

- Replaced classes that look for SqlAlchemy instances with predicates - General clean-up of code

RasmusWL assigned yoff and RasmusWL Jun 28, 2021

RasmusWL added 5 commits June 29, 2021 10:58

Python: Fix concepts-tests for SQLAlchemy

eac1c5d

Merge branch 'main' into python-use-sqlalchemy

684f51a

Python: Fixup after merging main

cb11239

Python: Add taint-tests for SQLAlchemy

ef48734

Python: Add taint-step for sqlalchemy.text

a5a7f3e

RasmusWL requested changes Jun 29, 2021

View reviewed changes

Merge pull request #1 from RasmusWL/python-use-sqlalchemy

2a65917

Minor updates to SQL alchemy PR

mrthankyou commented Jun 29, 2021

View reviewed changes

Add SQLEscape module

986f2f4

Add SQLEscapySanitizerCall class

0be2c6b

RasmusWL previously approved these changes Jun 30, 2021

View reviewed changes

mrthankyou mentioned this pull request Jun 30, 2021

[Python]: Add SqlAlchemy support for SQL injection query github/securitylab#390

Closed

1 task

Query only vulnerable methods

9e01338

mrthankyou dismissed RasmusWL’s stale review via 9e01338 July 18, 2021 21:13

RasmusWL unassigned yoff Jul 22, 2021

RasmusWL approved these changes Jul 22, 2021

View reviewed changes

RasmusWL merged commit 802d9bd into github:main Jul 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python: Add SqlAlchemy model #5680

Python: Add SqlAlchemy model #5680

mrthankyou commented Apr 15, 2021 •

edited

Loading

mrthankyou Apr 15, 2021

RasmusWL Apr 19, 2021

mrthankyou commented Apr 19, 2021

mrthankyou commented Apr 19, 2021 •

edited

Loading

mrthankyou commented Apr 27, 2021

RasmusWL commented Apr 28, 2021

RasmusWL commented Apr 28, 2021

mrthankyou commented May 10, 2021

RasmusWL left a comment

mrthankyou Jun 29, 2021

mrthankyou Jun 29, 2021

RasmusWL Jun 30, 2021

RasmusWL left a comment

RasmusWL Jun 30, 2021

mrthankyou commented Jul 19, 2021

RasmusWL commented Jul 19, 2021

mrthankyou commented Jul 19, 2021

Python: Add SqlAlchemy model #5680

Python: Add SqlAlchemy model #5680

Conversation

mrthankyou commented Apr 15, 2021 • edited Loading

mrthankyou Apr 15, 2021

Choose a reason for hiding this comment

RasmusWL Apr 19, 2021

Choose a reason for hiding this comment

mrthankyou commented Apr 19, 2021

mrthankyou commented Apr 19, 2021 • edited Loading

mrthankyou commented Apr 27, 2021

RasmusWL commented Apr 28, 2021

RasmusWL commented Apr 28, 2021

mrthankyou commented May 10, 2021

RasmusWL left a comment

Choose a reason for hiding this comment

mrthankyou Jun 29, 2021

Choose a reason for hiding this comment

mrthankyou Jun 29, 2021

Choose a reason for hiding this comment

RasmusWL Jun 30, 2021

Choose a reason for hiding this comment

RasmusWL left a comment

Choose a reason for hiding this comment

RasmusWL Jun 30, 2021

Choose a reason for hiding this comment

mrthankyou commented Jul 19, 2021

RasmusWL commented Jul 19, 2021

mrthankyou commented Jul 19, 2021

mrthankyou commented Apr 15, 2021 •

edited

Loading

mrthankyou commented Apr 19, 2021 •

edited

Loading