Merge pull request #2129 from RasmusWL/python-update-django

Python: update django support

Author: tausbn

change-notes/1.23/analysis-python.md

@@ -20,3 +20,8 @@
 |----------------------------|------------------------|------------|
 | Unreachable code | Fewer false positives | Analysis now accounts for uses of `contextlib.suppress` to suppress exceptions. |
 | `__iter__` method returns a non-iterator | Better alert message | Alert now highlights which class is expected to be an iterator. |
+
+
+## Changes to QL libraries
+
+* Django library now recognizes positional arguments from a `django.conf.urls.url` regex (Django version 1.x)

python/ql/src/Security/CWE-089/SqlInjection.qhelp

@@ -21,20 +21,29 @@ or prepared statements.
 
 <example>
 <p>
-In the following snippet, from an example django app, 
-a name is stored in the database using two different queries.
+In the following snippet, a user is fetched from the database using three
+different queries.
 </p>
 
 <p>
 In the first case, the query string is built by
-directly using string formatting from a user-supplied request attribute.
+directly using string formatting from a user-supplied request parameter.
 The parameter may include quote characters, so this
 code is vulnerable to a SQL injection attack.
 </p>
 
 <p>
 In the second case, the user-supplied request attribute is passed
-to the database using query parameters.
+to the database using query parameters. The database connector library will
+take care of escaping and inserting quotes as needed.
+</p>
+
+<p>
+In the third case, the placeholder in the SQL string has been manually quoted. Since most
+databaseconnector libraries will insert their own quotes, doing so yourself will make the code
+vulnerable to SQL injection attacks. In this example, if <code>username</code> was
+<code>; DROP ALL TABLES -- </code>, the final SQL query would be
+<code>SELECT * FROM users WHERE username = ''; DROP ALL TABLES -- ''</code>
 </p>
 
 <sample src="examples/sql_injection.py" />

python/ql/src/Security/CWE-089/examples/sql_injection.py

@@ -1,21 +1,19 @@
-
-from django.conf.urls import patterns, url
+from django.conf.urls import url
 from django.db import connection
 
 
-def save_name(request):
-
-    if request.method == 'POST':
-        name = request.POST.get('name')
-        curs = connection.cursor()
-        #BAD -- Using string formatting
-        curs.execute(
-            "insert into names_file ('name') values ('%s')" % name)
-        #GOOD -- Using parameters
-        curs.execute(
-            "insert into names_file ('name') values ('%s')", name)
+def show_user(request, username):
+    with connection.cursor() as cursor:
+        # BAD -- Using string formatting
+        cursor.execute("SELECT * FROM users WHERE username = '%s'" % username)
+        user = cursor.fetchone()
 
+        # GOOD -- Using parameters
+        cursor.execute("SELECT * FROM users WHERE username = %s", username)
+        user = cursor.fetchone()
 
-urlpatterns = patterns(url(r'^save_name/$',
-                           upload, name='save_name'))
+        # BAD -- Manually quoting placeholder (%s)
+        cursor.execute("SELECT * FROM users WHERE username = '%s'", username)
+        user = cursor.fetchone()
 
+urlpatterns = [url(r'^users/(?P<username>[^/]+)$', show_user)]

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

@@ -0,0 +1,40 @@
+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)
+    )
+}
+
+class DjangoRouteRegex extends RegexString {
+    DjangoRouteRegex() { django_route(_, this.getAFlowNode(), _) }
+}
+
+class DjangoRoute extends CallNode {
+    DjangoRoute() { django_route(this, _, _) }
+
+    FunctionValue getViewFunction() { django_route(this, _, result) }
+
+    string getNamedArgument() {
+        exists(DjangoRouteRegex regex |
+            django_route(this, regex.getAFlowNode(), _) and
+            regex.getGroupName(_, _) = result
+        )
+    }
+
+    /**
+      * 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
+            result = count(regex.getGroupNumber(_, _))
+        )
+    }
+}

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

@@ -54,33 +54,6 @@ class DjangoModelObjects extends TaintSource {
     override string toString() { result = "django.db.models.Model.objects" }
 }
 
-/** A write to a field of a django model, which is a vulnerable to external data. */
-class DjangoModelFieldWrite extends SqlInjectionSink {
-    DjangoModelFieldWrite() {
-        exists(AttrNode attr, DjangoModel model |
-            this = attr and attr.isStore() and attr.getObject(_).pointsTo(model)
-        )
-    }
-
-    override predicate sinks(TaintKind kind) { kind instanceof ExternalStringKind }
-
-    override string toString() { result = "django model field write" }
-}
-
-/** A direct reference to a django model object, which is vulnerable to external data. */
-class DjangoModelDirectObjectReference extends TaintSink {
-    DjangoModelDirectObjectReference() {
-        exists(CallNode objects_get_call, ControlFlowNode objects | this = objects_get_call.getAnArg() |
-            objects_get_call.getFunction().(AttrNode).getObject("get") = objects and
-            any(DjangoDbTableObjects objs).taints(objects)
-        )
-    }
-
-    override predicate sinks(TaintKind kind) { kind instanceof ExternalStringKind }
-
-    override string toString() { result = "django model object reference" }
-}
-
 /**
  * A call to the `raw` method on a django model. This allows a raw SQL query
  * to be sent to the database, which is a security risk.

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

@@ -1,7 +1,7 @@
 import python
-import semmle.python.regex
 import semmle.python.security.TaintTracking
 import semmle.python.web.Http
+import semmle.python.web.django.General
 
 /** A django.request.HttpRequest object */
 class DjangoRequest extends TaintKind {
@@ -52,7 +52,7 @@ abstract class DjangoRequestSource extends HttpRequestTaintSource {
 private class DjangoFunctionBasedViewRequestArgument extends DjangoRequestSource {
     DjangoFunctionBasedViewRequestArgument() {
         exists(FunctionValue view |
-            url_dispatch(_, _, view) and
+            django_route(_, _, view) and
             this = view.getScope().getArg(0).asName().getAFlowNode()
         )
     }
@@ -67,7 +67,7 @@ private class DjangoView extends ClassValue {
 }
 
 private FunctionValue djangoViewHttpMethod() {
-    exists(DjangoView view | view.attr(httpVerbLower()) = result)
+    exists(DjangoView view | view.lookup(httpVerbLower()) = result)
 }
 
 class DjangoClassBasedViewRequestArgument extends DjangoRequestSource {
@@ -76,41 +76,18 @@ class DjangoClassBasedViewRequestArgument extends DjangoRequestSource {
     }
 }
 
-/* ***********  Routing  ********* */
-/* Function based views */
-predicate url_dispatch(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)
-    )
-}
-
-class UrlRegex extends RegexString {
-    UrlRegex() { url_dispatch(_, this.getAFlowNode(), _) }
-}
-
-class UrlRouting extends CallNode {
-    UrlRouting() { url_dispatch(this, _, _) }
-
-    FunctionValue getViewFunction() { url_dispatch(this, _, result) }
-
-    string getNamedArgument() {
-        exists(UrlRegex regex |
-            url_dispatch(this, regex.getAFlowNode(), _) and
-            regex.getGroupName(_, _) = result
-        )
-    }
-}
-
 /** An argument specified in a url routing table */
-class HttpRequestParameter extends HttpRequestTaintSource {
-    HttpRequestParameter() {
-        exists(UrlRouting url |
-            this.(ControlFlowNode).getNode() = url
-                        .getViewFunction()
-                        .getScope()
-                        .getArgByName(url.getNamedArgument())
+class DjangoRequestParameter extends HttpRequestTaintSource {
+    DjangoRequestParameter() {
+        exists(DjangoRoute route, Function f |
+            f = route.getViewFunction().getScope() |
+            this.(ControlFlowNode).getNode() = f.getArgByName(route.getNamedArgument())
+            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)
+            )
         )
     }