Merge pull request #5151 from RasmusWL/django-get-redirect-url

Python: Model get_redirect_url in django

Author: yoff

python/change-notes/2021-02-12-django-get_redirect_url.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Improved modeling of `django` to recognize request redirects from `get_redirect_url` on a `RedirectView` subclass.

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

@@ -2074,7 +2074,11 @@ private module Django {
       // 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
-      result.getName() = HTTP::httpVerbLower()
+      (
+        result.getName() = HTTP::httpVerbLower()
+        or
+        result.getName() = "get_redirect_url"
+      )
     }
 
     /**
@@ -2124,6 +2128,8 @@ private module Django {
   /**
    * A function that is a django route handler, meaning it handles incoming requests
    * with the django framework.
+   *
+   * Most functions take a django HttpRequest as a parameter (but not all).
    */
   private class DjangoRouteHandler extends Function {
     DjangoRouteHandler() {
@@ -2132,6 +2138,12 @@ private module Django {
       any(DjangoViewClass vc).getARequestHandler() = this
     }
 
+    /**
+     * Gets the index of the parameter where the first routed parameter can be passed --
+     * that is, the one just after any possible `self` or HttpRequest parameters.
+     */
+    int getFirstPossibleRoutedParamIndex() { result = 1 + this.getRequestParamIndex() }
+
     /** Gets the index of the request parameter. */
     int getRequestParamIndex() {
       not this.isMethod() and
@@ -2145,6 +2157,26 @@ private module Django {
     Parameter getRequestParam() { result = this.getArg(this.getRequestParamIndex()) }
   }
 
+  /**
+   * A method named `get_redirect_url` on a django view class.
+   *
+   * See https://docs.djangoproject.com/en/3.1/ref/class-based-views/base/#django.views.generic.base.RedirectView.get_redirect_url
+   *
+   * Note: this function only does something on a subclass of `RedirectView`, but since
+   * classes can be considered django view classes without us knowing their super-classes,
+   * we need to consider _any_ django view class. I don't expect any problems to come from this.
+   */
+  private class GetRedirectUrlFunction extends DjangoRouteHandler {
+    GetRedirectUrlFunction() {
+      this.getName() = "get_redirect_url" and
+      any(DjangoViewClass vc).getARequestHandler() = this
+    }
+
+    override int getFirstPossibleRoutedParamIndex() { result = 1 }
+
+    override int getRequestParamIndex() { none() }
+  }
+
   /** A data-flow node that sets up a route on a server, using the django framework. */
   abstract private class DjangoRouteSetup extends HTTP::Server::RouteSetup::Range, DataFlow::CfgNode {
     /** Gets the data-flow node that is used as the argument for the view handler. */
@@ -2175,7 +2207,7 @@ private module Django {
       // 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))
+      not result = any(int i | i < this.getFirstPossibleRoutedParamIndex() | this.getArg(i))
     }
 
     override string getFramework() { result = "Django" }
@@ -2215,7 +2247,8 @@ private module Django {
       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))
+        not result =
+          any(int i | i < routeHandler.getFirstPossibleRoutedParamIndex() | routeHandler.getArg(i))
       )
       or
       exists(string name |
@@ -2237,7 +2270,8 @@ private module Django {
       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))
+        not result =
+          any(int i | i < routeHandler.getFirstPossibleRoutedParamIndex() | routeHandler.getArg(i))
       )
       or
       exists(DjangoRouteHandler routeHandler, DjangoRouteRegex regex |
@@ -2249,7 +2283,9 @@ private module Django {
         not exists(regex.getGroupName(_, _)) and
         // first group will have group number 1
         result =
-          routeHandler.getArg(routeHandler.getRequestParamIndex() + regex.getGroupNumber(_, _))
+          routeHandler
+              .getArg(routeHandler.getFirstPossibleRoutedParamIndex() - 1 +
+                  regex.getGroupNumber(_, _))
         or
         result = routeHandler.getArgByName(regex.getGroupName(_, _))
       )
@@ -2445,4 +2481,31 @@ private module Django {
 
     override string getMimetypeDefault() { none() }
   }
+
+  // ---------------------------------------------------------------------------
+  // RedirectView handling
+  // ---------------------------------------------------------------------------
+  /**
+   * A return from a method named `get_redirect_url` on a django view class.
+   *
+   * Note that in reality, this only does something on a subclass of `RedirectView` --
+   * but until API graphs makes this easy to model, I took a shortcut in modeling
+   * preciseness.
+   *
+   * See https://docs.djangoproject.com/en/3.1/ref/class-based-views/base/#redirectview
+   */
+  private class DjangoRedirectViewGetRedirectUrlReturn extends HTTP::Server::HttpRedirectResponse::Range,
+    DataFlow::CfgNode {
+    DjangoRedirectViewGetRedirectUrlReturn() {
+      node = any(GetRedirectUrlFunction f).getAReturnValueFlowNode()
+    }
+
+    override DataFlow::Node getRedirectLocation() { result = this }
+
+    override DataFlow::Node getBody() { none() }
+
+    override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
+
+    override string getMimetypeDefault() { none() }
+  }
 }

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

@@ -1,3 +1,4 @@
+| response_test.py:61 | ok   | get_redirect_url | foo |
 | taint_test.py:8 | ok   | test_taint | bar |
 | taint_test.py:8 | ok   | test_taint | foo |
 | taint_test.py:9 | ok   | test_taint | baz |

python/ql/test/experimental/library-tests/frameworks/django-v1/response_test.py renamed to python/ql/test/experimental/library-tests/frameworks/django-v2-v3/response_test.py

@@ -1,4 +1,5 @@
 from django.http.response import HttpResponse, HttpResponseRedirect, HttpResponsePermanentRedirect, JsonResponse, HttpResponseNotFound
+from django.views.generic import RedirectView
 import django.shortcuts
 
 # Not an XSS sink, since the Content-Type is not "text/html"
@@ -54,6 +55,14 @@ def redirect_shortcut(request):
     return django.shortcuts.redirect(next) # $ HttpResponse HttpRedirectResponse redirectLocation=next
 
 
+class CustomRedirectView(RedirectView):
+
+    def get_redirect_url(self, foo): # $ requestHandler routedParameter=foo
+        ensure_tainted(foo)
+        next = "https://example.com/{}".format(foo)
+        return next # $ HttpResponse HttpRedirectResponse redirectLocation=next
+
+
 # Ensure that simple subclasses are still vuln to XSS
 def xss__not_found(request):
     return HttpResponseNotFound(request.GET.get("name"))  # $HttpResponse mimetype=text/html responseBody=Attribute()

python/ql/test/experimental/library-tests/frameworks/django-v2-v3/testapp/urls.py

@@ -15,4 +15,7 @@
 
     path("basic-view-handler/", views.MyBasicViewHandler.as_view()),  # $routeSetup="basic-view-handler/"
     path("custom-inheritance-view-handler/", views.MyViewHandlerWithCustomInheritance.as_view()),  # $routeSetup="custom-inheritance-view-handler/"
+
+    path("CustomRedirectView/<foo>", views.CustomRedirectView.as_view()),  # $routeSetup="CustomRedirectView/<foo>"
+    path("CustomRedirectView2/<foo>", views.CustomRedirectView2.as_view()),  # $routeSetup="CustomRedirectView2/<foo>"
 ]

python/ql/test/experimental/library-tests/frameworks/django-v2-v3/testapp/views.py

@@ -1,5 +1,5 @@
 from django.http import HttpRequest, HttpResponse
-from django.views import View
+from django.views.generic import View, RedirectView
 from django.views.decorators.csrf import csrf_exempt
 
 
@@ -32,3 +32,16 @@ class MyViewHandlerWithCustomInheritance(MyCustomViewBaseClass):
     def get(self, request: HttpRequest): # $ requestHandler
         print(self.request.GET)
         return HttpResponse("MyViewHandlerWithCustomInheritance: GET") # $ HttpResponse
+
+# RedirectView
+# See docs at https://docs.djangoproject.com/en/3.1/ref/class-based-views/base/#redirectview
+class CustomRedirectView(RedirectView):
+
+    def get_redirect_url(self, foo): # $ requestHandler routedParameter=foo
+        next = "https://example.com/{}".format(foo)
+        return next # $ HttpResponse HttpRedirectResponse redirectLocation=next
+
+
+class CustomRedirectView2(RedirectView):
+
+    url = "https://example.com/%(foo)s"