Merge pull request #4981 from RasmusWL/port-url-redirect-query

Python: Port url redirect query

Author: yoff

python/change-notes/2021-01-19-port-url-redirect-query.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Ported URL redirection (`py/url-redirection`) query to use new data-flow library. This might result in different results, but overall a more robust and accurate analysis.

python/ql/src/Security/CWE-601/UrlRedirect.ql

@@ -12,30 +12,10 @@
  */
 
 import python
-import semmle.python.security.Paths
-import semmle.python.web.HttpRedirect
-import semmle.python.web.HttpRequest
-import semmle.python.security.strings.Untrusted
+import semmle.python.security.dataflow.UrlRedirect
+import DataFlow::PathGraph
 
-/** Url redirection is a problem only if the user controls the prefix of the URL */
-class UntrustedPrefixStringKind extends UntrustedStringKind {
-  override TaintKind getTaintForFlowStep(ControlFlowNode fromnode, ControlFlowNode tonode) {
-    result = UntrustedStringKind.super.getTaintForFlowStep(fromnode, tonode) and
-    not tonode.(BinaryExprNode).getRight() = fromnode
-  }
-}
-
-class UrlRedirectConfiguration extends TaintTracking::Configuration {
-  UrlRedirectConfiguration() { this = "URL redirect configuration" }
-
-  override predicate isSource(TaintTracking::Source source) {
-    source instanceof HttpRequestTaintSource
-  }
-
-  override predicate isSink(TaintTracking::Sink sink) { sink instanceof HttpRedirectTaintSink }
-}
-
-from UrlRedirectConfiguration config, TaintedPathSource src, TaintedPathSink sink
-where config.hasFlowPath(src, sink)
-select sink.getSink(), src, sink, "Untrusted URL redirection due to $@.", src.getSource(),
-  "a user-provided value"
+from UrlRedirectConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Untrusted URL redirection due to $@.", source.getNode(),
+  "A user-provided value"

python/ql/src/experimental/Security-old-dataflow/CWE-601/UrlRedirect.ql

@@ -0,0 +1,41 @@
+/**
+ * @name URL redirection from remote source
+ * @description URL redirection based on unvalidated user input
+ *              may cause redirection to malicious web sites.
+ * @kind path-problem
+ * @problem.severity error
+ * @sub-severity low
+ * @id py/url-redirection
+ * @tags security
+ *       external/cwe/cwe-601
+ * @precision high
+ */
+
+import python
+import semmle.python.security.Paths
+import semmle.python.web.HttpRedirect
+import semmle.python.web.HttpRequest
+import semmle.python.security.strings.Untrusted
+
+/** Url redirection is a problem only if the user controls the prefix of the URL */
+class UntrustedPrefixStringKind extends UntrustedStringKind {
+  override TaintKind getTaintForFlowStep(ControlFlowNode fromnode, ControlFlowNode tonode) {
+    result = UntrustedStringKind.super.getTaintForFlowStep(fromnode, tonode) and
+    not tonode.(BinaryExprNode).getRight() = fromnode
+  }
+}
+
+class UrlRedirectConfiguration extends TaintTracking::Configuration {
+  UrlRedirectConfiguration() { this = "URL redirect configuration" }
+
+  override predicate isSource(TaintTracking::Source source) {
+    source instanceof HttpRequestTaintSource
+  }
+
+  override predicate isSink(TaintTracking::Sink sink) { sink instanceof HttpRedirectTaintSink }
+}
+
+from UrlRedirectConfiguration config, TaintedPathSource src, TaintedPathSink sink
+where config.hasFlowPath(src, sink)
+select sink.getSink(), src, sink, "Untrusted URL redirection due to $@.", src.getSource(),
+  "a user-provided value"

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

@@ -473,5 +473,40 @@ module HTTP {
         }
       }
     }
+
+    /**
+     * A data-flow node that creates a HTTP redirect response on a server.
+     *
+     * Note: we don't require that this redirect must be sent to a client (a kind of
+     * "if a tree falls in a forest and nobody hears it" situation).
+     *
+     * Extend this class to refine existing API models. If you want to model new APIs,
+     * extend `HttpRedirectResponse::Range` instead.
+     */
+    class HttpRedirectResponse extends HttpResponse {
+      override HttpRedirectResponse::Range range;
+
+      HttpRedirectResponse() { this = range }
+
+      /** Gets the data-flow node that specifies the location of this HTTP redirect response. */
+      DataFlow::Node getRedirectLocation() { result = range.getRedirectLocation() }
+    }
+
+    /** Provides a class for modeling new HTTP redirect response APIs. */
+    module HttpRedirectResponse {
+      /**
+       * A data-flow node that creates a HTTP redirect response on a server.
+       *
+       * Note: we don't require that this redirect must be sent to a client (a kind of
+       * "if a tree falls in a forest and nobody hears it" situation).
+       *
+       * Extend this class to model new APIs. If you want to refine existing API models,
+       * extend `HttpResponse` instead.
+       */
+      abstract class Range extends HTTP::Server::HttpResponse::Range {
+        /** Gets the data-flow node that specifies the location of this HTTP redirect response. */
+        abstract DataFlow::Node getRedirectLocation();
+      }
+    }
   }
 }

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

@@ -35,7 +35,7 @@ private module Django {
    * WARNING: Only holds for a few predefined attributes.
    */
   private DataFlow::Node django_attr(DataFlow::TypeTracker t, string attr_name) {
-    attr_name in ["db", "urls", "http", "conf", "views"] and
+    attr_name in ["db", "urls", "http", "conf", "views", "shortcuts"] and
     (
       t.start() and
       result = DataFlow::importNode("django" + "." + attr_name)
@@ -724,7 +724,8 @@ private module Django {
            *
            * Use the predicate `HttpResponseRedirect::instance()` to get references to instances of `django.http.response.HttpResponseRedirect`.
            */
-          abstract class InstanceSource extends HttpResponse::InstanceSource, DataFlow::Node { }
+          abstract class InstanceSource extends HttpResponse::InstanceSource,
+            HTTP::Server::HttpRedirectResponse::Range, DataFlow::Node { }
 
           /** A direct instantiation of `django.http.response.HttpResponseRedirect`. */
           private class ClassInstantiation extends InstanceSource, DataFlow::CfgNode {
@@ -739,6 +740,10 @@ private module Django {
               result.asCfgNode() in [node.getArg(1), node.getArgByName("content")]
             }
 
+            override DataFlow::Node getRedirectLocation() {
+              result.asCfgNode() in [node.getArg(0), node.getArgByName("redirect_to")]
+            }
+
             // How to support the `headers` argument here?
             override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
 
@@ -790,7 +795,8 @@ private module Django {
            *
            * Use the predicate `HttpResponsePermanentRedirect::instance()` to get references to instances of `django.http.response.HttpResponsePermanentRedirect`.
            */
-          abstract class InstanceSource extends HttpResponse::InstanceSource, DataFlow::Node { }
+          abstract class InstanceSource extends HttpResponse::InstanceSource,
+            HTTP::Server::HttpRedirectResponse::Range, DataFlow::Node { }
 
           /** A direct instantiation of `django.http.response.HttpResponsePermanentRedirect`. */
           private class ClassInstantiation extends InstanceSource, DataFlow::CfgNode {
@@ -805,6 +811,10 @@ private module Django {
               result.asCfgNode() in [node.getArg(1), node.getArgByName("content")]
             }
 
+            override DataFlow::Node getRedirectLocation() {
+              result.asCfgNode() in [node.getArg(0), node.getArgByName("redirect_to")]
+            }
+
             // How to support the `headers` argument here?
             override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
 
@@ -1907,6 +1917,62 @@ private module Django {
         }
       }
     }
+
+    // -------------------------------------------------------------------------
+    // django.shortcuts
+    // -------------------------------------------------------------------------
+    /** Gets a reference to the `django.shortcuts` module. */
+    DataFlow::Node shortcuts() { result = django_attr("shortcuts") }
+
+    /** Provides models for the `django.shortcuts` module */
+    module shortcuts {
+      /**
+       * Gets a reference to the attribute `attr_name` of the `django.shortcuts` module.
+       * WARNING: Only holds for a few predefined attributes.
+       */
+      private DataFlow::Node shortcuts_attr(DataFlow::TypeTracker t, string attr_name) {
+        attr_name in ["redirect"] and
+        (
+          t.start() and
+          result = DataFlow::importNode("django.shortcuts" + "." + attr_name)
+          or
+          t.startInAttr(attr_name) and
+          result = shortcuts()
+        )
+        or
+        // Due to bad performance when using normal setup with `shortcuts_attr(t2, attr_name).track(t2, t)`
+        // we have inlined that code and forced a join
+        exists(DataFlow::TypeTracker t2 |
+          exists(DataFlow::StepSummary summary |
+            shortcuts_attr_first_join(t2, attr_name, result, summary) and
+            t = t2.append(summary)
+          )
+        )
+      }
+
+      pragma[nomagic]
+      private predicate shortcuts_attr_first_join(
+        DataFlow::TypeTracker t2, string attr_name, DataFlow::Node res,
+        DataFlow::StepSummary summary
+      ) {
+        DataFlow::StepSummary::step(shortcuts_attr(t2, attr_name), res, summary)
+      }
+
+      /**
+       * Gets a reference to the attribute `attr_name` of the `django.shortcuts` module.
+       * WARNING: Only holds for a few predefined attributes.
+       */
+      private DataFlow::Node shortcuts_attr(string attr_name) {
+        result = shortcuts_attr(DataFlow::TypeTracker::end(), attr_name)
+      }
+
+      /**
+       * Gets a reference to the `django.shortcuts.redirect` function
+       *
+       * See https://docs.djangoproject.com/en/3.1/topics/http/shortcuts/#redirect
+       */
+      DataFlow::Node redirect() { result = shortcuts_attr("redirect") }
+    }
   }
 
   // ---------------------------------------------------------------------------
@@ -2230,4 +2296,39 @@ private module Django {
       )
     }
   }
+
+  // ---------------------------------------------------------------------------
+  // django.shortcuts.redirect
+  // ---------------------------------------------------------------------------
+  /**
+   * A call to `django.shortcuts.redirect`.
+   *
+   * Note: This works differently depending on what argument is used.
+   * _One_ option is to redirect to a full URL.
+   *
+   * See https://docs.djangoproject.com/en/3.1/topics/http/shortcuts/#redirect
+   */
+  private class DjangoShortcutsRedirectCall extends HTTP::Server::HttpRedirectResponse::Range,
+    DataFlow::CfgNode {
+    override CallNode node;
+
+    DjangoShortcutsRedirectCall() { node.getFunction() = django::shortcuts::redirect().asCfgNode() }
+
+    /**
+     * Gets the data-flow node that specifies the location of this HTTP redirect response.
+     *
+     * Note: For `django.shortcuts.redirect`, the result might not be a full URL
+     * (as usually expected by this method), but could be a relative URL,
+     * a string identifying a view, or a Django model.
+     */
+    override DataFlow::Node getRedirectLocation() {
+      result.asCfgNode() in [node.getArg(0), node.getArgByName("to")]
+    }
+
+    override DataFlow::Node getBody() { none() }
+
+    override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
+
+    override string getMimetypeDefault() { none() }
+  }
 }

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

@@ -34,7 +34,7 @@ private module FlaskModel {
    * WARNING: Only holds for a few predefined attributes.
    */
   private DataFlow::Node flask_attr(DataFlow::TypeTracker t, string attr_name) {
-    attr_name in ["request", "make_response", "Response", "views"] and
+    attr_name in ["request", "make_response", "Response", "views", "redirect"] and
     (
       t.start() and
       result = DataFlow::importNode("flask" + "." + attr_name)
@@ -669,4 +669,31 @@ private module FlaskModel {
 
     override string getMimetypeDefault() { result = "text/html" }
   }
+
+  /**
+   * A call to the `flask.redirect` function.
+   *
+   * See https://flask.palletsprojects.com/en/1.1.x/api/#flask.redirect
+   */
+  private class FlaskRedirectCall extends HTTP::Server::HttpRedirectResponse::Range,
+    DataFlow::CfgNode {
+    override CallNode node;
+
+    FlaskRedirectCall() { node.getFunction() = flask_attr("redirect").asCfgNode() }
+
+    override DataFlow::Node getRedirectLocation() {
+      result.asCfgNode() in [node.getArg(0), node.getArgByName("location")]
+    }
+
+    override DataFlow::Node getBody() { none() }
+
+    override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
+
+    override string getMimetypeDefault() {
+      // note that while you're not able to set content yourself, the function will
+      // actually fill out some default content, that is served with mimetype
+      // `text/html`.
+      result = "text/html"
+    }
+  }
 }

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

@@ -216,6 +216,17 @@ private module Tornado {
         /** Gets a reference to one of the methods `get_arguments`, `get_body_arguments`, `get_query_arguments`. */
         DataFlow::Node argumentsMethod() { result = argumentsMethod(DataFlow::TypeTracker::end()) }
 
+        /** Gets a reference the `redirect` method. */
+        private DataFlow::Node redirectMethod(DataFlow::TypeTracker t) {
+          t.startInAttr("redirect") and
+          result = instance()
+          or
+          exists(DataFlow::TypeTracker t2 | result = redirectMethod(t2).track(t2, t))
+        }
+
+        /** Gets a reference the `redirect` method. */
+        DataFlow::Node redirectMethod() { result = redirectMethod(DataFlow::TypeTracker::end()) }
+
         /** Gets a reference to the `write` method. */
         private DataFlow::Node writeMethod(DataFlow::TypeTracker t) {
           t.startInAttr("write") and
@@ -556,7 +567,31 @@ private module Tornado {
   // Response modeling
   // ---------------------------------------------------------------------------
   /**
-   * A call to `tornado.web.RequestHandler.write` method.
+   * A call to the `tornado.web.RequestHandler.redirect` method.
+   *
+   * See https://www.tornadoweb.org/en/stable/web.html?highlight=write#tornado.web.RequestHandler.redirect
+   */
+  private class TornadoRequestHandlerRedirectCall extends HTTP::Server::HttpRedirectResponse::Range,
+    DataFlow::CfgNode {
+    override CallNode node;
+
+    TornadoRequestHandlerRedirectCall() {
+      node.getFunction() = tornado::web::RequestHandler::redirectMethod().asCfgNode()
+    }
+
+    override DataFlow::Node getRedirectLocation() {
+      result.asCfgNode() in [node.getArg(0), node.getArgByName("url")]
+    }
+
+    override DataFlow::Node getBody() { none() }
+
+    override string getMimetypeDefault() { none() }
+
+    override DataFlow::Node getMimetypeOrContentTypeArg() { none() }
+  }
+
+  /**
+   * A call to the `tornado.web.RequestHandler.write` method.
    *
    * See https://www.tornadoweb.org/en/stable/web.html?highlight=write#tornado.web.RequestHandler.write
    */