Python: Reduce FPs in Django due to bad XSS taint-sinks

Fixes github/codeql-python-team#38

Author: RasmusWL

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(DjangoXSSVulnResponse cls |
+        exists(DjangoXSSVulnerableResponse cls |
             cls.getACall() = this
         )
     }
@@ -40,12 +40,17 @@ 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, DjangoXSSVulnResponse cls |
-            call = cls.getACall()
-        |
-            call.getArg(0) = this
-            or
-            call.getArgByName("content") = this
+        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
+                )
+            )
         )
     }
 

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

@@ -2,38 +2,64 @@ import python
 
 /** A Class that is a Django Response (subclass of `django.http.HttpResponse`). */
 class DjangoResponse extends ClassValue {
+    ClassValue base;
+
     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
-        )
+        ) and
+        this.getASuperType() = base
     }
 }
 
 /** A Class that is a Django Redirect Response (subclass of `django.http.HttpResponseRedirectBase`). */
 class DjangoRedirectResponse extends DjangoResponse {
     DjangoRedirectResponse() {
-        exists(ClassValue base |
+        exists(ClassValue redirect_base |
             // version 1.x
-            base = Value::named("django.http.response.HttpResponseRedirectBase")
+            redirect_base = Value::named("django.http.response.HttpResponseRedirectBase")
             or
             // version 2.x and 3.x
-            base = Value::named("django.http.HttpResponseRedirectBase")
+            redirect_base = Value::named("django.http.HttpResponseRedirectBase")
         |
-            this.getASuperType() = base
+            this.getASuperType() = redirect_base
         )
     }
 }
 
 /** A Class that is a Django Response, and is vulnerable to XSS. */
-class DjangoXSSVulnResponse extends DjangoResponse {
-    DjangoXSSVulnResponse() {
+class DjangoXSSVulnerableResponse extends DjangoResponse {
+    DjangoXSSVulnerableResponse() {
+        // 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
     }
+
+    // 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) {
+        result = call.getArg(0)
+        or
+        result = call.getArgByName("content")
+    }
+
+    /** Gets the `content_type` argument of a `call` to the constructor */
+    ControlFlowNode getContentTypeArg(CallNode call) {
+        result = call.getArg(1)
+        or
+        result = call.getArgByName("content_type")
+    }
 }

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

@@ -8,10 +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: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: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 |
 | views_2x_3x.py:12:25:12:52 | django.Response(...) | externally controlled string |
 | views_2x_3x.py:16:25:16:53 | django.Response(...) | externally controlled string |
@@ -25,7 +23,5 @@
 | 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: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:124:33:124:55 | django.Response(...) | externally controlled string |
+| views_2x_3x.py:128:25:128:47 | django.Response(...) | externally controlled string |

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

@@ -98,6 +98,10 @@ def fp_manual_content_type(reuqest):
 def fp_redirect(request):
     return HttpResponseRedirect(request.GET.get("next"))
 
-# Ensure that subclasses are still vuln to XSS
+# Ensure that simple subclasses are still vuln to XSS
 def tp_not_found(request):
     return HttpResponseNotFound(request.GET.get("name"))
+
+# Ensure we still have a XSS sink when manually setting the content_type to HTML
+def tp_manual_response_type(request):
+    return HttpResponse(request.GET.get("name"), content_type="text/html; charset=utf-8")

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

@@ -103,22 +103,26 @@ def not_valid_identifier(request):
 # FP reported in https://github.com/github/codeql-python-team/issues/38
 def fp_json_response(request):
     # implicitly sets Content-Type to "application/json"
-    return JsonResponse({"foo": request.GET.get("foo")}) # TODO
+    return JsonResponse({"foo": request.GET.get("foo")})
 
 # Not an XSS sink, since the Content-Type is not "text/html"
 def fp_manual_json_response(request):
     json_data = '{"json": "{}"}'.format(request.GET.get("foo"))
-    return HttpResponse(json_data, content_type="application/json") # TODO
+    return HttpResponse(json_data, content_type="application/json")
 
 # Not an XSS sink, since the Content-Type is not "text/html"
 def fp_manual_content_type(reuqest):
-    return HttpResponse('<img src="0" onerror="alert(1)">', content_type="text/plain") # TODO
+    return HttpResponse('<img src="0" onerror="alert(1)">', content_type="text/plain")
 
 # 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"))
 
-# Ensure that subclasses are still vuln to XSS
+# Ensure that simple subclasses are still vuln to XSS
 def tp_not_found(request):
     return HttpResponseNotFound(request.GET.get("name"))
+
+# Ensure we still have a XSS sink when manually setting the content_type to HTML
+def tp_manual_response_type(request):
+    return HttpResponse(request.GET.get("name"), content_type="text/html; charset=utf-8")

python/ql/test/query-tests/Security/lib/django/http/response.py

@@ -1,13 +1,21 @@
 class HttpResponseBase(object):
     status_code = 200
 
+    def __init__(self, content_type=None, status=None, reason=None, charset=None):
+        pass
+
 
 class HttpResponse(HttpResponseBase):
-    pass
+    def __init__(self, content=b"", *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        # Content is a bytestring. See the `content` property methods.
+        self.content = content
 
 
 class HttpResponseRedirectBase(HttpResponse):
-    pass
+    def __init__(self, redirect_to, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+        ...
 
 
 class HttpResponsePermanentRedirect(HttpResponseRedirectBase):