Merge pull request #4864 from RasmusWL/django-request-handler-without-route

Python: Model Django request handler without route

Author: yoff

python/change-notes/2020-12-21-django-with-unknown-route.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Improved modeling of `django` to recognize request handlers on `View` classes without known route, thereby leading to more sources of remote user input (`RemoteFlowSource`).

python/ql/src/semmle/python/Concepts.qll

@@ -313,8 +313,12 @@ module HTTP {
       /** Gets the URL pattern for this route, if it can be statically determined. */
       string getUrlPattern() { result = range.getUrlPattern() }
 
-      /** Gets a function that will handle incoming requests for this route, if any. */
-      Function getARouteHandler() { result = range.getARouteHandler() }
+      /**
+       * Gets a function that will handle incoming requests for this route, if any.
+       *
+       * NOTE: This will be modified in the near future to have a `RequestHandler` result, instead of a `Function`.
+       */
+      Function getARequestHandler() { result = range.getARequestHandler() }
 
       /**
        * Gets a parameter that will receive parts of the url when handling incoming
@@ -343,8 +347,12 @@ module HTTP {
           )
         }
 
-        /** Gets a function that will handle incoming requests for this route, if any. */
-        abstract Function getARouteHandler();
+        /**
+         * Gets a function that will handle incoming requests for this route, if any.
+         *
+         * NOTE: This will be modified in the near future to have a `RequestHandler` result, instead of a `Function`.
+         */
+        abstract Function getARequestHandler();
 
         /**
          * Gets a parameter that will receive parts of the url when handling incoming
@@ -354,8 +362,57 @@ module HTTP {
       }
     }
 
+    /**
+     * A function that will handle incoming HTTP requests.
+     *
+     * Extend this class to refine existing API models. If you want to model new APIs,
+     * extend `RequestHandler::Range` instead.
+     */
+    class RequestHandler extends Function {
+      RequestHandler::Range range;
+
+      RequestHandler() { this = range }
+
+      /**
+       * Gets a parameter that could receive parts of the url when handling incoming
+       * requests, if any. These automatically become a `RemoteFlowSource`.
+       */
+      Parameter getARoutedParameter() { result = range.getARoutedParameter() }
+    }
+
+    /** Provides a class for modeling new HTTP request handlers. */
+    module RequestHandler {
+      /**
+       * A function that will handle incoming HTTP requests.
+       *
+       * Extend this class to model new APIs. If you want to refine existing API models,
+       * extend `RequestHandler` instead.
+       *
+       * Only extend this class if you can't provide a `RouteSetup`, since we handle that case automatically.
+       */
+      abstract class Range extends Function {
+        /**
+         * Gets a parameter that could receive parts of the url when handling incoming
+         * requests, if any. These automatically become a `RemoteFlowSource`.
+         */
+        abstract Parameter getARoutedParameter();
+      }
+    }
+
+    private class RequestHandlerFromRouteSetup extends RequestHandler::Range {
+      RouteSetup rs;
+
+      RequestHandlerFromRouteSetup() { this = rs.getARequestHandler() }
+
+      override Parameter getARoutedParameter() {
+        result = rs.getARoutedParameter() and
+        result in [this.getArg(_), this.getArgByName(_)]
+      }
+    }
+
+    /** A parameter that will receive parts of the url when handling an incoming request. */
     private class RoutedParameter extends RemoteFlowSource::Range, DataFlow::ParameterNode {
-      RoutedParameter() { this.getParameter() = any(RouteSetup setup).getARoutedParameter() }
+      RoutedParameter() { this.getParameter() = any(RequestHandler handler).getARoutedParameter() }
 
       override string getSourceType() { result = "RoutedParameter" }
     }

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

@@ -1676,7 +1676,7 @@ private module Django {
     DjangoViewClassDef() { this.getABase() = django::views::generic::View::subclassRef().asExpr() }
 
     /** Gets a function that could handle incoming requests, if any. */
-    DjangoRouteHandler getARouteHandler() {
+    DjangoRouteHandler getARequestHandler() {
       // TODO: This doesn't handle attribute assignment. Should be OK, but analysis is not as complete as with
       // points-to and `.lookup`, which would handle `post = my_post_handler` inside class def
       result = this.getAMethod() and
@@ -1725,7 +1725,7 @@ private module Django {
     DjangoRouteHandler() {
       exists(djangoRouteHandlerFunctionTracker(this))
       or
-      any(DjangoViewClassDef vc).getARouteHandler() = this
+      any(DjangoViewClassDef vc).getARequestHandler() = this
     }
 
     /** Gets the index of the request parameter. */
@@ -1746,16 +1746,33 @@ private module Django {
     /** Gets the data-flow node that is used as the argument for the view handler. */
     abstract DataFlow::Node getViewArg();
 
-    final override DjangoRouteHandler getARouteHandler() {
+    final override DjangoRouteHandler getARequestHandler() {
       djangoRouteHandlerFunctionTracker(result) = getViewArg()
       or
       exists(DjangoViewClassDef vc |
         getViewArg() = vc.asViewResult() and
-        result = vc.getARouteHandler()
+        result = vc.getARequestHandler()
       )
     }
   }
 
+  /** A request handler defined in a django view class, that has no known route. */
+  private class DjangoViewClassHandlerWithoutKnownRoute extends HTTP::Server::RequestHandler::Range,
+    DjangoRouteHandler {
+    DjangoViewClassHandlerWithoutKnownRoute() {
+      exists(DjangoViewClassDef vc | vc.getARequestHandler() = this) and
+      not exists(DjangoRouteSetup setup | setup.getARequestHandler() = this)
+    }
+
+    override Parameter getARoutedParameter() {
+      // Since we don't know the URL pattern, we simply mark all parameters as a routed
+      // parameter. This should give us more RemoteFlowSources but could also lead to
+      // more FPs. If this turns out to be the wrong tradeoff, we can always change our mind.
+      result in [this.getArg(_), this.getArgByName(_)] and
+      not result = any(int i | i <= this.getRequestParamIndex() | this.getArg(i))
+    }
+  }
+
   /**
    * Gets the regex that is used by django to find routed parameters when using `django.urls.path`.
    *
@@ -1787,14 +1804,14 @@ private module Django {
       // If we don't know the URL pattern, we simply mark all parameters as a routed
       // parameter. This should give us more RemoteFlowSources but could also lead to
       // more FPs. If this turns out to be the wrong tradeoff, we can always change our mind.
-      exists(DjangoRouteHandler routeHandler | routeHandler = this.getARouteHandler() |
+      exists(DjangoRouteHandler routeHandler | routeHandler = this.getARequestHandler() |
         not exists(this.getUrlPattern()) and
         result in [routeHandler.getArg(_), routeHandler.getArgByName(_)] and
         not result = any(int i | i <= routeHandler.getRequestParamIndex() | routeHandler.getArg(i))
       )
       or
       exists(string name |
-        result = this.getARouteHandler().getArgByName(name) and
+        result = this.getARequestHandler().getArgByName(name) and
         exists(string match |
           match = this.getUrlPattern().regexpFind(pathRoutedParameterRegex(), _, _) and
           name = match.regexpCapture(pathRoutedParameterRegex(), 2)
@@ -1809,14 +1826,14 @@ private module Django {
       // If we don't know the URL pattern, we simply mark all parameters as a routed
       // parameter. This should give us more RemoteFlowSources but could also lead to
       // more FPs. If this turns out to be the wrong tradeoff, we can always change our mind.
-      exists(DjangoRouteHandler routeHandler | routeHandler = this.getARouteHandler() |
+      exists(DjangoRouteHandler routeHandler | routeHandler = this.getARequestHandler() |
         not exists(this.getUrlPattern()) and
         result in [routeHandler.getArg(_), routeHandler.getArgByName(_)] and
         not result = any(int i | i <= routeHandler.getRequestParamIndex() | routeHandler.getArg(i))
       )
       or
       exists(DjangoRouteHandler routeHandler, DjangoRouteRegex regex |
-        routeHandler = this.getARouteHandler() and
+        routeHandler = this.getARequestHandler() and
         regex.getRouteSetup() = this
       |
         // either using named capture groups (passed as keyword arguments) or using
@@ -1888,10 +1905,13 @@ private module Django {
   // ---------------------------------------------------------------------------
   // HttpRequest taint modeling
   // ---------------------------------------------------------------------------
-  class DjangoRouteHandlerRequestParam extends django::http::request::HttpRequest::InstanceSource,
+  /** A parameter that will receive the django `HttpRequest` instance when a request handler is invoked. */
+  private class DjangoRequestHandlerRequestParam extends django::http::request::HttpRequest::InstanceSource,
     RemoteFlowSource::Range, DataFlow::ParameterNode {
-    DjangoRouteHandlerRequestParam() {
-      this.getParameter() = any(DjangoRouteSetup setup).getARouteHandler().getRequestParam()
+    DjangoRequestHandlerRequestParam() {
+      this.getParameter() = any(DjangoRouteSetup setup).getARequestHandler().getRequestParam()
+      or
+      this.getParameter() = any(DjangoViewClassHandlerWithoutKnownRoute setup).getRequestParam()
     }
 
     override string getSourceType() { result = "django.http.request.HttpRequest" }

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

@@ -275,10 +275,10 @@ private module FlaskModel {
       // parameter. This should give us more RemoteFlowSources but could also lead to
       // more FPs. If this turns out to be the wrong tradeoff, we can always change our mind.
       not exists(this.getUrlPattern()) and
-      result = this.getARouteHandler().getArgByName(_)
+      result = this.getARequestHandler().getArgByName(_)
       or
       exists(string name |
-        result = this.getARouteHandler().getArgByName(name) and
+        result = this.getARequestHandler().getArgByName(name) and
         exists(string match |
           match = this.getUrlPattern().regexpFind(werkzeug_rule_re(), _, _) and
           name = match.regexpCapture(werkzeug_rule_re(), 4)
@@ -301,7 +301,7 @@ private module FlaskModel {
       result.asCfgNode() in [node.getArg(0), node.getArgByName("rule")]
     }
 
-    override Function getARouteHandler() { result.getADecorator().getAFlowNode() = node }
+    override Function getARequestHandler() { result.getADecorator().getAFlowNode() = node }
   }
 
   /**
@@ -318,7 +318,7 @@ private module FlaskModel {
       result.asCfgNode() in [node.getArg(0), node.getArgByName("rule")]
     }
 
-    override Function getARouteHandler() {
+    override Function getARequestHandler() {
       exists(DataFlow::Node view_func_arg, DataFlow::LocalSourceNode func_src |
         view_func_arg.asCfgNode() in [node.getArg(2), node.getArgByName("view_func")] and
         func_src.flowsTo(view_func_arg) and
@@ -484,7 +484,7 @@ private module FlaskModel {
   private class FlaskRouteHandlerReturn extends HTTP::Server::HttpResponse::Range, DataFlow::CfgNode {
     FlaskRouteHandlerReturn() {
       exists(Function routeHandler |
-        routeHandler = any(FlaskRouteSetup rs).getARouteHandler() and
+        routeHandler = any(FlaskRouteSetup rs).getARequestHandler() and
         node = routeHandler.getAReturnValueFlowNode()
       )
     }

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

@@ -1616,6 +1616,20 @@ private module Stdlib {
         )
       }
     }
+
+    /**
+     * The entry-point for handling a request with a `BaseHTTPRequestHandler` subclass.
+     *
+     * Not essential for any functionality, but provides a consistent modeling.
+     */
+    private class RequestHandlerFunc extends HTTP::Server::RequestHandler::Range {
+      RequestHandlerFunc() {
+        this = any(HTTPRequestHandlerClassDef cls).getAMethod() and
+        this.getName() = "do_" + HTTP::httpVerb()
+      }
+
+      override Parameter getARoutedParameter() { none() }
+    }
   }
 
   // ---------------------------------------------------------------------------

python/ql/src/semmle/python/web/HttpConstants.qll

@@ -1,4 +1,4 @@
-/** Gets an HTTP verb */
+/** Gets an HTTP verb, in upper case */
 string httpVerb() {
   result = "GET" or
   result = "POST" or

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

@@ -4,19 +4,19 @@
 from django.views.generic import View
 
 
-def url_match_xss(request, foo, bar, no_taint=None):  # $routeHandler routedParameter=foo routedParameter=bar
+def url_match_xss(request, foo, bar, no_taint=None):  # $requestHandler routedParameter=foo routedParameter=bar
     return HttpResponse('url_match_xss: {} {}'.format(foo, bar))  # $HttpResponse
 
 
-def get_params_xss(request):  # $routeHandler
+def get_params_xss(request):  # $requestHandler
     return HttpResponse(request.GET.get("untrusted"))  # $HttpResponse
 
 
-def post_params_xss(request):  # $routeHandler
+def post_params_xss(request):  # $requestHandler
     return HttpResponse(request.POST.get("untrusted"))  # $HttpResponse
 
 
-def http_resp_write(request):  # $routeHandler
+def http_resp_write(request):  # $requestHandler
     rsp = HttpResponse()  # $HttpResponse
     rsp.write(request.GET.get("untrusted"))  # $HttpResponse
     return rsp
@@ -26,22 +26,22 @@ class Foo(object):
     # Note: since Foo is used as the super type in a class view, it will be able to handle requests.
 
 
-    def post(self, request, untrusted):  # $ MISSING: routeHandler routedParameter=untrusted
+    def post(self, request, untrusted):  # $ MISSING: requestHandler routedParameter=untrusted
         return HttpResponse('Foo post: {}'.format(untrusted))  # $HttpResponse
 
 
 class ClassView(View, Foo):
 
-    def get(self, request, untrusted):  # $ routeHandler routedParameter=untrusted
+    def get(self, request, untrusted):  # $ requestHandler routedParameter=untrusted
         return HttpResponse('ClassView get: {}'.format(untrusted))  # $HttpResponse
 
 
-def show_articles(request, page_number=1):  # $routeHandler routedParameter=page_number
+def show_articles(request, page_number=1):  # $requestHandler routedParameter=page_number
     page_number = int(page_number)
     return HttpResponse('articles page: {}'.format(page_number))  # $HttpResponse
 
 
-def xxs_positional_arg(request, arg0, arg1, no_taint=None):  # $routeHandler routedParameter=arg0 routedParameter=arg1
+def xxs_positional_arg(request, arg0, arg1, no_taint=None):  # $requestHandler routedParameter=arg0 routedParameter=arg1
     return HttpResponse('xxs_positional_arg: {} {}'.format(arg0, arg1))  # $HttpResponse
 
 
@@ -62,7 +62,7 @@ def xxs_positional_arg(request, arg0, arg1, no_taint=None):  # $routeHandler rou
 ################################################################################
 # Using patterns() for routing
 
-def show_user(request, username):  # $routeHandler routedParameter=username
+def show_user(request, username):  # $requestHandler routedParameter=username
     return HttpResponse('show_user {}'.format(username))  # $HttpResponse
 
 
@@ -71,7 +71,7 @@ def show_user(request, username):  # $routeHandler routedParameter=username
 ################################################################################
 # Show we understand the keyword arguments to django.conf.urls.url
 
-def kw_args(request):  # $routeHandler
+def kw_args(request):  # $requestHandler
     return HttpResponse('kw_args')  # $HttpResponse
 
 urlpatterns = [