Skip to content

Commit be1cad8

Browse files
RasmusWLRasmusWL
committed
Python: Resolve all meth = obj.meth; meth() TODOs
It would probably have been easier to do this as the _first_ thing... but that's too late now 😓
1 parent 6f63c03 commit be1cad8

File tree

9 files changed

+81
-190
lines changed

9 files changed

+81
-190
lines changed

‎python/.vscode/ql.code-snippets‎

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -201,24 +201,17 @@
201201
" */",
202202
" private class AdditionalTaintStep extends TaintTracking::AdditionalTaintStep {",
203203
" override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {",
204-
" // Methods",
205-
" //",
206-
" // TODO: When we have tools that make it easy, model these properly to handle",
207-
" // `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach",
208-
" // (since it allows us to at least capture the most common cases).",
204+
" // normal (non-async) methods",
209205
" nodeFrom = instance() and",
210-
" exists(DataFlow::AttrRead attr | attr.getObject() = nodeFrom |",
211-
" // normal (non-async) methods",
212-
" attr.getAttributeName() in [\"TODO\"] and",
213-
" nodeTo.(DataFlow::CallCfgNode).getFunction() = attr",
214-
" or",
215-
" // async methods",
216-
" exists(Await await, DataFlow::CallCfgNode call |",
217-
" attr.getAttributeName() in [\"TODO\"] and",
218-
" call.getFunction() = attr and",
219-
" await.getValue() = call.asExpr() and",
220-
" nodeTo.asExpr() = await",
221-
" )",
206+
" nodeTo.(DataFlow::MethodCallNode).calls(nodeFrom, [\"TODO\"])",
207+
" or",
208+
" // async methods",
209+
" exists(DataFlow::MethodCallNode call, Await await |",
210+
" nodeTo.asExpr() = await and",
211+
" nodeFrom = instance()",
212+
" |",
213+
" await.getValue() = any(DataFlow::Node awaitable | call.flowsTo(awaitable)).asExpr() and",
214+
" call.calls(nodeFrom, [\"TODO\"])",
222215
" )",
223216
" or",
224217
" // Attributes",

‎python/ql/src/semmle/python/frameworks/Aiohttp.qll‎

Lines changed: 25 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -359,27 +359,21 @@ module AiohttpWebModel {
359359
*/
360360
private class AiohttpStreamReaderAdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
361361
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
362-
// Methods
363-
//
364-
// TODO: When we have tools that make it easy, model these properly to handle
365-
// `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
366-
// (since it allows us to at least capture the most common cases).
367-
nodeFrom = StreamReader::instance() and
368-
exists(DataFlow::AttrRead attr | attr.getObject() = nodeFrom |
369-
// normal methods
370-
attr.getAttributeName() in ["read_nowait"] and
371-
nodeTo.(DataFlow::CallCfgNode).getFunction() = attr
372-
or
373-
// async methods
374-
exists(Await await, DataFlow::CallCfgNode call |
375-
attr.getAttributeName() in [
376-
"read", "readany", "readexactly", "readline", "readchunk", "iter_chunked",
377-
"iter_any", "iter_chunks"
378-
] and
379-
call.getFunction() = attr and
380-
await.getValue() = call.asExpr() and
381-
nodeTo.asExpr() = await
382-
)
362+
// normal (non-async) methods
363+
nodeFrom = instance() and
364+
nodeTo.(DataFlow::MethodCallNode).calls(nodeFrom, ["read_nowait"])
365+
or
366+
// async methods
367+
exists(DataFlow::MethodCallNode call, Await await |
368+
nodeTo.asExpr() = await and
369+
nodeFrom = instance()
370+
|
371+
await.getValue() = any(DataFlow::Node awaitable | call.flowsTo(awaitable)).asExpr() and
372+
call.calls(nodeFrom,
373+
[
374+
"read", "readany", "readexactly", "readline", "readchunk", "iter_chunked", "iter_any",
375+
"iter_chunks"
376+
])
383377
)
384378
}
385379
}
@@ -438,24 +432,17 @@ module AiohttpWebModel {
438432
*/
439433
private class AiohttpRequestAdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
440434
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
441-
// Methods
442-
//
443-
// TODO: When we have tools that make it easy, model these properly to handle
444-
// `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
445-
// (since it allows us to at least capture the most common cases).
435+
// normal (non-async) methods
446436
nodeFrom = Request::instance() and
447-
exists(DataFlow::AttrRead attr | attr.getObject() = nodeFrom |
448-
// normal methods
449-
attr.getAttributeName() in ["clone", "get_extra_info"] and
450-
nodeTo.(DataFlow::CallCfgNode).getFunction() = attr
451-
or
452-
// async methods
453-
exists(Await await, DataFlow::CallCfgNode call |
454-
attr.getAttributeName() in ["read", "text", "json", "multipart", "post"] and
455-
call.getFunction() = attr and
456-
await.getValue() = call.asExpr() and
457-
nodeTo.asExpr() = await
458-
)
437+
nodeTo.(DataFlow::MethodCallNode).calls(nodeFrom, ["clone", "get_extra_info"])
438+
or
439+
// async methods
440+
exists(DataFlow::MethodCallNode call, Await await |
441+
nodeTo.asExpr() = await and
442+
nodeFrom = Request::instance()
443+
|
444+
await.getValue() = any(DataFlow::Node awaitable | call.flowsTo(awaitable)).asExpr() and
445+
call.calls(nodeFrom, ["read", "text", "json", "multipart", "post"])
459446
)
460447
or
461448
// Attributes

‎python/ql/src/semmle/python/frameworks/Django.qll‎

Lines changed: 8 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -348,17 +348,11 @@ private module Django {
348348
nodeTo = call
349349
)
350350
or
351-
// Methods
352-
//
353-
// TODO: When we have tools that make it easy, model these properly to handle
354-
// `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
355-
// (since it allows us to at least capture the most common cases).
351+
// normal (non-async) methods
356352
nodeFrom = instance() and
357-
exists(DataFlow::AttrRead attr | attr.getObject() = nodeFrom |
358-
// methods (non-async)
359-
attr.getAttributeName() in ["getlist", "lists", "popitem", "dict", "urlencode"] and
360-
nodeTo.(DataFlow::CallCfgNode).getFunction() = attr
361-
)
353+
nodeTo
354+
.(DataFlow::MethodCallNode)
355+
.calls(nodeFrom, ["getlist", "lists", "popitem", "dict", "urlencode"])
362356
}
363357
}
364358
}
@@ -2044,18 +2038,11 @@ private module PrivateDjango {
20442038

20452039
private class DjangoHttpRequestAdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
20462040
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
2047-
// Methods
2048-
//
2049-
// TODO: When we have tools that make it easy, model these properly to handle
2050-
// `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
2051-
// (since it allows us to at least capture the most common cases).
2041+
// normal (non-async) methods
20522042
nodeFrom = django::http::request::HttpRequest::instance() and
2053-
exists(DataFlow::AttrRead attr | attr.getObject() = nodeFrom |
2054-
attr.getAttributeName() in [
2055-
"get_full_path", "get_full_path_info", "read", "readline", "readlines"
2056-
] and
2057-
nodeTo.(DataFlow::CallCfgNode).getFunction() = attr
2058-
)
2043+
nodeTo
2044+
.(DataFlow::MethodCallNode)
2045+
.calls(nodeFrom, ["get_full_path", "get_full_path_info", "read", "readline", "readlines"])
20592046
or
20602047
// special handling of the `build_absolute_uri` method, see
20612048
// https://docs.djangoproject.com/en/3.0/ref/request-response/#django.http.HttpRequest.build_absolute_uri

‎python/ql/src/semmle/python/frameworks/Multidict.qll‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -71,17 +71,9 @@ module Multidict {
7171
nodeTo = call
7272
)
7373
or
74-
// Methods
75-
//
76-
// TODO: When we have tools that make it easy, model these properly to handle
77-
// `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
78-
// (since it allows us to at least capture the most common cases).
74+
// normal (non-async) methods
7975
nodeFrom = instance() and
80-
exists(DataFlow::AttrRead attr | attr.getObject() = nodeFrom |
81-
// methods (non-async)
82-
attr.getAttributeName() in ["getone", "getall"] and
83-
nodeTo.(DataFlow::CallCfgNode).getFunction() = attr
84-
)
76+
nodeTo.(DataFlow::MethodCallNode).calls(nodeFrom, ["getone", "getall"])
8577
}
8678
}
8779
}

‎python/ql/src/semmle/python/frameworks/Stdlib.qll‎

Lines changed: 6 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -101,17 +101,11 @@ module Stdlib {
101101
*/
102102
private class AdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
103103
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
104-
// Methods
105-
//
106-
// TODO: When we have tools that make it easy, model these properly to handle
107-
// `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
108-
// (since it allows us to at least capture the most common cases).
104+
// normal (non-async) methods
109105
nodeFrom = instance() and
110-
exists(DataFlow::AttrRead attr | attr.getObject() = nodeFrom |
111-
// normal (non-async) methods
112-
attr.getAttributeName() in ["get_all", "as_bytes", "as_string", "keys"] and
113-
nodeTo.(DataFlow::CallCfgNode).getFunction() = attr
114-
)
106+
nodeTo
107+
.(DataFlow::MethodCallNode)
108+
.calls(nodeFrom, ["get_all", "as_bytes", "as_string", "keys"])
115109
}
116110
}
117111
}
@@ -149,17 +143,9 @@ module Stdlib {
149143
*/
150144
private class AdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
151145
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
152-
// Methods
153-
//
154-
// TODO: When we have tools that make it easy, model these properly to handle
155-
// `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
156-
// (since it allows us to at least capture the most common cases).
146+
// normal (non-async) methods
157147
nodeFrom = instance() and
158-
exists(DataFlow::AttrRead attr | attr.getObject() = nodeFrom |
159-
// normal (non-async) methods
160-
attr.getAttributeName() in ["output", "js_output"] and
161-
nodeTo.(DataFlow::CallCfgNode).getFunction() = attr
162-
)
148+
nodeTo.(DataFlow::MethodCallNode).calls(nodeFrom, ["output", "js_output"])
163149
or
164150
// Attributes
165151
nodeFrom = instance() and

‎python/ql/src/semmle/python/frameworks/Tornado.qll‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -50,17 +50,9 @@ private module Tornado {
5050
*/
5151
private class AdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
5252
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
53-
// Methods
54-
//
55-
// TODO: When we have tools that make it easy, model these properly to handle
56-
// `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
57-
// (since it allows us to at least capture the most common cases).
53+
// normal (non-async) methods
5854
nodeFrom = instance() and
59-
exists(DataFlow::AttrRead attr | attr.getObject() = nodeFrom |
60-
// normal (non-async) methods
61-
attr.getAttributeName() in ["get_list", "get_all"] and
62-
nodeTo.(DataFlow::CallCfgNode).getFunction() = attr
63-
)
55+
nodeTo.(DataFlow::MethodCallNode).calls(nodeFrom, ["get_list", "get_all"])
6456
}
6557
}
6658
}

‎python/ql/src/semmle/python/frameworks/Twisted.qll‎

Lines changed: 12 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -130,20 +130,15 @@ private module Twisted {
130130
*/
131131
private class TwistedRequestAdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
132132
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
133-
// Methods
134-
//
135-
// TODO: When we have tools that make it easy, model these properly to handle
136-
// `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
137-
// (since it allows us to at least capture the most common cases).
133+
// normal (non-async) methods
138134
nodeFrom = Request::instance() and
139-
exists(DataFlow::AttrRead attr | attr.getObject() = nodeFrom |
140-
// normal (non-async) methods
141-
attr.getAttributeName() in [
142-
"getCookie", "getHeader", "getAllHeaders", "getUser", "getPassword", "getHost",
143-
"getRequestHostname"
144-
] and
145-
nodeTo.(DataFlow::CallCfgNode).getFunction() = attr
146-
)
135+
nodeTo
136+
.(DataFlow::MethodCallNode)
137+
.calls(nodeFrom,
138+
[
139+
"getCookie", "getHeader", "getAllHeaders", "getUser", "getPassword", "getHost",
140+
"getRequestHostname"
141+
])
147142
or
148143
// Attributes
149144
nodeFrom = Request::instance() and
@@ -198,17 +193,8 @@ private module Twisted {
198193
*
199194
* See https://twistedmatrix.com/documents/21.2.0/api/twisted.web.server.Request.html#write
200195
*/
201-
class TwistedRequestWriteCall extends HTTP::Server::HttpResponse::Range, DataFlow::CallCfgNode {
202-
TwistedRequestWriteCall() {
203-
// TODO: When we have tools that make it easy, model these properly to handle
204-
// `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
205-
// (since it allows us to at least capture the most common cases).
206-
exists(DataFlow::AttrRead read |
207-
this.getFunction() = read and
208-
read.getObject() = Request::instance() and
209-
read.getAttributeName() = "write"
210-
)
211-
}
196+
class TwistedRequestWriteCall extends HTTP::Server::HttpResponse::Range, DataFlow::MethodCallNode {
197+
TwistedRequestWriteCall() { this.calls(Request::instance(), "write") }
212198

213199
override DataFlow::Node getBody() {
214200
result.asCfgNode() in [node.getArg(0), node.getArgByName("data")]
@@ -225,17 +211,8 @@ private module Twisted {
225211
* See https://twistedmatrix.com/documents/21.2.0/api/twisted.web.http.Request.html#redirect
226212
*/
227213
class TwistedRequestRedirectCall extends HTTP::Server::HttpRedirectResponse::Range,
228-
DataFlow::CallCfgNode {
229-
TwistedRequestRedirectCall() {
230-
// TODO: When we have tools that make it easy, model these properly to handle
231-
// `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
232-
// (since it allows us to at least capture the most common cases).
233-
exists(DataFlow::AttrRead read |
234-
this.getFunction() = read and
235-
read.getObject() = Request::instance() and
236-
read.getAttributeName() = "redirect"
237-
)
238-
}
214+
DataFlow::MethodCallNode {
215+
TwistedRequestRedirectCall() { this.calls(Request::instance(), "redirect") }
239216

240217
override DataFlow::Node getBody() { none() }
241218

‎python/ql/src/semmle/python/frameworks/Werkzeug.qll‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -154,17 +154,11 @@ module Werkzeug {
154154
*/
155155
class HeadersAdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
156156
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
157-
// Methods
158-
//
159-
// TODO: When we have tools that make it easy, model these properly to handle
160-
// `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
161-
// (since it allows us to at least capture the most common cases).
157+
// normal (non-async) methods
162158
nodeFrom = instance() and
163-
exists(DataFlow::AttrRead attr | attr.getObject() = nodeFrom |
164-
// methods (non-async)
165-
attr.getAttributeName() in ["getlist", "get_all", "popitem", "to_wsgi_list"] and
166-
nodeTo.(DataFlow::CallCfgNode).getFunction() = attr
167-
)
159+
nodeTo
160+
.(DataFlow::MethodCallNode)
161+
.calls(nodeFrom, ["getlist", "get_all", "popitem", "to_wsgi_list"])
168162
}
169163
}
170164
}

‎python/ql/src/semmle/python/frameworks/Yarl.qll‎

Lines changed: 12 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -63,40 +63,23 @@ module Yarl {
6363
nodeTo = call
6464
)
6565
or
66-
// Methods
67-
//
68-
// TODO: When we have tools that make it easy, model these properly to handle
69-
// `meth = obj.meth; meth()`. Until then, we'll use this more syntactic approach
70-
// (since it allows us to at least capture the most common cases).
71-
exists(DataFlow::AttrRead attr |
72-
// methods (that replaces part of URL, taken as only arguments)
73-
attr.getAttributeName() in [
66+
// normal (non-async) methods
67+
nodeFrom = instance() and
68+
nodeTo.(DataFlow::MethodCallNode).calls(nodeFrom, ["human_repr"])
69+
or
70+
// methods that give an altered URL. taint both from object, and form argument
71+
// (to result of call)
72+
exists(DataFlow::MethodCallNode call |
73+
call.calls(instance(),
74+
[
7475
"with_scheme", "with_user", "with_password", "with_host", "with_port", "with_path",
7576
"with_query", "with_query", "update_query", "update_query", "with_fragment",
7677
"with_name",
7778
// join is a bit different, but is still correct to add here :+1:
7879
"join"
79-
] and
80-
(
81-
// obj -> obj.meth()
82-
nodeFrom = instance() and
83-
attr.getObject() = nodeFrom and
84-
nodeTo.(DataFlow::CallCfgNode).getFunction() = attr
85-
or
86-
// argument of obj.meth() -> obj.meth()
87-
attr.getObject() = instance() and
88-
nodeTo.(DataFlow::CallCfgNode).getFunction() = attr and
89-
nodeFrom in [
90-
nodeTo.(DataFlow::CallCfgNode).getArg(_),
91-
nodeTo.(DataFlow::CallCfgNode).getArgByName(_)
92-
]
93-
)
94-
or
95-
// other methods
96-
nodeFrom = instance() and
97-
attr.getObject() = nodeFrom and
98-
attr.getAttributeName() in ["human_repr"] and
99-
nodeTo.(DataFlow::CallCfgNode).getFunction() = attr
80+
]) and
81+
nodeTo = call and
82+
nodeFrom in [call.getObject(), call.getArg(_), call.getArgByName(_)]
10083
)
10184
or
10285
// Attributes

0 commit comments

Comments
 (0)