Python: Proper redirect taint sinks for Django

Also a major restructuring of the code. A bit controversial since it
renames/moves classes that are already public.

Fixes github#3466

Author: RasmusWL

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

@@ -11,10 +11,29 @@ private import semmle.python.web.django.Shared
 private import semmle.python.web.Http
 
 /**
- * Represents an argument to the `django.redirect` function.
+ * The URL argument for a call to the `django.shortcuts.redirect` function.
  */
-class DjangoRedirect extends HttpRedirectTaintSink {
-    override string toString() { result = "django.redirect" }
+class DjangoShortcutsRedirectSink extends HttpRedirectTaintSink {
+    override string toString() { result = "DjangoShortcutsRedirectSink" }
 
-    DjangoRedirect() { this = redirect().getACall().getAnArg() }
+    DjangoShortcutsRedirectSink() {
+        this = Value::named("django.shortcuts.redirect").(FunctionValue).getArgumentForCall(_, 0)
+    }
+}
+
+/**
+ * The URL argument when instantiating a Django Redirect Response.
+ */
+class DjangoRedirectResponseSink extends HttpRedirectTaintSink {
+    DjangoRedirectResponseSink() {
+        exists(CallNode call |
+            call = any(DjangoRedirectResponse rr).getACall()
+        |
+            this = call.getArg(0)
+            or
+            this = call.getArgByName("redirect_to")
+        )
+    }
+
+    override string toString() { result = "DjangoRedirectResponseSink" }
 }

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

@@ -4,38 +4,20 @@ import semmle.python.security.strings.Basic
 private import semmle.python.web.django.Shared
 private import semmle.python.web.Http
 
-/**
- * A django.http.response.Response object
- * This isn't really a "taint", but we use the value tracking machinery to
- * track the flow of response objects.
- */
-class DjangoResponse extends TaintKind {
-    DjangoResponse() { this = "django.response.HttpResponse" }
+/** INTERNAL class used for tracking a django response object. */
+private class DjangoResponseKind extends TaintKind {
+    DjangoResponseKind() { this = "django.response.HttpResponse" }
 }
 
-private ClassValue theDjangoHttpResponseClass() {
-    (
-        // version 1.x
-        result = Value::named("django.http.response.HttpResponse")
-        or
-        // version 2.x
-        // https://docs.djangoproject.com/en/2.2/ref/request-response/#httpresponse-objects
-        result = Value::named("django.http.HttpResponse")
-    ) and
-    // TODO: does this do anything? when could they be the same???
-    not result = theDjangoHttpRedirectClass()
-}
-
-/** internal class used for tracking a django response. */
+/** INTENRAL taint-source used for tracking a django response. */
 private class DjangoResponseSource extends TaintSource {
     DjangoResponseSource() {
-        exists(ClassValue cls |
-            cls.getASuperType() = theDjangoHttpResponseClass() and
+        exists(DjangoXSSVulnResponse cls |
             cls.getACall() = this
         )
     }
 
-    override predicate isSourceOf(TaintKind kind) { kind instanceof DjangoResponse }
+    override predicate isSourceOf(TaintKind kind) { kind instanceof DjangoResponseKind }
 
     override string toString() { result = "django.http.response.HttpResponse" }
 }
@@ -45,7 +27,7 @@ class DjangoResponseWrite extends HttpResponseTaintSink {
     DjangoResponseWrite() {
         exists(AttrNode meth, CallNode call |
             call.getFunction() = meth and
-            any(DjangoResponse response).taints(meth.getObject("write")) and
+            any(DjangoResponseKind response).taints(meth.getObject("write")) and
             this = call.getArg(0)
         )
     }
@@ -58,9 +40,8 @@ class DjangoResponseWrite extends HttpResponseTaintSink {
 /** An argument to initialization of a django response, which is vulnerable to external data (xss) */
 class DjangoResponseContent extends HttpResponseTaintSink {
     DjangoResponseContent() {
-        exists(CallNode call, ClassValue cls |
-            cls.getASuperType() = theDjangoHttpResponseClass() and
-            call.getFunction().pointsTo(cls)
+        exists(CallNode call, DjangoXSSVulnResponse cls |
+            call = cls.getACall()
         |
             call.getArg(0) = this
             or
@@ -75,7 +56,7 @@ class DjangoResponseContent extends HttpResponseTaintSink {
 
 class DjangoCookieSet extends CookieSet, CallNode {
     DjangoCookieSet() {
-        any(DjangoResponse r).taints(this.getFunction().(AttrNode).getObject("set_cookie"))
+        any(DjangoResponseKind r).taints(this.getFunction().(AttrNode).getObject("set_cookie"))
     }
 
     override string toString() { result = CallNode.super.toString() }

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

@@ -1,12 +1,39 @@
 import python
 
-/** django.shortcuts.redirect */
-FunctionValue redirect() { result = Value::named("django.shortcuts.redirect") }
+/** A Class that is a Django Response (subclass of `django.http.HttpResponse`). */
+class DjangoResponse extends ClassValue {
+    DjangoResponse() {
+        exists(ClassValue base |
+            // version 1.x
+            base = Value::named("django.http.response.HttpResponse")
+            or
+            // version 2.x and 3.x
+            // https://docs.djangoproject.com/en/2.2/ref/request-response/#httpresponse-objects
+            base = Value::named("django.http.HttpResponse")
+        |
+            this.getASuperType() = base
+        )
+    }
+}
+
+/** A Class that is a Django Redirect Response (subclass of `django.http.HttpResponseRedirectBase`). */
+class DjangoRedirectResponse extends DjangoResponse {
+    DjangoRedirectResponse() {
+        exists(ClassValue base |
+            // version 1.x
+            base = Value::named("django.http.response.HttpResponseRedirectBase")
+            or
+            // version 2.x and 3.x
+            base = Value::named("django.http.HttpResponseRedirectBase")
+        |
+            this.getASuperType() = base
+        )
+    }
+}
 
-ClassValue theDjangoHttpRedirectClass() {
-    // version 1.x
-    result = Value::named("django.http.response.HttpResponseRedirectBase")
-    or
-    // version 2.x
-    result = Value::named("django.http.HttpResponseRedirectBase")
+/** A Class that is a Django Response, and is vulnerable to XSS. */
+class DjangoXSSVulnResponse extends DjangoResponse {
+    DjangoXSSVulnResponse() {
+        not this instanceof DjangoRedirectResponse
+    }
 }

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

@@ -1,2 +1,4 @@
-| test_1x.py:13:21:13:24 | django.redirect | externally controlled string |
-| test_2x_3x.py:13:21:13:24 | django.redirect | externally controlled string |
+| test_1x.py:13:21:13:24 | DjangoShortcutsRedirectSink | externally controlled string |
+| test_2x_3x.py:13:21:13:24 | DjangoShortcutsRedirectSink | externally controlled string |
+| views_1x.py:99:33:99:55 | DjangoRedirectResponseSink | externally controlled string |
+| views_2x_3x.py:120:33:120:55 | DjangoRedirectResponseSink | externally controlled string |

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

@@ -11,7 +11,6 @@
 | views_1x.py:85:25:85:55 | django.Response(...) | externally controlled string |
 | views_1x.py:90:25:90:33 | django.Response(...) | externally controlled string |
 | views_1x.py:94:25:94:58 | django.Response(...) | externally controlled string |
-| views_1x.py:99:33:99:55 | django.Response(...) | externally controlled string |
 | views_1x.py:103:33:103:55 | django.Response(...) | externally controlled string |
 | views_2x_3x.py:8:25:8:63 | django.Response(...) | externally controlled string |
 | views_2x_3x.py:12:25:12:52 | django.Response(...) | externally controlled string |
@@ -29,5 +28,4 @@
 | views_2x_3x.py:106:25:106:55 | django.Response(...) | externally controlled string |
 | views_2x_3x.py:111:25:111:33 | django.Response(...) | externally controlled string |
 | views_2x_3x.py:115:25:115:58 | django.Response(...) | externally controlled string |
-| views_2x_3x.py:120:33:120:55 | django.Response(...) | externally controlled string |
 | views_2x_3x.py:124:33:124:55 | django.Response(...) | externally controlled string |

python/ql/test/library-tests/web/django/views_2x_3x.py

@@ -117,7 +117,7 @@ def fp_manual_content_type(reuqest):
 # XSS FP reported in https://github.com/github/codeql/issues/3466
 # Note: This should be a open-redirect sink, but not a XSS sink.
 def fp_redirect(request):
-    return HttpResponseRedirect(request.GET.get("next")) # TODO
+    return HttpResponseRedirect(request.GET.get("next"))
 
 # Ensure that subclasses are still vuln to XSS
 def tp_not_found(request):