Skip to content

Commit 178c54e

Browse files
codeql-cicodeql-ci
authored
Merge pull request github#5139 from RasmusWL/django-improvements
Approved by yoff
2 parents 77af7ed + 1651f81 commit 178c54e

File tree

6 files changed

+260
-95
lines changed

6 files changed

+260
-95
lines changed

‎python/change-notes/2021-02-10-django-improvements.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
lgtm,codescanning
2+
* Improved modeling of `django` to recognize request handlers functions that are decorated (for example with `django.views.decorators.http.require_GET`). This leads to more sources of remote user input (`RemoteFlowSource`), since we correctly identify the first parameter as being passed a django request.
3+
* Improved modeling of django View classes. We now consider any class using in a routing setup with `<class>.as_view()` as django view class. This leads to more sources of remote user input (`RemoteFlowSource`), since we correctly identify the first parameter as being passed a django request.
4+
* Improved modeling of `django`, so for View classes we now model `self.request`, `self.args`, and `self.kwargs` as sources of remote user input (`RemoteFlowSource`).

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

Lines changed: 127 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1975,6 +1975,14 @@ private module Django {
19751975
}
19761976
}
19771977

1978+
/**
1979+
* Gets the last decorator call for the function `func`, if `func` has decorators.
1980+
*/
1981+
private Expr lastDecoratorCall(Function func) {
1982+
result = func.getDefinition().(FunctionExpr).getADecoratorCall() and
1983+
not exists(Call other_decorator | other_decorator.getArg(0) = result)
1984+
}
1985+
19781986
// ---------------------------------------------------------------------------
19791987
// routing modeling
19801988
// ---------------------------------------------------------------------------
@@ -1987,7 +1995,18 @@ private module Django {
19871995
*/
19881996
private DataFlow::Node djangoRouteHandlerFunctionTracker(DataFlow::TypeTracker t, Function func) {
19891997
t.start() and
1990-
result = DataFlow::exprNode(func.getDefinition())
1998+
(
1999+
not exists(func.getADecorator()) and
2000+
result.asExpr() = func.getDefinition()
2001+
or
2002+
// If the function has decorators, we still want to model the function as being
2003+
// the request handler for a route setup. In such situations, we must track the
2004+
// last decorator call instead of the function itself.
2005+
//
2006+
// Note that this means that we blindly ignore what the decorator actually does to
2007+
// the function, which seems like an OK tradeoff.
2008+
result.asExpr() = lastDecoratorCall(func)
2009+
)
19912010
or
19922011
exists(DataFlow::TypeTracker t2 |
19932012
result = djangoRouteHandlerFunctionTracker(t2, func).track(t2, t)
@@ -2005,18 +2024,15 @@ private module Django {
20052024
result = djangoRouteHandlerFunctionTracker(DataFlow::TypeTracker::end(), func)
20062025
}
20072026

2008-
/** A django View class defined in project code. */
2009-
class DjangoViewClassDef extends Class {
2010-
DjangoViewClassDef() { this.getABase() = django::views::generic::View::subclassRef().asExpr() }
2011-
2012-
/** Gets a function that could handle incoming requests, if any. */
2013-
DjangoRouteHandler getARequestHandler() {
2014-
// TODO: This doesn't handle attribute assignment. Should be OK, but analysis is not as complete as with
2015-
// points-to and `.lookup`, which would handle `post = my_post_handler` inside class def
2016-
result = this.getAMethod() and
2017-
result.getName() = HTTP::httpVerbLower()
2018-
}
2019-
2027+
/**
2028+
* In order to recognize a class as being a django view class, based on the `as_view`
2029+
* call, we need to be able to track such calls on _any_ class. This is provided by
2030+
* the member predicates of this QL class.
2031+
*
2032+
* As such, a Python class being part of `DjangoViewClassHelper` doesn't signify that
2033+
* we model it as a django view class.
2034+
*/
2035+
class DjangoViewClassHelper extends Class {
20202036
/** Gets a reference to this class. */
20212037
private DataFlow::Node getARef(DataFlow::TypeTracker t) {
20222038
t.start() and
@@ -2051,15 +2067,69 @@ private module Django {
20512067
DataFlow::Node asViewResult() { result = asViewResult(DataFlow::TypeTracker::end()) }
20522068
}
20532069

2070+
/** A class that we consider a django View class. */
2071+
abstract class DjangoViewClass extends DjangoViewClassHelper {
2072+
/** Gets a function that could handle incoming requests, if any. */
2073+
Function getARequestHandler() {
2074+
// TODO: This doesn't handle attribute assignment. Should be OK, but analysis is not as complete as with
2075+
// points-to and `.lookup`, which would handle `post = my_post_handler` inside class def
2076+
result = this.getAMethod() and
2077+
result.getName() = HTTP::httpVerbLower()
2078+
}
2079+
2080+
/**
2081+
* Gets a reference to instances of this class, originating from a self parameter of
2082+
* a method defined on this class.
2083+
*
2084+
* Note: TODO: This doesn't take MRO into account
2085+
* Note: TODO: This doesn't take staticmethod/classmethod into account
2086+
*/
2087+
private DataFlow::Node getASelfRef(DataFlow::TypeTracker t) {
2088+
t.start() and
2089+
result.(DataFlow::ParameterNode).getParameter() = this.getAMethod().getArg(0)
2090+
or
2091+
exists(DataFlow::TypeTracker t2 | result = this.getASelfRef(t2).track(t2, t))
2092+
}
2093+
2094+
/**
2095+
* Gets a reference to instances of this class, originating from a self parameter of
2096+
* a method defined on this class.
2097+
*
2098+
* Note: TODO: This doesn't take MRO into account
2099+
* Note: TODO: This doesn't take staticmethod/classmethod into account
2100+
*/
2101+
DataFlow::Node getASelfRef() { result = this.getASelfRef(DataFlow::TypeTracker::end()) }
2102+
}
2103+
2104+
/**
2105+
* A class that is used in a route-setup, with `<class>.as_view()`, therefore being
2106+
* considered a django View class.
2107+
*/
2108+
class DjangoViewClassFromRouteSetup extends DjangoViewClass {
2109+
DjangoViewClassFromRouteSetup() {
2110+
exists(DjangoRouteSetup setup | setup.getViewArg() = this.asViewResult())
2111+
}
2112+
}
2113+
2114+
/**
2115+
* A class that has a super-type which is a django View class, therefore also
2116+
* becoming a django View class.
2117+
*/
2118+
class DjangoViewClassFromSuperClass extends DjangoViewClass {
2119+
DjangoViewClassFromSuperClass() {
2120+
this.getABase() = django::views::generic::View::subclassRef().asExpr()
2121+
}
2122+
}
2123+
20542124
/**
20552125
* A function that is a django route handler, meaning it handles incoming requests
20562126
* with the django framework.
20572127
*/
20582128
private class DjangoRouteHandler extends Function {
20592129
DjangoRouteHandler() {
2060-
exists(djangoRouteHandlerFunctionTracker(this))
2130+
exists(DjangoRouteSetup route | route.getViewArg() = djangoRouteHandlerFunctionTracker(this))
20612131
or
2062-
any(DjangoViewClassDef vc).getARequestHandler() = this
2132+
any(DjangoViewClass vc).getARequestHandler() = this
20632133
}
20642134

20652135
/** Gets the index of the request parameter. */
@@ -2083,7 +2153,7 @@ private module Django {
20832153
final override DjangoRouteHandler getARequestHandler() {
20842154
djangoRouteHandlerFunctionTracker(result) = getViewArg()
20852155
or
2086-
exists(DjangoViewClassDef vc |
2156+
exists(DjangoViewClass vc |
20872157
getViewArg() = vc.asViewResult() and
20882158
result = vc.getARequestHandler()
20892159
)
@@ -2094,7 +2164,7 @@ private module Django {
20942164
private class DjangoViewClassHandlerWithoutKnownRoute extends HTTP::Server::RequestHandler::Range,
20952165
DjangoRouteHandler {
20962166
DjangoViewClassHandlerWithoutKnownRoute() {
2097-
exists(DjangoViewClassDef vc | vc.getARequestHandler() = this) and
2167+
exists(DjangoViewClass vc | vc.getARequestHandler() = this) and
20982168
not exists(DjangoRouteSetup setup | setup.getARequestHandler() = this)
20992169
}
21002170

@@ -2267,6 +2337,46 @@ private module Django {
22672337
override string getSourceType() { result = "django.http.request.HttpRequest" }
22682338
}
22692339

2340+
/**
2341+
* A read of the `request` attribute on a reference to an instance of a View class,
2342+
* which is the request being processed currently.
2343+
*
2344+
* See https://docs.djangoproject.com/en/3.1/topics/class-based-views/generic-display/#dynamic-filtering
2345+
*/
2346+
private class DjangoViewClassRequestAttributeRead extends django::http::request::HttpRequest::InstanceSource,
2347+
RemoteFlowSource::Range, DataFlow::Node {
2348+
DjangoViewClassRequestAttributeRead() {
2349+
exists(DataFlow::AttrRead read | this = read |
2350+
read.getObject() = any(DjangoViewClass vc).getASelfRef() and
2351+
read.getAttributeName() = "request"
2352+
)
2353+
}
2354+
2355+
override string getSourceType() {
2356+
result = "django.http.request.HttpRequest (attribute on self in View class)"
2357+
}
2358+
}
2359+
2360+
/**
2361+
* A read of the `args` or `kwargs` attribute on a reference to an instance of a View class,
2362+
* which contains the routed parameters captured from the URL route.
2363+
*
2364+
* See https://docs.djangoproject.com/en/3.1/topics/class-based-views/generic-display/#dynamic-filtering
2365+
*/
2366+
private class DjangoViewClassRoutedParamsAttributeRead extends RemoteFlowSource::Range,
2367+
DataFlow::Node {
2368+
DjangoViewClassRoutedParamsAttributeRead() {
2369+
exists(DataFlow::AttrRead read | this = read |
2370+
read.getObject() = any(DjangoViewClass vc).getASelfRef() and
2371+
read.getAttributeName() in ["args", "kwargs"]
2372+
)
2373+
}
2374+
2375+
override string getSourceType() {
2376+
result = "django routed param from attribute on self in View class"
2377+
}
2378+
}
2379+
22702380
private class DjangoHttpRequstAdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
22712381
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
22722382
nodeFrom = django::http::request::HttpRequest::instance() and

‎python/ql/test/experimental/library-tests/frameworks/django-v2-v3/TestTaint.expected

Lines changed: 83 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -1,82 +1,88 @@
1-
| taint_test.py:7 | ok | test_taint | bar |
2-
| taint_test.py:7 | ok | test_taint | foo |
3-
| taint_test.py:8 | ok | test_taint | baz |
4-
| taint_test.py:14 | ok | test_taint | request |
5-
| taint_test.py:16 | ok | test_taint | request.body |
6-
| taint_test.py:17 | ok | test_taint | request.path |
7-
| taint_test.py:18 | ok | test_taint | request.path_info |
8-
| taint_test.py:22 | ok | test_taint | request.method |
9-
| taint_test.py:24 | ok | test_taint | request.encoding |
10-
| taint_test.py:25 | ok | test_taint | request.content_type |
11-
| taint_test.py:28 | ok | test_taint | request.content_params |
12-
| taint_test.py:29 | ok | test_taint | request.content_params["key"] |
13-
| taint_test.py:30 | ok | test_taint | request.content_params.get(..) |
14-
| taint_test.py:34 | ok | test_taint | request.GET |
15-
| taint_test.py:35 | ok | test_taint | request.GET["key"] |
16-
| taint_test.py:36 | ok | test_taint | request.GET.get(..) |
17-
| taint_test.py:37 | fail | test_taint | request.GET.getlist(..) |
18-
| taint_test.py:38 | fail | test_taint | request.GET.getlist(..)[0] |
19-
| taint_test.py:39 | ok | test_taint | request.GET.pop(..) |
20-
| taint_test.py:40 | ok | test_taint | request.GET.pop(..)[0] |
21-
| taint_test.py:41 | ok | test_taint | request.GET.popitem()[0] |
22-
| taint_test.py:42 | ok | test_taint | request.GET.popitem()[1] |
23-
| taint_test.py:43 | ok | test_taint | request.GET.popitem()[1][0] |
24-
| taint_test.py:44 | fail | test_taint | request.GET.dict() |
25-
| taint_test.py:45 | fail | test_taint | request.GET.dict()["key"] |
26-
| taint_test.py:46 | fail | test_taint | request.GET.urlencode() |
27-
| taint_test.py:49 | ok | test_taint | request.POST |
28-
| taint_test.py:52 | ok | test_taint | request.COOKIES |
29-
| taint_test.py:53 | ok | test_taint | request.COOKIES["key"] |
30-
| taint_test.py:54 | ok | test_taint | request.COOKIES.get(..) |
31-
| taint_test.py:57 | ok | test_taint | request.FILES |
32-
| taint_test.py:58 | ok | test_taint | request.FILES["key"] |
33-
| taint_test.py:59 | fail | test_taint | request.FILES["key"].content_type |
34-
| taint_test.py:60 | fail | test_taint | request.FILES["key"].content_type_extra |
35-
| taint_test.py:61 | fail | test_taint | request.FILES["key"].content_type_extra["key"] |
36-
| taint_test.py:62 | fail | test_taint | request.FILES["key"].charset |
37-
| taint_test.py:63 | fail | test_taint | request.FILES["key"].name |
38-
| taint_test.py:64 | fail | test_taint | request.FILES["key"].file |
39-
| taint_test.py:65 | fail | test_taint | request.FILES["key"].file.read() |
40-
| taint_test.py:67 | ok | test_taint | request.FILES.get(..) |
41-
| taint_test.py:68 | fail | test_taint | request.FILES.get(..).name |
42-
| taint_test.py:69 | fail | test_taint | request.FILES.getlist(..) |
43-
| taint_test.py:70 | fail | test_taint | request.FILES.getlist(..)[0] |
44-
| taint_test.py:71 | fail | test_taint | request.FILES.getlist(..)[0].name |
45-
| taint_test.py:72 | fail | test_taint | request.FILES.dict() |
46-
| taint_test.py:73 | fail | test_taint | request.FILES.dict()["key"] |
47-
| taint_test.py:74 | fail | test_taint | request.FILES.dict()["key"].name |
48-
| taint_test.py:77 | ok | test_taint | request.META |
49-
| taint_test.py:78 | ok | test_taint | request.META["HTTP_USER_AGENT"] |
50-
| taint_test.py:79 | ok | test_taint | request.META.get(..) |
51-
| taint_test.py:82 | ok | test_taint | request.headers |
52-
| taint_test.py:83 | ok | test_taint | request.headers["user-agent"] |
53-
| taint_test.py:84 | ok | test_taint | request.headers["USER_AGENT"] |
54-
| taint_test.py:87 | ok | test_taint | request.resolver_match |
55-
| taint_test.py:88 | fail | test_taint | request.resolver_match.args |
56-
| taint_test.py:89 | fail | test_taint | request.resolver_match.args[0] |
57-
| taint_test.py:90 | fail | test_taint | request.resolver_match.kwargs |
58-
| taint_test.py:91 | fail | test_taint | request.resolver_match.kwargs["key"] |
59-
| taint_test.py:93 | fail | test_taint | request.get_full_path() |
60-
| taint_test.py:94 | fail | test_taint | request.get_full_path_info() |
61-
| taint_test.py:98 | fail | test_taint | request.read() |
62-
| taint_test.py:99 | fail | test_taint | request.readline() |
63-
| taint_test.py:100 | fail | test_taint | request.readlines() |
64-
| taint_test.py:101 | fail | test_taint | request.readlines()[0] |
65-
| taint_test.py:102 | fail | test_taint | ListComp |
66-
| taint_test.py:108 | ok | test_taint | args |
67-
| taint_test.py:109 | ok | test_taint | args[0] |
68-
| taint_test.py:110 | ok | test_taint | kwargs |
69-
| taint_test.py:111 | ok | test_taint | kwargs["key"] |
70-
| taint_test.py:115 | ok | test_taint | request.current_app |
71-
| taint_test.py:120 | ok | test_taint | request.get_host() |
72-
| taint_test.py:121 | ok | test_taint | request.get_port() |
73-
| taint_test.py:128 | fail | test_taint | request.build_absolute_uri() |
74-
| taint_test.py:129 | fail | test_taint | request.build_absolute_uri(..) |
1+
| taint_test.py:8 | ok | test_taint | bar |
2+
| taint_test.py:8 | ok | test_taint | foo |
3+
| taint_test.py:9 | ok | test_taint | baz |
4+
| taint_test.py:15 | ok | test_taint | request |
5+
| taint_test.py:17 | ok | test_taint | request.body |
6+
| taint_test.py:18 | ok | test_taint | request.path |
7+
| taint_test.py:19 | ok | test_taint | request.path_info |
8+
| taint_test.py:23 | ok | test_taint | request.method |
9+
| taint_test.py:25 | ok | test_taint | request.encoding |
10+
| taint_test.py:26 | ok | test_taint | request.content_type |
11+
| taint_test.py:29 | ok | test_taint | request.content_params |
12+
| taint_test.py:30 | ok | test_taint | request.content_params["key"] |
13+
| taint_test.py:31 | ok | test_taint | request.content_params.get(..) |
14+
| taint_test.py:35 | ok | test_taint | request.GET |
15+
| taint_test.py:36 | ok | test_taint | request.GET["key"] |
16+
| taint_test.py:37 | ok | test_taint | request.GET.get(..) |
17+
| taint_test.py:38 | fail | test_taint | request.GET.getlist(..) |
18+
| taint_test.py:39 | fail | test_taint | request.GET.getlist(..)[0] |
19+
| taint_test.py:40 | ok | test_taint | request.GET.pop(..) |
20+
| taint_test.py:41 | ok | test_taint | request.GET.pop(..)[0] |
21+
| taint_test.py:42 | ok | test_taint | request.GET.popitem()[0] |
22+
| taint_test.py:43 | ok | test_taint | request.GET.popitem()[1] |
23+
| taint_test.py:44 | ok | test_taint | request.GET.popitem()[1][0] |
24+
| taint_test.py:45 | fail | test_taint | request.GET.dict() |
25+
| taint_test.py:46 | fail | test_taint | request.GET.dict()["key"] |
26+
| taint_test.py:47 | fail | test_taint | request.GET.urlencode() |
27+
| taint_test.py:50 | ok | test_taint | request.POST |
28+
| taint_test.py:53 | ok | test_taint | request.COOKIES |
29+
| taint_test.py:54 | ok | test_taint | request.COOKIES["key"] |
30+
| taint_test.py:55 | ok | test_taint | request.COOKIES.get(..) |
31+
| taint_test.py:58 | ok | test_taint | request.FILES |
32+
| taint_test.py:59 | ok | test_taint | request.FILES["key"] |
33+
| taint_test.py:60 | fail | test_taint | request.FILES["key"].content_type |
34+
| taint_test.py:61 | fail | test_taint | request.FILES["key"].content_type_extra |
35+
| taint_test.py:62 | fail | test_taint | request.FILES["key"].content_type_extra["key"] |
36+
| taint_test.py:63 | fail | test_taint | request.FILES["key"].charset |
37+
| taint_test.py:64 | fail | test_taint | request.FILES["key"].name |
38+
| taint_test.py:65 | fail | test_taint | request.FILES["key"].file |
39+
| taint_test.py:66 | fail | test_taint | request.FILES["key"].file.read() |
40+
| taint_test.py:68 | ok | test_taint | request.FILES.get(..) |
41+
| taint_test.py:69 | fail | test_taint | request.FILES.get(..).name |
42+
| taint_test.py:70 | fail | test_taint | request.FILES.getlist(..) |
43+
| taint_test.py:71 | fail | test_taint | request.FILES.getlist(..)[0] |
44+
| taint_test.py:72 | fail | test_taint | request.FILES.getlist(..)[0].name |
45+
| taint_test.py:73 | fail | test_taint | request.FILES.dict() |
46+
| taint_test.py:74 | fail | test_taint | request.FILES.dict()["key"] |
47+
| taint_test.py:75 | fail | test_taint | request.FILES.dict()["key"].name |
48+
| taint_test.py:78 | ok | test_taint | request.META |
49+
| taint_test.py:79 | ok | test_taint | request.META["HTTP_USER_AGENT"] |
50+
| taint_test.py:80 | ok | test_taint | request.META.get(..) |
51+
| taint_test.py:83 | ok | test_taint | request.headers |
52+
| taint_test.py:84 | ok | test_taint | request.headers["user-agent"] |
53+
| taint_test.py:85 | ok | test_taint | request.headers["USER_AGENT"] |
54+
| taint_test.py:88 | ok | test_taint | request.resolver_match |
55+
| taint_test.py:89 | fail | test_taint | request.resolver_match.args |
56+
| taint_test.py:90 | fail | test_taint | request.resolver_match.args[0] |
57+
| taint_test.py:91 | fail | test_taint | request.resolver_match.kwargs |
58+
| taint_test.py:92 | fail | test_taint | request.resolver_match.kwargs["key"] |
59+
| taint_test.py:94 | fail | test_taint | request.get_full_path() |
60+
| taint_test.py:95 | fail | test_taint | request.get_full_path_info() |
61+
| taint_test.py:99 | fail | test_taint | request.read() |
62+
| taint_test.py:100 | fail | test_taint | request.readline() |
63+
| taint_test.py:101 | fail | test_taint | request.readlines() |
64+
| taint_test.py:102 | fail | test_taint | request.readlines()[0] |
65+
| taint_test.py:103 | fail | test_taint | ListComp |
66+
| taint_test.py:109 | ok | test_taint | args |
67+
| taint_test.py:110 | ok | test_taint | args[0] |
68+
| taint_test.py:111 | ok | test_taint | kwargs |
69+
| taint_test.py:112 | ok | test_taint | kwargs["key"] |
70+
| taint_test.py:116 | ok | test_taint | request.current_app |
71+
| taint_test.py:121 | ok | test_taint | request.get_host() |
72+
| taint_test.py:122 | ok | test_taint | request.get_port() |
73+
| taint_test.py:129 | fail | test_taint | request.build_absolute_uri() |
7574
| taint_test.py:130 | fail | test_taint | request.build_absolute_uri(..) |
76-
| taint_test.py:133 | ok | test_taint | request.build_absolute_uri(..) |
75+
| taint_test.py:131 | fail | test_taint | request.build_absolute_uri(..) |
7776
| taint_test.py:134 | ok | test_taint | request.build_absolute_uri(..) |
78-
| taint_test.py:142 | ok | test_taint | request.get_signed_cookie(..) |
77+
| taint_test.py:135 | ok | test_taint | request.build_absolute_uri(..) |
7978
| taint_test.py:143 | ok | test_taint | request.get_signed_cookie(..) |
8079
| taint_test.py:144 | ok | test_taint | request.get_signed_cookie(..) |
81-
| taint_test.py:148 | fail | test_taint | request.get_signed_cookie(..) |
80+
| taint_test.py:145 | ok | test_taint | request.get_signed_cookie(..) |
8281
| taint_test.py:149 | fail | test_taint | request.get_signed_cookie(..) |
82+
| taint_test.py:150 | fail | test_taint | request.get_signed_cookie(..) |
83+
| taint_test.py:157 | ok | some_method | self.request |
84+
| taint_test.py:158 | ok | some_method | self.request.GET["key"] |
85+
| taint_test.py:160 | ok | some_method | self.args |
86+
| taint_test.py:161 | ok | some_method | self.args[0] |
87+
| taint_test.py:163 | ok | some_method | self.kwargs |
88+
| taint_test.py:164 | ok | some_method | self.kwargs["key"] |

0 commit comments

Comments
 (0)