Python: Model get_redirect_url in django

Author: RasmusWL

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. */
@@ -2173,7 +2205,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))
     }
   }
 
@@ -2211,7 +2243,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 |
@@ -2233,7 +2266,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 |
@@ -2245,7 +2279,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(_, _))
       )
@@ -2441,4 +2477,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-v2-v3/response_test.py

@@ -57,9 +57,10 @@ def redirect_shortcut(request):
 
 class CustomRedirectView(RedirectView):
 
-    def get_redirect_url(self, foo): # $ MISSING: routedParameter=foo
+    def get_redirect_url(self, foo): # $ requestHandler routedParameter=foo
+        ensure_tainted(foo)
         next = "https://example.com/{}".format(foo)
-        return next # $ MISSING: HttpResponse HttpRedirectResponse redirectLocation=next
+        return next # $ HttpResponse HttpRedirectResponse redirectLocation=next
 
 
 # Ensure that simple subclasses are still vuln to XSS

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

@@ -37,9 +37,9 @@ def get(self, request: HttpRequest): # $ requestHandler
 # 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): # $ MISSING: routedParameter=foo
+    def get_redirect_url(self, foo): # $ requestHandler routedParameter=foo
         next = "https://example.com/{}".format(foo)
-        return next # $ MISSING: HttpResponse HttpRedirectResponse redirectLocation=next
+        return next # $ HttpResponse HttpRedirectResponse redirectLocation=next
 
 
 class CustomRedirectView2(RedirectView):