Merge pull request github#2858 from RasmusWL/python-support-django2

Approved by tausbn

Author: semmle-qlci

change-notes/1.24/analysis-python.md

@@ -4,6 +4,8 @@ The following changes in version 1.24 affect Python analysis in all applications
 
 ## General improvements
 
+Support for Django version 2.x and 3.x
+
 ## New queries
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |

python/ql/src/semmle/python/web/bottle/General.qll

@@ -36,7 +36,7 @@ class BottleRoute extends ControlFlowNode {
 
     Function getFunction() { bottle_route(this, _, result) }
 
-    Parameter getNamedArgument() {
+    Parameter getANamedArgument() {
         exists(string name, Function func |
             func = this.getFunction() and
             func.getArgByName(name) = result and

python/ql/src/semmle/python/web/bottle/Request.qll

@@ -71,7 +71,7 @@ class UntrustedFile extends TaintKind {
 /** Parameter to a bottle request handler function */
 class BottleRequestParameter extends HttpRequestTaintSource {
     BottleRequestParameter() {
-        exists(BottleRoute route | route.getNamedArgument() = this.(ControlFlowNode).getNode())
+        exists(BottleRoute route | route.getANamedArgument() = this.(ControlFlowNode).getNode())
     }
 
     override predicate isSourceOf(TaintKind kind) { kind instanceof UntrustedStringKind }

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

@@ -2,39 +2,142 @@ import python
 import semmle.python.regex
 import semmle.python.web.Http
 
-predicate django_route(CallNode call, ControlFlowNode regex, FunctionValue view) {
-    exists(FunctionValue url |
-        Value::named("django.conf.urls.url") = url and
-        url.getArgumentForCall(call, 0) = regex and
-        url.getArgumentForCall(call, 1).pointsTo(view)
+// TODO: Since django uses `path = partial(...)`, our analysis doesn't understand this is
+// a FunctionValue, so we can't use `FunctionValue.getArgumentForCall`
+// https://github.com/django/django/blob/master/django/urls/conf.py#L76
+abstract class DjangoRoute extends CallNode {
+    DjangoViewHandler getViewHandler() {
+        result = view_handler_from_view_arg(this.getArg(1))
+        or
+        result = view_handler_from_view_arg(this.getArgByName("view"))
+    }
+
+    abstract string getANamedArgument();
+
+    /**
+     * Get the number of positional arguments that will be passed to the view.
+     * Will only return a result if there are no named arguments.
+     */
+    abstract int getNumPositionalArguments();
+}
+
+/**
+ * For function based views -- also see `DjangoClassBasedViewHandler`
+ * https://docs.djangoproject.com/en/1.11/topics/http/views/
+ * https://docs.djangoproject.com/en/3.0/topics/http/views/
+ */
+class DjangoViewHandler extends PythonFunctionValue {
+
+    /** Gets the index of the 'request' argument */
+    int getRequestArgIndex() {
+        result = 0
+    }
+}
+
+/**
+ * Class based views
+ * https://docs.djangoproject.com/en/1.11/topics/class-based-views/
+ * https://docs.djangoproject.com/en/3.0/topics/class-based-views/
+ */
+private class DjangoViewClass extends ClassValue {
+    DjangoViewClass() {
+        Value::named("django.views.generic.View") = this.getASuperType()
+        or
+        Value::named("django.views.View") = this.getASuperType()
+    }
+}
+
+class DjangoClassBasedViewHandler extends DjangoViewHandler {
+    DjangoClassBasedViewHandler() {
+        exists(DjangoViewClass cls |
+            cls.lookup(httpVerbLower()) = this
+        )
+    }
+
+    override int getRequestArgIndex() {
+        // due to `self` being the first parameter
+        result = 1
+    }
+}
+
+/**
+ * Gets the function that will handle requests when `view_arg` is used as the view argument to a
+ * django route. That is, this methods handles Class-based Views and its `as_view()` function.
+ */
+private DjangoViewHandler view_handler_from_view_arg(ControlFlowNode view_arg) {
+    // Function-based view
+    result = view_arg.pointsTo()
+    or
+    // Class-based view
+    exists(ClassValue cls |
+        cls = view_arg.(CallNode).getFunction().(AttrNode).getObject("as_view").pointsTo() and
+        result = cls.lookup(httpVerbLower())
     )
 }
 
+// We need this "dummy" class, since otherwise the regex argument would not be considered
+// a regex (RegexString is abstract)
 class DjangoRouteRegex extends RegexString {
-    DjangoRouteRegex() { django_route(_, this.getAFlowNode(), _) }
+    DjangoRouteRegex() { exists(DjangoRegexRoute route | route.getRouteArg() = this.getAFlowNode()) }
 }
 
-class DjangoRoute extends CallNode {
-    DjangoRoute() { django_route(this, _, _) }
+class DjangoRegexRoute extends DjangoRoute {
+    ControlFlowNode route;
+
+    DjangoRegexRoute() {
+        exists(FunctionValue route_maker |
+            // Django 1.x: https://docs.djangoproject.com/en/1.11/ref/urls/#django.conf.urls.url
+            Value::named("django.conf.urls.url") = route_maker and
+            route_maker.getArgumentForCall(this, 0) = route
+        )
+        or
+        // Django 2.x and 3.x: https://docs.djangoproject.com/en/3.0/ref/urls/#re-path
+        this = Value::named("django.urls.re_path").getACall() and
+        (
+            route = this.getArg(0)
+            or
+            route = this.getArgByName("route")
+        )
+    }
 
-    FunctionValue getViewFunction() { django_route(this, _, result) }
+    ControlFlowNode getRouteArg() { result = route }
 
-    string getNamedArgument() {
-        exists(DjangoRouteRegex regex |
-            django_route(this, regex.getAFlowNode(), _) and
-            regex.getGroupName(_, _) = result
+    override string getANamedArgument() {
+        exists(DjangoRouteRegex regex | regex.getAFlowNode() = route |
+            result = regex.getGroupName(_, _)
         )
     }
 
-    /**
-      * Get the number of positional arguments that will be passed to the view.
-      * Will only return a result if there are no named arguments.
-      */
-    int getNumPositionalArguments() {
-        exists(DjangoRouteRegex regex |
-            django_route(this, regex.getAFlowNode(), _) and
-            not exists(string s | s = regex.getGroupName(_, _)) and
+    override int getNumPositionalArguments() {
+        not exists(this.getANamedArgument()) and
+        exists(DjangoRouteRegex regex | regex.getAFlowNode() = route |
             result = count(regex.getGroupNumber(_, _))
         )
     }
 }
+
+class DjangoPathRoute extends DjangoRoute {
+    ControlFlowNode route;
+
+    DjangoPathRoute() {
+        // Django 2.x and 3.x: https://docs.djangoproject.com/en/3.0/ref/urls/#path
+        this = Value::named("django.urls.path").getACall() and
+        (
+            route = this.getArg(0)
+            or
+            route = this.getArgByName("route")
+        )
+    }
+
+    override string getANamedArgument() {
+        // regexp taken from django:
+        // https://github.com/django/django/blob/7d1bf29977bb368d7c28e7c6eb146db3b3009ae7/django/urls/resolvers.py#L199
+        exists(StrConst route_str, string match |
+            route_str = route.getNode() and
+            match = route_str.getText().regexpFind("<(?:(?<converter>[^>:]+):)?(?<parameter>\\w+)>", _, _) and
+            result = match.regexpCapture("<(?:(?<converter>[^>:]+):)?(?<parameter>\\w+)>", 2)
+        )
+    }
+
+    override int getNumPositionalArguments() { none() }
+}

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

@@ -17,9 +17,6 @@ class DjangoRedirect extends HttpRedirectTaintSink {
     override string toString() { result = "django.redirect" }
 
     DjangoRedirect() {
-        exists(CallNode call |
-            redirect().getACall() = call and
-            this = call.getAnArg()
-        )
+        this = redirect().getACall().getAnArg()
     }
 }

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

@@ -39,54 +39,35 @@ class DjangoQueryDict extends TaintKind {
     }
 }
 
-abstract class DjangoRequestSource extends HttpRequestTaintSource {
-    override string toString() { result = "Django request source" }
-
-    override predicate isSourceOf(TaintKind kind) { kind instanceof DjangoRequest }
-}
-
-/**
- * Function based views
- * https://docs.djangoproject.com/en/1.11/topics/http/views/
- */
-private class DjangoFunctionBasedViewRequestArgument extends DjangoRequestSource {
-    DjangoFunctionBasedViewRequestArgument() {
-        exists(FunctionValue view |
-            django_route(_, _, view) and
-            this = view.getScope().getArg(0).asName().getAFlowNode()
+/** A Django request parameter */
+class DjangoRequestSource extends HttpRequestTaintSource {
+    DjangoRequestSource() {
+        exists(DjangoRoute route, DjangoViewHandler view, int request_arg_index |
+            route.getViewHandler() = view and
+            request_arg_index = view.getRequestArgIndex() and
+            this = view.getScope().getArg(request_arg_index).asName().getAFlowNode()
         )
     }
-}
 
-/**
- * Class based views
- * https://docs.djangoproject.com/en/1.11/topics/class-based-views/
- */
-private class DjangoView extends ClassValue {
-    DjangoView() { Value::named("django.views.generic.View") = this.getASuperType() }
-}
-
-private FunctionValue djangoViewHttpMethod() {
-    exists(DjangoView view | view.lookup(httpVerbLower()) = result)
-}
+    override string toString() { result = "Django request source" }
 
-class DjangoClassBasedViewRequestArgument extends DjangoRequestSource {
-    DjangoClassBasedViewRequestArgument() {
-        this = djangoViewHttpMethod().getScope().getArg(1).asName().getAFlowNode()
-    }
+    override predicate isSourceOf(TaintKind kind) { kind instanceof DjangoRequest }
 }
 
 /** An argument specified in a url routing table */
 class DjangoRequestParameter extends HttpRequestTaintSource {
     DjangoRequestParameter() {
-        exists(DjangoRoute route, Function f |
-            f = route.getViewFunction().getScope() |
-            this.(ControlFlowNode).getNode() = f.getArgByName(route.getNamedArgument())
+        exists(DjangoRoute route, Function f, DjangoViewHandler view, int request_arg_index |
+            route.getViewHandler() = view and
+            request_arg_index = view.getRequestArgIndex() and
+            f = view.getScope()
+        |
+            this.(ControlFlowNode).getNode() = f.getArgByName(route.getANamedArgument())
             or
             exists(int i | i >= 0 |
                 i < route.getNumPositionalArguments() and
                 // +1 because first argument is always the request
-                this.(ControlFlowNode).getNode() = f.getArg(i+1)
+                this.(ControlFlowNode).getNode() = f.getArg(request_arg_index + 1 + i)
             )
         )
     }

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

@@ -14,7 +14,15 @@ class DjangoResponse extends TaintKind {
 }
 
 private ClassValue theDjangoHttpResponseClass() {
-    result = Value::named("django.http.response.HttpResponse") and
+    (
+        // 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()
 }
 

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

@@ -4,5 +4,9 @@ import python
 FunctionValue redirect() { result = Value::named("django.shortcuts.redirect") }
 
 ClassValue theDjangoHttpRedirectClass() {
+    // version 1.x
     result = Value::named("django.http.response.HttpResponseRedirectBase")
+    or
+    // version 2.x
+    result = Value::named("django.http.HttpResponseRedirectBase")
 }

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

@@ -0,0 +1,2 @@
+| 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 |

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

@@ -0,0 +1,7 @@
+import python
+import semmle.python.web.HttpRedirect
+import semmle.python.security.strings.Untrusted
+
+from HttpRedirectTaintSink sink, TaintKind kind
+where sink.sinks(kind)
+select sink, kind