Skip to content

Commit 3bad60c

Browse files
owen-mcowen-mc
committed
Fix MISSING alert
1 parent 434a994 commit 3bad60c

3 files changed

Lines changed: 71 additions & 3 deletions

File tree

‎python/ql/lib/semmle/python/dataflow/new/internal/TypeTrackingImpl.qll‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,9 @@ module TypeTrackingInput implements Shared::TypeTrackingInput<Location> {
167167
}
168168

169169
/** Holds if there is a level step from `nodeFrom` to `nodeTo`, which may depend on the call graph. */
170-
predicate levelStepCall(Node nodeFrom, LocalSourceNode nodeTo) { none() }
170+
predicate levelStepCall(Node nodeFrom, LocalSourceNode nodeTo) {
171+
instanceFieldStep(nodeFrom, nodeTo)
172+
}
171173

172174
/** Holds if there is a level step from `nodeFrom` to `nodeTo`, which does not depend on the call graph. */
173175
predicate levelStepNoCall(Node nodeFrom, LocalSourceNode nodeTo) {
@@ -337,6 +339,20 @@ module TypeTrackingInput implements Shared::TypeTrackingInput<Location> {
337339
)
338340
}
339341

342+
/**
343+
* Holds if `read` reads attribute `attr` from an instance of `cls`, where the instance
344+
* is referred to from outside the methods of `cls` (i.e. an access of the form
345+
* `instance.attr`, where `instance` is a reference to an instance of `cls`).
346+
*
347+
* This complements `selfAttrRef`, which only handles `self.attr` accesses inside the
348+
* methods of `cls`. Unlike `selfAttrRef`, this depends on the call graph (via
349+
* `classInstanceTracker`), so steps using it must be reported as `levelStepCall`.
350+
*/
351+
private predicate instanceAttrRead(Class cls, string attr, DataFlowPublic::AttrRead read) {
352+
read.getObject() = DataFlowDispatch::classInstanceTracker(cls) and
353+
read.mayHaveAttributeName(attr)
354+
}
355+
340356
/**
341357
* Holds if `nodeFrom` is written to attribute `self.attr` in some instance method of a
342358
* class, and `nodeTo` reads attribute `self.attr` in some (possibly different) instance
@@ -364,6 +380,28 @@ module TypeTrackingInput implements Shared::TypeTrackingInput<Location> {
364380
)
365381
}
366382

383+
/**
384+
* Holds if `nodeFrom` is written to attribute `self.attr` in some instance method of a
385+
* class, and `nodeTo` reads attribute `attr` from an instance of the same class outside
386+
* its methods (e.g. `instance.attr`).
387+
*
388+
* This is the cross-instance counterpart of `localFieldStep`: it relates a write of
389+
* `self.attr` inside the class to a read of `attr` on a reference to an instance of the
390+
* class. Identifying instances relies on the call graph (via `classInstanceTracker`), so
391+
* this step is reported as `levelStepCall` rather than `levelStepNoCall`.
392+
*
393+
* Like `localFieldStep`, this is an over-approximation: it is both instance-insensitive
394+
* and order-insensitive.
395+
*/
396+
private predicate instanceFieldStep(Node nodeFrom, LocalSourceNode nodeTo) {
397+
exists(Class cls, string attr, DataFlowPublic::AttrWrite write, DataFlowPublic::AttrRead read |
398+
selfAttrRef(cls, attr, write) and
399+
nodeFrom = write.getValue() and
400+
instanceAttrRead(cls, attr, read) and
401+
nodeTo = read
402+
)
403+
}
404+
367405
/**
368406
* Holds if data can flow from `node1` to `node2` in a way that discards call contexts.
369407
*/

‎python/ql/test/query-tests/Security/CWE-089-SqlInjection/SqlInjection.expected‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,9 @@
11
#select
2+
| 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 |
3+
| 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 |
4+
| 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 |
5+
| 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 |
6+
| 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 |
27
| 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 |
38
| 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 |
49
| 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 @@
1621
| 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 |
1722
| 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 |
1823
edges
24+
| app.py:20:18:20:21 | ControlFlowNode for name | app.py:21:5:21:9 | ControlFlowNode for query | provenance | |
25+
| app.py:21:5:21:9 | ControlFlowNode for query | app.py:23:20:23:24 | ControlFlowNode for query | provenance | |
26+
| app.py:27:19:27:22 | ControlFlowNode for name | app.py:28:5:28:9 | ControlFlowNode for query | provenance | |
27+
| app.py:28:5:28:9 | ControlFlowNode for query | app.py:30:20:30:24 | ControlFlowNode for query | provenance | |
28+
| app.py:34:19:34:22 | ControlFlowNode for name | app.py:35:5:35:9 | ControlFlowNode for query | provenance | |
29+
| app.py:35:5:35:9 | ControlFlowNode for query | app.py:37:20:37:24 | ControlFlowNode for query | provenance | |
30+
| app.py:41:19:41:22 | ControlFlowNode for name | app.py:42:5:42:9 | ControlFlowNode for query | provenance | |
31+
| app.py:42:5:42:9 | ControlFlowNode for query | app.py:44:20:44:24 | ControlFlowNode for query | provenance | |
32+
| app.py:48:19:48:22 | ControlFlowNode for name | app.py:49:5:49:9 | ControlFlowNode for query | provenance | |
33+
| app.py:49:5:49:9 | ControlFlowNode for query | app.py:51:20:51:24 | ControlFlowNode for query | provenance | |
1934
| sql_injection.py:14:15:14:22 | ControlFlowNode for username | sql_injection.py:21:24:21:77 | ControlFlowNode for BinaryExpr | provenance | |
2035
| sql_injection.py:14:15:14:22 | ControlFlowNode for username | sql_injection.py:24:38:24:95 | ControlFlowNode for BinaryExpr | provenance | |
2136
| 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
3348
| sqlalchemy_textclause.py:23:15:23:22 | ControlFlowNode for username | sqlalchemy_textclause.py:50:18:50:25 | ControlFlowNode for username | provenance | |
3449
| sqlalchemy_textclause.py:23:15:23:22 | ControlFlowNode for username | sqlalchemy_textclause.py:51:24:51:31 | ControlFlowNode for username | provenance | |
3550
nodes
51+
| app.py:20:18:20:21 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
52+
| app.py:21:5:21:9 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
53+
| app.py:23:20:23:24 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
54+
| app.py:27:19:27:22 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
55+
| app.py:28:5:28:9 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
56+
| app.py:30:20:30:24 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
57+
| app.py:34:19:34:22 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
58+
| app.py:35:5:35:9 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
59+
| app.py:37:20:37:24 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
60+
| app.py:41:19:41:22 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
61+
| app.py:42:5:42:9 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
62+
| app.py:44:20:44:24 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
63+
| app.py:48:19:48:22 | ControlFlowNode for name | semmle.label | ControlFlowNode for name |
64+
| app.py:49:5:49:9 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
65+
| app.py:51:20:51:24 | ControlFlowNode for query | semmle.label | ControlFlowNode for query |
3666
| sql_injection.py:14:15:14:22 | ControlFlowNode for username | semmle.label | ControlFlowNode for username |
3767
| sql_injection.py:21:24:21:77 | ControlFlowNode for BinaryExpr | semmle.label | ControlFlowNode for BinaryExpr |
3868
| sql_injection.py:24:38:24:95 | ControlFlowNode for BinaryExpr | semmle.label | ControlFlowNode for BinaryExpr |

‎python/ql/test/query-tests/Security/CWE-089-SqlInjection/app.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,10 @@ async def unsafe2(name: str): # $ Source
3131
cursor.close()
3232

3333
@app.get("/unsafe3/")
34-
async def unsafe3(name: str): # $ MISSING: Source
34+
async def unsafe3(name: str): # $ Source
3535
query = "select * from users where name=" + name
3636
cursor = hdb_con3.cursor()
37-
cursor.execute(query) # $ MISSING: Alert
37+
cursor.execute(query) # $ Alert
3838
cursor.close()
3939

4040
@app.get("/unsafe4/")

0 commit comments

Comments
 (0)