From 3ab5fd5ca45f6df134b83777d20a98c385153233 Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Mon, 2 Oct 2023 14:58:21 +0100 Subject: [PATCH 01/13] Add RestFramework handler kwargs --- python/ql/lib/semmle/python/frameworks/RestFramework.qll | 3 +++ 1 file changed, 3 insertions(+) diff --git a/python/ql/lib/semmle/python/frameworks/RestFramework.qll b/python/ql/lib/semmle/python/frameworks/RestFramework.qll index 49eb3cbe7e6..4e327491ca6 100644 --- a/python/ql/lib/semmle/python/frameworks/RestFramework.qll +++ b/python/ql/lib/semmle/python/frameworks/RestFramework.qll @@ -194,6 +194,9 @@ private module RestFramework { exists(RestFrameworkApiViewClass vc | this.getParameter() = vc.getARequestHandler().(PrivateDjango::DjangoRouteHandler).getRequestParam() + or + // retrieve(self, request, **kwargs) + this.getParameter() = vc.getARequestHandler().(PrivateDjango::DjangoRouteHandler).getKwarg() ) or // annotated with @api_view decorator From a23904ca39db059807e4061e4f54c89291496177 Mon Sep 17 00:00:00 2001 From: Mathew Payne Date: Mon, 2 Oct 2023 15:09:11 +0100 Subject: [PATCH 02/13] Add taint tests --- .../frameworks/rest_framework/taint_test.py | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py b/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py index ff30b00ca99..31daba8d6c6 100644 --- a/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py +++ b/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py @@ -107,12 +107,30 @@ class MyClass(APIView): return Response("ok") # $ HttpResponse +# Viewsets +# see https://www.django-rest-framework.org/api-guide/viewsets/ + +class MyModelViewSet(viewsets.ModelViewSet): + def retrieve(self, request, *args, **kwargs): # $ requestHandler + ensure_tainted( + request, # $ tainted + request.GET, # $ tainted + request.GET.get("pk"), # $ tainted + ) + + ensure_tainted( + kwargs, # $ tainted + kwargs["pk"], # $ tainted + kwargs.get("pk"), # $ tainted + ) + return Response("retrieve") # $ HttpResponse # fake setup, you can't actually run this urlpatterns = [ path("test-taint/", test_taint), # $ routeSetup="test-taint/" - path("ClassView/", MyClass.as_view()), # $ routeSetup="ClassView/" + path("ClassView/", MyClass.as_view()), # $ routeSetup="ClassView/", + path("MyModelViewSet/", MyModelViewSet.as_view()) # $ routeSetup="MyModelViewSet/", ] # tests with no route-setup, but we can still tell that these are using Django REST From 46e44a0036d829a8e19a4e1d126fc16682cf943a Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 23 Oct 2023 16:42:17 +0200 Subject: [PATCH 03/13] Python: Fix import --- .../test/library-tests/frameworks/rest_framework/taint_test.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py b/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py index 31daba8d6c6..83b5429437e 100644 --- a/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py +++ b/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py @@ -1,5 +1,6 @@ from rest_framework.decorators import api_view, parser_classes from rest_framework.views import APIView +from rest_framework.viewsets import ModelViewSet from rest_framework.request import Request from rest_framework.response import Response from rest_framework.parsers import JSONParser @@ -110,7 +111,7 @@ class MyClass(APIView): # Viewsets # see https://www.django-rest-framework.org/api-guide/viewsets/ -class MyModelViewSet(viewsets.ModelViewSet): +class MyModelViewSet(ModelViewSet): def retrieve(self, request, *args, **kwargs): # $ requestHandler ensure_tainted( request, # $ tainted From 60e7786b04ba2567e328f253d8cf3cc94947d863 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 23 Oct 2023 16:44:54 +0200 Subject: [PATCH 04/13] Python: Use explicit keyword parameter --- .../frameworks/rest_framework/taint_test.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py b/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py index 83b5429437e..66167a75539 100644 --- a/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py +++ b/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py @@ -112,18 +112,19 @@ class MyClass(APIView): # see https://www.django-rest-framework.org/api-guide/viewsets/ class MyModelViewSet(ModelViewSet): - def retrieve(self, request, *args, **kwargs): # $ requestHandler + def retrieve(self, request, routed_param): # $ requestHandler routedParameter=routed_param ensure_tainted( request, # $ tainted request.GET, # $ tainted request.GET.get("pk"), # $ tainted + request.data # $ tainted ) - ensure_tainted( - kwargs, # $ tainted - kwargs["pk"], # $ tainted - kwargs.get("pk"), # $ tainted - ) + ensure_tainted(routed_param) # $ tainted + + # same as for standard Django view + ensure_tainted(self.args, self.kwargs) # $ tainted + return Response("retrieve") # $ HttpResponse From 8b23140a08ddeecc3d053de2659303bb80cf80a2 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 23 Oct 2023 16:45:08 +0200 Subject: [PATCH 05/13] Python: Remove trailing `,` --- .../library-tests/frameworks/rest_framework/taint_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py b/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py index 66167a75539..8100cad0e9e 100644 --- a/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py +++ b/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py @@ -131,8 +131,8 @@ class MyModelViewSet(ModelViewSet): # fake setup, you can't actually run this urlpatterns = [ path("test-taint/", test_taint), # $ routeSetup="test-taint/" - path("ClassView/", MyClass.as_view()), # $ routeSetup="ClassView/", - path("MyModelViewSet/", MyModelViewSet.as_view()) # $ routeSetup="MyModelViewSet/", + path("ClassView/", MyClass.as_view()), # $ routeSetup="ClassView/" + path("MyModelViewSet/", MyModelViewSet.as_view()) # $ routeSetup="MyModelViewSet/" ] # tests with no route-setup, but we can still tell that these are using Django REST From 24687b41562024bf0f3b597dcf174c290eaee533 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 23 Oct 2023 16:49:20 +0200 Subject: [PATCH 06/13] Python: Add test highlighting missing routed parameter flow to `**kwargs` parameter of request handler function --- .../frameworks/django-v2-v3/taint_test.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/python/ql/test/library-tests/frameworks/django-v2-v3/taint_test.py b/python/ql/test/library-tests/frameworks/django-v2-v3/taint_test.py index aa6f78be509..5b50ddfe098 100644 --- a/python/ql/test/library-tests/frameworks/django-v2-v3/taint_test.py +++ b/python/ql/test/library-tests/frameworks/django-v2-v3/taint_test.py @@ -174,8 +174,20 @@ class ClassView(View): ) +def kwargs_param(request, **kwargs): # $ requestHandler + ensure_tainted( + kwargs, # $ MISSING: tainted + kwargs["foo"], # $ MISSING: tainted + kwargs["bar"] # $ MISSING: tainted + ) + + ensure_tainted(request) # $ tainted + + # fake setup, you can't actually run this urlpatterns = [ path("test-taint//", test_taint), # $ routeSetup="test-taint//" path("ClassView/", ClassView.as_view()), # $ routeSetup="ClassView/" + path("test-kwargs_param//", kwargs_param), # $ routeSetup="test-kwargs_param//" + ] From e8f548ab52f0880e06f404fd45ee702bdbe37d1c Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Mon, 23 Oct 2023 17:15:26 +0200 Subject: [PATCH 07/13] Python: Model routed parameter flow to *args and **kwargs in Django + rest framework --- python/ql/lib/semmle/python/Concepts.qll | 5 ++++- .../lib/semmle/python/frameworks/Django.qll | 21 +++++++++++++++---- .../python/frameworks/RestFramework.qll | 5 ++++- .../frameworks/django-v2-v3/taint_test.py | 8 +++---- 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/python/ql/lib/semmle/python/Concepts.qll b/python/ql/lib/semmle/python/Concepts.qll index 81c7bd2990e..f7acd89bd62 100644 --- a/python/ql/lib/semmle/python/Concepts.qll +++ b/python/ql/lib/semmle/python/Concepts.qll @@ -928,7 +928,10 @@ module Http { override Parameter getARoutedParameter() { result = rs.getARoutedParameter() and - result in [this.getArg(_), this.getArgByName(_)] + result in [ + this.getArg(_), this.getArgByName(_), this.getVararg().(Parameter), + this.getKwarg().(Parameter) + ] } override string getFramework() { result = rs.getFramework() } diff --git a/python/ql/lib/semmle/python/frameworks/Django.qll b/python/ql/lib/semmle/python/frameworks/Django.qll index 28aaa0a227d..e55f0b91ff5 100644 --- a/python/ql/lib/semmle/python/frameworks/Django.qll +++ b/python/ql/lib/semmle/python/frameworks/Django.qll @@ -2416,7 +2416,10 @@ module PrivateDjango { // 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 + result in [ + this.getArg(_), this.getArgByName(_), // + this.getVararg().(Parameter), this.getKwarg().(Parameter), // TODO: These sources should be modeled as storing content! + ] and not result = any(int i | i < this.getFirstPossibleRoutedParamIndex() | this.getArg(i)) } @@ -2452,13 +2455,20 @@ module PrivateDjango { // more FPs. If this turns out to be the wrong tradeoff, we can always change our mind. exists(DjangoRouteHandler routeHandler | routeHandler = this.getARequestHandler() | not exists(this.getUrlPattern()) and - result in [routeHandler.getArg(_), routeHandler.getArgByName(_)] and + result in [ + routeHandler.getArg(_), routeHandler.getArgByName(_), // + routeHandler.getVararg().(Parameter), routeHandler.getKwarg().(Parameter), // TODO: These sources should be modeled as storing content! + ] and not result = any(int i | i < routeHandler.getFirstPossibleRoutedParamIndex() | routeHandler.getArg(i)) ) or exists(string name | - result = this.getARequestHandler().getArgByName(name) and + ( + result = this.getARequestHandler().getKwarg() // TODO: These sources should be modeled as storing content! + or + result = this.getARequestHandler().getArgByName(name) + ) and exists(string match | match = this.getUrlPattern().regexpFind(pathRoutedParameterRegex(), _, _) and name = match.regexpCapture(pathRoutedParameterRegex(), 2) @@ -2475,7 +2485,10 @@ module PrivateDjango { // more FPs. If this turns out to be the wrong tradeoff, we can always change our mind. exists(DjangoRouteHandler routeHandler | routeHandler = this.getARequestHandler() | not exists(this.getUrlPattern()) and - result in [routeHandler.getArg(_), routeHandler.getArgByName(_)] and + result in [ + routeHandler.getArg(_), routeHandler.getArgByName(_), // + routeHandler.getVararg().(Parameter), routeHandler.getKwarg().(Parameter), // TODO: These sources should be modeled as storing content! + ] and not result = any(int i | i < routeHandler.getFirstPossibleRoutedParamIndex() | routeHandler.getArg(i)) ) diff --git a/python/ql/lib/semmle/python/frameworks/RestFramework.qll b/python/ql/lib/semmle/python/frameworks/RestFramework.qll index 4e327491ca6..dba5a63353a 100644 --- a/python/ql/lib/semmle/python/frameworks/RestFramework.qll +++ b/python/ql/lib/semmle/python/frameworks/RestFramework.qll @@ -172,7 +172,10 @@ private module RestFramework { // 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 + result in [ + this.getArg(_), this.getArgByName(_), // + this.getVararg().(Parameter), this.getKwarg().(Parameter), // TODO: These sources should be modeled as storing content! + ] and not result = any(int i | i < this.getFirstPossibleRoutedParamIndex() | this.getArg(i)) } diff --git a/python/ql/test/library-tests/frameworks/django-v2-v3/taint_test.py b/python/ql/test/library-tests/frameworks/django-v2-v3/taint_test.py index 5b50ddfe098..a6850269ea0 100644 --- a/python/ql/test/library-tests/frameworks/django-v2-v3/taint_test.py +++ b/python/ql/test/library-tests/frameworks/django-v2-v3/taint_test.py @@ -174,11 +174,11 @@ class ClassView(View): ) -def kwargs_param(request, **kwargs): # $ requestHandler +def kwargs_param(request, **kwargs): # $ requestHandler routedParameter=kwargs ensure_tainted( - kwargs, # $ MISSING: tainted - kwargs["foo"], # $ MISSING: tainted - kwargs["bar"] # $ MISSING: tainted + kwargs, # $ tainted + kwargs["foo"], # $ tainted + kwargs["bar"] # $ tainted ) ensure_tainted(request) # $ tainted From c51c15ae74853c4a36618e5cccc072f123c66154 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 21 Nov 2023 13:01:01 +0100 Subject: [PATCH 08/13] Python: Add test of routed parameters to *args Also move the **kwargs and *args test to a more appropriate file --- .../frameworks/django-v2-v3/routing_test.py | 37 +++++++++++++++++++ .../frameworks/django-v2-v3/taint_test.py | 12 ------ 2 files changed, 37 insertions(+), 12 deletions(-) diff --git a/python/ql/test/library-tests/frameworks/django-v2-v3/routing_test.py b/python/ql/test/library-tests/frameworks/django-v2-v3/routing_test.py index acdbe2cfa42..64542bb51f0 100644 --- a/python/ql/test/library-tests/frameworks/django-v2-v3/routing_test.py +++ b/python/ql/test/library-tests/frameworks/django-v2-v3/routing_test.py @@ -150,3 +150,40 @@ class UnknownViewSubclass(UnknownViewSuperclass): urlpatterns = [ path("UnknownViewSubclass/", UnknownViewSubclass.as_view()), # $ routeSetup="UnknownViewSubclass/" ] + +################################################################################ +# Routing to *args and **kwargs +################################################################################ + +def kwargs_param(request, **kwargs): # $ requestHandler routedParameter=kwargs + ensure_tainted( + kwargs, # $ tainted + kwargs["foo"], # $ tainted + kwargs["bar"] # $ tainted + ) + + ensure_tainted(request) # $ tainted + + +def star_args_param(request, *args): # $ requestHandler MISSING: routedParameter=args + ensure_tainted( + args, # $ MISSING: tainted + args[0], # $ MISSING: tainted + args[1], # $ MISSING: tainted + ) + ensure_tainted(request) # $ tainted + + +def star_args_param_check(request, foo, bar): # $ requestHandler routedParameter=foo routedParameter=bar + ensure_tainted( + foo, # $ tainted + bar, # $ tainted + ) + ensure_tainted(request) # $ tainted + + +urlpatterns = [ + path("test-kwargs_param//", kwargs_param), # $ routeSetup="test-kwargs_param//" + re_path("test-star_args_param/([^/]+)/(.+)", star_args_param), # $ routeSetup="test-star_args_param/([^/]+)/(.+)" + re_path("test-star_args_param_check/([^/]+)/(.+)", star_args_param_check), # $ routeSetup="test-star_args_param_check/([^/]+)/(.+)" +] diff --git a/python/ql/test/library-tests/frameworks/django-v2-v3/taint_test.py b/python/ql/test/library-tests/frameworks/django-v2-v3/taint_test.py index a6850269ea0..aa6f78be509 100644 --- a/python/ql/test/library-tests/frameworks/django-v2-v3/taint_test.py +++ b/python/ql/test/library-tests/frameworks/django-v2-v3/taint_test.py @@ -174,20 +174,8 @@ class ClassView(View): ) -def kwargs_param(request, **kwargs): # $ requestHandler routedParameter=kwargs - ensure_tainted( - kwargs, # $ tainted - kwargs["foo"], # $ tainted - kwargs["bar"] # $ tainted - ) - - ensure_tainted(request) # $ tainted - - # fake setup, you can't actually run this urlpatterns = [ path("test-taint//", test_taint), # $ routeSetup="test-taint//" path("ClassView/", ClassView.as_view()), # $ routeSetup="ClassView/" - path("test-kwargs_param//", kwargs_param), # $ routeSetup="test-kwargs_param//" - ] From 36a846ee32e372baa701ae6c4482f7052ffa5bdb Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 21 Nov 2023 13:08:45 +0100 Subject: [PATCH 09/13] Python: Fix django regex path handling --- .../lib/semmle/python/frameworks/Django.qll | 21 +++++++++++++------ .../frameworks/django-v2-v3/routing_test.py | 8 +++---- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/Django.qll b/python/ql/lib/semmle/python/frameworks/Django.qll index e55f0b91ff5..38e488a3ea5 100644 --- a/python/ql/lib/semmle/python/frameworks/Django.qll +++ b/python/ql/lib/semmle/python/frameworks/Django.qll @@ -2501,13 +2501,22 @@ module PrivateDjango { // either using named capture groups (passed as keyword arguments) or using // unnamed capture groups (passed as positional arguments) not exists(regex.getGroupName(_, _)) and - // first group will have group number 1 - result = - routeHandler - .getArg(routeHandler.getFirstPossibleRoutedParamIndex() - 1 + - regex.getGroupNumber(_, _)) + ( + // first group will have group number 1 + result = + routeHandler + .getArg(routeHandler.getFirstPossibleRoutedParamIndex() - 1 + + regex.getGroupNumber(_, _)) + or + result = routeHandler.getVararg() + ) or - result = routeHandler.getArgByName(regex.getGroupName(_, _)) + exists(regex.getGroupName(_, _)) and + ( + result = routeHandler.getArgByName(regex.getGroupName(_, _)) + or + result = routeHandler.getKwarg() + ) ) } } diff --git a/python/ql/test/library-tests/frameworks/django-v2-v3/routing_test.py b/python/ql/test/library-tests/frameworks/django-v2-v3/routing_test.py index 64542bb51f0..8f36f55ee26 100644 --- a/python/ql/test/library-tests/frameworks/django-v2-v3/routing_test.py +++ b/python/ql/test/library-tests/frameworks/django-v2-v3/routing_test.py @@ -165,11 +165,11 @@ def kwargs_param(request, **kwargs): # $ requestHandler routedParameter=kwargs ensure_tainted(request) # $ tainted -def star_args_param(request, *args): # $ requestHandler MISSING: routedParameter=args +def star_args_param(request, *args): # $ requestHandler routedParameter=args ensure_tainted( - args, # $ MISSING: tainted - args[0], # $ MISSING: tainted - args[1], # $ MISSING: tainted + args, # $ tainted + args[0], # $ tainted + args[1], # $ tainted ) ensure_tainted(request) # $ tainted From 1bc8a6de61b9da22f7e0a0278ecf194fcd661019 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 21 Nov 2023 13:46:23 +0100 Subject: [PATCH 10/13] Python: Fixup mistaken modelling --- python/ql/lib/semmle/python/frameworks/RestFramework.qll | 3 --- 1 file changed, 3 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/RestFramework.qll b/python/ql/lib/semmle/python/frameworks/RestFramework.qll index dba5a63353a..02c39a573f2 100644 --- a/python/ql/lib/semmle/python/frameworks/RestFramework.qll +++ b/python/ql/lib/semmle/python/frameworks/RestFramework.qll @@ -197,9 +197,6 @@ private module RestFramework { exists(RestFrameworkApiViewClass vc | this.getParameter() = vc.getARequestHandler().(PrivateDjango::DjangoRouteHandler).getRequestParam() - or - // retrieve(self, request, **kwargs) - this.getParameter() = vc.getARequestHandler().(PrivateDjango::DjangoRouteHandler).getKwarg() ) or // annotated with @api_view decorator From 37d03ee0f341771b73c77648fdf381367bc51ac0 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 21 Nov 2023 13:46:55 +0100 Subject: [PATCH 11/13] Python: Accept .expected changes Note that in this case, since there is a known `django.urls.path` route-setup, we know that the request-handler will only be passed keyword arguments, so it is not a mistake that `*args` is not considered a routed-parameter here (although it certainly wouldn't have hurt us if we did consider it a routed-parameter either). --- .../test/library-tests/frameworks/rest_framework/taint_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py b/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py index 8100cad0e9e..c4c12b387ee 100644 --- a/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py +++ b/python/ql/test/library-tests/frameworks/rest_framework/taint_test.py @@ -90,7 +90,7 @@ def test_taint(request: Request, routed_param): # $ requestHandler routedParamet class MyClass(APIView): - def initial(self, request, *args, **kwargs): # $ requestHandler + def initial(self, request, *args, **kwargs): # $ requestHandler routedParameter=kwargs # this method will be called before processing any request ensure_tainted(request) # $ tainted From a0867b4f662eb690111dceb36bbf2c568a40a77a Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 21 Nov 2023 13:54:02 +0100 Subject: [PATCH 12/13] Python: More HTTP request handler *args/**kwargs modeling I looked through all `override Parameter getARoutedParameter() {` in our codebase, and we now modeling *args/**kwargs for all of them :+1: --- .../ql/lib/semmle/python/frameworks/FastApi.qll | 3 +++ python/ql/lib/semmle/python/frameworks/Flask.qll | 9 +++++++++ .../ql/lib/semmle/python/frameworks/Tornado.qll | 16 ++++++++++++++-- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/python/ql/lib/semmle/python/frameworks/FastApi.qll b/python/ql/lib/semmle/python/frameworks/FastApi.qll index 07dbd982653..0c4599d89ad 100644 --- a/python/ql/lib/semmle/python/frameworks/FastApi.qll +++ b/python/ql/lib/semmle/python/frameworks/FastApi.qll @@ -66,6 +66,9 @@ private module FastApi { result = this.getARequestHandler().getArgByName(_) and // type-annotated with `Response` not any(Response::RequestHandlerParam src).asExpr() = result + or + // **kwargs + result = this.getARequestHandler().getKwarg() } override DataFlow::Node getUrlPatternArg() { diff --git a/python/ql/lib/semmle/python/frameworks/Flask.qll b/python/ql/lib/semmle/python/frameworks/Flask.qll index 9c2e6cf08ed..3feee0cf668 100644 --- a/python/ql/lib/semmle/python/frameworks/Flask.qll +++ b/python/ql/lib/semmle/python/frameworks/Flask.qll @@ -279,6 +279,9 @@ module Flask { name = match.regexpCapture(werkzeug_rule_re(), 4) ) ) + or + // **kwargs + result = this.getARequestHandler().getKwarg() } override string getFramework() { result = "Flask" } @@ -347,6 +350,12 @@ module Flask { // 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 = this.getArg(0) + or + // *args + result = this.getVararg() + or + // **kwargs + result = this.getKwarg() } override string getFramework() { result = "Flask" } diff --git a/python/ql/lib/semmle/python/frameworks/Tornado.qll b/python/ql/lib/semmle/python/frameworks/Tornado.qll index 73c016ed927..a8afbea57d9 100644 --- a/python/ql/lib/semmle/python/frameworks/Tornado.qll +++ b/python/ql/lib/semmle/python/frameworks/Tornado.qll @@ -416,7 +416,10 @@ module Tornado { // more FPs. If this turns out to be the wrong tradeoff, we can always change our mind. exists(Function requestHandler | requestHandler = this.getARequestHandler() | not exists(this.getUrlPattern()) and - result in [requestHandler.getArg(_), requestHandler.getArgByName(_)] and + result in [ + requestHandler.getArg(_), requestHandler.getArgByName(_), + requestHandler.getVararg().(Parameter), requestHandler.getKwarg().(Parameter) + ] and not result = requestHandler.getArg(0) ) or @@ -429,6 +432,12 @@ module Tornado { result = requestHandler.getArg(regex.getGroupNumber(_, _)) or result = requestHandler.getArgByName(regex.getGroupName(_, _)) + or + exists(regex.getGroupNumber(_, _)) and + result = requestHandler.getVararg() + or + exists(regex.getGroupName(_, _)) and + result = requestHandler.getKwarg() ) } } @@ -446,7 +455,10 @@ module Tornado { // 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 + result in [ + this.getArg(_), this.getArgByName(_), this.getVararg().(Parameter), + this.getKwarg().(Parameter) + ] and not result = this.getArg(0) } From 63fcaca82f2bdf9aa060c091fdc416f9b36d5998 Mon Sep 17 00:00:00 2001 From: Rasmus Wriedt Larsen Date: Tue, 21 Nov 2023 13:58:39 +0100 Subject: [PATCH 13/13] Python: add change-note --- .../change-notes/2023-11-21-request-handler-args-kwargs.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 python/ql/lib/change-notes/2023-11-21-request-handler-args-kwargs.md diff --git a/python/ql/lib/change-notes/2023-11-21-request-handler-args-kwargs.md b/python/ql/lib/change-notes/2023-11-21-request-handler-args-kwargs.md new file mode 100644 index 00000000000..7215f1a2ab3 --- /dev/null +++ b/python/ql/lib/change-notes/2023-11-21-request-handler-args-kwargs.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added modeling of `*args` and `**kwargs` as routed-parameters in request handlers for django/flask/FastAPI/tornado.