From 3bad60cf53e126d0c8ed1da7c064df7cc6d86855 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Fri, 12 Jun 2026 13:43:24 +0100 Subject: [PATCH] Fix MISSING alert --- .../new/internal/TypeTrackingImpl.qll | 40 ++++++++++++++++++- .../SqlInjection.expected | 30 ++++++++++++++ .../Security/CWE-089-SqlInjection/app.py | 4 +- 3 files changed, 71 insertions(+), 3 deletions(-) diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackingImpl.qll b/python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackingImpl.qll index a242c1d8e50..b76f8aeb9a5 100644 --- a/python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackingImpl.qll +++ b/python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackingImpl.qll @@ -167,7 +167,9 @@ module TypeTrackingInput implements Shared::TypeTrackingInput { } /** Holds if there is a level step from `nodeFrom` to `nodeTo`, which may depend on the call graph. */ - predicate levelStepCall(Node nodeFrom, LocalSourceNode nodeTo) { none() } + predicate levelStepCall(Node nodeFrom, LocalSourceNode nodeTo) { + instanceFieldStep(nodeFrom, nodeTo) + } /** Holds if there is a level step from `nodeFrom` to `nodeTo`, which does not depend on the call graph. */ predicate levelStepNoCall(Node nodeFrom, LocalSourceNode nodeTo) { @@ -337,6 +339,20 @@ module TypeTrackingInput implements Shared::TypeTrackingInput { ) } + /** + * Holds if `read` reads attribute `attr` from an instance of `cls`, where the instance + * is referred to from outside the methods of `cls` (i.e. an access of the form + * `instance.attr`, where `instance` is a reference to an instance of `cls`). + * + * This complements `selfAttrRef`, which only handles `self.attr` accesses inside the + * methods of `cls`. Unlike `selfAttrRef`, this depends on the call graph (via + * `classInstanceTracker`), so steps using it must be reported as `levelStepCall`. + */ + private predicate instanceAttrRead(Class cls, string attr, DataFlowPublic::AttrRead read) { + read.getObject() = DataFlowDispatch::classInstanceTracker(cls) and + read.mayHaveAttributeName(attr) + } + /** * Holds if `nodeFrom` is written to attribute `self.attr` in some instance method of a * class, and `nodeTo` reads attribute `self.attr` in some (possibly different) instance @@ -364,6 +380,28 @@ module TypeTrackingInput implements Shared::TypeTrackingInput { ) } + /** + * Holds if `nodeFrom` is written to attribute `self.attr` in some instance method of a + * class, and `nodeTo` reads attribute `attr` from an instance of the same class outside + * its methods (e.g. `instance.attr`). + * + * This is the cross-instance counterpart of `localFieldStep`: it relates a write of + * `self.attr` inside the class to a read of `attr` on a reference to an instance of the + * class. Identifying instances relies on the call graph (via `classInstanceTracker`), so + * this step is reported as `levelStepCall` rather than `levelStepNoCall`. + * + * Like `localFieldStep`, this is an over-approximation: it is both instance-insensitive + * and order-insensitive. + */ + private predicate instanceFieldStep(Node nodeFrom, LocalSourceNode nodeTo) { + exists(Class cls, string attr, DataFlowPublic::AttrWrite write, DataFlowPublic::AttrRead read | + selfAttrRef(cls, attr, write) and + nodeFrom = write.getValue() and + instanceAttrRead(cls, attr, read) and + nodeTo = read + ) + } + /** * Holds if data can flow from `node1` to `node2` in a way that discards call contexts. */ diff --git a/python/ql/test/query-tests/Security/CWE-089-SqlInjection/SqlInjection.expected b/python/ql/test/query-tests/Security/CWE-089-SqlInjection/SqlInjection.expected index c1958c23858..8f60394d8a2 100644 --- a/python/ql/test/query-tests/Security/CWE-089-SqlInjection/SqlInjection.expected +++ b/python/ql/test/query-tests/Security/CWE-089-SqlInjection/SqlInjection.expected @@ -1,4 +1,9 @@ #select +| app.py:23:20:23:24 | ControlFlowNode for query | app.py:20:18:20:21 | ControlFlowNode for name | app.py:23:20:23:24 | ControlFlowNode for query | This SQL query depends on a $@. | app.py:20:18:20:21 | ControlFlowNode for name | user-provided value | +| app.py:30:20:30:24 | ControlFlowNode for query | app.py:27:19:27:22 | ControlFlowNode for name | app.py:30:20:30:24 | ControlFlowNode for query | This SQL query depends on a $@. | app.py:27:19:27:22 | ControlFlowNode for name | user-provided value | +| app.py:37:20:37:24 | ControlFlowNode for query | app.py:34:19:34:22 | ControlFlowNode for name | app.py:37:20:37:24 | ControlFlowNode for query | This SQL query depends on a $@. | app.py:34:19:34:22 | ControlFlowNode for name | user-provided value | +| app.py:44:20:44:24 | ControlFlowNode for query | app.py:41:19:41:22 | ControlFlowNode for name | app.py:44:20:44:24 | ControlFlowNode for query | This SQL query depends on a $@. | app.py:41:19:41:22 | ControlFlowNode for name | user-provided value | +| app.py:51:20:51:24 | ControlFlowNode for query | app.py:48:19:48:22 | ControlFlowNode for name | app.py:51:20:51:24 | ControlFlowNode for query | This SQL query depends on a $@. | app.py:48:19:48:22 | ControlFlowNode for name | user-provided value | | sql_injection.py:21:24:21:77 | ControlFlowNode for BinaryExpr | sql_injection.py:14:15:14:22 | ControlFlowNode for username | sql_injection.py:21:24:21:77 | ControlFlowNode for BinaryExpr | This SQL query depends on a $@. | sql_injection.py:14:15:14:22 | ControlFlowNode for username | user-provided value | | sql_injection.py:24:38:24:95 | ControlFlowNode for BinaryExpr | sql_injection.py:14:15:14:22 | ControlFlowNode for username | sql_injection.py:24:38:24:95 | ControlFlowNode for BinaryExpr | This SQL query depends on a $@. | sql_injection.py:14:15:14:22 | ControlFlowNode for username | user-provided value | | sql_injection.py:25:26:25:83 | ControlFlowNode for BinaryExpr | sql_injection.py:14:15:14:22 | ControlFlowNode for username | sql_injection.py:25:26:25:83 | ControlFlowNode for BinaryExpr | This SQL query depends on a $@. | sql_injection.py:14:15:14:22 | ControlFlowNode for username | user-provided value | @@ -16,6 +21,16 @@ | sqlalchemy_textclause.py:50:18:50:25 | ControlFlowNode for username | sqlalchemy_textclause.py:23:15:23:22 | ControlFlowNode for username | sqlalchemy_textclause.py:50:18:50:25 | ControlFlowNode for username | This SQL query depends on a $@. | sqlalchemy_textclause.py:23:15:23:22 | ControlFlowNode for username | user-provided value | | sqlalchemy_textclause.py:51:24:51:31 | ControlFlowNode for username | sqlalchemy_textclause.py:23:15:23:22 | ControlFlowNode for username | sqlalchemy_textclause.py:51:24:51:31 | ControlFlowNode for username | This SQL query depends on a $@. | sqlalchemy_textclause.py:23:15:23:22 | ControlFlowNode for username | user-provided value | edges +| app.py:20:18:20:21 | ControlFlowNode for name | app.py:21:5:21:9 | ControlFlowNode for query | provenance | | +| app.py:21:5:21:9 | ControlFlowNode for query | app.py:23:20:23:24 | ControlFlowNode for query | provenance | | +| app.py:27:19:27:22 | ControlFlowNode for name | app.py:28:5:28:9 | ControlFlowNode for query | provenance | | +| app.py:28:5:28:9 | ControlFlowNode for query | app.py:30:20:30:24 | ControlFlowNode for query | provenance | | +| app.py:34:19:34:22 | ControlFlowNode for name | app.py:35:5:35:9 | ControlFlowNode for query | provenance | | +| app.py:35:5:35:9 | ControlFlowNode for query | app.py:37:20:37:24 | ControlFlowNode for query | provenance | | +| app.py:41:19:41:22 | ControlFlowNode for name | app.py:42:5:42:9 | ControlFlowNode for query | provenance | | +| app.py:42:5:42:9 | ControlFlowNode for query | app.py:44:20:44:24 | ControlFlowNode for query | provenance | | +| app.py:48:19:48:22 | ControlFlowNode for name | app.py:49:5:49:9 | ControlFlowNode for query | provenance | | +| app.py:49:5:49:9 | ControlFlowNode for query | app.py:51:20:51:24 | ControlFlowNode for query | provenance | | | sql_injection.py:14:15:14:22 | ControlFlowNode for username | sql_injection.py:21:24:21:77 | ControlFlowNode for BinaryExpr | provenance | | | sql_injection.py:14:15:14:22 | ControlFlowNode for username | sql_injection.py:24:38:24:95 | ControlFlowNode for BinaryExpr | provenance | | | sql_injection.py:14:15:14:22 | ControlFlowNode for username | sql_injection.py:25:26:25:83 | ControlFlowNode for BinaryExpr | provenance | | @@ -33,6 +48,21 @@ edges | sqlalchemy_textclause.py:23:15:23:22 | ControlFlowNode for username | sqlalchemy_textclause.py:50:18:50:25 | ControlFlowNode for username | provenance | | | sqlalchemy_textclause.py:23:15:23:22 | ControlFlowNode for username | sqlalchemy_textclause.py:51:24:51:31 | ControlFlowNode for username | provenance | | nodes +| app.py:20:18:20:21 | ControlFlowNode for name | semmle.label | ControlFlowNode for name | +| app.py:21:5:21:9 | ControlFlowNode for query | semmle.label | ControlFlowNode for query | +| app.py:23:20:23:24 | ControlFlowNode for query | semmle.label | ControlFlowNode for query | +| app.py:27:19:27:22 | ControlFlowNode for name | semmle.label | ControlFlowNode for name | +| app.py:28:5:28:9 | ControlFlowNode for query | semmle.label | ControlFlowNode for query | +| app.py:30:20:30:24 | ControlFlowNode for query | semmle.label | ControlFlowNode for query | +| app.py:34:19:34:22 | ControlFlowNode for name | semmle.label | ControlFlowNode for name | +| app.py:35:5:35:9 | ControlFlowNode for query | semmle.label | ControlFlowNode for query | +| app.py:37:20:37:24 | ControlFlowNode for query | semmle.label | ControlFlowNode for query | +| app.py:41:19:41:22 | ControlFlowNode for name | semmle.label | ControlFlowNode for name | +| app.py:42:5:42:9 | ControlFlowNode for query | semmle.label | ControlFlowNode for query | +| app.py:44:20:44:24 | ControlFlowNode for query | semmle.label | ControlFlowNode for query | +| app.py:48:19:48:22 | ControlFlowNode for name | semmle.label | ControlFlowNode for name | +| app.py:49:5:49:9 | ControlFlowNode for query | semmle.label | ControlFlowNode for query | +| app.py:51:20:51:24 | ControlFlowNode for query | semmle.label | ControlFlowNode for query | | sql_injection.py:14:15:14:22 | ControlFlowNode for username | semmle.label | ControlFlowNode for username | | sql_injection.py:21:24:21:77 | ControlFlowNode for BinaryExpr | semmle.label | ControlFlowNode for BinaryExpr | | sql_injection.py:24:38:24:95 | ControlFlowNode for BinaryExpr | semmle.label | ControlFlowNode for BinaryExpr | diff --git a/python/ql/test/query-tests/Security/CWE-089-SqlInjection/app.py b/python/ql/test/query-tests/Security/CWE-089-SqlInjection/app.py index 8046f1ef52e..4de61346d8f 100644 --- a/python/ql/test/query-tests/Security/CWE-089-SqlInjection/app.py +++ b/python/ql/test/query-tests/Security/CWE-089-SqlInjection/app.py @@ -31,10 +31,10 @@ async def unsafe2(name: str): # $ Source cursor.close() @app.get("/unsafe3/") -async def unsafe3(name: str): # $ MISSING: Source +async def unsafe3(name: str): # $ Source query = "select * from users where name=" + name cursor = hdb_con3.cursor() - cursor.execute(query) # $ MISSING: Alert + cursor.execute(query) # $ Alert cursor.close() @app.get("/unsafe4/")