Merge pull request #4605 from RasmusWL/python-fix-django-response-modeling

Python: fix django response modeling

Author: yoff

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

@@ -629,8 +629,7 @@ private module Django {
             t.start() and
             result = response_attr("HttpResponse")
             or
-            // TODO: remove/expand this part of the template as needed
-            // Handle `http.HttpResponse` alias
+            // Handle `django.http.HttpResponse` alias
             t.start() and
             result = http_attr("HttpResponse")
             or
@@ -670,7 +669,7 @@ private module Django {
               result.asCfgNode() in [node.getArg(1), node.getArgByName("content_type")]
             }
 
-            override string getMimetypeDefault() { result = "text/html; charset=utf-8" }
+            override string getMimetypeDefault() { result = "text/html" }
           }
 
           /** Gets a reference to an instance of `django.http.response.HttpResponse`. */
@@ -700,8 +699,7 @@ private module Django {
             t.start() and
             result = response_attr("HttpResponseRedirect")
             or
-            // TODO: remove/expand this part of the template as needed
-            // Handle `http.HttpResponseRedirect` alias
+            // Handle `django.http.HttpResponseRedirect` alias
             t.start() and
             result = http_attr("HttpResponseRedirect")
             or
@@ -732,13 +730,16 @@ private module Django {
             ClassInstantiation() { node.getFunction() = classRef().asCfgNode() }
 
             override DataFlow::Node getBody() {
-              result.asCfgNode() in [node.getArg(0), node.getArgByName("redirect_to")]
+              // note that even though browsers like Chrome usually doesn't fetch the
+              // content of a redirect, it is possible to observe the body (for example,
+              // with cURL).
+              result.asCfgNode() in [node.getArg(1), node.getArgByName("content")]
             }
 
             // How to support the `headers` argument here?
             override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
 
-            override string getMimetypeDefault() { result = "text/html; charset=utf-8" }
+            override string getMimetypeDefault() { result = "text/html" }
           }
 
           /** Gets a reference to an instance of `django.http.response.HttpResponseRedirect`. */
@@ -764,8 +765,7 @@ private module Django {
             t.start() and
             result = response_attr("HttpResponsePermanentRedirect")
             or
-            // TODO: remove/expand this part of the template as needed
-            // Handle `http.HttpResponsePermanentRedirect` alias
+            // Handle `django.http.HttpResponsePermanentRedirect` alias
             t.start() and
             result = http_attr("HttpResponsePermanentRedirect")
             or
@@ -796,13 +796,16 @@ private module Django {
             ClassInstantiation() { node.getFunction() = classRef().asCfgNode() }
 
             override DataFlow::Node getBody() {
-              result.asCfgNode() in [node.getArg(0), node.getArgByName("redirect_to")]
+              // note that even though browsers like Chrome usually doesn't fetch the
+              // content of a redirect, it is possible to observe the body (for example,
+              // with cURL).
+              result.asCfgNode() in [node.getArg(1), node.getArgByName("content")]
             }
 
             // How to support the `headers` argument here?
             override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
 
-            override string getMimetypeDefault() { result = "text/html; charset=utf-8" }
+            override string getMimetypeDefault() { result = "text/html" }
           }
 
           /** Gets a reference to an instance of `django.http.response.HttpResponsePermanentRedirect`. */
@@ -829,7 +832,7 @@ private module Django {
             result = response_attr("HttpResponseNotModified")
             or
             // TODO: remove/expand this part of the template as needed
-            // Handle `http.HttpResponseNotModified` alias
+            // Handle `django.http.HttpResponseNotModified` alias
             t.start() and
             result = http_attr("HttpResponseNotModified")
             or
@@ -890,8 +893,7 @@ private module Django {
             t.start() and
             result = response_attr("HttpResponseBadRequest")
             or
-            // TODO: remove/expand this part of the template as needed
-            // Handle `http.HttpResponseBadRequest` alias
+            // Handle `django.http.HttpResponseBadRequest` alias
             t.start() and
             result = http_attr("HttpResponseBadRequest")
             or
@@ -928,7 +930,7 @@ private module Django {
             // How to support the `headers` argument here?
             override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
 
-            override string getMimetypeDefault() { result = "text/html; charset=utf-8" }
+            override string getMimetypeDefault() { result = "text/html" }
           }
 
           /** Gets a reference to an instance of `django.http.response.HttpResponseBadRequest`. */
@@ -954,8 +956,7 @@ private module Django {
             t.start() and
             result = response_attr("HttpResponseNotFound")
             or
-            // TODO: remove/expand this part of the template as needed
-            // Handle `http.HttpResponseNotFound` alias
+            // Handle `django.http.HttpResponseNotFound` alias
             t.start() and
             result = http_attr("HttpResponseNotFound")
             or
@@ -992,7 +993,7 @@ private module Django {
             // How to support the `headers` argument here?
             override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
 
-            override string getMimetypeDefault() { result = "text/html; charset=utf-8" }
+            override string getMimetypeDefault() { result = "text/html" }
           }
 
           /** Gets a reference to an instance of `django.http.response.HttpResponseNotFound`. */
@@ -1018,8 +1019,7 @@ private module Django {
             t.start() and
             result = response_attr("HttpResponseForbidden")
             or
-            // TODO: remove/expand this part of the template as needed
-            // Handle `http.HttpResponseForbidden` alias
+            // Handle `django.http.HttpResponseForbidden` alias
             t.start() and
             result = http_attr("HttpResponseForbidden")
             or
@@ -1056,7 +1056,7 @@ private module Django {
             // How to support the `headers` argument here?
             override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
 
-            override string getMimetypeDefault() { result = "text/html; charset=utf-8" }
+            override string getMimetypeDefault() { result = "text/html" }
           }
 
           /** Gets a reference to an instance of `django.http.response.HttpResponseForbidden`. */
@@ -1082,8 +1082,7 @@ private module Django {
             t.start() and
             result = response_attr("HttpResponseNotAllowed")
             or
-            // TODO: remove/expand this part of the template as needed
-            // Handle `http.HttpResponseNotAllowed` alias
+            // Handle `django.http.HttpResponseNotAllowed` alias
             t.start() and
             result = http_attr("HttpResponseNotAllowed")
             or
@@ -1121,7 +1120,7 @@ private module Django {
             // How to support the `headers` argument here?
             override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
 
-            override string getMimetypeDefault() { result = "text/html; charset=utf-8" }
+            override string getMimetypeDefault() { result = "text/html" }
           }
 
           /** Gets a reference to an instance of `django.http.response.HttpResponseNotAllowed`. */
@@ -1147,8 +1146,7 @@ private module Django {
             t.start() and
             result = response_attr("HttpResponseGone")
             or
-            // TODO: remove/expand this part of the template as needed
-            // Handle `http.HttpResponseGone` alias
+            // Handle `django.http.HttpResponseGone` alias
             t.start() and
             result = http_attr("HttpResponseGone")
             or
@@ -1185,7 +1183,7 @@ private module Django {
             // How to support the `headers` argument here?
             override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
 
-            override string getMimetypeDefault() { result = "text/html; charset=utf-8" }
+            override string getMimetypeDefault() { result = "text/html" }
           }
 
           /** Gets a reference to an instance of `django.http.response.HttpResponseGone`. */
@@ -1211,8 +1209,7 @@ private module Django {
             t.start() and
             result = response_attr("HttpResponseServerError")
             or
-            // TODO: remove/expand this part of the template as needed
-            // Handle `http.HttpResponseServerError` alias
+            // Handle `django.http.HttpResponseServerError` alias
             t.start() and
             result = http_attr("HttpResponseServerError")
             or
@@ -1249,7 +1246,7 @@ private module Django {
             // How to support the `headers` argument here?
             override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
 
-            override string getMimetypeDefault() { result = "text/html; charset=utf-8" }
+            override string getMimetypeDefault() { result = "text/html" }
           }
 
           /** Gets a reference to an instance of `django.http.response.HttpResponseServerError`. */
@@ -1275,8 +1272,7 @@ private module Django {
             t.start() and
             result = response_attr("JsonResponse")
             or
-            // TODO: remove/expand this part of the template as needed
-            // Handle `http.JsonResponse` alias
+            // Handle `django.http.JsonResponse` alias
             t.start() and
             result = http_attr("JsonResponse")
             or
@@ -1342,8 +1338,7 @@ private module Django {
             t.start() and
             result = response_attr("StreamingHttpResponse")
             or
-            // TODO: remove/expand this part of the template as needed
-            // Handle `http.StreamingHttpResponse` alias
+            // Handle `django.http.StreamingHttpResponse` alias
             t.start() and
             result = http_attr("StreamingHttpResponse")
             or
@@ -1380,7 +1375,7 @@ private module Django {
             // How to support the `headers` argument here?
             override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
 
-            override string getMimetypeDefault() { result = "text/html; charset=utf-8" }
+            override string getMimetypeDefault() { result = "text/html" }
           }
 
           /** Gets a reference to an instance of `django.http.response.StreamingHttpResponse`. */
@@ -1406,8 +1401,7 @@ private module Django {
             t.start() and
             result = response_attr("FileResponse")
             or
-            // TODO: remove/expand this part of the template as needed
-            // Handle `http.FileResponse` alias
+            // Handle `django.http.FileResponse` alias
             t.start() and
             result = http_attr("FileResponse")
             or
@@ -1444,7 +1438,10 @@ private module Django {
             // How to support the `headers` argument here?
             override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
 
-            override string getMimetypeDefault() { result = "text/html; charset=utf-8" }
+            override string getMimetypeDefault() {
+              // see https://github.com/django/django/blob/ebb08d19424c314c75908bc6048ff57c2f872269/django/http/response.py#L471-L479
+              result = "application/octet-stream"
+            }
           }
 
           /** Gets a reference to an instance of `django.http.response.FileResponse`. */

python/ql/test/experimental/library-tests/frameworks/django-v1/response_test.py

@@ -18,19 +18,27 @@ def safe__manual_content_type(request):
 # XSS FP reported in https://github.com/github/codeql/issues/3466
 # Note: This should be an open-redirect sink, but not an XSS sink.
 def or__redirect(request):
-    return HttpResponseRedirect(request.GET.get("next"))  # $HttpResponse mimetype="text/html; charset=utf-8" responseBody=Attribute()
+    return HttpResponseRedirect(request.GET.get("next"))  # $HttpResponse mimetype=text/html
+
+def information_exposure_through_redirect(request, as_kw=False):
+    # This is a contrived example, but possible
+    private = "private"
+    if as_kw:
+        return HttpResponseRedirect(request.GET.get("next"), content=private)  # $HttpResponse mimetype=text/html responseBody=private
+    else:
+        return HttpResponseRedirect(request.GET.get("next"), private)  # $HttpResponse mimetype=text/html responseBody=private
 
 # Ensure that simple subclasses are still vuln to XSS
 def xss__not_found(request):
-    return HttpResponseNotFound(request.GET.get("name"))  # $HttpResponse mimetype="text/html; charset=utf-8" responseBody=Attribute()
+    return HttpResponseNotFound(request.GET.get("name"))  # $HttpResponse mimetype=text/html responseBody=Attribute()
 
 # Ensure we still have an XSS sink when manually setting the content_type to HTML
 def xss__manual_response_type(request):
     return HttpResponse(request.GET.get("name"), content_type="text/html; charset=utf-8")  # $HttpResponse mimetype=text/html responseBody=Attribute()
 
 def xss__write(request):
-    response = HttpResponse()  # $HttpResponse mimetype="text/html; charset=utf-8"
-    response.write(request.GET.get("name"))  # $HttpResponse mimetype="text/html; charset=utf-8" responseBody=Attribute()
+    response = HttpResponse()  # $HttpResponse mimetype=text/html
+    response.write(request.GET.get("name"))  # $HttpResponse mimetype=text/html responseBody=Attribute()
 
 # This is safe but probably a bug if the argument to `write` is not a result of `json.dumps` or similar.
 def safe__write_json(request):
@@ -50,4 +58,4 @@ def __init__(self, banner, content, *args, **kwargs):
         super().__init__(content, *args, content_type="text/html", **kwargs)
 
 def safe__custom_json_response(request):
-    return CustomJsonResponse("ACME Responses", {"foo": request.GET.get("foo")})  # $HttpResponse mimetype=application/json MISSING: responseBody=Dict SPURIOUS: responseBody="ACME Responses"
+    return CustomJsonResponse("ACME Responses", {"foo": request.GET.get("foo")})  # $HttpResponse mimetype=application/json MISSING: responseBody=Dict SPURIOUS: responseBody="ACME Responses"