Skip to content

Commit 5a0babe

Browse files
committed
Python: Add support for Django 2.x and 3.x
I changed the django mock to support both 1.x and 2.x routing APIs, which is not really a nice long term solution.
1 parent adec76d commit 5a0babe

File tree

14 files changed

+307
-21
lines changed

14 files changed

+307
-21
lines changed

python/ql/src/semmle/python/web/django/General.qll

Lines changed: 90 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,38 +2,111 @@ import python
22
import semmle.python.regex
33
import semmle.python.web.Http
44

5-
predicate django_route(CallNode call, ControlFlowNode regex, FunctionValue view) {
6-
exists(FunctionValue url |
7-
Value::named("django.conf.urls.url") = url and
8-
url.getArgumentForCall(call, 0) = regex and
9-
url.getArgumentForCall(call, 1).pointsTo(view)
5+
// TODO: Since django uses `path = partial(...)`, our analysis doesn't understand this is
6+
// a FunctionValue, so we can't use `FunctionValue.getArgumentForCall`
7+
// https://github.com/django/django/blob/master/django/urls/conf.py#L76
8+
9+
private predicate django_regex_route(CallNode call, ControlFlowNode regex, FunctionValue view) {
10+
exists(Value route_maker |
11+
(
12+
// Django 1.x
13+
Value::named("django.conf.urls.url") = route_maker and
14+
route_maker.(FunctionValue).getArgumentForCall(call, 0) = regex and
15+
route_maker.(FunctionValue).getArgumentForCall(call, 1).pointsTo(view)
16+
)
17+
or
18+
(
19+
// Django 2.x and 3.x: https://docs.djangoproject.com/en/3.0/ref/urls/#re-path
20+
Value::named("django.urls.re_path") = route_maker and
21+
route_maker.getACall() = call and
22+
(
23+
call.getArg(0) = regex
24+
or
25+
call.getArgByName("route") = regex
26+
27+
) and
28+
(
29+
call.getArg(1).pointsTo(view)
30+
or
31+
call.getArgByName("view").pointsTo(view)
32+
)
33+
)
34+
)
35+
}
36+
37+
private predicate django_path_route(CallNode call, ControlFlowNode route, FunctionValue view) {
38+
exists(Value route_maker |
39+
// Django 2.x and 3.x: https://docs.djangoproject.com/en/3.0/ref/urls/#path
40+
Value::named("django.urls.path") = route_maker and
41+
route_maker.getACall() = call and
42+
(
43+
call.getArg(0) = route
44+
or
45+
call.getArgByName("route") = route
46+
47+
) and
48+
(
49+
call.getArg(1).pointsTo(view)
50+
or
51+
call.getArgByName("view").pointsTo(view)
52+
)
1053
)
1154
}
1255

1356
class DjangoRouteRegex extends RegexString {
14-
DjangoRouteRegex() { django_route(_, this.getAFlowNode(), _) }
57+
DjangoRouteRegex() { django_regex_route(_, this.getAFlowNode(), _) }
1558
}
1659

17-
class DjangoRoute extends CallNode {
18-
DjangoRoute() { django_route(this, _, _) }
60+
abstract class DjangoRoute extends CallNode {
61+
62+
abstract FunctionValue getViewFunction();
1963

20-
FunctionValue getViewFunction() { django_route(this, _, result) }
64+
abstract string getANamedArgument();
65+
66+
/**
67+
* Get the number of positional arguments that will be passed to the view.
68+
* Will only return a result if there are no named arguments.
69+
*/
70+
abstract int getNumPositionalArguments();
71+
}
2172

22-
string getANamedArgument() {
73+
class DjangoPathRoute extends DjangoRoute {
74+
75+
DjangoPathRoute() { django_path_route(this, _, _) }
76+
77+
override FunctionValue getViewFunction() { django_path_route(this, _, result) }
78+
79+
override string getANamedArgument() {
80+
// regexp taken from django:
81+
// https://github.com/django/django/blob/7d1bf29977bb368d7c28e7c6eb146db3b3009ae7/django/urls/resolvers.py#L199
82+
exists(StrConst route, string match |
83+
django_path_route(this, route.getAFlowNode(), _) and
84+
match = route.getText().regexpFind("<(?:(?<converter>[^>:]+):)?(?<parameter>\\w+)>", _, _) and
85+
result = match.regexpCapture("<(?:(?<converter>[^>:]+):)?(?<parameter>\\w+)>", 2)
86+
)
87+
}
88+
89+
override int getNumPositionalArguments() {
90+
none()
91+
}
92+
}
93+
94+
class DjangoRegexRoute extends DjangoRoute {
95+
DjangoRegexRoute() { django_regex_route(this, _, _) }
96+
97+
override FunctionValue getViewFunction() { django_regex_route(this, _, result) }
98+
99+
override string getANamedArgument() {
23100
exists(DjangoRouteRegex regex |
24-
django_route(this, regex.getAFlowNode(), _) and
101+
django_regex_route(this, regex.getAFlowNode(), _) and
25102
regex.getGroupName(_, _) = result
26103
)
27104
}
28105

29-
/**
30-
* Get the number of positional arguments that will be passed to the view.
31-
* Will only return a result if there are no named arguments.
32-
*/
33-
int getNumPositionalArguments() {
106+
override int getNumPositionalArguments() {
34107
exists(DjangoRouteRegex regex |
35108
not exists(this.getANamedArgument()) and
36-
django_route(this, regex.getAFlowNode(), _) and
109+
django_regex_route(this, regex.getAFlowNode(), _) and
37110
result = count(regex.getGroupNumber(_, _))
38111
)
39112
}

python/ql/src/semmle/python/web/django/Request.qll

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,11 +48,12 @@ abstract class DjangoRequestSource extends HttpRequestTaintSource {
4848
/**
4949
* Function based views
5050
* https://docs.djangoproject.com/en/1.11/topics/http/views/
51+
* https://docs.djangoproject.com/en/3.0/topics/http/views/
5152
*/
5253
private class DjangoFunctionBasedViewRequestArgument extends DjangoRequestSource {
5354
DjangoFunctionBasedViewRequestArgument() {
54-
exists(FunctionValue view |
55-
django_route(_, _, view) and
55+
exists(DjangoRoute route, FunctionValue view |
56+
route.getViewFunction() = view and
5657
this = view.getScope().getArg(0).asName().getAFlowNode()
5758
)
5859
}
@@ -61,9 +62,14 @@ private class DjangoFunctionBasedViewRequestArgument extends DjangoRequestSource
6162
/**
6263
* Class based views
6364
* https://docs.djangoproject.com/en/1.11/topics/class-based-views/
65+
* https://docs.djangoproject.com/en/3.0/topics/class-based-views/
6466
*/
6567
private class DjangoView extends ClassValue {
66-
DjangoView() { Value::named("django.views.generic.View") = this.getASuperType() }
68+
DjangoView() {
69+
Value::named("django.views.generic.View") = this.getASuperType()
70+
or
71+
Value::named("django.views.View") = this.getASuperType()
72+
}
6773
}
6874

6975
private FunctionValue djangoViewHttpMethod() {

python/ql/src/semmle/python/web/django/Response.qll

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,15 @@ class DjangoResponse extends TaintKind {
1414
}
1515

1616
private ClassValue theDjangoHttpResponseClass() {
17-
result = Value::named("django.http.response.HttpResponse") and
17+
(
18+
// version 1.x
19+
result = Value::named("django.http.response.HttpResponse")
20+
or
21+
// version 2.x
22+
// https://docs.djangoproject.com/en/2.2/ref/request-response/#httpresponse-objects
23+
result = Value::named("django.http.HttpResponse")
24+
) and
25+
// TODO: does this do anything? when could they be the same???
1826
not result = theDjangoHttpRedirectClass()
1927
}
2028

python/ql/src/semmle/python/web/django/Shared.qll

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,9 @@ import python
44
FunctionValue redirect() { result = Value::named("django.shortcuts.redirect") }
55

66
ClassValue theDjangoHttpRedirectClass() {
7+
// version 1.x
78
result = Value::named("django.http.response.HttpResponseRedirectBase")
9+
or
10+
// version 2.x
11+
result = Value::named("django.http.HttpResponseRedirectBase")
812
}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
| test_1x.py:13:21:13:24 | django.redirect | externally controlled string |
2+
| test_2x_3x.py:13:21:13:24 | django.redirect | externally controlled string |

python/ql/test/library-tests/web/django/HttpResponseSinks.expected

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,3 +8,16 @@
88
| views_1x.py:45:25:45:70 | django.Response(...) | externally controlled string |
99
| views_1x.py:66:25:66:55 | django.Response(...) | externally controlled string |
1010
| views_1x.py:75:25:75:33 | django.Response(...) | externally controlled string |
11+
| views_2x_3x.py:8:25:8:63 | django.Response(...) | externally controlled string |
12+
| views_2x_3x.py:12:25:12:52 | django.Response(...) | externally controlled string |
13+
| views_2x_3x.py:16:25:16:53 | django.Response(...) | externally controlled string |
14+
| views_2x_3x.py:21:15:21:42 | django.Response.write(...) | externally controlled string |
15+
| views_2x_3x.py:30:29:30:60 | django.Response(...) | externally controlled string |
16+
| views_2x_3x.py:36:29:36:65 | django.Response(...) | externally controlled string |
17+
| views_2x_3x.py:41:25:41:63 | django.Response(...) | externally controlled string |
18+
| views_2x_3x.py:45:25:45:70 | django.Response(...) | externally controlled string |
19+
| views_2x_3x.py:66:25:66:40 | django.Response(...) | externally controlled string |
20+
| views_2x_3x.py:79:25:79:61 | django.Response(...) | externally controlled string |
21+
| views_2x_3x.py:82:25:82:69 | django.Response(...) | externally controlled string |
22+
| views_2x_3x.py:85:25:85:64 | django.Response(...) | externally controlled string |
23+
| views_2x_3x.py:88:25:88:32 | django.Response(...) | externally controlled string |

python/ql/test/library-tests/web/django/HttpSources.expected

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@
22
| test_1x.py:6:28:6:31 | path | externally controlled string |
33
| test_1x.py:12:19:12:25 | request | django.request.HttpRequest |
44
| test_1x.py:12:28:12:31 | path | externally controlled string |
5+
| test_2x_3x.py:6:19:6:25 | request | django.request.HttpRequest |
6+
| test_2x_3x.py:6:28:6:31 | path | externally controlled string |
7+
| test_2x_3x.py:12:19:12:25 | request | django.request.HttpRequest |
8+
| test_2x_3x.py:12:28:12:31 | path | externally controlled string |
59
| views_1x.py:7:19:7:25 | request | django.request.HttpRequest |
610
| views_1x.py:7:28:7:30 | foo | externally controlled string |
711
| views_1x.py:7:33:7:35 | bar | externally controlled string |
@@ -18,3 +22,26 @@
1822
| views_1x.py:65:15:65:21 | request | django.request.HttpRequest |
1923
| views_1x.py:65:24:65:31 | username | externally controlled string |
2024
| views_1x.py:74:13:74:19 | request | django.request.HttpRequest |
25+
| views_2x_3x.py:7:19:7:25 | request | django.request.HttpRequest |
26+
| views_2x_3x.py:7:28:7:30 | foo | externally controlled string |
27+
| views_2x_3x.py:7:33:7:35 | bar | externally controlled string |
28+
| views_2x_3x.py:11:20:11:26 | request | django.request.HttpRequest |
29+
| views_2x_3x.py:15:21:15:27 | request | django.request.HttpRequest |
30+
| views_2x_3x.py:19:21:19:27 | request | django.request.HttpRequest |
31+
| views_2x_3x.py:29:20:29:26 | request | django.request.HttpRequest |
32+
| views_2x_3x.py:35:19:35:25 | request | django.request.HttpRequest |
33+
| views_2x_3x.py:39:19:39:25 | request | django.request.HttpRequest |
34+
| views_2x_3x.py:39:28:39:38 | page_number | externally controlled string |
35+
| views_2x_3x.py:44:24:44:30 | request | django.request.HttpRequest |
36+
| views_2x_3x.py:44:33:44:36 | arg0 | externally controlled string |
37+
| views_2x_3x.py:44:39:44:42 | arg1 | externally controlled string |
38+
| views_2x_3x.py:78:17:78:23 | request | django.request.HttpRequest |
39+
| views_2x_3x.py:78:26:78:36 | page_number | externally controlled string |
40+
| views_2x_3x.py:81:17:81:23 | request | django.request.HttpRequest |
41+
| views_2x_3x.py:81:26:81:28 | foo | externally controlled string |
42+
| views_2x_3x.py:81:31:81:33 | bar | externally controlled string |
43+
| views_2x_3x.py:81:36:81:38 | baz | externally controlled string |
44+
| views_2x_3x.py:84:17:84:23 | request | django.request.HttpRequest |
45+
| views_2x_3x.py:84:26:84:28 | foo | externally controlled string |
46+
| views_2x_3x.py:84:31:84:33 | bar | externally controlled string |
47+
| views_2x_3x.py:87:26:87:32 | request | django.request.HttpRequest |
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
"""tests for Django 2.x and 3.x"""
2+
from django.urls import path
3+
from django.shortcuts import redirect, render
4+
5+
6+
def with_template(request, path='default'):
7+
env = {'path': path}
8+
# We would need to understand django templates to know if this is safe or not
9+
return render(request, 'possibly-vulnerable-template.html', env)
10+
11+
12+
def vuln_redirect(request, path):
13+
return redirect(path)
14+
15+
16+
urlpatterns = [
17+
path('/<path>', with_template),
18+
path('/redirect/<path>', vuln_redirect),
19+
]
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
"""testing views for Django 2.x and 3.x"""
2+
from django.urls import path, re_path
3+
from django.http import HttpResponse
4+
from django.views import View
5+
6+
7+
def url_match_xss(request, foo, bar, no_taint=None):
8+
return HttpResponse('url_match_xss: {} {}'.format(foo, bar))
9+
10+
11+
def get_params_xss(request):
12+
return HttpResponse(request.GET.get("untrusted"))
13+
14+
15+
def post_params_xss(request):
16+
return HttpResponse(request.POST.get("untrusted"))
17+
18+
19+
def http_resp_write(request):
20+
rsp = HttpResponse()
21+
rsp.write(request.GET.get("untrusted"))
22+
return rsp
23+
24+
25+
class Foo(object):
26+
# Note: since Foo is used as the super type in a class view, it will be able to handle requests.
27+
28+
# TODO: Currently we don't flag `untrusted` as a DjangoRequestParameter
29+
def post(self, request, untrusted):
30+
return HttpResponse('Foo post: {}'.format(untrusted))
31+
32+
33+
class ClassView(View, Foo):
34+
# TODO: Currently we don't flag `untrusted` as a DjangoRequestParameter
35+
def get(self, request, untrusted):
36+
return HttpResponse('ClassView get: {}'.format(untrusted))
37+
38+
39+
def show_articles(request, page_number=1):
40+
page_number = int(page_number)
41+
return HttpResponse('articles page: {}'.format(page_number))
42+
43+
44+
def xxs_positional_arg(request, arg0, arg1, no_taint=None):
45+
return HttpResponse('xxs_positional_arg: {} {}'.format(arg0, arg1))
46+
47+
48+
urlpatterns = [
49+
re_path(r'^url_match/(?P<foo>[^/]+)/(?P<bar>[^/]+)$', url_match_xss),
50+
re_path(r'^get_params$', get_params_xss),
51+
re_path(r'^post_params$', post_params_xss),
52+
re_path(r'^http_resp_write$', http_resp_write),
53+
re_path(r'^class_view/(?P<untrusted>.+)$', ClassView.as_view()),
54+
55+
# one pattern to support `articles/page-<n>` and ensuring that articles/ goes to page-1
56+
re_path(r'articles/^(?:page-(?P<page_number>\d+)/)?$', show_articles),
57+
# passing as positional argument is not the recommended way of doing things, but it is certainly
58+
# possible
59+
re_path(r'^([^/]+)/(?:foo|bar)/([^/]+)$', xxs_positional_arg, name='xxs_positional_arg'),
60+
]
61+
62+
63+
# Show we understand the keyword arguments to from django.urls.re_path
64+
65+
def re_path_kwargs(request):
66+
return HttpResponse('re_path_kwargs')
67+
68+
69+
urlpatterns = [
70+
re_path(view=re_path_kwargs, regex=r'^specifying-as-kwargs-is-not-a-problem$')
71+
]
72+
73+
################################################################################
74+
# Using path
75+
################################################################################
76+
77+
# saying page_number is an externally controlled *string* is a bit strange, when we have an int converter :O
78+
def page_number(request, page_number=1):
79+
return HttpResponse('page_number: {}'.format(page_number))
80+
81+
def foo_bar_baz(request, foo, bar, baz):
82+
return HttpResponse('foo_bar_baz: {} {} {}'.format(foo, bar, baz))
83+
84+
def path_kwargs(request, foo, bar):
85+
return HttpResponse('path_kwargs: {} {} {}'.format(foo, bar))
86+
87+
def not_valid_identifier(request):
88+
return HttpResponse('<foo!>')
89+
90+
urlpatterns = [
91+
path('articles/', page_number),
92+
path('articles/page-<int:page_number>', page_number),
93+
path('<int:foo>/<str:bar>/<baz>', foo_bar_baz, name='foo-bar-baz'),
94+
95+
path(view=path_kwargs, route='<foo>/<bar>'),
96+
97+
# We should not report there is a request parameter called `not_valid!`
98+
path('not_valid/<not_valid!>', not_valid_identifier),
99+
]
100+
101+
102+
################################################################################
103+
104+
105+
# We should abort if a decorator is used. As demonstrated below, anything might happen
106+
107+
# def reverse_kwargs(f):
108+
# @wraps(f)
109+
# def f_(*args, **kwargs):
110+
# new_kwargs = dict()
111+
# for key, value in kwargs.items():
112+
# new_kwargs[key[::-1]] = value
113+
# return f(*args, **new_kwargs)
114+
# return f_
115+
116+
# @reverse_kwargs
117+
# def decorators_can_do_anything(request, oof, foo=None):
118+
# return HttpResponse('This is a mess'[::-1])
119+
120+
# urlpatterns = [
121+
# path('rev/<foo>', decorators_can_do_anything),
122+
# ]

python/ql/test/query-tests/Security/lib/django/conf/urls.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,5 +2,6 @@
22
def url(regex, view, kwargs=None, name=None):
33
pass
44

5+
56
def patterns(*urls):
67
pass

0 commit comments

Comments
 (0)