Python: Model Django response sinks that are not vuln to XSS

Since HttpResponse is not *only* used for XSS, it is still valuable to know the
content is send as part of the response.

The *proper* solution to this problem of not all HttpResponses being vulnerable
to XSS is probably to define a new abstract class in Http.qll called
HttpResponseXSSVulnerableSink (or similar). I would like to model a few more
libraries/frameworks before fully comitting to an approach though.

python/ql/src/Security/CWE-079/ReflectedXss.ql

@@ -28,7 +28,12 @@ class ReflectedXssConfiguration extends TaintTracking::Configuration {
         source instanceof HttpRequestTaintSource
     }
 
-    override predicate isSink(TaintTracking::Sink sink) { sink instanceof HttpResponseTaintSink }
+    override predicate isSink(TaintTracking::Sink sink) {
+        sink instanceof HttpResponseTaintSink and
+        not sink instanceof DjangoResponseContent
+        or
+        sink instanceof DjangoResponseContentXSSVulnerable
+    }
 }
 
 from ReflectedXssConfiguration config, TaintedPathSource src, TaintedPathSink sink

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

@@ -27,7 +27,7 @@ class DjangoShortcutsRedirectSink extends HttpRedirectTaintSink {
 class DjangoRedirectResponseSink extends HttpRedirectTaintSink {
     DjangoRedirectResponseSink() {
         exists(CallNode call |
-            call = any(DjangoRedirectResponse rr).getACall()
+            call = any(DjangoRedirectResponseClass cls).getACall()
         |
             this = call.getArg(0)
             or

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

@@ -12,7 +12,7 @@ private class DjangoResponseKind extends TaintKind {
 /** INTENRAL taint-source used for tracking a django response. */
 private class DjangoResponseSource extends TaintSource {
     DjangoResponseSource() {
-        exists(DjangoXSSVulnerableResponse cls |
+        exists(DjangoContentResponseClass cls |
             cls.getACall() = this
         )
     }
@@ -37,21 +37,16 @@ class DjangoResponseWrite extends HttpResponseTaintSink {
     override string toString() { result = "django.Response.write(...)" }
 }
 
-/** An argument to initialization of a django response, which is vulnerable to external data (xss) */
+/**
+ * An argument to initialization of a django response.
+ */
 class DjangoResponseContent extends HttpResponseTaintSink {
+    DjangoContentResponseClass cls;
+    CallNode call;
+
     DjangoResponseContent() {
-        exists(CallNode call, DjangoXSSVulnerableResponse cls |
-            call = cls.getACall() and
-            this = cls.getContentArg(call) and
-            (
-                not exists(cls.getContentTypeArg(call))
-                or
-                exists(StringValue s |
-                    cls.getContentTypeArg(call).pointsTo(s) and
-                    s.getText().indexOf("text/html") = 0
-                )
-            )
-        )
+        call = cls.getACall() and
+        this = cls.getContentArg(call)
     }
 
     override predicate sinks(TaintKind kind) { kind instanceof StringKind }
@@ -59,6 +54,22 @@ class DjangoResponseContent extends HttpResponseTaintSink {
     override string toString() { result = "django.Response(...)" }
 }
 
+/**
+ * An argument to initialization of a django response, which is vulnerable to external data (XSS).
+ */
+class DjangoResponseContentXSSVulnerable extends DjangoResponseContent {
+    override DjangoXSSVulnerableResponseClass cls;
+
+    DjangoResponseContentXSSVulnerable() {
+        not exists(cls.getContentTypeArg(call))
+        or
+        exists(StringValue s |
+            cls.getContentTypeArg(call).pointsTo(s) and
+            s.getText().indexOf("text/html") = 0
+        )
+    }
+}
+
 class DjangoCookieSet extends CookieSet, CallNode {
     DjangoCookieSet() {
         any(DjangoResponseKind r).taints(this.getFunction().(AttrNode).getObject("set_cookie"))

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

@@ -1,25 +1,8 @@
 import python
 
-/** A Class that is a Django Response (subclass of `django.http.HttpResponse`). */
-class DjangoResponse extends ClassValue {
-    ClassValue base;
-
-    DjangoResponse() {
-        (
-            // 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")
-        ) and
-        this.getASuperType() = base
-    }
-}
-
-/** A Class that is a Django Redirect Response (subclass of `django.http.HttpResponseRedirectBase`). */
-class DjangoRedirectResponse extends DjangoResponse {
-    DjangoRedirectResponse() {
+/** A class that is a Django Redirect Response (subclass of `django.http.HttpResponseRedirectBase`). */
+class DjangoRedirectResponseClass extends ClassValue {
+    DjangoRedirectResponseClass() {
         exists(ClassValue redirect_base |
             // version 1.x
             redirect_base = Value::named("django.http.response.HttpResponseRedirectBase")
@@ -32,32 +15,57 @@ class DjangoRedirectResponse extends DjangoResponse {
     }
 }
 
+/**
+ * A class that is a Django Response, which can contain content.
+ * A subclass of `django.http.HttpResponse` that is not a `DjangoRedirectResponseClass`.
+ */
+class DjangoContentResponseClass extends ClassValue {
+    ClassValue base;
+
+    DjangoContentResponseClass() {
+        (
+            // 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")
+        ) and
+        this.getASuperType() = base
+    }
+
+    // The reason these two method are defined in this class (and not in the Sink
+    // definition that uses this class), is that if we were to add support for
+    // `django.http.response.HttpResponseNotAllowed` it would make much more sense to add
+    // the custom logic in this class (or subclass), than to handle all of it in the sink
+    // definition.
+
+    /** Gets the `content` argument of a `call` to the constructor */
+    ControlFlowNode getContentArg(CallNode call) { none() }
+
+    /** Gets the `content_type` argument of a `call` to the constructor */
+    ControlFlowNode getContentTypeArg(CallNode call) { none() }
+}
+
 /** A Class that is a Django Response, and is vulnerable to XSS. */
-class DjangoXSSVulnerableResponse extends DjangoResponse {
-    DjangoXSSVulnerableResponse() {
+class DjangoXSSVulnerableResponseClass extends DjangoContentResponseClass{
+    DjangoXSSVulnerableResponseClass() {
         // We want to avoid FPs on subclasses that are not exposed to XSS, for example `JsonResponse`.
         // The easiest way is to disregard any subclass that has a special `__init__` method.
         // It's not guaranteed to remove all FPs, or not to generate FNs, but compared to our
         // previous implementation that would treat 0-th argument to _any_ subclass as a sink,
         // this gets us much closer to reality.
         this.lookup("__init__") = base.lookup("__init__") and
-        not this instanceof DjangoRedirectResponse
+        not this instanceof DjangoRedirectResponseClass
     }
 
-    // The reason these two method are defined in this class (and no in the Sink
-    // definition that uses this class), is that if we were to add support for `HttpResponseNotAllowed`
-    // it would make much more sense to add the custom logic in this class (or subclass), than to handle all of it
-    // in the sink definition.
-
-    /** Gets the `content` argument of a `call` to the constructor */
-    ControlFlowNode getContentArg(CallNode call) {
+    override ControlFlowNode getContentArg(CallNode call) {
         result = call.getArg(0)
         or
         result = call.getArgByName("content")
     }
 
-    /** Gets the `content_type` argument of a `call` to the constructor */
-    ControlFlowNode getContentTypeArg(CallNode call) {
+    override ControlFlowNode getContentTypeArg(CallNode call) {
         result = call.getArg(1)
         or
         result = call.getArgByName("content_type")

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

@@ -8,6 +8,8 @@
 | views_1x.py:45:25:45:70 | django.Response(...) | externally controlled string |
 | views_1x.py:66:25:66:55 | django.Response(...) | externally controlled string |
 | views_1x.py:75:25:75:33 | 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:103:33:103:55 | django.Response(...) | externally controlled string |
 | views_1x.py:107:25:107:47 | django.Response(...) | externally controlled string |
 | views_2x_3x.py:8:25:8:63 | django.Response(...) | externally controlled string |
@@ -23,5 +25,7 @@
 | views_2x_3x.py:82:25:82:69 | django.Response(...) | externally controlled string |
 | views_2x_3x.py:85:25:85:64 | django.Response(...) | externally controlled string |
 | views_2x_3x.py:88:25:88:32 | 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:124:33:124:55 | django.Response(...) | externally controlled string |
 | views_2x_3x.py:128:25:128:47 | django.Response(...) | externally controlled string |