From a9bbe1d0879e5fcee1e06af6f4809b464e48f026 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 21 Dec 2020 15:59:45 +0100 Subject: [PATCH 01/11] Python: Test Django un-routed class-based route handler --- .../library-tests/frameworks/django-v2-v3/routing_test.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/routing_test.py b/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/routing_test.py index 5fea4ccc046..4564ebd0aa9 100644 --- a/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/routing_test.py +++ b/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/routing_test.py @@ -107,3 +107,11 @@ def deprecated(request): # $routeHandler urlpatterns = [ url(r"^deprecated/", deprecated), # $routeSetup="^deprecated/" ] + + +class PossiblyNotRouted(View): + # Even if our analysis can't find a route-setup for this class, we should still + # consider it to be a handle incoming HTTP requests + + def get(self, request, possibly_not_routed=42): # $ MISSING: routeHandler routedParameter=possibly_not_routed + return HttpResponse('PossiblyNotRouted get: {}'.format(possibly_not_routed)) # $HttpResponse From 004ff38e229774309419a1fe81c7b3cd26095f9e Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 21 Dec 2020 17:20:19 +0100 Subject: [PATCH 02/11] Python: Add separate RequestHandler concept Since I really want to use our existing infrastructure to model that we can recognize something as a request handler without it having a route, we need this as a separate concept. All tests have been adjusted. The early modeling was based on flask, where all request-handling is based on handling requests from a specific route. But with the standard library handling and handlers without routes, the naming had to change. --- python/ql/src/semmle/python/Concepts.qll | 55 ++++++++++++++++++- .../src/semmle/python/frameworks/Django.qll | 18 +++--- .../ql/src/semmle/python/frameworks/Flask.qll | 10 ++-- .../frameworks/django-v1/routing_test.py | 20 +++---- .../frameworks/django-v2-v3/routing_test.py | 30 +++++----- .../frameworks/django-v2-v3/taint_test.py | 2 +- .../frameworks/django-v2-v3/testapp/views.py | 14 ++--- .../frameworks/flask/old_test.py | 20 +++---- .../frameworks/flask/response_test.py | 40 +++++++------- .../frameworks/flask/routing_test.py | 8 +-- .../frameworks/flask/taint_test.py | 16 +++--- .../frameworks/stdlib/http_server.py | 4 +- .../test/experimental/meta/ConceptsTest.qll | 17 +++--- 13 files changed, 152 insertions(+), 102 deletions(-) diff --git a/python/ql/src/semmle/python/Concepts.qll b/python/ql/src/semmle/python/Concepts.qll index 9b61d6a6fae..9a30ebf4871 100644 --- a/python/ql/src/semmle/python/Concepts.qll +++ b/python/ql/src/semmle/python/Concepts.qll @@ -314,7 +314,7 @@ module HTTP { string getUrlPattern() { result = range.getUrlPattern() } /** Gets a function that will handle incoming requests for this route, if any. */ - Function getARouteHandler() { result = range.getARouteHandler() } + Function getARequestHandler() { result = range.getARequestHandler() } /** * Gets a parameter that will receive parts of the url when handling incoming @@ -344,7 +344,7 @@ module HTTP { } /** Gets a function that will handle incoming requests for this route, if any. */ - abstract Function getARouteHandler(); + abstract Function getARequestHandler(); /** * Gets a parameter that will receive parts of the url when handling incoming @@ -354,8 +354,57 @@ module HTTP { } } + /** + * A function that will handle incoming HTTP requests. + * + * Extend this class to refine existing API models. If you want to model new APIs, + * extend `RequestHandler::Range` instead. + */ + class RequestHandler extends Function { + RequestHandler::Range range; + + RequestHandler() { this = range } + + /** + * Gets a parameter that could receive parts of the url when handling incoming + * requests, if any. These automatically become a `RemoteFlowSource`. + */ + Parameter getARoutedParameter() { result = range.getARoutedParameter() } + } + + /** Provides a class for modeling new HTTP request handlers. */ + module RequestHandler { + /** + * A function that will handle incoming HTTP requests. + * + * Extend this class to model new APIs. If you want to refine existing API models, + * extend `RequestHandler` instead. + * + * Only extend this class if you can't provide a `RouteSetup`, since we handle that case automatically. + */ + abstract class Range extends Function { + /** + * Gets a parameter that could receive parts of the url when handling incoming + * requests, if any. These automatically become a `RemoteFlowSource`. + */ + abstract Parameter getARoutedParameter(); + } + } + + private class RequestHandlerFromRouteSetup extends RequestHandler::Range { + RouteSetup rs; + + RequestHandlerFromRouteSetup() { this = rs.getARequestHandler() } + + override Parameter getARoutedParameter() { + result = rs.getARoutedParameter() and + result in [this.getArg(_), this.getArgByName(_)] + } + } + + /** A parameter that will receive parts of the url when handling an incoming request. */ private class RoutedParameter extends RemoteFlowSource::Range, DataFlow::ParameterNode { - RoutedParameter() { this.getParameter() = any(RouteSetup setup).getARoutedParameter() } + RoutedParameter() { this.getParameter() = any(RequestHandler setup).getARoutedParameter() } override string getSourceType() { result = "RoutedParameter" } } diff --git a/python/ql/src/semmle/python/frameworks/Django.qll b/python/ql/src/semmle/python/frameworks/Django.qll index c3c23f3f5df..f4cbe3d9ea5 100644 --- a/python/ql/src/semmle/python/frameworks/Django.qll +++ b/python/ql/src/semmle/python/frameworks/Django.qll @@ -1676,7 +1676,7 @@ private module Django { DjangoViewClassDef() { this.getABase() = django::views::generic::View::subclassRef().asExpr() } /** Gets a function that could handle incoming requests, if any. */ - DjangoRouteHandler getARouteHandler() { + DjangoRouteHandler getARequestHandler() { // TODO: This doesn't handle attribute assignment. Should be OK, but analysis is not as complete as with // points-to and `.lookup`, which would handle `post = my_post_handler` inside class def result = this.getAMethod() and @@ -1725,7 +1725,7 @@ private module Django { DjangoRouteHandler() { exists(djangoRouteHandlerFunctionTracker(this)) or - any(DjangoViewClassDef vc).getARouteHandler() = this + any(DjangoViewClassDef vc).getARequestHandler() = this } /** Gets the index of the request parameter. */ @@ -1746,12 +1746,12 @@ private module Django { /** Gets the data-flow node that is used as the argument for the view handler. */ abstract DataFlow::Node getViewArg(); - final override DjangoRouteHandler getARouteHandler() { + final override DjangoRouteHandler getARequestHandler() { djangoRouteHandlerFunctionTracker(result) = getViewArg() or exists(DjangoViewClassDef vc | getViewArg() = vc.asViewResult() and - result = vc.getARouteHandler() + result = vc.getARequestHandler() ) } } @@ -1787,14 +1787,14 @@ private module Django { // If we don't know the URL pattern, we simply mark all parameters as a routed // parameter. This should give us more RemoteFlowSources but could also lead to // more FPs. If this turns out to be the wrong tradeoff, we can always change our mind. - exists(DjangoRouteHandler routeHandler | routeHandler = this.getARouteHandler() | + exists(DjangoRouteHandler routeHandler | routeHandler = this.getARequestHandler() | not exists(this.getUrlPattern()) and result in [routeHandler.getArg(_), routeHandler.getArgByName(_)] and not result = any(int i | i <= routeHandler.getRequestParamIndex() | routeHandler.getArg(i)) ) or exists(string name | - result = this.getARouteHandler().getArgByName(name) and + result = this.getARequestHandler().getArgByName(name) and exists(string match | match = this.getUrlPattern().regexpFind(pathRoutedParameterRegex(), _, _) and name = match.regexpCapture(pathRoutedParameterRegex(), 2) @@ -1809,14 +1809,14 @@ private module Django { // If we don't know the URL pattern, we simply mark all parameters as a routed // parameter. This should give us more RemoteFlowSources but could also lead to // more FPs. If this turns out to be the wrong tradeoff, we can always change our mind. - exists(DjangoRouteHandler routeHandler | routeHandler = this.getARouteHandler() | + exists(DjangoRouteHandler routeHandler | routeHandler = this.getARequestHandler() | not exists(this.getUrlPattern()) and result in [routeHandler.getArg(_), routeHandler.getArgByName(_)] and not result = any(int i | i <= routeHandler.getRequestParamIndex() | routeHandler.getArg(i)) ) or exists(DjangoRouteHandler routeHandler, DjangoRouteRegex regex | - routeHandler = this.getARouteHandler() and + routeHandler = this.getARequestHandler() and regex.getRouteSetup() = this | // either using named capture groups (passed as keyword arguments) or using @@ -1891,7 +1891,7 @@ private module Django { class DjangoRouteHandlerRequestParam extends django::http::request::HttpRequest::InstanceSource, RemoteFlowSource::Range, DataFlow::ParameterNode { DjangoRouteHandlerRequestParam() { - this.getParameter() = any(DjangoRouteSetup setup).getARouteHandler().getRequestParam() + this.getParameter() = any(DjangoRouteSetup setup).getARequestHandler().getRequestParam() } override string getSourceType() { result = "django.http.request.HttpRequest" } diff --git a/python/ql/src/semmle/python/frameworks/Flask.qll b/python/ql/src/semmle/python/frameworks/Flask.qll index 3a420312be2..fa929e3d02b 100644 --- a/python/ql/src/semmle/python/frameworks/Flask.qll +++ b/python/ql/src/semmle/python/frameworks/Flask.qll @@ -275,10 +275,10 @@ private module FlaskModel { // parameter. This should give us more RemoteFlowSources but could also lead to // more FPs. If this turns out to be the wrong tradeoff, we can always change our mind. not exists(this.getUrlPattern()) and - result = this.getARouteHandler().getArgByName(_) + result = this.getARequestHandler().getArgByName(_) or exists(string name | - result = this.getARouteHandler().getArgByName(name) and + result = this.getARequestHandler().getArgByName(name) and exists(string match | match = this.getUrlPattern().regexpFind(werkzeug_rule_re(), _, _) and name = match.regexpCapture(werkzeug_rule_re(), 4) @@ -301,7 +301,7 @@ private module FlaskModel { result.asCfgNode() in [node.getArg(0), node.getArgByName("rule")] } - override Function getARouteHandler() { result.getADecorator().getAFlowNode() = node } + override Function getARequestHandler() { result.getADecorator().getAFlowNode() = node } } /** @@ -318,7 +318,7 @@ private module FlaskModel { result.asCfgNode() in [node.getArg(0), node.getArgByName("rule")] } - override Function getARouteHandler() { + override Function getARequestHandler() { exists(DataFlow::Node view_func_arg, DataFlow::Node func_src | view_func_arg.asCfgNode() in [node.getArg(2), node.getArgByName("view_func")] and DataFlow::localFlow(func_src, view_func_arg) and @@ -484,7 +484,7 @@ private module FlaskModel { private class FlaskRouteHandlerReturn extends HTTP::Server::HttpResponse::Range, DataFlow::CfgNode { FlaskRouteHandlerReturn() { exists(Function routeHandler | - routeHandler = any(FlaskRouteSetup rs).getARouteHandler() and + routeHandler = any(FlaskRouteSetup rs).getARequestHandler() and node = routeHandler.getAReturnValueFlowNode() ) } diff --git a/python/ql/test/experimental/library-tests/frameworks/django-v1/routing_test.py b/python/ql/test/experimental/library-tests/frameworks/django-v1/routing_test.py index 3388d97b04e..f0d51577e2e 100644 --- a/python/ql/test/experimental/library-tests/frameworks/django-v1/routing_test.py +++ b/python/ql/test/experimental/library-tests/frameworks/django-v1/routing_test.py @@ -4,19 +4,19 @@ from django.http.response import HttpResponse from django.views.generic import View -def url_match_xss(request, foo, bar, no_taint=None): # $routeHandler routedParameter=foo routedParameter=bar +def url_match_xss(request, foo, bar, no_taint=None): # $requestHandler routedParameter=foo routedParameter=bar return HttpResponse('url_match_xss: {} {}'.format(foo, bar)) # $HttpResponse -def get_params_xss(request): # $routeHandler +def get_params_xss(request): # $requestHandler return HttpResponse(request.GET.get("untrusted")) # $HttpResponse -def post_params_xss(request): # $routeHandler +def post_params_xss(request): # $requestHandler return HttpResponse(request.POST.get("untrusted")) # $HttpResponse -def http_resp_write(request): # $routeHandler +def http_resp_write(request): # $requestHandler rsp = HttpResponse() # $HttpResponse rsp.write(request.GET.get("untrusted")) # $HttpResponse return rsp @@ -26,22 +26,22 @@ class Foo(object): # Note: since Foo is used as the super type in a class view, it will be able to handle requests. - def post(self, request, untrusted): # $ MISSING: routeHandler routedParameter=untrusted + def post(self, request, untrusted): # $ MISSING: requestHandler routedParameter=untrusted return HttpResponse('Foo post: {}'.format(untrusted)) # $HttpResponse class ClassView(View, Foo): - def get(self, request, untrusted): # $ routeHandler routedParameter=untrusted + def get(self, request, untrusted): # $ requestHandler routedParameter=untrusted return HttpResponse('ClassView get: {}'.format(untrusted)) # $HttpResponse -def show_articles(request, page_number=1): # $routeHandler routedParameter=page_number +def show_articles(request, page_number=1): # $requestHandler routedParameter=page_number page_number = int(page_number) return HttpResponse('articles page: {}'.format(page_number)) # $HttpResponse -def xxs_positional_arg(request, arg0, arg1, no_taint=None): # $routeHandler routedParameter=arg0 routedParameter=arg1 +def xxs_positional_arg(request, arg0, arg1, no_taint=None): # $requestHandler routedParameter=arg0 routedParameter=arg1 return HttpResponse('xxs_positional_arg: {} {}'.format(arg0, arg1)) # $HttpResponse @@ -62,7 +62,7 @@ urlpatterns = [ ################################################################################ # Using patterns() for routing -def show_user(request, username): # $routeHandler routedParameter=username +def show_user(request, username): # $requestHandler routedParameter=username return HttpResponse('show_user {}'.format(username)) # $HttpResponse @@ -71,7 +71,7 @@ urlpatterns = patterns(url(r"^users/(?P[^/]+)", show_user)) # $routeS ################################################################################ # Show we understand the keyword arguments to django.conf.urls.url -def kw_args(request): # $routeHandler +def kw_args(request): # $requestHandler return HttpResponse('kw_args') # $HttpResponse urlpatterns = [ diff --git a/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/routing_test.py b/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/routing_test.py index 4564ebd0aa9..9b28f7ffeb8 100644 --- a/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/routing_test.py +++ b/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/routing_test.py @@ -4,19 +4,19 @@ from django.http import HttpResponse, HttpResponseRedirect, JsonResponse, HttpRe from django.views import View -def url_match_xss(request, foo, bar, no_taint=None): # $routeHandler routedParameter=foo routedParameter=bar +def url_match_xss(request, foo, bar, no_taint=None): # $requestHandler routedParameter=foo routedParameter=bar return HttpResponse('url_match_xss: {} {}'.format(foo, bar)) # $HttpResponse -def get_params_xss(request): # $routeHandler +def get_params_xss(request): # $requestHandler return HttpResponse(request.GET.get("untrusted")) # $HttpResponse -def post_params_xss(request): # $routeHandler +def post_params_xss(request): # $requestHandler return HttpResponse(request.POST.get("untrusted")) # $HttpResponse -def http_resp_write(request): # $routeHandler +def http_resp_write(request): # $requestHandler rsp = HttpResponse() # $HttpResponse rsp.write(request.GET.get("untrusted")) # $HttpResponse return rsp @@ -26,22 +26,22 @@ class Foo(object): # Note: since Foo is used as the super type in a class view, it will be able to handle requests. - def post(self, request, untrusted): # $ MISSING: routeHandler routedParameter=untrusted + def post(self, request, untrusted): # $ MISSING: requestHandler routedParameter=untrusted return HttpResponse('Foo post: {}'.format(untrusted)) # $HttpResponse class ClassView(View, Foo): - def get(self, request, untrusted): # $ routeHandler routedParameter=untrusted + def get(self, request, untrusted): # $ requestHandler routedParameter=untrusted return HttpResponse('ClassView get: {}'.format(untrusted)) # $HttpResponse -def show_articles(request, page_number=1): # $routeHandler routedParameter=page_number +def show_articles(request, page_number=1): # $requestHandler routedParameter=page_number page_number = int(page_number) return HttpResponse('articles page: {}'.format(page_number)) # $HttpResponse -def xxs_positional_arg(request, arg0, arg1, no_taint=None): # $routeHandler routedParameter=arg0 routedParameter=arg1 +def xxs_positional_arg(request, arg0, arg1, no_taint=None): # $requestHandler routedParameter=arg0 routedParameter=arg1 return HttpResponse('xxs_positional_arg: {} {}'.format(arg0, arg1)) # $HttpResponse @@ -62,7 +62,7 @@ urlpatterns = [ # Show we understand the keyword arguments to django.urls.re_path -def re_path_kwargs(request): # $routeHandler +def re_path_kwargs(request): # $requestHandler return HttpResponse('re_path_kwargs') # $HttpResponse @@ -75,16 +75,16 @@ urlpatterns = [ ################################################################################ # saying page_number is an externally controlled *string* is a bit strange, when we have an int converter :O -def page_number(request, page_number=1): # $routeHandler routedParameter=page_number +def page_number(request, page_number=1): # $requestHandler routedParameter=page_number return HttpResponse('page_number: {}'.format(page_number)) # $HttpResponse -def foo_bar_baz(request, foo, bar, baz): # $routeHandler routedParameter=foo routedParameter=bar routedParameter=baz +def foo_bar_baz(request, foo, bar, baz): # $requestHandler routedParameter=foo routedParameter=bar routedParameter=baz return HttpResponse('foo_bar_baz: {} {} {}'.format(foo, bar, baz)) # $HttpResponse -def path_kwargs(request, foo, bar): # $routeHandler routedParameter=foo routedParameter=bar +def path_kwargs(request, foo, bar): # $requestHandler routedParameter=foo routedParameter=bar return HttpResponse('path_kwargs: {} {} {}'.format(foo, bar)) # $HttpResponse -def not_valid_identifier(request): # $routeHandler +def not_valid_identifier(request): # $requestHandler return HttpResponse('') # $HttpResponse urlpatterns = [ @@ -101,7 +101,7 @@ urlpatterns = [ # This version 1.x way of defining urls is deprecated in Django 3.1, but still works from django.conf.urls import url -def deprecated(request): # $routeHandler +def deprecated(request): # $requestHandler return HttpResponse('deprecated') # $HttpResponse urlpatterns = [ @@ -113,5 +113,5 @@ class PossiblyNotRouted(View): # Even if our analysis can't find a route-setup for this class, we should still # consider it to be a handle incoming HTTP requests - def get(self, request, possibly_not_routed=42): # $ MISSING: routeHandler routedParameter=possibly_not_routed + def get(self, request, possibly_not_routed=42): # $ MISSING: requestHandler routedParameter=possibly_not_routed return HttpResponse('PossiblyNotRouted get: {}'.format(possibly_not_routed)) # $HttpResponse diff --git a/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/taint_test.py b/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/taint_test.py index 6e0529f0fc3..a0db86f2961 100644 --- a/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/taint_test.py +++ b/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/taint_test.py @@ -3,7 +3,7 @@ from django.urls import path from django.http import HttpRequest -def test_taint(request: HttpRequest, foo, bar, baz=None): # $routeHandler routedParameter=foo routedParameter=bar +def test_taint(request: HttpRequest, foo, bar, baz=None): # $requestHandler routedParameter=foo routedParameter=bar ensure_tainted(foo, bar) ensure_not_tainted(baz) diff --git a/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/testapp/views.py b/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/testapp/views.py index 86517930d06..ea8c392f3d4 100644 --- a/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/testapp/views.py +++ b/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/testapp/views.py @@ -3,31 +3,31 @@ from django.views import View from django.views.decorators.csrf import csrf_exempt -def foo(request: HttpRequest): # $routeHandler +def foo(request: HttpRequest): # $requestHandler return HttpResponse("foo") # $HttpResponse -def bar_baz(request: HttpRequest): # $routeHandler +def bar_baz(request: HttpRequest): # $requestHandler return HttpResponse("bar_baz") # $HttpResponse -def deprecated(request: HttpRequest): # $routeHandler +def deprecated(request: HttpRequest): # $requestHandler return HttpResponse("deprecated") # $HttpResponse class MyBasicViewHandler(View): - def get(self, request: HttpRequest): # $ routeHandler + def get(self, request: HttpRequest): # $ requestHandler return HttpResponse("MyViewHandler: GET") # $ HttpResponse - def post(self, request: HttpRequest): # $ routeHandler + def post(self, request: HttpRequest): # $ requestHandler return HttpResponse("MyViewHandler: POST") # $ HttpResponse class MyCustomViewBaseClass(View): - def post(self, request: HttpRequest): # $ MISSING: routeHandler + def post(self, request: HttpRequest): # $ MISSING: requestHandler return HttpResponse("MyCustomViewBaseClass: POST") # $ HttpResponse class MyViewHandlerWithCustomInheritance(MyCustomViewBaseClass): - def get(self, request: HttpRequest): # $ routeHandler + def get(self, request: HttpRequest): # $ requestHandler return HttpResponse("MyViewHandlerWithCustomInheritance: GET") # $ HttpResponse diff --git a/python/ql/test/experimental/library-tests/frameworks/flask/old_test.py b/python/ql/test/experimental/library-tests/frameworks/flask/old_test.py index 5609e80006f..4e251f6f306 100644 --- a/python/ql/test/experimental/library-tests/frameworks/flask/old_test.py +++ b/python/ql/test/experimental/library-tests/frameworks/flask/old_test.py @@ -4,14 +4,14 @@ from flask import Flask, request, make_response app = Flask(__name__) @app.route("/") # $routeSetup="/" -def hello_world(): # $routeHandler +def hello_world(): # $requestHandler return "Hello World!" # $HttpResponse from flask.views import MethodView class MyView(MethodView): - def get(self, user_id): # $ MISSING: routeHandler + def get(self, user_id): # $ MISSING: requestHandler if user_id is None: # return a list of users pass @@ -25,42 +25,42 @@ app.add_url_rule('/the/', defaults={'user_id': None}, # $routeSetup="/the/" view_func=the_view, methods=['GET',]) @app.route("/dangerous") # $routeSetup="/dangerous" -def dangerous(): # $routeHandler +def dangerous(): # $requestHandler return request.args.get('payload') # $HttpResponse @app.route("/dangerous-with-cfg-split") # $routeSetup="/dangerous-with-cfg-split" -def dangerous2(): # $routeHandler +def dangerous2(): # $requestHandler x = request.form['param0'] if request.method == "POST": return request.form['param1'] # $HttpResponse return None # $ SPURIOUS: HttpResponse @app.route("/unsafe") # $routeSetup="/unsafe" -def unsafe(): # $routeHandler +def unsafe(): # $requestHandler first_name = request.args.get('name', '') return make_response("Your name is " + first_name) # $HttpResponse @app.route("/safe") # $routeSetup="/safe" -def safe(): # $routeHandler +def safe(): # $requestHandler first_name = request.args.get('name', '') return make_response("Your name is " + escape(first_name)) # $HttpResponse @app.route("/hello/") # $routeSetup="/hello/" -def hello(name): # $routeHandler routedParameter=name +def hello(name): # $requestHandler routedParameter=name return make_response("Your name is " + name) # $HttpResponse @app.route("/foo/") # $routeSetup="/foo/" -def foo(subpath): # $routeHandler routedParameter=subpath +def foo(subpath): # $requestHandler routedParameter=subpath return make_response("The subpath is " + subpath) # $HttpResponse @app.route("/multiple/") # $routeSetup="/multiple/" @app.route("/multiple/foo/") # $routeSetup="/multiple/foo/" @app.route("/multiple/bar/") # $routeSetup="/multiple/bar/" -def multiple(foo=None, bar=None): # $routeHandler routedParameter=foo routedParameter=bar +def multiple(foo=None, bar=None): # $requestHandler routedParameter=foo routedParameter=bar return make_response("foo={!r} bar={!r}".format(foo, bar)) # $HttpResponse @app.route("/complex/") # $routeSetup="/complex/" -def complex(lang_code): # $routeHandler routedParameter=lang_code +def complex(lang_code): # $requestHandler routedParameter=lang_code return make_response("lang_code {}".format(lang_code)) # $HttpResponse if __name__ == "__main__": diff --git a/python/ql/test/experimental/library-tests/frameworks/flask/response_test.py b/python/ql/test/experimental/library-tests/frameworks/flask/response_test.py index ce57b5f01f0..b067f173882 100644 --- a/python/ql/test/experimental/library-tests/frameworks/flask/response_test.py +++ b/python/ql/test/experimental/library-tests/frameworks/flask/response_test.py @@ -6,12 +6,12 @@ app = Flask(__name__) @app.route("/html1") # $routeSetup="/html1" -def html1(): # $routeHandler +def html1(): # $requestHandler return "

hello

" # $HttpResponse mimetype=text/html responseBody="

hello

" @app.route("/html2") # $routeSetup="/html2" -def html2(): # $routeHandler +def html2(): # $requestHandler # note that response saved in a variable intentionally -- we wan the annotations to # show that we recognize the response creation, and not the return (hopefully). (and # do the same in the following of the file) @@ -20,7 +20,7 @@ def html2(): # $routeHandler @app.route("/html3") # $routeSetup="/html3" -def html3(): # $routeHandler +def html3(): # $requestHandler resp = app.make_response("

hello

") # $HttpResponse mimetype=text/html responseBody="

hello

" return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @@ -30,13 +30,13 @@ def html3(): # $routeHandler @app.route("/html4") # $routeSetup="/html4" -def html4(): # $routeHandler +def html4(): # $requestHandler resp = Response("

hello

") # $HttpResponse mimetype=text/html responseBody="

hello

" return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @app.route("/html5") # $routeSetup="/html5" -def html5(): # $routeHandler +def html5(): # $requestHandler # note: flask.Flask.response_class is set to `flask.Response` by default. # it can be overridden, but we don't try to handle that right now. resp = Flask.response_class("

hello

") # $HttpResponse mimetype=text/html responseBody="

hello

" @@ -44,7 +44,7 @@ def html5(): # $routeHandler @app.route("/html6") # $routeSetup="/html6" -def html6(): # $routeHandler +def html6(): # $requestHandler # note: app.response_class (flask.Flask.response_class) is set to `flask.Response` by default. # it can be overridden, but we don't try to handle that right now. resp = app.response_class("

hello

") # $HttpResponse mimetype=text/html responseBody="

hello

" @@ -52,14 +52,14 @@ def html6(): # $routeHandler @app.route("/html7") # $routeSetup="/html7" -def html7(): # $routeHandler +def html7(): # $requestHandler resp = make_response() # $HttpResponse mimetype=text/html resp.set_data("

hello

") # $ MISSING: responseBody="

hello

" return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @app.route("/jsonify") # $routeSetup="/jsonify" -def jsonify_route(): # $routeHandler +def jsonify_route(): # $requestHandler data = {"foo": "bar"} resp = jsonify(data) # $ MISSING: HttpResponse mimetype=application/json responseBody=data return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @@ -69,7 +69,7 @@ def jsonify_route(): # $routeHandler ################################################################################ @app.route("/tricky-return1") # $routeSetup="/tricky-return1" -def tricky_return1(): # $routeHandler +def tricky_return1(): # $requestHandler if "raw" in request.args: resp = "

hellu

" else: @@ -83,7 +83,7 @@ def helper(): return make_response("

hello

") # $HttpResponse mimetype=text/html responseBody="

hello

" @app.route("/tricky-return2") # $routeSetup="/tricky-return2" -def tricky_return2(): # $routeHandler +def tricky_return2(): # $requestHandler resp = helper() return resp # $HttpResponse mimetype=text/html responseBody=resp @@ -94,14 +94,14 @@ def tricky_return2(): # $routeHandler @app.route("/content-type/response-modification1") # $routeSetup="/content-type/response-modification1" -def response_modification1(): # $routeHandler +def response_modification1(): # $requestHandler resp = make_response("

hello

") # $HttpResponse mimetype=text/html responseBody="

hello

" resp.content_type = "text/plain" # $ MISSING: HttpResponse mimetype=text/plain return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @app.route("/content-type/response-modification2") # $routeSetup="/content-type/response-modification2" -def response_modification2(): # $routeHandler +def response_modification2(): # $requestHandler resp = make_response("

hello

") # $HttpResponse mimetype=text/html responseBody="

hello

" resp.headers["content-type"] = "text/plain" # $ MISSING: HttpResponse mimetype=text/plain return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @@ -112,33 +112,33 @@ def response_modification2(): # $routeHandler @app.route("/content-type/Response1") # $routeSetup="/content-type/Response1" -def Response1(): # $routeHandler +def Response1(): # $requestHandler resp = Response("

hello

", mimetype="text/plain") # $HttpResponse mimetype=text/plain responseBody="

hello

" return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @app.route("/content-type/Response2") # $routeSetup="/content-type/Response2" -def Response2(): # $routeHandler +def Response2(): # $requestHandler resp = Response("

hello

", content_type="text/plain; charset=utf-8") # $HttpResponse mimetype=text/plain responseBody="

hello

" return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @app.route("/content-type/Response3") # $routeSetup="/content-type/Response3" -def Response3(): # $routeHandler +def Response3(): # $requestHandler # content_type argument takes priority (and result is text/plain) resp = Response("

hello

", content_type="text/plain; charset=utf-8", mimetype="text/html") # $HttpResponse mimetype=text/plain responseBody="

hello

" return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @app.route("/content-type/Response4") # $routeSetup="/content-type/Response4" -def Response4(): # $routeHandler +def Response4(): # $requestHandler # note: capitalization of Content-Type does not matter resp = Response("

hello

", headers={"Content-TYPE": "text/plain"}) # $HttpResponse responseBody="

hello

" SPURIOUS: mimetype=text/html MISSING: mimetype=text/plain return resp # $ SPURIOUS: HttpResponse mimetype=text/html responseBody=resp @app.route("/content-type/Response5") # $routeSetup="/content-type/Response5" -def Response5(): # $routeHandler +def Response5(): # $requestHandler # content_type argument takes priority (and result is text/plain) # note: capitalization of Content-Type does not matter resp = Response("

hello

", headers={"Content-TYPE": "text/html"}, content_type="text/plain; charset=utf-8") # $HttpResponse mimetype=text/plain responseBody="

hello

" @@ -146,7 +146,7 @@ def Response5(): # $routeHandler @app.route("/content-type/Response6") # $routeSetup="/content-type/Response6" -def Response6(): # $routeHandler +def Response6(): # $requestHandler # mimetype argument takes priority over header (and result is text/plain) # note: capitalization of Content-Type does not matter resp = Response("

hello

", headers={"Content-TYPE": "text/html"}, mimetype="text/plain") # $HttpResponse mimetype=text/plain responseBody="

hello

" @@ -154,7 +154,7 @@ def Response6(): # $routeHandler @app.route("/content-type/Flask-response-class") # $routeSetup="/content-type/Flask-response-class" -def Flask_response_class(): # $routeHandler +def Flask_response_class(): # $requestHandler # note: flask.Flask.response_class is set to `flask.Response` by default. # it can be overridden, but we don't try to handle that right now. resp = Flask.response_class("

hello

", mimetype="text/plain") # $HttpResponse mimetype=text/plain responseBody="

hello

" @@ -162,7 +162,7 @@ def Flask_response_class(): # $routeHandler @app.route("/content-type/app-response-class") # $routeSetup="/content-type/app-response-class" -def app_response_class(): # $routeHandler +def app_response_class(): # $requestHandler # note: app.response_class (flask.Flask.response_class) is set to `flask.Response` by default. # it can be overridden, but we don't try to handle that right now. resp = app.response_class("

hello

", mimetype="text/plain") # $HttpResponse mimetype=text/plain responseBody="

hello

" diff --git a/python/ql/test/experimental/library-tests/frameworks/flask/routing_test.py b/python/ql/test/experimental/library-tests/frameworks/flask/routing_test.py index a5e40b5c123..1a6028e8026 100644 --- a/python/ql/test/experimental/library-tests/frameworks/flask/routing_test.py +++ b/python/ql/test/experimental/library-tests/frameworks/flask/routing_test.py @@ -6,24 +6,24 @@ app = Flask(__name__) SOME_ROUTE = "/some/route" @app.route(SOME_ROUTE) # $routeSetup="/some/route" -def some_route(): # $routeHandler +def some_route(): # $requestHandler return make_response("some_route") # $HttpResponse -def index(): # $routeHandler +def index(): # $requestHandler return make_response("index") # $HttpResponse app.add_url_rule('/index', 'index', index) # $routeSetup="/index" # We don't support this yet, and I think that's OK -def later_set(): # $ MISSING: routeHandler +def later_set(): # $ MISSING: requestHandler return make_response("later_set") # $HttpResponse app.add_url_rule('/later-set', 'later_set', view_func=None) # $routeSetup="/later-set" app.view_functions['later_set'] = later_set @app.route(UNKNOWN_ROUTE) # $routeSetup -def unkown_route(foo, bar): # $routeHandler routedParameter=foo routedParameter=bar +def unkown_route(foo, bar): # $requestHandler routedParameter=foo routedParameter=bar return make_response("unkown_route") # $HttpResponse diff --git a/python/ql/test/experimental/library-tests/frameworks/flask/taint_test.py b/python/ql/test/experimental/library-tests/frameworks/flask/taint_test.py index 58335997875..31c0c7e6121 100644 --- a/python/ql/test/experimental/library-tests/frameworks/flask/taint_test.py +++ b/python/ql/test/experimental/library-tests/frameworks/flask/taint_test.py @@ -2,7 +2,7 @@ from flask import Flask, request app = Flask(__name__) @app.route("/test_taint//") # $routeSetup="/test_taint//" -def test_taint(name = "World!", number="0", foo="foo"): # $routeHandler routedParameter=name routedParameter=number +def test_taint(name = "World!", number="0", foo="foo"): # $requestHandler routedParameter=name routedParameter=number ensure_tainted(name, number) ensure_not_tainted(foo) @@ -192,7 +192,7 @@ def test_taint(name = "World!", number="0", foo="foo"): # $routeHandler routedP @app.route("/debug//", methods=['GET']) # $routeSetup="/debug//" -def debug(foo, bar): # $routeHandler routedParameter=foo routedParameter=bar +def debug(foo, bar): # $requestHandler routedParameter=foo routedParameter=bar print("request.view_args", request.view_args) print("request.headers {!r}".format(request.headers)) @@ -203,7 +203,7 @@ def debug(foo, bar): # $routeHandler routedParameter=foo routedParameter=bar return 'ok' # $HttpResponse @app.route("/stream", methods=['POST']) # $routeSetup="/stream" -def stream(): # $routeHandler +def stream(): # $requestHandler print(request.path) s = request.stream print(s) @@ -213,7 +213,7 @@ def stream(): # $routeHandler return 'ok' # $HttpResponse @app.route("/input_stream", methods=['POST']) # $routeSetup="/input_stream" -def input_stream(): # $routeHandler +def input_stream(): # $requestHandler print(request.path) s = request.input_stream print(s) @@ -224,14 +224,14 @@ def input_stream(): # $routeHandler return 'ok' # $HttpResponse @app.route("/form", methods=['POST']) # $routeSetup="/form" -def form(): # $routeHandler +def form(): # $requestHandler print(request.path) print("request.form", request.form) return 'ok' # $HttpResponse @app.route("/cache_control", methods=['POST']) # $routeSetup="/cache_control" -def cache_control(): # $routeHandler +def cache_control(): # $requestHandler print(request.path) print("request.cache_control.max_age", request.cache_control.max_age, type(request.cache_control.max_age)) print("request.cache_control.max_stale", request.cache_control.max_stale, type(request.cache_control.max_stale)) @@ -240,7 +240,7 @@ def cache_control(): # $routeHandler return 'ok' # $HttpResponse @app.route("/file_upload", methods=['POST']) # $routeSetup="/file_upload" -def file_upload(): # $routeHandler +def file_upload(): # $requestHandler print(request.path) for k,v in request.files.items(): print(k, v, v.name, v.filename, v.stream) @@ -248,7 +248,7 @@ def file_upload(): # $routeHandler return 'ok' # $HttpResponse @app.route("/args", methods=['GET']) # $routeSetup="/args" -def args(): # $routeHandler +def args(): # $requestHandler print(request.path) print("request.args", request.args) diff --git a/python/ql/test/experimental/library-tests/frameworks/stdlib/http_server.py b/python/ql/test/experimental/library-tests/frameworks/stdlib/http_server.py index 33a3594894f..f74cc171136 100644 --- a/python/ql/test/experimental/library-tests/frameworks/stdlib/http_server.py +++ b/python/ql/test/experimental/library-tests/frameworks/stdlib/http_server.py @@ -78,7 +78,7 @@ class MyHandler(BaseHTTPRequestHandler): ensure_tainted(form) - def do_GET(self): + def do_GET(self): # $ MISSING: requestHandler # send_response will log a line to stderr self.send_response(200) self.send_header("Content-type", "text/plain; charset=utf-8") @@ -88,7 +88,7 @@ class MyHandler(BaseHTTPRequestHandler): print(self.headers) - def do_POST(self): + def do_POST(self): # $ MISSING: requestHandler form = cgi.FieldStorage( self.rfile, self.headers, diff --git a/python/ql/test/experimental/meta/ConceptsTest.qll b/python/ql/test/experimental/meta/ConceptsTest.qll index 98423b95688..b031499e5e2 100644 --- a/python/ql/test/experimental/meta/ConceptsTest.qll +++ b/python/ql/test/experimental/meta/ConceptsTest.qll @@ -142,7 +142,9 @@ class SqlExecutionTest extends InlineExpectationsTest { class HttpServerRouteSetupTest extends InlineExpectationsTest { HttpServerRouteSetupTest() { this = "HttpServerRouteSetupTest" } - override string getARelevantTag() { result in ["routeSetup", "routeHandler", "routedParameter"] } + override string getARelevantTag() { + result in ["routeSetup", "requestHandler", "routedParameter"] + } override predicate hasActualResult(Location location, string element, string tag, string value) { exists(HTTP::Server::RouteSetup setup | @@ -157,16 +159,15 @@ class HttpServerRouteSetupTest extends InlineExpectationsTest { tag = "routeSetup" ) or - exists(HTTP::Server::RouteSetup setup, Function func | - func = setup.getARouteHandler() and - location = func.getLocation() and - element = func.toString() and + exists(HTTP::Server::RequestHandler handler | + location = handler.getLocation() and + element = handler.toString() and value = "" and - tag = "routeHandler" + tag = "requestHandler" ) or - exists(HTTP::Server::RouteSetup setup, Parameter param | - param = setup.getARoutedParameter() and + exists(HTTP::Server::RequestHandler handler, Parameter param | + param = handler.getARoutedParameter() and location = param.getLocation() and element = param.toString() and value = param.asName().getId() and From d4d6f0ca0c55eded35b2a0b23ba8d71ed6ad67ef Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 21 Dec 2020 17:38:43 +0100 Subject: [PATCH 03/11] Python: Model django request handlers without known route --- .../2020-12-21-django-with-unknown-route.md | 2 ++ .../ql/src/semmle/python/frameworks/Django.qll | 17 +++++++++++++++++ .../frameworks/django-v2-v3/routing_test.py | 2 +- .../frameworks/django-v2-v3/testapp/views.py | 2 +- 4 files changed, 21 insertions(+), 2 deletions(-) create mode 100644 python/change-notes/2020-12-21-django-with-unknown-route.md diff --git a/python/change-notes/2020-12-21-django-with-unknown-route.md b/python/change-notes/2020-12-21-django-with-unknown-route.md new file mode 100644 index 00000000000..6ac353cbef4 --- /dev/null +++ b/python/change-notes/2020-12-21-django-with-unknown-route.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* Improved modeling of `django` to recognize request handlers on `View` classes without known route, thereby leading to more sources of remote user input (`RemoteFlowSource`). diff --git a/python/ql/src/semmle/python/frameworks/Django.qll b/python/ql/src/semmle/python/frameworks/Django.qll index f4cbe3d9ea5..171a06dc97a 100644 --- a/python/ql/src/semmle/python/frameworks/Django.qll +++ b/python/ql/src/semmle/python/frameworks/Django.qll @@ -1756,6 +1756,23 @@ private module Django { } } + /** A request handler defined in a django view class, that has no known route. */ + private class DjangoViewClassHandlerWithoutKnownRoute extends HTTP::Server::RequestHandler::Range, + DjangoRouteHandler { + DjangoViewClassHandlerWithoutKnownRoute() { + exists(DjangoViewClassDef vc | vc.getARequestHandler() = this) and + not exists(DjangoRouteSetup setup | setup.getARequestHandler() = this) + } + + override Parameter getARoutedParameter() { + // Since we don't know the URL pattern, we simply mark all parameters as a routed + // parameter. This should give us more RemoteFlowSources but could also lead to + // more FPs. If this turns out to be the wrong tradeoff, we can always change our mind. + result in [this.getArg(_), this.getArgByName(_)] and + not result = any(int i | i <= this.getRequestParamIndex() | this.getArg(i)) + } + } + /** * Gets the regex that is used by django to find routed parameters when using `django.urls.path`. * diff --git a/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/routing_test.py b/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/routing_test.py index 9b28f7ffeb8..e744c5ba89d 100644 --- a/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/routing_test.py +++ b/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/routing_test.py @@ -113,5 +113,5 @@ class PossiblyNotRouted(View): # Even if our analysis can't find a route-setup for this class, we should still # consider it to be a handle incoming HTTP requests - def get(self, request, possibly_not_routed=42): # $ MISSING: requestHandler routedParameter=possibly_not_routed + def get(self, request, possibly_not_routed=42): # $ requestHandler routedParameter=possibly_not_routed return HttpResponse('PossiblyNotRouted get: {}'.format(possibly_not_routed)) # $HttpResponse diff --git a/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/testapp/views.py b/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/testapp/views.py index ea8c392f3d4..55c60a551a0 100644 --- a/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/testapp/views.py +++ b/python/ql/test/experimental/library-tests/frameworks/django-v2-v3/testapp/views.py @@ -24,7 +24,7 @@ class MyBasicViewHandler(View): class MyCustomViewBaseClass(View): - def post(self, request: HttpRequest): # $ MISSING: requestHandler + def post(self, request: HttpRequest): # $ requestHandler return HttpResponse("MyCustomViewBaseClass: POST") # $ HttpResponse From 05ab6cd54a806449be0b1b7003532deda11f59a8 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 21 Dec 2020 17:41:46 +0100 Subject: [PATCH 04/11] Python: Add RemoteFlowSource for django handler without route A bit scary that we don't have any tests to indicate that I forgot to add this :O --- python/ql/src/semmle/python/frameworks/Django.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/python/ql/src/semmle/python/frameworks/Django.qll b/python/ql/src/semmle/python/frameworks/Django.qll index 171a06dc97a..74c1856a79b 100644 --- a/python/ql/src/semmle/python/frameworks/Django.qll +++ b/python/ql/src/semmle/python/frameworks/Django.qll @@ -1909,6 +1909,8 @@ private module Django { RemoteFlowSource::Range, DataFlow::ParameterNode { DjangoRouteHandlerRequestParam() { this.getParameter() = any(DjangoRouteSetup setup).getARequestHandler().getRequestParam() + or + this.getParameter() = any(DjangoViewClassHandlerWithoutKnownRoute setup).getRequestParam() } override string getSourceType() { result = "django.http.request.HttpRequest" } From 71a6ef5b00c21ab4bdad6380a7bfc65ccbf10e0d Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 21 Dec 2020 17:55:33 +0100 Subject: [PATCH 05/11] Python: Model RequestHandler from standard library explicitly --- python/ql/src/semmle/python/frameworks/Stdlib.qll | 14 ++++++++++++++ python/ql/src/semmle/python/web/HttpConstants.qll | 2 +- .../library-tests/frameworks/stdlib/http_server.py | 4 ++-- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/python/ql/src/semmle/python/frameworks/Stdlib.qll b/python/ql/src/semmle/python/frameworks/Stdlib.qll index 8840e1bba60..5120534d014 100644 --- a/python/ql/src/semmle/python/frameworks/Stdlib.qll +++ b/python/ql/src/semmle/python/frameworks/Stdlib.qll @@ -1616,6 +1616,20 @@ private module Stdlib { ) } } + + /** + * The entry-point for handling a request with a `BaseHTTPRequestHandler` subclass. + * + * Not essential for any functionality, but provides a consistent modeling. + */ + private class RequestHandlerFunc extends HTTP::Server::RequestHandler::Range { + RequestHandlerFunc() { + this = any(HTTPRequestHandlerClassDef cls).getAMethod() and + this.getName() = "do_" + HTTP::httpVerb() + } + + override Parameter getARoutedParameter() { none() } + } } // --------------------------------------------------------------------------- diff --git a/python/ql/src/semmle/python/web/HttpConstants.qll b/python/ql/src/semmle/python/web/HttpConstants.qll index b3b389faf62..d3e8b0412a4 100644 --- a/python/ql/src/semmle/python/web/HttpConstants.qll +++ b/python/ql/src/semmle/python/web/HttpConstants.qll @@ -1,4 +1,4 @@ -/** Gets an HTTP verb */ +/** Gets an HTTP verb, in upper case */ string httpVerb() { result = "GET" or result = "POST" or diff --git a/python/ql/test/experimental/library-tests/frameworks/stdlib/http_server.py b/python/ql/test/experimental/library-tests/frameworks/stdlib/http_server.py index f74cc171136..8d996acb41d 100644 --- a/python/ql/test/experimental/library-tests/frameworks/stdlib/http_server.py +++ b/python/ql/test/experimental/library-tests/frameworks/stdlib/http_server.py @@ -78,7 +78,7 @@ class MyHandler(BaseHTTPRequestHandler): ensure_tainted(form) - def do_GET(self): # $ MISSING: requestHandler + def do_GET(self): # $ requestHandler # send_response will log a line to stderr self.send_response(200) self.send_header("Content-type", "text/plain; charset=utf-8") @@ -88,7 +88,7 @@ class MyHandler(BaseHTTPRequestHandler): print(self.headers) - def do_POST(self): # $ MISSING: requestHandler + def do_POST(self): # $ requestHandler form = cgi.FieldStorage( self.rfile, self.headers, From bc4a0bcbebbb84426bf1da1cf18c6ba1666eb3b4 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 22 Dec 2020 11:27:11 +0100 Subject: [PATCH 06/11] Python: Split request handler / route setup concept tests Not doing so earlier was just a mistake. --- python/ql/test/experimental/meta/ConceptsTest.qll | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/python/ql/test/experimental/meta/ConceptsTest.qll b/python/ql/test/experimental/meta/ConceptsTest.qll index b031499e5e2..1deef32857b 100644 --- a/python/ql/test/experimental/meta/ConceptsTest.qll +++ b/python/ql/test/experimental/meta/ConceptsTest.qll @@ -142,9 +142,7 @@ class SqlExecutionTest extends InlineExpectationsTest { class HttpServerRouteSetupTest extends InlineExpectationsTest { HttpServerRouteSetupTest() { this = "HttpServerRouteSetupTest" } - override string getARelevantTag() { - result in ["routeSetup", "requestHandler", "routedParameter"] - } + override string getARelevantTag() { result in ["routeSetup"] } override predicate hasActualResult(Location location, string element, string tag, string value) { exists(HTTP::Server::RouteSetup setup | @@ -158,7 +156,15 @@ class HttpServerRouteSetupTest extends InlineExpectationsTest { ) and tag = "routeSetup" ) - or + } +} + +class HttpServerRequestHandlerTest extends InlineExpectationsTest { + HttpServerRequestHandlerTest() { this = "HttpServerRequestHandlerTest" } + + override string getARelevantTag() { result in ["requestHandler", "routedParameter"] } + + override predicate hasActualResult(Location location, string element, string tag, string value) { exists(HTTP::Server::RequestHandler handler | location = handler.getLocation() and element = handler.toString() and From dc0d9403314e95cd0d6d4a3abcad3b194e04187a Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 22 Dec 2020 11:28:29 +0100 Subject: [PATCH 07/11] Python: Ensure all concept tests ignore irrelevant results Since this was causing a CI error. also changed things a bit so we do it in a consistent way :) --- .../test/experimental/meta/ConceptsTest.qll | 40 +++++++++++-------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/python/ql/test/experimental/meta/ConceptsTest.qll b/python/ql/test/experimental/meta/ConceptsTest.qll index 1deef32857b..6b3544da30e 100644 --- a/python/ql/test/experimental/meta/ConceptsTest.qll +++ b/python/ql/test/experimental/meta/ConceptsTest.qll @@ -111,6 +111,7 @@ class CodeExecutionTest extends InlineExpectationsTest { override string getARelevantTag() { result = "getCode" } override predicate hasActualResult(Location location, string element, string tag, string value) { + exists(location.getFile().getRelativePath()) and exists(CodeExecution ce, DataFlow::Node code | exists(location.getFile().getRelativePath()) and code = ce.getCode() and @@ -128,6 +129,7 @@ class SqlExecutionTest extends InlineExpectationsTest { override string getARelevantTag() { result = "getSql" } override predicate hasActualResult(Location location, string element, string tag, string value) { + exists(location.getFile().getRelativePath()) and exists(SqlExecution e, DataFlow::Node sql | exists(location.getFile().getRelativePath()) and sql = e.getSql() and @@ -145,6 +147,7 @@ class HttpServerRouteSetupTest extends InlineExpectationsTest { override string getARelevantTag() { result in ["routeSetup"] } override predicate hasActualResult(Location location, string element, string tag, string value) { + exists(location.getFile().getRelativePath()) and exists(HTTP::Server::RouteSetup setup | location = setup.getLocation() and element = setup.toString() and @@ -165,19 +168,22 @@ class HttpServerRequestHandlerTest extends InlineExpectationsTest { override string getARelevantTag() { result in ["requestHandler", "routedParameter"] } override predicate hasActualResult(Location location, string element, string tag, string value) { - exists(HTTP::Server::RequestHandler handler | - location = handler.getLocation() and - element = handler.toString() and - value = "" and - tag = "requestHandler" - ) - or - exists(HTTP::Server::RequestHandler handler, Parameter param | - param = handler.getARoutedParameter() and - location = param.getLocation() and - element = param.toString() and - value = param.asName().getId() and - tag = "routedParameter" + exists(location.getFile().getRelativePath()) and + ( + exists(HTTP::Server::RequestHandler handler | + location = handler.getLocation() and + element = handler.toString() and + value = "" and + tag = "requestHandler" + ) + or + exists(HTTP::Server::RequestHandler handler, Parameter param | + param = handler.getARoutedParameter() and + location = param.getLocation() and + element = param.toString() and + value = param.asName().getId() and + tag = "routedParameter" + ) ) } } @@ -198,7 +204,7 @@ class HttpServerHttpResponseTest extends InlineExpectationsTest { // flask tests more readable since adding full annotations for HttpResponses in the // the tests for routing setup is both annoying and not very useful. location.getFile() = file and - tag = getARelevantTag() and + exists(file.getRelativePath()) and ( exists(HTTP::Server::HttpResponse response | location = response.getLocation() and @@ -237,8 +243,8 @@ class FileSystemAccessTest extends InlineExpectationsTest { override string getARelevantTag() { result = "getAPathArgument" } override predicate hasActualResult(Location location, string element, string tag, string value) { + exists(location.getFile().getRelativePath()) and exists(FileSystemAccess a, DataFlow::Node path | - exists(location.getFile().getRelativePath()) and path = a.getAPathArgument() and location = a.getLocation() and element = path.toString() and @@ -254,8 +260,8 @@ class PathNormalizationTest extends InlineExpectationsTest { override string getARelevantTag() { result = "pathNormalization" } override predicate hasActualResult(Location location, string element, string tag, string value) { + exists(location.getFile().getRelativePath()) and exists(Path::PathNormalization n | - exists(location.getFile().getRelativePath()) and location = n.getLocation() and element = n.toString() and value = "" and @@ -270,8 +276,8 @@ class SafeAccessCheckTest extends InlineExpectationsTest { override string getARelevantTag() { result in ["checks", "branch"] } override predicate hasActualResult(Location location, string element, string tag, string value) { + exists(location.getFile().getRelativePath()) and exists(Path::SafeAccessCheck c, DataFlow::Node checks, boolean branch | - exists(location.getFile().getRelativePath()) and c.checks(checks.asCfgNode(), branch) and location = c.getLocation() and ( From 3094aedf14806cb310bfdd9b92e3979492c05f33 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 22 Dec 2020 14:42:53 +0100 Subject: [PATCH 08/11] Python: Fix regression in ConceptTests I accidentially deleted that line :D --- python/ql/test/experimental/meta/ConceptsTest.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/python/ql/test/experimental/meta/ConceptsTest.qll b/python/ql/test/experimental/meta/ConceptsTest.qll index 6b3544da30e..0516fe5dac3 100644 --- a/python/ql/test/experimental/meta/ConceptsTest.qll +++ b/python/ql/test/experimental/meta/ConceptsTest.qll @@ -205,6 +205,8 @@ class HttpServerHttpResponseTest extends InlineExpectationsTest { // the tests for routing setup is both annoying and not very useful. location.getFile() = file and exists(file.getRelativePath()) and + // we need to do this step since we expect subclasses could override getARelevantTag + tag = getARelevantTag() and ( exists(HTTP::Server::HttpResponse response | location = response.getLocation() and From 141b9adc4d38eda5ea3cd9a5b19688979a303c8f Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 11 Jan 2021 11:18:59 +0100 Subject: [PATCH 09/11] Python: Minor refactoring Co-authored-by: yoff --- python/ql/src/semmle/python/Concepts.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/semmle/python/Concepts.qll b/python/ql/src/semmle/python/Concepts.qll index 9a30ebf4871..a90df346b2f 100644 --- a/python/ql/src/semmle/python/Concepts.qll +++ b/python/ql/src/semmle/python/Concepts.qll @@ -404,7 +404,7 @@ module HTTP { /** A parameter that will receive parts of the url when handling an incoming request. */ private class RoutedParameter extends RemoteFlowSource::Range, DataFlow::ParameterNode { - RoutedParameter() { this.getParameter() = any(RequestHandler setup).getARoutedParameter() } + RoutedParameter() { this.getParameter() = any(RequestHandler handler).getARoutedParameter() } override string getSourceType() { result = "RoutedParameter" } } From 828bb9a90249ac9e4894d158f7b4c8d5a91e203b Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 11 Jan 2021 11:28:40 +0100 Subject: [PATCH 10/11] Python: Small refactor for request param modeling in Django --- python/ql/src/semmle/python/frameworks/Django.qll | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/python/ql/src/semmle/python/frameworks/Django.qll b/python/ql/src/semmle/python/frameworks/Django.qll index 74c1856a79b..3e7283208e7 100644 --- a/python/ql/src/semmle/python/frameworks/Django.qll +++ b/python/ql/src/semmle/python/frameworks/Django.qll @@ -1905,9 +1905,10 @@ private module Django { // --------------------------------------------------------------------------- // HttpRequest taint modeling // --------------------------------------------------------------------------- - class DjangoRouteHandlerRequestParam extends django::http::request::HttpRequest::InstanceSource, + /** A parameter that will receive the django `HttpRequest` instance when a request handler is invoked. */ + private class DjangoRequestHandlerRequestParam extends django::http::request::HttpRequest::InstanceSource, RemoteFlowSource::Range, DataFlow::ParameterNode { - DjangoRouteHandlerRequestParam() { + DjangoRequestHandlerRequestParam() { this.getParameter() = any(DjangoRouteSetup setup).getARequestHandler().getRequestParam() or this.getParameter() = any(DjangoViewClassHandlerWithoutKnownRoute setup).getRequestParam() From 2ba7ed49400b91f768e21c28d86bf53f2c2e65ea Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 12 Jan 2021 13:32:43 +0100 Subject: [PATCH 11/11] Python: Add note about future work for getARequestHandler --- python/ql/src/semmle/python/Concepts.qll | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/python/ql/src/semmle/python/Concepts.qll b/python/ql/src/semmle/python/Concepts.qll index 6eda29e6c51..a79eaa2dc55 100644 --- a/python/ql/src/semmle/python/Concepts.qll +++ b/python/ql/src/semmle/python/Concepts.qll @@ -313,7 +313,11 @@ module HTTP { /** Gets the URL pattern for this route, if it can be statically determined. */ string getUrlPattern() { result = range.getUrlPattern() } - /** Gets a function that will handle incoming requests for this route, if any. */ + /** + * Gets a function that will handle incoming requests for this route, if any. + * + * NOTE: This will be modified in the near future to have a `RequestHandler` result, instead of a `Function`. + */ Function getARequestHandler() { result = range.getARequestHandler() } /** @@ -343,7 +347,11 @@ module HTTP { ) } - /** Gets a function that will handle incoming requests for this route, if any. */ + /** + * Gets a function that will handle incoming requests for this route, if any. + * + * NOTE: This will be modified in the near future to have a `RequestHandler` result, instead of a `Function`. + */ abstract Function getARequestHandler(); /**